Re: [PATCH v9 6/8] iio: adc: Support ROHM BD79124 ADC

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Mar 21, 2025 at 10:01:00AM +0200, Matti Vaittinen wrote:
> On 20/03/2025 15:16, Andy Shevchenko wrote:
> > On Thu, Mar 20, 2025 at 10:22:00AM +0200, Matti Vaittinen wrote:


...

> > > +struct bd79124_raw {
> > > +	u8 val_bit0_3; /* Is set in high bits of the byte */
> > > +	u8 val_bit4_11;
> > > +};
> > 
> > Again, this is confusing.
> > 
> > Just put a bit order map in the comment as I suggested previously.
> > When I see variable name containing bit range like above I think
> > about the same bit order, i.e. with your comment it makes like this
> > 
> > bit number	7 6 5 4 3 2 1 0
> > data bit	0 1 2 3 x x x x
> 
> Gah. I think I now understand what you're after. And, I agree, I haven't
> been as clear as I could've been.
> 
> The pit numbers in the struct members:
> 	u8 val_bit0_3; and u8 val_bit4_11;
> 
> are _not_ intended to represent the bit ordering - only the bit positions.
> Like, bits from bit 0 to bit 3 are stored in high bits of this u8 - where
> the "0 to 3" was just picked as order based on it being from the smaller to
> greater (which I believe is grammatically typical) - not based on how the
> bits are ordered in the register. If the order of the bits was indeed
> reverted, then we should see much more complex conversions than what is
> presented in these macros.
> 
> I will update the variable names to:
> 
> val_bit3_0; and val_bit11_4; I think it should sort out the confusion. I
> won't go to bit level representation of the full registers:
> 
> > bit number	7 6 5 4 3 2 1 0
> > data bit	3 2 1 0 x x x x
> 
> and
> 
> > bit number	7 6 5 4 3 2 1 0
> > data bit	b a 9 8 7 6 5 4
> 
> because it suggests there is something very strange in the registers (which
> is not the case) - and it is hard to spot if some bits have indeed changed
> the place.

You can get rid of all of these by simply using __le16. I do not understand why
it's not used so far. I thought that bits are mirrored, that may explain the
case, but now I do not see any problem to use __le16 directly.

-- 
With Best Regards,
Andy Shevchenko