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

Andy Shevchenko
2021-11-19T14:49:22Z

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.

> +};

--
With Best Regards,
Andy Shevchenko