linux-kernel - Re: [PATCH v4 09/16] gpio: support ROHM BD71815 GPOs

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] [day] [month] [year] [list]

Date:   Sun, 28 Mar 2021 19:59:52 +0300
From:   Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Lee Jones <lee.jones@...aro.org>, Rob Herring <robh+dt@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-power <linux-power@...rohmeurope.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v4 09/16] gpio: support ROHM BD71815 GPOs

Hi Again Andy,

On Fri, 2021-03-26 at 19:51 +0200, Andy Shevchenko wrote:
> On Fri, Mar 26, 2021 at 3:33 PM Matti Vaittinen
> <matti.vaittinen@...rohmeurope.com> wrote:
> > On Fri, 2021-03-26 at 13:26 +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
> > > <matti.vaittinen@...rohmeurope.com> wrote:
> 
> 
> > > > +       if (!bd71815->e5_pin_is_gpo && offset)
> > > > +               return;
> > > 
> > > I wonder if you can use valid_mask instead of this.
> > 
> > Do you mean re-naming the e5_pin_is_gpo to valid_mask? Or do you
> > mean
> > some GPIO framework internal feature allowing to define valid pins?
> > (If
> > my memory serves me right one can set invalid pins from DT - but by
> > default all pins are valid and here we want to invalidate a pin by
> > default). For renaming I don't see the value - if internal feature
> > can
> > be used then there may be value. Thanks for the pointer, I'll look
> > what
> > I find.
> 
> I mean to utilize internal valid_mask bitmap. Yes, you may fill it as
> valid at the start of the driver and then simply call __clear_bit() /
> clear_bit() against one you wanted to disable. Then core will take
> care of the rest (AFAIR).

Right. I see there is the init_valid_mask callback which could be
populated. OTOH, I think this check is actually not needed at all if we
set the ngpios based on the DT flag. The check in set/get functions was
just a symptom of my paranoia. Anyways, I owe you as I just learned
something new :)

> > > > +       int ret;
> > > > +       struct bd71815_gpio *g;
> > > > +       struct device *dev;
> > > > +       struct device *parent;
> ...
> 
> > > > +       parent = dev->parent;
> > 
> > It is not always obvious (especially for someone not reading MFD
> > driver
> > code frequently) why we use parent device for some things and the
> > device being probed to some other stuff. Typically this is not
> > needed
> > if the device is not MFD sub-device. And again, the comments in the
> > middle of declaration block look confusing to me. I think removing
> > comments and moving these to declaration make readability _much_
> > worse.
> 
> I disagree with you here again. To me it's like completely unneeded
> churn.
> 

I've seen even experienced developers making mistakes by binding the
lifetime of sub-device stuff to parent device's lifetime. I've also
seen even experienced developers who haven't used to dealing with MFD
getting confused when they see parent device's dt-node or regmap being
used. My view on this is that if the comment helps next reader to avoid
a mistake or understand why something is done - then it is worthy. I
have rarely seen comments which make code less readable or
understandable.

> > > > +       g->chip.of_node = parent->of_node;
> > > 
> > > Redundant. GPIO library does it for you and even better.
> > 
> > So I can nowadays just omit this? Thanks!
> 
> For a long time. I haven't checked the date when it started like
> this,
> but couple of years sounds like a good approximation.

Ok. Thanks for pointing it out.


Best Regards
	Matti Vaittinen