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

[Matti Vaittinen]

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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx> 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