iio: industrialio-core: look for aliases to request device index

  From: Syunya Ohshio <syunya.ohshio@atmark-techno.com>

When using dtb overlays it can be difficult to predict which iio device
will get assigned what index, and there is no easy way to create
symlinks for /sys nodes through udev so to simplify userspace code make
it possible to request fixed indices for iio devices in device tree.

For platforms without device trees of_alias_get_id will just fail and
ida_alloc_range will behave as ida_alloc currently does.

For platforms with device trees, they can not set an alias, for example
this would try to get 10 from the ida for the device corresponding to
adc2:
aliases {
  iio10 = &adc2
};

To: Jonathan Cameron <jic23@kernel.org>
Cc: Guido Günther <agx@sigxcpu.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Syunya Ohshio <syunya.ohshio@atmark-techno.com>
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---

Hello! We are facing an issue on one of our device where iio devices
aren't numbered as we'd like in some situations, and I feel like we
could do better than the only alternative I found of making symlinks
directly to /sys in /dev as e.g.
https://git.toradex.com/cgit/meta-toradex-bsp-common.git/tree/recipes-core/udev/files/verdin-imx8mm/toradex-adc.sh?h=kirkstone-6.x.y

Ultimately we'd just like to able to designate a stable path for our
users to use in their application and tell them it won't change even if
we fiddle with the overlays a bit, which is a problem we had as current
init is done in whatever order device tree nodes are processed, and that
in turn depends on how the overlays are applied.
If you can think of a better way of doing it then we'll be happy to
consider something else.
Otherwise aliases seem like it could do a good job, and isn't too
surprising for users - the main downside I can see would be that it
doesn't help platforms without device trees but I honestly don't see
what would work well in a more generic way -- looking at
/sys/bus/iio/devices/iio:deviceX/name to decide what we're looking at
is a bit of a hassle.

Thanks!


 .../devicetree/bindings/iio/common.yaml         |  9 +++++++--
 drivers/iio/industrialio-core.c                 | 17 ++++++++++++++++-
 2 files changed, 23 insertions(+), 3 deletions(-)
  

On 28/02/2024 06:12, Dominique Martinet wrote:
> From: Syunya Ohshio <syunya.ohshio@atmark-techno.com>
> 
> When using dtb overlays it can be difficult to predict which iio device
> will get assigned what index, and there is no easy way to create
> symlinks for /sys nodes through udev so to simplify userspace code make
> it possible to request fixed indices for iio devices in device tree.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

> 
> For platforms without device trees of_alias_get_id will just fail and
> ida_alloc_range will behave as ida_alloc currently does.
> 
> For platforms with device trees, they can not set an alias, for example
> this would try to get 10 from the ida for the device corresponding to
> adc2:
> aliases {
>   iio10 = &adc2
> };

Sorry, that's why you have labels and compatibles.

Best regards,
Krzysztof
  

Krzysztof Kozlowski wrote on Wed, Feb 28, 2024 at 08:16:03AM +0100:
> On 28/02/2024 06:12, Dominique Martinet wrote:
> > From: Syunya Ohshio <syunya.ohshio@atmark-techno.com>
> > 
> > When using dtb overlays it can be difficult to predict which iio device
> > will get assigned what index, and there is no easy way to create
> > symlinks for /sys nodes through udev so to simplify userspace code make
> > it possible to request fixed indices for iio devices in device tree.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.

Sorry, I assumed that was already the case and didn't think of checking
that from what I was given, I'll fix the prefix to "iio: core: .." in v2

> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.

Hm, I did check that and do not get any warning about the code itself:

$ git show --format=email | ./scripts/checkpatch.pl -q
WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst

total: 0 errors, 1 warnings, 61 lines checked

What are you thinking of?

Regarding the dt binding, I'm not actually changing a binding so I
didn't think of rechecking after adding the note, but I guess it still
ought to be separate; I'll split it in v2.

