Re: [PATCH] i2c: tegra: Add ACPI support

On Fri, Nov 19, 2021 at 3:37 PM Akhil R akhilrajeev@nvidia.com wrote:

Add support for ACPI based device registration so that the driver can be also enabled through ACPI table.

the ACPI

...

•   if (has_acpi_companion(i2c_dev->dev)) {
  

You are checkin for the companion and using a handle, why not check for a handle explicitly?

•           acpi_evaluate_object(ACPI_HANDLE(i2c_dev->dev), "_RST",
  

•                                NULL, NULL);
  

•   } else {
  

•           err = reset_control_reset(i2c_dev->rst);
  

•           WARN_ON_ONCE(err);
  

•   }
  

...

•   if (i2c_dev->nclocks == 0)
  

•           return;
  

Why? Make clocks optional.

...

•   i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
  

•   if (IS_ERR(i2c_dev->rst)) {
  

•           dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
  

•                         "failed to get reset control\n");
  

•           return PTR_ERR(i2c_dev->rst);
  

Besides the fact this should be as simple as

return dev_err_probe(...)

•   }
  

•   if (!has_acpi_companion(&pdev->dev)) {
  

...why do you do this?

•           i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
  

•           if (IS_ERR(i2c_dev->rst)) {
  

•                   dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
  

•                                 "failed to get reset control\n");
  

•                   return PTR_ERR(i2c_dev->rst);
  

•           }
  

...

+static const struct acpi_device_id tegra_i2c_acpi_match[] = {

•   {.id = "NVDA0101", .driver_data = (kernel_ulong_t)&tegra210_i2c_hw},
  

•   {.id = "NVDA0201", .driver_data = (kernel_ulong_t)&tegra186_i2c_hw},
  

•   {.id = "NVDA0301", .driver_data = (kernel_ulong_t)&tegra194_i2c_hw},
  

•   { },
  

No comma for the terminator entry.

+};