Re: [PATCH v4 1/2] iio: imu: inv_icm42600: add WoM support

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 14 Jun 2025 00:02:52 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jun 13, 2025 at 8:14 PM Jean-Baptiste Maneyrol
> <Jean-Baptiste.Maneyrol@tdk.com> wrote:
>  >From: Andy Shevchenko <andriy.shevchenko@intel.com>  
> > >Sent: Friday, June 13, 2025 17:01
> > >On Fri, Jun 13, 2025 at 05:41:47PM +0300, Andy Shevchenko wrote:  
> > >> On Fri, Jun 13, 2025 at 01:43:58PM +0000, Jean-Baptiste Maneyrol wrote:  
> > >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > >> > >Sent: Friday, June 13, 2025 14:54
> > >> > >On Fri, Jun 13, 2025 at 03:53:36PM +0300, Andy Shevchenko wrote:  
> > >> > >> On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:  
> > >> > >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > >> > >> > >Sent: Friday, June 13, 2025 10:29
> > >> > >> > >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:  
> 
> ...
> 
> > >> > >> > >Overall, looking to this patch again, I think it would be better to prepend it
> > >> > >> > >by replacing *int*_t types by the respective uXX ones. Because in this patch
> > >> > >> > >we add dozens of new ones which increases an unneeded churn in the future.
> > >> > >> > >  
> > >> > >> > In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
> > >> > >> > to keep *int*_t types. If it need to be changed, we can change afterward the
> > >> > >> > whole driver types with a replace tool and send it in a separate patch.  
> > >> > >>
> > >> > >> It will be never ending story, sorry. We need someone to solve this tech debt.
> > >> > >> And since this patch adds more than 3 new users of it, I think it's a candidate
> > >> > >> to embrace the burden.  
> > >> > >
> > >> > >For your convenience I can mock-up a change...  
> > >> >
> > >> > It looks like there's something I don't understand in the kernel Documentation about
> > >> > types then.
> > >> > Quoting Documentation/process/coding-style.rst, section 5.d:
> > >> > ---
> > >> > New types which are identical to standard C99 types, in certain exceptional circumstances.
> > >> >
> > >> > Although it would only take a short amount of time for the eyes and brain to become accustomed
> > >> > to the standard types like uint32_t, some people object to their use anyway.
> > >> >
> > >> > Therefore, the Linux-specific u8/u16/u32/u64 types and their signed equivalents which are
> > >> > identical to standard types are permitted -- although they are not mandatory in new code
> > >> > of your own.
> > >> >
> > >> > When editing existing code which already uses one or the other set of types, you should
> > >> > conform to the existing choices in that code.
> > >> > ---
> > >> >
> > >> > My understanding is that uXX are not mandatory for new code. You can use types like *int*_t.
> > >> > But you need to conform afterward to the existing choice. That's why this driver was
> > >> > done initially with *int*_t types, and that patches are conforming to this choice.  
> > >>
> > >> This part of the documentation has a lot of room for different interpretations.
> > >> One [1] may consider this as uXX superior, another, like you, that it's okay
> > >> to use.  In any case Greg KH prefers uXX over uintXX_t. And he is also in
> > >> the chain of maintainers here. Feel free to amend the Documentation. But
> > >> be sure all stakeholders will see your proposal (like Greg KH and other
> > >> key maintainers).
> > >>  
> > >> > By looking at all Linux drivers, there are plenty of them using *int*_t, even
> > >> > inside iio:  
> > >>
> > >> $ git grep -l 'u\?int[0-9][0-9]\?_t' -- drivers/iio/ | wc -l
> > >> 59
> > >>
> > >> $ git ls-files drivers/iio*.c | wc -l
> > >> 640
> > >>
> > >> Less than 10%.
> > >>  
> > >> > Then, why it is mandatory to change this driver to use uXX instead?  
> > >>
> > >> TO be consistent. With the above wording in the documentation I may argue that
> > >> entire subsystem should be consistent and at least in IIO we have tons of patch
> > >> series that are against the whole subsystem to do one style change or another
> > >> (look at the recent memset() vs. {} for initialisation).
> > >>
> > >> [1] https://urldefense.com/v3/__https://lore.kernel.org/all/20250409180953.398686-1-matchstick@neverthere.org/__;!!FtrhtPsWDhZ6tw!DVTvkgDsymM7132dB-wjei-s0JxYiivZxtzEHfWjsrn_6toqTXA__hm2nPUh7jmectCXcP9Z3OAh0hMm-WD6eQAHOtdiGbYQqsw$[lore[.]kernel[.]org]  
> > >
> > >Oh, this [2] is golden!
> > >You may found support for your arguments and for mine in that thread, but the
> > >bottom line is: what do maintainers of IIO prefer? (Taking into account that it
> > >goes via Greg KH)
> > >
> > >[2]: https://urldefense.com/v3/__https://lore.kernel.org/all/20210423230609.13519-1-alx.manpages@gmail.com/__;!!FtrhtPsWDhZ6tw!DVTvkgDsymM7132dB-wjei-s0JxYiivZxtzEHfWjsrn_6toqTXA__hm2nPUh7jmectCXcP9Z3OAh0hMm-WD6eQAHOtdiuFc54eI$[lore[.]kernel[.]org]  
> >
> > If this is required, I can do it. I would just want to know if this is mandatory
> > since we already have a couple of drivers merged using standard types and
> > other drivers planned to be merged.  
> 
> Let's wait for others to speak up. Especially maintainers.
> 
> > Can I do it in the same series or should it be in a separate patch before this
> > series?  
> 
> Same series, just a prerequisite patch. Note, I have one locally, I
> just need to send it and you can use it, but the reason why I haven't
> sent is the same — I want to know the official position of the IIO
> subsystem about this.
> 

I'm generally not keen on taking strong opinions that add a barrier to
code submission / new contributors when there are arguments both ways.
I am getting fussier on style though in my old age - mostly driven by
just how many drivers we now have and the advantages of consistency
of arbitrary decisions.

So I'll state a strong preference for the kernel internal types u8 etc
and would like to see patches converting larger drivers that are
significantly copied for new code over like this one.

Thanks

Jonathan