[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250316100123.4fa32464@jic23-huawei>
Date: Sun, 16 Mar 2025 10:01:23 +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 Sun, 16 Mar 2025 09:52:33 +0000
Jonathan Cameron <jic23@...nel.org> wrote:
> 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
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
Powered by blists - more mailing lists