Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen]

On 10/14/22 16:42, Jonathan Cameron wrote:
> On Wed, 12 Oct 2022 10:40:38 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
>
> > On 10/10/22 16:20, Vaittinen, Matti wrote:
> > > On 10/10/22 14:58, Andy Shevchenko wrote:
> > > > On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
> > > > ...
> > > >   
> > > > > > > +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
> > > > > > > +			       sizeof(s16));
> > > >   
> > > > > > No endianess awareness (sizeof __le16 / __be16)
> > > >   
> > > > > > > +	if (ret)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	*val = data->buffer[0];
> > > > > >
> > > > > > Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).
> > > > >
> > > > > I have probably misunderstood something but I don't see why we should use
> > > > > 'endianess awareness' in drivers? I thought the IIO framework code takes
> > > > > care of the endianes conversions based on scan_type so each individual
> > > > > driver does not need to do that. That however has been just my assumption. I
> > > > > will need to check this. Thanks for pointing it out.
> > > >
> > > > The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.
> >
> > Following is some hand waving and speculation after my quick code read.
> > So, I may be utterly wrong in which case please do correct me...
> >
> > Anyways, it seems to me that you're correct. The endianness field is
> > only used by the IIO to build the channel information for user-space so
> > that applications reading data can parse it. As far as I understand, the
> > driver does not need to do the conversions for user-space, but the
> > user-space tools should inspect the type information and do the
> > conversion. I think it makes sense as user-space applications may be
> > better equipped to do some maths. It also may be some applications do
> > not want to spend cycles doing the conversion but the conversions can be
> > done later "offline" for the captured raw data. So omitting conversion
> > in the IIO driver kind of makes sense to me.
>
> That was indeed the original reasonining for buffered data path
> (note the endian marker is for scans only which only apply in buffered
>   / chardev case).

So, in a case where we "push_to_buffers" the data, we can leave the data 
to use the endianess we advertise via endianess info field?

> It's less obvious for the sysfs path as that's inherently slow.
> We could have made this a problem for the IIO core, but we didn't :)

But again, as far as I understood, the user-space is still expected to 
read the sysfs field for "scan_elements/in_accel_<channel>_type"? I 
guess it would be confusing to say "le:s16/16>>0" there while returning 
CPU native endianess values from sysfs files?

> > I haven't thoroughly looked (and I have never used) the in-kernel IIO
> > APIs for getting the data. A quick look at the
> > include/linux/iio/consumer.h allows me to assume the iio_chan_spec can
> > be obtained by the consumer drivers. This should make the endianess
> > information available for the consumer drivers as well. So, again,
> > consumer drivers can parse the raw-format data themself.
>
> yes consumers should be be endian aware if they are using the
> callback buffer route to get the data.  Now you mention it, we
> may well have cases where that isn't handled correctly.
> There are few enough users of that interface that it might well work
> by coincidence rather than design. oops.
>
> > I have this far only used the sysfs and iio_generic_buffer on a
> > little-endian machine so I have had no issues with the little-endian
> > data and I have only observed the code. Hence I can not really say if my
> > reasoning is correct - or if it is how IIO has been designed to operate.
> > But based on my quick study I don't see a need for the IIO driver to do
> > endianess conversion to any other format but what is indicated by
> > scan_type. Specifically for KX022A, the data is already 16B LE when read
> > from the sensor. This is also advertised by scan_type so no conversion
> > should be needed (unless, of course, I am mistaken :]).
>
> Ah. I'd missed that. Data storage should reflect the read back endianness
> and for the read_raw path you need to perform the conversion in driver
> (but not the high perf push to buffers path).

Oh, really? I think it might be confusing to say "le:s16/16>>0" in 
"scan_elements/in_accel_<channel>_type" but return something else from 
the in_accel_<channel>_raw. Especially the "raw" word at the end of the 
file signals the data is in non converted raw format.

I take your word for that if you say this is what the user-space 
expects, it just is not what I did expect. Well, I do very little work 
on the user-space these days ;) Still just to be on safe side - do you 
mean I should convert the data returned from read_raw to the CPU endianess?

> Sure we could probably have handled read_raw in tree as well but we didn't
> and probably too late to sensibly fix that now.  One of many things we'd
> probably do differently if we were starting again.

Well, this is pretty usual story :) Predicting the future is hard. My 
crystal ball ran out of batteries a long ago ;)

Best Regards
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~