From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Jonathan Cameron <jic23@kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Jagath Jog J <jagathjog1996@gmail.com>,
Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
Cosmin Tanislav <demonsingur@gmail.com>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer
Date: Tue, 18 Oct 2022 14:10:59 +0300 [thread overview]
Message-ID: <e34d8586-a471-81d6-d09c-f2e0d9884628@gmail.com> (raw)
In-Reply-To: <20221014144247.00001eb1@huawei.com>
On 10/14/22 16:42, Jonathan Cameron wrote:
> On Wed, 12 Oct 2022 10:40:38 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> 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! ~~
next prev parent reply other threads:[~2022-10-18 11:11 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-06 14:35 [RFC PATCH v2 0/5] iio: Support ROHM/Kionix kx022a Matti Vaittinen
2022-10-06 14:36 ` [RFC PATCH v2 1/5] regulator: Add devm helpers for get and enable Matti Vaittinen
2022-10-06 16:17 ` Andy Shevchenko
2022-10-09 12:24 ` Jonathan Cameron
2022-10-10 6:11 ` Andy Shevchenko
2022-10-10 9:24 ` Matti Vaittinen
2022-10-14 12:42 ` Jonathan Cameron
2022-10-10 4:13 ` Matti Vaittinen
2022-10-10 6:12 ` Andy Shevchenko
2022-10-06 14:37 ` [RFC PATCH v2 2/5] " Matti Vaittinen
2022-10-06 14:37 ` [RFC PATCH v2 3/5] dt-bindings: iio: Add KX022A accelerometer Matti Vaittinen
2022-10-06 15:23 ` Krzysztof Kozlowski
2022-10-06 15:32 ` Matti Vaittinen
2022-10-09 12:27 ` Jonathan Cameron
2022-10-10 9:28 ` Matti Vaittinen
2022-10-06 14:38 ` [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM " Matti Vaittinen
2022-10-06 18:32 ` Andy Shevchenko
2022-10-07 7:04 ` Joe Perches
2022-10-07 9:22 ` Andy Shevchenko
2022-10-07 9:23 ` Andy Shevchenko
2022-10-09 12:33 ` Jonathan Cameron
2022-10-10 6:15 ` Andy Shevchenko
2022-10-11 9:10 ` Vaittinen, Matti
2022-10-14 13:22 ` Jonathan Cameron
2022-10-18 11:27 ` Matti Vaittinen
2022-10-18 12:42 ` Andy Shevchenko
2022-10-10 9:12 ` Matti Vaittinen
2022-10-10 11:58 ` Andy Shevchenko
2022-10-10 13:20 ` Vaittinen, Matti
2022-10-10 14:09 ` Andy Shevchenko
2022-10-11 6:56 ` Vaittinen, Matti
2022-10-14 13:34 ` Jonathan Cameron
2022-10-12 7:40 ` Matti Vaittinen
2022-10-14 13:42 ` Jonathan Cameron
2022-10-18 11:10 ` Matti Vaittinen [this message]
2022-10-24 16:41 ` Jonathan Cameron
2022-10-06 14:38 ` [RFC PATCH v2 5/5] MAINTAINERS: Add KX022A maintainer entry Matti Vaittinen
2022-10-09 12:38 ` Jonathan Cameron
2022-10-10 9:31 ` Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e34d8586-a471-81d6-d09c-f2e0d9884628@gmail.com \
--to=mazziesaccount@gmail.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=Matti.Vaittinen@fi.rohmeurope.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=demonsingur@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jagathjog1996@gmail.com \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nikita.yoush@cogentembedded.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).