> > For platforms without device trees of_alias_get_id will just fail and
> > ida_alloc_range will behave as ida_alloc currently does.
> > 
> > For platforms with device trees, they can not set an alias, for example
> > this would try to get 10 from the ida for the device corresponding to
> > adc2:
> > aliases {
> >   iio10 = &adc2
> > };
> 
> Sorry, that's why you have labels and compatibles.

I'm not sure I understand this comment -- would you rather this doesn't
use aliases but instead add a new label (e.g. `iio,index = <10>` or
whatever) to the iio node itself?

Setting up a fixed alias seems to be precisely what aliases are about
(e.g. setting rtc0 will make a specific node become /dev/rtc0, same with
ethernet0, gpio, i2c, mmc, serial...), I'm not sure I agree a new label
would be more appropriate here, but perhaps I'm missing some context?


Thanks,
  

On 28/02/2024 08:31, Dominique Martinet wrote:
> Krzysztof Kozlowski wrote on Wed, Feb 28, 2024 at 08:16:03AM +0100:
>> On 28/02/2024 06:12, Dominique Martinet wrote:
>>> From: Syunya Ohshio <syunya.ohshio@atmark-techno.com>
>>>
>>> When using dtb overlays it can be difficult to predict which iio device
>>> will get assigned what index, and there is no easy way to create
>>> symlinks for /sys nodes through udev so to simplify userspace code make
>>> it possible to request fixed indices for iio devices in device tree.
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
> 
> Sorry, I assumed that was already the case and didn't think of checking
> that from what I was given, I'll fix the prefix to "iio: core: .." in v2
> 
>> Please run scripts/checkpatch.pl and fix reported warnings. Some
>> warnings can be ignored, but the code here looks like it needs a fix.
>> Feel free to get in touch if the warning is not clear.
> 
> Hm, I did check that and do not get any warning about the code itself:
> 
> $ git show --format=email | ./scripts/checkpatch.pl -q
> WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst


> 
> total: 0 errors, 1 warnings, 61 lines checked
> 
> What are you thinking of?

You have warning right there.

> 
> Regarding the dt binding, I'm not actually changing a binding so I
> didn't think of rechecking after adding the note, but I guess it still
> ought to be separate; I'll split it in v2.
> 
>>> For platforms without device trees of_alias_get_id will just fail and
>>> ida_alloc_range will behave as ida_alloc currently does.
>>>
>>> For platforms with device trees, they can not set an alias, for example
>>> this would try to get 10 from the ida for the device corresponding to
>>> adc2:
>>> aliases {
>>>   iio10 = &adc2
>>> };
>>
>> Sorry, that's why you have labels and compatibles.
> 
> I'm not sure I understand this comment -- would you rather this doesn't
> use aliases but instead add a new label (e.g. `iio,index = <10>` or
> whatever) to the iio node itself?

No, the devices already have label property.

> 
> Setting up a fixed alias seems to be precisely what aliases are about
> (e.g. setting rtc0 will make a specific node become /dev/rtc0, same with
> ethernet0, gpio, i2c, mmc, serial...), I'm not sure I agree a new label
> would be more appropriate here, but perhaps I'm missing some context?

Maybe I don't get your point, but your email said "sysfs", so why do you
refer to /dev?

Best regards,
Krzysztof
  

Krzysztof Kozlowski wrote on Wed, Feb 28, 2024 at 08:42:46AM +0100:
> >> Sorry, that's why you have labels and compatibles.
>
> > Setting up a fixed alias seems to be precisely what aliases are about
> > (e.g. setting rtc0 will make a specific node become /dev/rtc0, same with
> > ethernet0, gpio, i2c, mmc, serial...), I'm not sure I agree a new label
> > would be more appropriate here, but perhaps I'm missing some context?
> 
> Maybe I don't get your point, but your email said "sysfs", so why do you
> refer to /dev?

I wrote /dev/rtc0, but it also sets the name in /sys, right?
For example /sys/class/rtc/rtc0

As far as I'm aware iio also creates character devices in /dev with the
same name (/dev/iio/iio:deviceX), but our application doesn't use these
at all and has to? look in /sys directly, so normal udev SYMLINK+=
unfortunately isn't applicable or I wouldn't be bothering with all
this..

