Re: [PATCH v4 6/9] platform/x86: bus-multi-instantiate: Reorganize I2C functions

Hi,

Sorry some more remarks after all...

On 1/20/22 14:43, Stefan Binding wrote:

From: Lucas Tanure tanureal@opensource.cirrus.com

Reorganize I2C functions to accommodate SPI support Split the probe and factor out parts of the code that will be used in the SPI support

Signed-off-by: Lucas Tanure tanureal@opensource.cirrus.com Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com

---

drivers/platform/x86/bus-multi-instantiate.c | 150 ++++++++++++------- 1 file changed, 96 insertions(+), 54 deletions(-)

diff --git a/drivers/platform/x86/bus-multi-instantiate.c b/drivers/platform/x86/bus-multi-instantiate.c index 982dfecfd27c..50f1540762e9 100644 --- a/drivers/platform/x86/bus-multi-instantiate.c +++ b/drivers/platform/x86/bus-multi-instantiate.c @@ -29,85 +29,129 @@ struct bmi_instance {

struct bmi { int i2c_num;

• struct i2c_client *i2c_devs[];

• struct i2c_client **i2c_devs;

};

-static int bmi_probe(struct platform_device *pdev) +static int bmi_get_irq(struct platform_device *pdev, struct acpi_device *adev,

•        const struct bmi_instance *inst)
  

+{

• int ret;
• switch (inst->flags & IRQ_RESOURCE_TYPE) {
• case IRQ_RESOURCE_GPIO:

• ret = acpi_dev_gpio_irq_get(adev, inst->irq_idx);
  

• break;
  

• case IRQ_RESOURCE_APIC:

• ret = platform_get_irq(pdev, inst->irq_idx);
  

• break;
  

• default:

• ret = 0;
  

• break;
  

• }
• if (ret < 0)

• dev_err_probe(&pdev->dev, ret, "Error requesting irq at index %d: %d\n",
  

• 	      inst->irq_idx, ret);
  

• return ret;

+}

+static void bmi_devs_unregister(struct bmi *bmi) +{

• while (bmi->i2c_num > 0)

• i2c_unregister_device(bmi->i2c_devs[--bmi->i2c_num]);
  

+}

+/**

• • bmi_i2c_probe - Instantiate multiple I2C devices from inst array
• • @pdev: Platform device
• • @adev: ACPI device
• • @bmi: Internal struct for Bus multi instantiate driver
• • @inst: Array of instances to probe
• • Returns the number of I2C devices instantiate, Zero if none is found or a negative error code.
• */

+static int bmi_i2c_probe(struct platform_device *pdev, struct acpi_device *adev, struct bmi *bmi,

• 	 const struct bmi_instance *inst_array)
  

{ struct i2c_board_info board_info = {};

• const struct bmi_instance *inst; struct device *dev = &pdev->dev;
• struct acpi_device *adev;
• struct bmi *bmi; char name[32];
• int i, ret;

• int i, ret = 0, count;

• inst = device_get_match_data(dev);
• if (!inst) {

• dev_err(dev, "Error ACPI match data is missing\n");
  

• return -ENODEV;
  

• }
• adev = ACPI_COMPANION(dev);
• /* Count number of clients to instantiate */ ret = i2c_acpi_client_count(adev);
• if (ret < 0)

• if (ret <= 0) return ret;

Please change this to:

return ret == 0 ? -ENODEV : ret;

This helps making return value handler in the caller cleaner, also see my upcoming review of patch 7/9.

• count = ret;

• bmi = devm_kmalloc(dev, struct_size(bmi, i2c_devs, ret), GFP_KERNEL);
• if (!bmi)

• bmi->i2c_devs = devm_kcalloc(dev, count, sizeof(*bmi->i2c_devs), GFP_KERNEL);
• if (!bmi->i2c_devs) return -ENOMEM;

• bmi->i2c_num = ret;
• for (i = 0; i < bmi->i2c_num && inst[i].type; i++) {

• for (i = 0; i < count && inst_array[i].type; i++) { memset(&board_info, 0, sizeof(board_info));

• strlcpy(board_info.type, inst[i].type, I2C_NAME_SIZE);
  

• snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), inst[i].type, i);
  

• strscpy(board_info.type, inst_array[i].type, I2C_NAME_SIZE);
  

• snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), inst_array[i].type, i);
  

  board_info.dev_name = name;

