linux-kernel - Re: [PATCH v3 09/22] i2c: tegra: Clean up probe function

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]

Date:   Thu, 3 Sep 2020 16:48:34 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Michał Mirosław <mirq-linux@...e.qmqm.pl>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        linux-tegra@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 09/22] i2c: tegra: Clean up probe function

03.09.2020 14:17, Andy Shevchenko пишет:
> On Thu, Sep 3, 2020 at 3:54 AM Dmitry Osipenko <digetx@...il.com> wrote:
>>
>> The driver's probe function code is difficult to read and follow. This
>> patch reorders code of the probe function, forming logical groups that are
>> easy to work with. The clock and hardware initializations are factored
>> out into separate functions in order to keep code clean and ease error
>> unwinding.
>>
>> Driver now makes use of devm_platform_get_and_ioremap_resource() and
>> platform_get_irq() which are replacing boilerplate parts of the code.
>>
>> The dev_err_probe() is now used for reset control retrieval because reset
>> is now requested before clocks, and thus, BPMP driver that provides reset
>> controls for newer SoCs may cause the probe defer.
> 
>> The error message of devm_request_irq() is removed because this function
>> takes care of printing the message by itself.
> 
> I see no evidence of this.

Good catch! I confused it with the platform_get_irq() which prints the
message! I'll correct it in v4, thanks!

Anyways, the message of devm_request_irq() needs a correction since it
prints the number of vIRQ instead of the error code.

> ...
> 
>> +       of_property_read_u32(np, "clock-frequency", &bus_clk_rate);
>> +       i2c_dev->bus_clk_rate = bus_clk_rate;
> 
> Hmm... I dunno if Wolfram is going to implement a special helper
> exactly for this. I remember we discussed that with him during v5.8
> (?) times.

I now see that there is a i2c_parse_fw_timings() which parses
"clock-frequency" and other common properties. I could switch to use
that helper, but not sure whether it would be really worthwhile because
only one property is needed to be parsed. I'll consider this change for
v4, thank you for the suggestion!

> ...
> 
>> +static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev)
> 
> Hmm... Don't we have something like devm_clk_bulk_get_all() or so?
> 

Sounds like a good suggestion! I'll consider it for the v4, thanks!