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! ~~