> > I'm not sure I understand this comment -- would you rather this doesn't
> > use aliases but instead add a new label (e.g. `iio,index = <10>` or
> > whatever) to the iio node itself?
> 
> No, the devices already have label property.

Thank you for pointing me at the 'label' property, looking at other
subsystems e.g. leds I see paths in sysfs that use labels as I'd like it
to work for iio (/sys/class/leds/<label> and
/sys/devices/platform/<parent>/leds/<label>)

Unfortunately for iio it looks like labels isn't ignored, but instead
create a file in the sysfs directory of the device, e.g. I now have
/sys/bus/iio/devices/iio:device1/label which contains the label string,
so I'm not sure that can be changed easily as that'd be a change of API
for existing users for labels in iio devices?

(I checked briefly and didn't find any, but there seems to be an awful
lot of code in the iio drivers tree about labels so I'm not really
comfortable changing that without some more background on iio first...
Jonathan perhaps has an opinion on this?)


Thanks,
  

On Wed, 28 Feb 2024 17:11:59 +0900
Dominique Martinet <dominique.martinet@atmark-techno.com> wrote:

> Krzysztof Kozlowski wrote on Wed, Feb 28, 2024 at 08:42:46AM +0100:
> > >> Sorry, that's why you have labels and compatibles.  
> >  
> > > Setting up a fixed alias seems to be precisely what aliases are about
> > > (e.g. setting rtc0 will make a specific node become /dev/rtc0, same with
> > > ethernet0, gpio, i2c, mmc, serial...), I'm not sure I agree a new label
> > > would be more appropriate here, but perhaps I'm missing some context?  
> > 
> > Maybe I don't get your point, but your email said "sysfs", so why do you
> > refer to /dev?  
> 
> I wrote /dev/rtc0, but it also sets the name in /sys, right?
> For example /sys/class/rtc/rtc0
> 
> As far as I'm aware iio also creates character devices in /dev with the
> same name (/dev/iio/iio:deviceX), but our application doesn't use these
> at all and has to? look in /sys directly, so normal udev SYMLINK+=
> unfortunately isn't applicable or I wouldn't be bothering with all
> this..

A given IIO device driver may create multiple sysfs directories (registers
device + one or more triggers), so I'm not sure how this would work.

> 
> > > I'm not sure I understand this comment -- would you rather this doesn't
> > > use aliases but instead add a new label (e.g. `iio,index = <10>` or
> > > whatever) to the iio node itself?  
> > 
> > No, the devices already have label property.  
> 
> Thank you for pointing me at the 'label' property, looking at other
> subsystems e.g. leds I see paths in sysfs that use labels as I'd like it
> to work for iio (/sys/class/leds/<label> and
> /sys/devices/platform/<parent>/leds/<label>)
> 
> Unfortunately for iio it looks like labels isn't ignored, but instead
> create a file in the sysfs directory of the device, e.g. I now have
> /sys/bus/iio/devices/iio:device1/label which contains the label string,
> so I'm not sure that can be changed easily as that'd be a change of API
> for existing users for labels in iio devices?

Yes, don't touch that ABI.  IIO software assumes naming of
iio\:deviceX etc.

> 
> (I checked briefly and didn't find any, but there seems to be an awful
> lot of code in the iio drivers tree about labels so I'm not really
> comfortable changing that without some more background on iio first...
> Jonathan perhaps has an opinion on this?)

There are labels for channels as well as devices, but the short description
you have above is it.

I don't see why that isn't sufficient for your use case though?
What does a directory name matter when you can write a few lines of
code to retrieve the IIO device that you want.

If this was day 0 maybe we could support renaming devices like this
but we have a long history of those names not being stable and label
+ parentage of the IIO devices being used to establish stable identification.

Anything beyond a trivial single use script is going to need to deal with
not having stable names (old kernel, dt without alias etc) so this doesn't
help in general.

Jonathan

> 
> 
> Thanks,
  

Thank you for the quick reply,