• switch (inst[i].flags & IRQ_RESOURCE_TYPE) {
  

• case IRQ_RESOURCE_GPIO:
  

• 	ret = acpi_dev_gpio_irq_get(adev, inst[i].irq_idx);
  

• 	if (ret < 0) {
  

• 		dev_err(dev, "Error requesting irq at index %d: %d\n",
  

• 				inst[i].irq_idx, ret);
  

• 		goto error;
  

• 	}
  

• 	board_info.irq = ret;
  

• 	break;
  

• case IRQ_RESOURCE_APIC:
  

• 	ret = platform_get_irq(pdev, inst[i].irq_idx);
  

• 	if (ret < 0) {
  

• 		dev_dbg(dev, "Error requesting irq at index %d: %d\n",
  

• 			inst[i].irq_idx, ret);
  

• 		goto error;
  

• 	}
  

• 	board_info.irq = ret;
  

• 	break;
  

• default:
  

• 	board_info.irq = 0;
  

• 	break;
  

• }
  

• ret = bmi_get_irq(pdev, adev, &inst_array[i]);
  

• if (ret < 0)
  

• 	goto error;
  

• board_info.irq = ret;
  

• bmi->i2c_devs[i] = i2c_acpi_new_device(dev, i, &board_info); if (IS_ERR(bmi->i2c_devs[i])) { ret = dev_err_probe(dev, PTR_ERR(bmi->i2c_devs[i]), "Error creating i2c-client, idx %d\n", i); goto error; }

• bmi->i2c_num++;
  

  }

• if (i < bmi->i2c_num) {

• if (bmi->i2c_num < count) { dev_err(dev, "Error finding driver, idx %d\n", i); ret = -ENODEV; goto error; }

• platform_set_drvdata(pdev, bmi);
• return 0;

• dev_info(dev, "Instantiate %d I2C devices.\n", bmi->i2c_num);

• return bmi->i2c_num;

And change this to return 0.

error:

• while (--i >= 0)

• i2c_unregister_device(bmi->i2c_devs[i]);
  

• dev_err_probe(dev, ret, "I2C error %d\n", ret);
• bmi_devs_unregister(bmi);
• return ret;

+}

+static int bmi_probe(struct platform_device *pdev) +{

• const struct bmi_instance *inst_array;

• struct device *dev = &pdev->dev;

• struct acpi_device *adev;

• struct bmi *bmi;

• int ret;

• inst_array = device_get_match_data(dev);

• if (!inst_array) {

• dev_err(dev, "Error ACPI match data is missing\n");
  

• return -ENODEV;
  

• }

• adev = ACPI_COMPANION(dev);

• if (!adev)

• return -ENODEV;
  

• bmi = devm_kzalloc(dev, sizeof(*bmi), GFP_KERNEL);

• if (!bmi)

• return -ENOMEM;
  

• platform_set_drvdata(pdev, bmi);

• ret = bmi_i2c_probe(pdev, adev, bmi, inst_array);

• if (ret > 0)

• return 0;
  

• if (ret == 0)

• ret = -ENODEV;
  
  

  return ret;

Then you can simplify the above to just:

return bmi_i2c_probe(pdev, adev, bmi, inst_array);

:)

Regards,

Hans

} @@ -115,10 +159,8 @@ static int bmi_probe(struct platform_device *pdev) static int bmi_remove(struct platform_device *pdev) { struct bmi *bmi = platform_get_drvdata(pdev);

• int i;

• for (i = 0; i < bmi->i2c_num; i++)

• i2c_unregister_device(bmi->i2c_devs[i]);
  

• bmi_devs_unregister(bmi);

  return 0;

}