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

[Akhil R]

> 
> 
> 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?
Okay.
> 
> > +               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?
The thought was to call out the error when using device tree and to ignore if using ACPI table. 
We are expecting the clocks to be initialized from the bootloader and to use the _RST method 
(instead of reset_control), when an ACPI table is used.
The problem I thought when making it optional is that an error could go unnoticed when using a 
device tree as well.

Best Regards,
Akhil