Jonathan Cameron wrote on Wed, Feb 28, 2024 at 02:24:41PM +0000:
> > > Maybe I don't get your point, but your email said "sysfs", so why do you
> > > refer to /dev?  
> > 
> > I wrote /dev/rtc0, but it also sets the name in /sys, right?
> > For example /sys/class/rtc/rtc0
> > 
> > As far as I'm aware iio also creates character devices in /dev with the
> > same name (/dev/iio/iio:deviceX), but our application doesn't use these
> > at all and has to? look in /sys directly, so normal udev SYMLINK+=
> > unfortunately isn't applicable or I wouldn't be bothering with all
> > this..
> 
> A given IIO device driver may create multiple sysfs directories (registers
> device + one or more triggers), so I'm not sure how this would work.

Thanks for pointing this out, the driver I'm using doesn't seem to
create extra triggers (iio_trigger_alloc doesn't seem to be called), but
the current patch would only affect iio_device_register, so presumably
would have no impact for these extra directories.
(There's also no impact without dt changes, it's only adding an extra
way of fixing the X of iio:deviceX everywhere)

> > Unfortunately for iio it looks like labels isn't ignored, but instead
> > create a file in the sysfs directory of the device, e.g. I now have
> > /sys/bus/iio/devices/iio:device1/label which contains the label string,
> > so I'm not sure that can be changed easily as that'd be a change of API
> > for existing users for labels in iio devices?
> 
> Yes, don't touch that ABI.  IIO software assumes naming of
> iio\:deviceX etc.
> 
> > (I checked briefly and didn't find any, but there seems to be an awful
> > lot of code in the iio drivers tree about labels so I'm not really
> > comfortable changing that without some more background on iio first...
> > Jonathan perhaps has an opinion on this?)
> 
> There are labels for channels as well as devices, but the short description
> you have above is it.
> 
> I don't see why that isn't sufficient for your use case though?

I'll have a lot of trouble arguing that one out as I agree it's "not
that hard" to check the names to get the correct IIO device, but it's
still definitely more work than being able to use fixed names.

For more background, we're selling a device+platform where our users can
write code themselves to read the sensors, with a variable number of
sensors (extension cards can be plugged in offline, reboot and you get
one more).
Adding an extra device currently comes in first and renames all
pre-existing ones because that's apparently the order linux gets them
from the dtb after adding overlays, and it'd "look better" if devices
get added in fixed order so our users don't need to deal with the
checking names/labels logic.

toradex apparently has the same need because they provide a script that
crates ugly links from /dev/xxx-adcY to /sys/.../in_voltageY_raw, so
it's not like we're the first ones to want something like this;
I think however that udev isn't appropriate to create links to /sys, so
having some way of fixing names in dts sounds better to me.

> What does a directory name matter when you can write a few lines of
> code to retrieve the IIO device that you want.
> 
> If this was day 0 maybe we could support renaming devices like this
> but we have a long history of those names not being stable and label
> + parentage of the IIO devices being used to establish stable identification.

I'm sure we can make something work out while preserving compatibility,
the patch I sent might not be great but it wouldn't bother anyone not
using said aliases.

aliases are apparently not appropriate for this (still not sure why?),
but if for example labels are better we could keep the current
iio:deviceX path (/sys/bus/iio/devices/iio:device0) with a label file in
it as current software expect, but add a brand new directory with a link
named as per the label itself (for example /sys/class/iio/<label>;
my understanding is that /sys/class is meant for "easier" links and we
don't have anything iio-related there yet)

That wouldn't break users checking through the existing paths, and
provide a new fixed path for anyone looking for it.

> Anything beyond a trivial single use script is going to need to deal with
> not having stable names (old kernel, dt without alias etc) so this doesn't
> help in general.

I'm sure any major software will have to deal with old kernels, but I
disagree that it doesn't help: in our case described above our users are
basically writing trivial scripts and won't ever be downgrading, we want
to guarantee they can rely on fixed names for our hardware because I
know for sure most won't be bothering to check.

Even for major softwares, any feature written now will hopefully be
considered generally available 20 years from now, we need to start
somewhere.

Thanks,