linux-kernel - Re: [PATCH] i2c: tegra: Add ACPI support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 Nov 2021 16:48:15 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Akhil R <akhilrajeev@...dia.com>
Cc:     Laxman Dewangan <ldewangan@...dia.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jon Hunter <jonathanh@...dia.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian Koenig <christian.koenig@....com>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        linux-tegra <linux-tegra@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH] i2c: tegra: Add ACPI support

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