Re: [PATCH] i2c: tegra: Add ACPI support
19.11.2021 16:32, Akhil R пишет:
> - 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);
> - }
> -
> tegra_i2c_parse_dt(i2c_dev);
>
> - err = tegra_i2c_init_clocks(i2c_dev);
> - if (err)
> - return err;
> + if (!has_acpi_companion(>dev)) {
> + 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);
> + }
> +
> + err = tegra_i2c_init_clocks(i2c_dev);
> + if (err)
> + return err;
> + }
What about to factor out the reset initialization into a separate function and
write it like this:
static int tegra_i2c_init_reset(i2c_dev)
{
if (has_acpi_companion(i2c_dev->dev)
return 0;
i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
if (IS_ERR(i2c_dev->rst))
return dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
"failed to get reset control\n");
return 0;
}
And then change tegra_i2c_init_clocks() to:
static int tegra_i2c_init_clocks(i2c_dev)
{
int err;
if (has_acpi_companion(i2c_dev->dev))
return 0;
...
}
This will make both reset/clocks initialization to look more consistent.
Re: [PATCH] i2c: tegra: Add ACPI support
19.11.2021 17:48, Andy Shevchenko пишет: >> + if (i2c_dev->nclocks == 0) >> + return; > Why? Make clocks optional. This check shouldn't be needed because both clk_disable() and clk_bulk_unprepare() should handle NULL/zero clocks without problems.
RE: [PATCH] i2c: tegra: Add ACPI support
>
>
> On Fri, Nov 19, 2021 at 3:37 PM Akhil R 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(>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
Re: [PATCH] i2c: tegra: Add ACPI support
On Fri, Nov 19, 2021 at 3:37 PM Akhil R 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(>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)_i2c_hw},
> + {.id = "NVDA0201", .driver_data = (kernel_ulong_t)_i2c_hw},
> + {.id = "NVDA0301", .driver_data = (kernel_ulong_t)_i2c_hw},
> + { },
No comma for the terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
