Message-ID: <c7ef9333-c044-4640-b126-7771a1135d87@gmail.com>
Date: Mon, 17 Mar 2025 08:52:09 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: 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 16/03/2025 11:52, Jonathan Cameron 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
> 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

This is close to my usual way, but not quite. I most often do:

PREFIX_REG_REGNAME
PREFIX_MASK_FIELDNAME
PREFIX_FIELDNAME_FIELDVALUE.

Problem with
PREFIX_REGNAME_FIELDNAME_MSK
compared to
PREFIX_MASK_FIELDNAME

tends to be the length of the define. Also, sometimes registers contain 
field(s) which have nothing to do with the register name. Sometimes it 
results confusing definitions. (Confusing meaning that seeing the actual 
read/write makes one to wonder what the register field is supposed to do).

> 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.

I can at least change the MASK to MSK and save one letter :) What comes 
to the ordering, I see you prefer having MSK / REG suffix at the end - 
while I like having it right after the prefix (just because it makes the 
MSK / REG to stay aligned - which in my eyes looks a little bit better). 
So, I'm not sure if I change it to your preference (which may end up 
being more common in IIO if it's what you prefer), or if I keep it the 
way I am used to.

>> ...
>>
>>>>> +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 sure can rebase if the series is not merged before the rc1 is out. I, 
however, rather not delay versions (unless explicitly asked to).

Sometimes postponing things to wait dependencies to get merged backfires 
due to some 'last minute' delays. Hence, I don't usually adapt to new 
APIs until they are in the rc1 or target subsystem's 'for-next' (or 
other suitable) -branch.

> 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,

No problem. You'll pick this when you think it's ready - and I'll rebase 
if new rc1 is out before that (and convert to set_rv() and 
set_multiple_rv() if they are included in the rc1).

If you merge before the set_rv() and set_multiple_rv() are in your tree, 
then a follow-up will be done when they emerge. :)

Yours,
	-- Matti

Powered by blists - more mailing lists