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
