Re: [PATCH v7 06/10] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 16 Mar 2025 09:52:33 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 14 Mar 2025 16:33:13 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Fri, Mar 14, 2025 at 09:31:58AM +0200, Matti Vaittinen wrote:  
> > > On 13/03/2025 15:19, Andy Shevchenko wrote:    
> > > > On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:    
> > 
> > ...  
> Picking out a few things to comment on...
> >   
> > > > > +#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
> > > > > +#define BD79124_LOW_LIMIT_MIN 0
> > > > > +#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)    
> > > > 
> > > > These masks are not named after registers (or I don't see it clearly),    
> > > 
> > > Naming is hard. I usually try to make a balance between:
> > > 
> > > 1) using names which explain the purpose when seen in the code (at call
> > > site)
> > > 2) keeping names short enough
> > > 3) following the naming convention in the data sheet
> > > 4) maintain relation to the register.
> > > 
> > > I put most emphasis to the 1, while 2 is important to me as well. 3 is
> > > _very_ nice to have, but it is often somehow contradicting with 1 and 2. 4
> > > is IMO just nice to have. The register is usually clearly visible at call
> > > site, and if someone adds new functionality (or checks the correctness of
> > > the masks/registers), then 3 is way more important.
> > > 
> > > I am open to any concrete suggestions though.    
> > 
> > From my point of view the starting point is 3, then one may apply 2 and 4,
> > the 1 may mangle the name so much that register data field name becomes quite
> > abstract.
> >   
> > > > it's
> > > > hard to understand which one relates to which register. Also, why not using
> > > > bitfield.h?    
> > > 
> > > I saw no need for it?    
> > 
> > Hmm... Okay, I think Jonathan will ask that if really needed.
> >   
> 
> I won't as I'm not a huge fan of bitfield.h. In many cases they bloat the code

Oops. You weren't talking about the regmap bitfields.  Ignore this.
This driver is using FIELD_PREP / FIELD_GET.  Maybe should be more extensive?

> and increase the writes that go over the bus.  Don't get me wrong, there
> are good usecases, but it's not a universal thing (unlike PREP_FIELD()
> etc which I love :)
> 
> I do favour burning a few characters to make field / register relationship
> clear though.  A few things can help I think.
> 
> Structuring defines and naming:
> I like using whitespace in subtle ways for this.
> 
> #define PREFIX_REGNAME_REG				0x00
> #define  PREFIX_REGNAME_FIELDNAME_MSK			GENMASK(X, Y)
> #define  PREFIX_REGNAME_FIELDNAME_FILEVALNAME  		0x3
> etc
> 
> Makes it easy to see if we have a mismatch going on
> 
> However, I don't insist on this in all cases as it is one of those
> "don't let perfect be the enemy of good" cases I think.
> 
> So Matti, good to have one last look at the defines and see if they
> can be wrangled into a slightly better form.
> 
> 
> .
> > ...
> >   
> > > > > +static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> > > > > +				    unsigned long *bits)    
> > > > 
> > > > Ditto, .set_multiple_rv().    
> > > 
> > > As you know, this started at v6.14 cycle which is still ongoing. I don't
> > > think .set_rv() and .set_multiple_rv() are available?    
> > 
> > You mean that you are still hope to have this series to be squeezed for
> > v6.15-rc1? That's not me who decides, but if not, those APIs will be part of
> > the v6.15-rc1 (at least they are pending in GPIO subsystem).
> >   
> I'd vote for a rebase on rc1 that should be really easy to for me to pick
> up.   I'd accept a follow up series though.   Ultimately won't affect
> when this series lands as very unlikely Linus will delay the release
> long enough for me to do another pull request this cycle,
> 
> Jonathan