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

[Andy Shevchenko]

On Fri, Nov 19, 2021 at 3:37 PM Akhil R <akhilrajeev@xxxxxxxxxx> 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