analogdevicesinc / libiio

A cross platform library for interfacing with local and remote Linux IIO devices
http://analogdevicesinc.github.io/libiio/
GNU Lesser General Public License v2.1
487 stars 316 forks source link

implement support for IIO 'label' field #536

Closed commodo closed 3 years ago

commodo commented 4 years ago

Recently, a 'label' property has been added to IIO devices to resolve the problem of having 2 devices with the same name. The IIO ABI states that the name of the IIO device should be the part-name. In that case, when there's a system with more than one [identical] part, the system shows 2+ devices with the same name.

Since these patches: https://patchwork.kernel.org/patch/11152725/ https://patchwork.kernel.org/patch/11152723/ It is now possible to add a DT 'label' property to identify each device. This is great, since we don't need to do anything in the driver, only the DT.

Now, libiio needs to take this into consideration. This can complicate things a bit, as now, there dev->name, dev->id and now a dev->label woudl appear. But for some usability this should help.

rgetz commented 4 years ago

Thanks - will add this to the 0.21 release.

Do we have any devices that we can test with this?

commodo commented 4 years ago

not yet; but all you do is add a new field in a device tree;

taking the example from here https://github.com/analogdevicesinc/linux/issues/1014


&pmod_spi {
                lo_pll0_tx_adf4351: adf4351-udc-tx-pmod@0 {
                        compatible = "adi,adf4351";
                        reg = <0>;

                        label = "lo_pll0_tx_adf4351";

                };

                lo_pll0_rx_adf4351: adf4351-udc-rx-pmod@1 {
                        compatible = "adi,adf4351";
                        reg = <1>;

                        label = "lo_pll0_rx_adf4351";

                 };
};

we can start adding these fields in all our DTs, as a rework/project; essentially what that does, is a /sys/bus/iio/devices/iio:deviceX/label attribute that reads just like the name attribute; but it's all controllable from DT;

i'll do a quick PR for that; and see; feel free to start proposing DT changes

commodo commented 4 years ago

feel free to start proposing DT changes

if you have some device that is more preferable than the one i mentioned, we can start with that to label it;

mnsgs commented 4 years ago

Hi @rgetz ,

I have a piece of custom hardware (based on multiple ina226s) which I could test upon if you like?

Thanks, Martin

rgetz commented 4 years ago

@mnsgs : Thanks for the offer - I have lots of platforms with many ADI parts on them. That's what I will test/validate on.

mnsgs commented 4 years ago

@rgetz sure, looking forward to the feature in any case though...

rgetz commented 4 years ago

Ok - was starting to plan this:

struct iio_device * iio_context_find_device(const struct iio_context *ctx, const char *name_or_label);

If the name (or ID or label) does not correspond to any known device, NULL is returned, errno is set to ENXIO (No such device or address).
If the name (or ID or label) corresponds to multiple known devices, NULL is returned, errno is set to EMLINK (Too many links).
If the name (or ID or label) corresponds to a single devices, the device structure is returned

right now, if there are multiple matches, it returns the first match. If we think that multiple names are common - this could cause problems...

const char * iio_device_get_name(const struct iio_device *dev);    (exists)
const char * iio_device_get_id(const struct iio_device *dev);      (exists)
const char * iio_device_get_label(const struct iio_device *dev);   (to be added)

That should be all we need? (other than printing it if it exists in iio_info and iio_attr?)

tfcollins commented 4 years ago

I might suggest adding

struct iio_device * iio_context_find_device_by_label(const struct iio_context *ctx, const char *label);
rgetz commented 4 years ago

I might suggest adding

struct iio_device * iio_context_find_device_by_label(const struct iio_context *ctx, const char *label);

that's what iio_context_find_device(ctx, 'label'); does. Today - you can pass in name, or id

(and soon label).

?

commodo commented 4 years ago
If the name (or ID or label) corresponds to multiple known devices, NULL is returned, errno is set to EMLINK (Too many links).

this looks like new behavior; but it's a bit better than the old one; we should account for the fact that maybe this would open a few bugs in other people's code; one way, would also be to add a flags parameter [maybe a iio_context_find_device_with_flags, which would provide a slightly more fine-grained control; like MATCH_DEFAULT, MATCH_BY_LABEL, MATCH_BY_NAME, etc

MATCH_DEFAULT would work like before; so, iio_context_find_device() would actually be iio_context_find_device_with_flags(MATCH_DEFAULT)

or what if we break API a bit? and make it just iio_context_find_device(ctx, name, flags) ? it breaks compilation, but at least signals user to check

another secondary option, is to also in the iio.h file the LIBIIO_VERSION / LIBIIO_API_VERSION macro [which is a numeric]; that way people would start using it in their code to make their code work across API changes;

pcercuei commented 3 years ago

If the label was introduced to avoid conflicting names, could libiio simply consider the label as the device's name, if provided?

nunojsa commented 3 years ago

or what if we break API a bit? and make it just iio_context_find_device(ctx, name, flags) ? it breaks compilation, but at least signals user to check

Hmm, hopefully we won't go by this road. As a user, I would be pretty pissed if I updated some library and afterwards my code does not compile anymore [we should not break our users]...

If the label was introduced to avoid conflicting names, could libiio simply consider the label as the device's name, if provided?

That's a fair point but a label and the device name are two different things in the IIO subsystem so IMO, libiio should reflect what's implemented in the kernel...

Nevertheless, it's nice to see some updates here as I think this is pretty important. We won't need to do stuff like this anymore after this is done...

commodo commented 3 years ago

yeah; i'd also probably implement the label as a new api/feature in libiio and let users make stuff on top of it; the name has been mostly been misused inside the ADI drivers, because it was using DT to add specific names to devices; this also means having to update osc and maybe libm2k/scopy/libad9361 to take into consideration a label first [if it exists] and only then fallback to the name

obviously this means a lot of DT updates on the kernel side [as well];

pcercuei commented 3 years ago

Ok, I'll work on adding iio_device_get_label unless someone is already working on it.

pcercuei commented 3 years ago

Please see https://github.com/analogdevicesinc/libiio/pull/661.

I don't have hardware I can easily test with at the moment (will come soon), please test.