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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <BN9PR12MB5273620B5C397CD425130704C09D9@BN9PR12MB5273.namprd12.prod.outlook.com>
Date:   Sat, 20 Nov 2021 07:36:19 +0000
From:   Akhil R <akhilrajeev@...dia.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Laxman Dewangan <ldewangan@...dia.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan 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" <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?
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