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

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]

Message-ID: <20250316095233.20d1a134@jic23-huawei>
Date: Sun, 16 Mar 2025 09:52:33 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Matti Vaittinen <mazziesaccount@...il.com>, Matti Vaittinen
 <matti.vaittinen@...rohmeurope.com>, Lars-Peter Clausen <lars@...afoo.de>,
 Nuno Sa <nuno.sa@...log.com>, David Lechner <dlechner@...libre.com>, Javier
 Carrasco <javier.carrasco.cruz@...il.com>, Olivier Moysan
 <olivier.moysan@...s.st.com>, Guillaume Stols <gstols@...libre.com>,
 Dumitru Ceclan <mitrutzceclan@...il.com>, Trevor Gamblin
 <tgamblin@...libre.com>, Matteo Martelli <matteomartelli3@...il.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>, João Paulo
 Gonçalves <joao.goncalves@...adex.com>, AngeloGioacchino
 Del Regno <angelogioacchino.delregno@...labora.com>,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v7 06/10] iio: adc: Support ROHM BD79124 ADC

On Fri, 14 Mar 2025 16:33:13 +0200
Andy Shevchenko <andriy.shevchenko@...ux.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
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