| Message ID | 20200508135348.15229-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: buffer: add support for multiple buffers | expand |
On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > [...] > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). > This is because the 'buffer->dev.parent = &indio_dev->dev'. > But I do feel this is correct. > So, now I don't know whether to leave it like that or symlink to shorter > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is > mostly to make the names unique. It would have looked weird to do > '/dev/buffer1' if I would have named the buffer devices 'bufferX'. > > So, now I'm thinking of whether all this is acceptable. > Or what is acceptable? > Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'? > What else should I consider moving forward? > What means forward? > Where did I leave my beer? Looking at how the /dev/ devices are named I think we can provide a name that is different from the dev_name() of the device. Have a look at device_get_devnode() in drivers/base/core.c. We should be able to provide the name for the chardev through the devnode() callback. While we are at this, do we want to move the new devices into an iio subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
On Sat, 9 May 2020 10:52:14 +0200 Lars-Peter Clausen <lars@metafoo.de> wrote: > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > [...] > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). > > This is because the 'buffer->dev.parent = &indio_dev->dev'. > > But I do feel this is correct. > > So, now I don't know whether to leave it like that or symlink to shorter > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is > > mostly to make the names unique. It would have looked weird to do > > '/dev/buffer1' if I would have named the buffer devices 'bufferX'. > > > > So, now I'm thinking of whether all this is acceptable. > > Or what is acceptable? > > Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'? > > What else should I consider moving forward? > > What means forward? > > Where did I leave my beer? > > Looking at how the /dev/ devices are named I think we can provide a name > that is different from the dev_name() of the device. Have a look at > device_get_devnode() in drivers/base/core.c. We should be able to > provide the name for the chardev through the devnode() callback. > > While we are at this, do we want to move the new devices into an iio > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? Possibly on the folder. I can't for the life of me remember why I decided not to do that the first time around - I'll leave it at the mysterious "it may turn out to be harder than you'd think..." Hopefully not ;) Do we want to make the naming a bit more self describing, something like iio/device0:buffer0? Given the legacy interface will be outside the directory anyway, could even do iio/device0/buffer0 with link to iio:device0 iio/device0/buffer1 with no legacy link. Ah, the bikeshedding fun we have ahead of us! I think this set is going to take too much thinking for a Sunday so may take me a little while to do a proper review... + I have a few other side projects I want to hammer on today :) Jonathan
On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > [External] > > On Sat, 9 May 2020 10:52:14 +0200 > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > [...] > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). > > > This is because the 'buffer->dev.parent = &indio_dev->dev'. > > > But I do feel this is correct. > > > So, now I don't know whether to leave it like that or symlink to shorter > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is > > > mostly to make the names unique. It would have looked weird to do > > > '/dev/buffer1' if I would have named the buffer devices 'bufferX'. > > > > > > So, now I'm thinking of whether all this is acceptable. > > > Or what is acceptable? > > > Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'? > > > What else should I consider moving forward? > > > What means forward? > > > Where did I leave my beer? > > > > Looking at how the /dev/ devices are named I think we can provide a name > > that is different from the dev_name() of the device. Have a look at > > device_get_devnode() in drivers/base/core.c. We should be able to > > provide the name for the chardev through the devnode() callback. > > > > While we are at this, do we want to move the new devices into an iio > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > Possibly on the folder. I can't for the life of me remember why I decided > not to do that the first time around - I'll leave it at the > mysterious "it may turn out to be harder than you'd think..." > Hopefully not ;) I was also thinking about the /dev/iio subfolder while doing this. I can copy that from /dev/input They seem to do it already. I don't know how difficult it would be. But it looks like a good precedent. My concern regarding going to use stuff from core [like device_get_devnode()] is that it seems to bypass some layers of kernel. If I do 'git grep device_get_devnode', I get: drivers/base/core.c: name = device_get_devnode(dev, &mode, &uid, &gid, &tmp); drivers/base/core.c: * device_get_devnode - path of device node file drivers/base/core.c:const char *device_get_devnode(struct device *dev, drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, &req.mode, &req.uid, &req.gid, &tmp); drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, NULL, NULL, NULL, &tmp); include/linux/device.h:extern const char *device_get_devnode(struct device *dev, (END) So, basically, most uses of device_get_devnode() are in core code, and I feel that this may be sanctioned somewhere by some core people, if I do it. I could be wrong, but if you disagree, I'll take your word for it. I tried using devtmpfs_create_node() directly, and it worked, but again, doing 'git grep devtmpfs_create_node' drivers/base/base.h:int devtmpfs_create_node(struct device *dev); drivers/base/base.h:static inline int devtmpfs_create_node(struct device *dev) { return 0; } drivers/base/core.c: devtmpfs_create_node(dev); drivers/base/devtmpfs.c:int devtmpfs_create_node(struct device *dev) drivers/s390/char/hmcdrv_dev.c: * See: devtmpfs.c, function devtmpfs_create_node() Most calls are in core, and the one in hmcdrv driver isn't convincing enough to do it. In fact, some may consider it dangerous as it allows drivers/frameworks to use it as precedent to do more name manipulation. Again, if you guys say that this would be acceptable, I can use device_get_devnode() and other stuff from core. > > Do we want to make the naming a bit more self describing, something like > iio/device0:buffer0? Given the legacy interface will be outside > the directory anyway, could even do > > iio/device0/buffer0 with link to iio:device0 > iio/device0/buffer1 with no legacy link. > > Ah, the bikeshedding fun we have ahead of us! This may also depend on how much code it takes to implement these many levels of folder hierarchy. > > I think this set is going to take too much thinking for a Sunday > so may take me a little while to do a proper review... > + I have a few other side projects I want to hammer on today :) > > Jonathan >
On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: >> [External] >> >> On Sat, 9 May 2020 10:52:14 +0200 >> Lars-Peter Clausen <lars@metafoo.de> wrote: >> >>> On 5/8/20 3:53 PM, Alexandru Ardelean wrote: >>>> [...] >>>> What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). >>>> This is because the 'buffer->dev.parent = &indio_dev->dev'. >>>> But I do feel this is correct. >>>> So, now I don't know whether to leave it like that or symlink to shorter >>>> versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. >>>> The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is >>>> mostly to make the names unique. It would have looked weird to do >>>> '/dev/buffer1' if I would have named the buffer devices 'bufferX'. >>>> >>>> So, now I'm thinking of whether all this is acceptable. >>>> Or what is acceptable? >>>> Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'? >>>> What else should I consider moving forward? >>>> What means forward? >>>> Where did I leave my beer? >>> Looking at how the /dev/ devices are named I think we can provide a name >>> that is different from the dev_name() of the device. Have a look at >>> device_get_devnode() in drivers/base/core.c. We should be able to >>> provide the name for the chardev through the devnode() callback. >>> >>> While we are at this, do we want to move the new devices into an iio >>> subfolder? So iio/buffer0:0 instead of iio:buffer0:0? >> Possibly on the folder. I can't for the life of me remember why I decided >> not to do that the first time around - I'll leave it at the >> mysterious "it may turn out to be harder than you'd think..." >> Hopefully not ;) > I was also thinking about the /dev/iio subfolder while doing this. > I can copy that from /dev/input > They seem to do it already. > I don't know how difficult it would be. But it looks like a good precedent. All you have to do is return "iio/..." from the devnode() callback. > > My concern regarding going to use stuff from core [like device_get_devnode()] is > that it seems to bypass some layers of kernel. > If I do 'git grep device_get_devnode', I get: > > drivers/base/core.c: name = device_get_devnode(dev, &mode, &uid, > &gid, &tmp); > drivers/base/core.c: * device_get_devnode - path of device node file > drivers/base/core.c:const char *device_get_devnode(struct device *dev, > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, &req.mode, > &req.uid, &req.gid, &tmp); > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, NULL, NULL, > NULL, &tmp); > include/linux/device.h:extern const char *device_get_devnode(struct device *dev, > (END) > > So, basically, most uses of device_get_devnode() are in core code, and I feel > that this may be sanctioned somewhere by some core people, if I do it. > I could be wrong, but if you disagree, I'll take your word for it. You are not supposed to use the function itself, you should implement the devnode() callback for the IIO bus, which is then used by the core device_get_devnode() function.
On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: > [External] > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > > > [External] > > > > > > On Sat, 9 May 2020 10:52:14 +0200 > > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > > > [...] > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). > > > > > This is because the 'buffer->dev.parent = &indio_dev->dev'. > > > > > But I do feel this is correct. > > > > > So, now I don't know whether to leave it like that or symlink to > > > > > shorter > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > > > > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is > > > > > mostly to make the names unique. It would have looked weird to do > > > > > '/dev/buffer1' if I would have named the buffer devices 'bufferX'. > > > > > > > > > > So, now I'm thinking of whether all this is acceptable. > > > > > Or what is acceptable? > > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'? > > > > > What else should I consider moving forward? > > > > > What means forward? > > > > > Where did I leave my beer? > > > > Looking at how the /dev/ devices are named I think we can provide a name > > > > that is different from the dev_name() of the device. Have a look at > > > > device_get_devnode() in drivers/base/core.c. We should be able to > > > > provide the name for the chardev through the devnode() callback. > > > > > > > > While we are at this, do we want to move the new devices into an iio > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > > Possibly on the folder. I can't for the life of me remember why I decided > > > not to do that the first time around - I'll leave it at the > > > mysterious "it may turn out to be harder than you'd think..." > > > Hopefully not ;) > > I was also thinking about the /dev/iio subfolder while doing this. > > I can copy that from /dev/input > > They seem to do it already. > > I don't know how difficult it would be. But it looks like a good precedent. > > All you have to do is return "iio/..." from the devnode() callback. I admit I did not look closely into drivers/input/input.c before mentioning this as as good precedent. But, I looks like /dev/inpput is a class. While IIO devices are a bus_type devices. Should we start implementing an IIO class? or? > > > My concern regarding going to use stuff from core [like > > device_get_devnode()] is > > that it seems to bypass some layers of kernel. > > If I do 'git grep device_get_devnode', I get: > > > > drivers/base/core.c: name = device_get_devnode(dev, &mode, &uid, > > &gid, &tmp); > > drivers/base/core.c: * device_get_devnode - path of device node file > > drivers/base/core.c:const char *device_get_devnode(struct device *dev, > > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, > > &req.mode, > > &req.uid, &req.gid, &tmp); > > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, NULL, > > NULL, > > NULL, &tmp); > > include/linux/device.h:extern const char *device_get_devnode(struct device > > *dev, > > (END) > > > > So, basically, most uses of device_get_devnode() are in core code, and I > > feel > > that this may be sanctioned somewhere by some core people, if I do it. > > I could be wrong, but if you disagree, I'll take your word for it. > You are not supposed to use the function itself, you should implement > the devnode() callback for the IIO bus, which is then used by the core > device_get_devnode() function. >
On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: > [External] > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: > > [External] > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > > > > [External] > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200 > > > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > > > > [...] > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). > > > > > > This is because the 'buffer->dev.parent = &indio_dev->dev'. > > > > > > But I do feel this is correct. > > > > > > So, now I don't know whether to leave it like that or symlink to > > > > > > shorter > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > > > > > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is > > > > > > mostly to make the names unique. It would have looked weird to do > > > > > > '/dev/buffer1' if I would have named the buffer devices 'bufferX'. > > > > > > > > > > > > So, now I'm thinking of whether all this is acceptable. > > > > > > Or what is acceptable? > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> > > > > > > 'iio:device3/buffer0'? > > > > > > What else should I consider moving forward? > > > > > > What means forward? > > > > > > Where did I leave my beer? > > > > > Looking at how the /dev/ devices are named I think we can provide a > > > > > name > > > > > that is different from the dev_name() of the device. Have a look at > > > > > device_get_devnode() in drivers/base/core.c. We should be able to > > > > > provide the name for the chardev through the devnode() callback. > > > > > > > > > > While we are at this, do we want to move the new devices into an iio > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > > > Possibly on the folder. I can't for the life of me remember why I > > > > decided > > > > not to do that the first time around - I'll leave it at the > > > > mysterious "it may turn out to be harder than you'd think..." > > > > Hopefully not ;) > > > I was also thinking about the /dev/iio subfolder while doing this. > > > I can copy that from /dev/input > > > They seem to do it already. > > > I don't know how difficult it would be. But it looks like a good > > > precedent. > > > > All you have to do is return "iio/..." from the devnode() callback. > > I admit I did not look closely into drivers/input/input.c before mentioning > this > as as good precedent. > > But, I looks like /dev/inpput is a class. > While IIO devices are a bus_type devices. > Should we start implementing an IIO class? or? What I should have highlighted [before] with this, is that there is no devnode() callback for the bus_type [type]. > > > > > My concern regarding going to use stuff from core [like > > > device_get_devnode()] is > > > that it seems to bypass some layers of kernel. > > > If I do 'git grep device_get_devnode', I get: > > > > > > drivers/base/core.c: name = device_get_devnode(dev, &mode, > > > &uid, > > > &gid, &tmp); > > > drivers/base/core.c: * device_get_devnode - path of device node file > > > drivers/base/core.c:const char *device_get_devnode(struct device *dev, > > > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, > > > &req.mode, > > > &req.uid, &req.gid, &tmp); > > > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, NULL, > > > NULL, > > > NULL, &tmp); > > > include/linux/device.h:extern const char *device_get_devnode(struct device > > > *dev, > > > (END) > > > > > > So, basically, most uses of device_get_devnode() are in core code, and I > > > feel > > > that this may be sanctioned somewhere by some core people, if I do it. > > > I could be wrong, but if you disagree, I'll take your word for it. > > You are not supposed to use the function itself, you should implement > > the devnode() callback for the IIO bus, which is then used by the core > > device_get_devnode() function. > >
On 5/11/20 3:24 PM, Ardelean, Alexandru wrote: > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: >> [External] >> >> On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: >>> [External] >>> >>> On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: >>>> On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: >>>>> [External] >>>>> >>>>> On Sat, 9 May 2020 10:52:14 +0200 >>>>> Lars-Peter Clausen <lars@metafoo.de> wrote: >>>>> >>>>>> On 5/8/20 3:53 PM, Alexandru Ardelean wrote: >>>>>>> [...] >>>>>>> What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). >>>>>>> This is because the 'buffer->dev.parent = &indio_dev->dev'. >>>>>>> But I do feel this is correct. >>>>>>> So, now I don't know whether to leave it like that or symlink to >>>>>>> shorter >>>>>>> versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. >>>>>>> The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is >>>>>>> mostly to make the names unique. It would have looked weird to do >>>>>>> '/dev/buffer1' if I would have named the buffer devices 'bufferX'. >>>>>>> >>>>>>> So, now I'm thinking of whether all this is acceptable. >>>>>>> Or what is acceptable? >>>>>>> Should I symlink 'iio:device3/iio:buffer3:0' -> >>>>>>> 'iio:device3/buffer0'? >>>>>>> What else should I consider moving forward? >>>>>>> What means forward? >>>>>>> Where did I leave my beer? >>>>>> Looking at how the /dev/ devices are named I think we can provide a >>>>>> name >>>>>> that is different from the dev_name() of the device. Have a look at >>>>>> device_get_devnode() in drivers/base/core.c. We should be able to >>>>>> provide the name for the chardev through the devnode() callback. >>>>>> >>>>>> While we are at this, do we want to move the new devices into an iio >>>>>> subfolder? So iio/buffer0:0 instead of iio:buffer0:0? >>>>> Possibly on the folder. I can't for the life of me remember why I >>>>> decided >>>>> not to do that the first time around - I'll leave it at the >>>>> mysterious "it may turn out to be harder than you'd think..." >>>>> Hopefully not ;) >>>> I was also thinking about the /dev/iio subfolder while doing this. >>>> I can copy that from /dev/input >>>> They seem to do it already. >>>> I don't know how difficult it would be. But it looks like a good >>>> precedent. >>> All you have to do is return "iio/..." from the devnode() callback. >> I admit I did not look closely into drivers/input/input.c before mentioning >> this >> as as good precedent. >> >> But, I looks like /dev/inpput is a class. >> While IIO devices are a bus_type devices. >> Should we start implementing an IIO class? or? > What I should have highlighted [before] with this, is that there is no devnode() > callback for the bus_type [type]. But there is one in device_type :)
On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote: > [External] > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote: > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: > > > [External] > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: > > > > [External] > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > > > > > > [External] > > > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200 > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > > > > > > [...] > > > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). > > > > > > > > This is because the 'buffer->dev.parent = &indio_dev->dev'. > > > > > > > > But I do feel this is correct. > > > > > > > > So, now I don't know whether to leave it like that or symlink to > > > > > > > > shorter > > > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > > > > > > > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' > > > > > > > > is > > > > > > > > mostly to make the names unique. It would have looked weird to > > > > > > > > do > > > > > > > > '/dev/buffer1' if I would have named the buffer devices > > > > > > > > 'bufferX'. > > > > > > > > > > > > > > > > So, now I'm thinking of whether all this is acceptable. > > > > > > > > Or what is acceptable? > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> > > > > > > > > 'iio:device3/buffer0'? > > > > > > > > What else should I consider moving forward? > > > > > > > > What means forward? > > > > > > > > Where did I leave my beer? > > > > > > > Looking at how the /dev/ devices are named I think we can provide > > > > > > > a > > > > > > > name > > > > > > > that is different from the dev_name() of the device. Have a look > > > > > > > at > > > > > > > device_get_devnode() in drivers/base/core.c. We should be able to > > > > > > > provide the name for the chardev through the devnode() callback. > > > > > > > > > > > > > > While we are at this, do we want to move the new devices into an > > > > > > > iio > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > > > > > Possibly on the folder. I can't for the life of me remember why I > > > > > > decided > > > > > > not to do that the first time around - I'll leave it at the > > > > > > mysterious "it may turn out to be harder than you'd think..." > > > > > > Hopefully not ;) > > > > > I was also thinking about the /dev/iio subfolder while doing this. > > > > > I can copy that from /dev/input > > > > > They seem to do it already. > > > > > I don't know how difficult it would be. But it looks like a good > > > > > precedent. > > > > All you have to do is return "iio/..." from the devnode() callback. > > > I admit I did not look closely into drivers/input/input.c before > > > mentioning > > > this > > > as as good precedent. > > > > > > But, I looks like /dev/inpput is a class. > > > While IIO devices are a bus_type devices. > > > Should we start implementing an IIO class? or? > > What I should have highlighted [before] with this, is that there is no > > devnode() > > callback for the bus_type [type]. > But there is one in device_type :) Many thanks :) That worked nicely. I now have: root@analog:~# ls /dev/iio/* /dev/iio/iio:device0 /dev/iio/iio:device1 /dev/iio/device3: buffer0 buffer1 buffer2 buffer3 /dev/iio/device4: buffer0 It looks like I can shift these around as needed. This is just an experiment. I managed to move the iio devices under /dev/iio, though probably the IIO devices will still be around as /dev/iio:deviceX for legacy reasons. Two things remain unresolved. 1. The name of the IIO buffer device. root@analog:/sys/bus/iio/devices# ls iio\:device3/ buffer in_voltage0_test_mode name events in_voltage1_test_mode of_node iio:buffer:3:0 in_voltage_sampling_frequency power iio:buffer:3:1 in_voltage_scale scan_elements iio:buffer:3:2 in_voltage_scale_available subsystem iio:buffer:3:3 in_voltage_test_mode_available uevent Right now, each buffer device is named 'iio:buffer:X:Y'. One suggesttion was 'iio:deviceX:bufferY' I'm suspecting the latter is preferred as when you sort the folders, buffers come right after the iio:deviceX folders in /sys/bus/iio/devices. I don't feel it matters much the device name of the IIO buffer if we symlink it to a shorter form. I'm guessing, we symlink these devices to short-hand 'bufferY' folders in each 'iio:deviceX'? So, you'd get something like: drwxr-xr-x 8 root root 0 May 11 14:40 . drwxr-xr-x 4 root root 0 May 11 14:35 .. lrwxrwxrwx 1 root root 0 May 11 14:35 buffer -> iio:buffer:3:0 lrwxrwxrwx 4 root root 0 May 11 14:35 buffer0 -> iio:buffer:3:0 lrwxrwxrwx 4 root root 0 May 11 14:35 buffer1 -> iio:buffer:3:1 lrwxrwxrwx 4 root root 0 May 11 14:35 buffer2 -> iio:buffer:3:2 lrwxrwxrwx 4 root root 0 May 11 14:35 buffer3 -> iio:buffer:3:3 drwxrwxrwx 2 root root 0 May 11 14:35 events drwxrwxrwx 4 root root 0 May 11 14:35 iio:buffer:3:0 drwxrwxrwx 4 root root 0 May 11 14:35 iio:buffer:3:1 drwxrwxrwx 4 root root 0 May 11 14:35 iio:buffer:3:2 drwxrwxrwx 4 root root 0 May 11 14:35 iio:buffer:3:3 -rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage0_test_mode -rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage1_test_mode -rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage_sampling_frequency -rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage_scale -rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage_scale_available -rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage_test_mode_available -rw-rw-rw- 1 root root 4096 May 11 14:35 name lrwxrwxrwx 1 root root 0 May 11 14:35 of_node -> ../../../../../firmware/devicetree/base/fpga-axi@0/axi-ad9680-hpc@44a10000 drwxrwxrwx 2 root root 0 May 11 14:35 power lrwxrwxrwx 1 root root 0 May 11 14:35 scan_elements -> iio:buffer:3:0/scan_elements lrwxrwxrwx 1 root root 0 May 11 14:35 subsystem -> ../../../../../bus/iio -rw-rw-rw- 1 root root 4096 May 11 14:35 uevent 1a. /sys/bus/iio/devices looks like this: iio:buffer:3:0 (this would become iio:device3:buffer0 ) iio:buffer:3:1 iio:buffer :3:2 iio:buffer:3:3 iio:buffer:4:0 iio:device0 iio:device1 iio:device2 iio:device3 iio: device4 One minor issue here is that the buffers get listed in the /sys/bus/iio/devices folder, because I'm adding them to the iio bus, to be able to get a chardev [from the pre-allocated chardev region of IIO]. libiio gets a little confused, as it sees these buffers are IIO buffer capable devices; iio:buffer:3:0: (buffer capable) 2 channels found: voltage0: (input, index: 0, format: le:S14/16>>0) voltage1: (input, index: 1, format: le:S14/16>>0) 5 device-specific attributes found: attr 0: data_available value: 0 attr 1: enable value: 0 attr 2: length value: 4096 attr 3: length_align_bytes value: 8 attr 4: watermark value: 2048 iio:buffer:3:1: (buffer capable) Hopefully, this is not a big problem, but let's see. 2. I know this is [still] stupid now; but any suggestions one how to symlink /dev/iio:device3 -> /dev/iio/device3/buffer0 ? Regarding this one, I may try a few things, but any suggestion is welcome. Thanks Alex
On 5/11/20 4:56 PM, Ardelean, Alexandru wrote: > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote: >> [External] >> >> On 5/11/20 3:24 PM, Ardelean, Alexandru wrote: >>> On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: >>>> [External] >>>> >>>> On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: >>>>> [External] >>>>> >>>>> On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: >>>>>> On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: >>>>>>> [External] >>>>>>> >>>>>>> On Sat, 9 May 2020 10:52:14 +0200 >>>>>>> Lars-Peter Clausen <lars@metafoo.de> wrote: >>>>>>> >>>>>>>> On 5/8/20 3:53 PM, Alexandru Ardelean wrote: >>>>>>>>> [...] >>>>>>>>> What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). >>>>>>>>> This is because the 'buffer->dev.parent = &indio_dev->dev'. >>>>>>>>> But I do feel this is correct. >>>>>>>>> So, now I don't know whether to leave it like that or symlink to >>>>>>>>> shorter >>>>>>>>> versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. >>>>>>>>> The reason for naming the IIO buffer devices to 'iio:bufferX:Y' >>>>>>>>> is >>>>>>>>> mostly to make the names unique. It would have looked weird to >>>>>>>>> do >>>>>>>>> '/dev/buffer1' if I would have named the buffer devices >>>>>>>>> 'bufferX'. >>>>>>>>> >>>>>>>>> So, now I'm thinking of whether all this is acceptable. >>>>>>>>> Or what is acceptable? >>>>>>>>> Should I symlink 'iio:device3/iio:buffer3:0' -> >>>>>>>>> 'iio:device3/buffer0'? >>>>>>>>> What else should I consider moving forward? >>>>>>>>> What means forward? >>>>>>>>> Where did I leave my beer? >>>>>>>> Looking at how the /dev/ devices are named I think we can provide >>>>>>>> a >>>>>>>> name >>>>>>>> that is different from the dev_name() of the device. Have a look >>>>>>>> at >>>>>>>> device_get_devnode() in drivers/base/core.c. We should be able to >>>>>>>> provide the name for the chardev through the devnode() callback. >>>>>>>> >>>>>>>> While we are at this, do we want to move the new devices into an >>>>>>>> iio >>>>>>>> subfolder? So iio/buffer0:0 instead of iio:buffer0:0? >>>>>>> Possibly on the folder. I can't for the life of me remember why I >>>>>>> decided >>>>>>> not to do that the first time around - I'll leave it at the >>>>>>> mysterious "it may turn out to be harder than you'd think..." >>>>>>> Hopefully not ;) >>>>>> I was also thinking about the /dev/iio subfolder while doing this. >>>>>> I can copy that from /dev/input >>>>>> They seem to do it already. >>>>>> I don't know how difficult it would be. But it looks like a good >>>>>> precedent. >>>>> All you have to do is return "iio/..." from the devnode() callback. >>>> I admit I did not look closely into drivers/input/input.c before >>>> mentioning >>>> this >>>> as as good precedent. >>>> >>>> But, I looks like /dev/inpput is a class. >>>> While IIO devices are a bus_type devices. >>>> Should we start implementing an IIO class? or? >>> What I should have highlighted [before] with this, is that there is no >>> devnode() >>> callback for the bus_type [type]. >> But there is one in device_type :) > Many thanks :) > That worked nicely. > > I now have: > > root@analog:~# ls /dev/iio/* > /dev/iio/iio:device0 /dev/iio/iio:device1 > > /dev/iio/device3: > buffer0 buffer1 buffer2 buffer3 > > /dev/iio/device4: > buffer0 > > > It looks like I can shift these around as needed. > This is just an experiment. > I managed to move the iio devices under /dev/iio, though probably the IIO > devices will still be around as /dev/iio:deviceX for legacy reasons. > > Two things remain unresolved. > 1. The name of the IIO buffer device. > > root@analog:/sys/bus/iio/devices# ls iio\:device3/ > buffer in_voltage0_test_mode name > events in_voltage1_test_mode of_node > iio:buffer:3:0 in_voltage_sampling_frequency power > iio:buffer:3:1 in_voltage_scale scan_elements > iio:buffer:3:2 in_voltage_scale_available subsystem > iio:buffer:3:3 in_voltage_test_mode_available uevent > > > Right now, each buffer device is named 'iio:buffer:X:Y'. > One suggesttion was 'iio:deviceX:bufferY' > I'm suspecting the latter is preferred as when you sort the folders, buffers > come right after the iio:deviceX folders in /sys/bus/iio/devices. > > I don't feel it matters much the device name of the IIO buffer if we symlink it > to a shorter form. > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders in each > 'iio:deviceX'? I think that would be a bit excessive. Only for the legacy buffer we need to have a symlink. > [...] > 2. I know this is [still] stupid now; but any suggestions one how to symlink > /dev/iio:device3 -> /dev/iio/device3/buffer0 ? > Does not seem to be possible. Userspace will have to take care of it. This means we need to keep legacy devices in /dev/ and only new buffers in /dev/iio/.
On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote: > [External] > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote: > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote: > > > [External] > > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote: > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: > > > > > [External] > > > > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: > > > > > > [External] > > > > > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > > > > > > > > [External] > > > > > > > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200 > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > > > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > > > > > > > > [...] > > > > > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to > > > > > > > > > > 3). > > > > > > > > > > This is because the 'buffer->dev.parent = &indio_dev->dev'. > > > > > > > > > > But I do feel this is correct. > > > > > > > > > > So, now I don't know whether to leave it like that or > > > > > > > > > > symlink to > > > > > > > > > > shorter > > > > > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > > > > > > > > > > The reason for naming the IIO buffer devices to > > > > > > > > > > 'iio:bufferX:Y' > > > > > > > > > > is > > > > > > > > > > mostly to make the names unique. It would have looked weird > > > > > > > > > > to > > > > > > > > > > do > > > > > > > > > > '/dev/buffer1' if I would have named the buffer devices > > > > > > > > > > 'bufferX'. > > > > > > > > > > > > > > > > > > > > So, now I'm thinking of whether all this is acceptable. > > > > > > > > > > Or what is acceptable? > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> > > > > > > > > > > 'iio:device3/buffer0'? > > > > > > > > > > What else should I consider moving forward? > > > > > > > > > > What means forward? > > > > > > > > > > Where did I leave my beer? > > > > > > > > > Looking at how the /dev/ devices are named I think we can > > > > > > > > > provide > > > > > > > > > a > > > > > > > > > name > > > > > > > > > that is different from the dev_name() of the device. Have a > > > > > > > > > look > > > > > > > > > at > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should be able > > > > > > > > > to > > > > > > > > > provide the name for the chardev through the devnode() > > > > > > > > > callback. > > > > > > > > > > > > > > > > > > While we are at this, do we want to move the new devices into > > > > > > > > > an > > > > > > > > > iio > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > > > > > > > Possibly on the folder. I can't for the life of me remember why > > > > > > > > I > > > > > > > > decided > > > > > > > > not to do that the first time around - I'll leave it at the > > > > > > > > mysterious "it may turn out to be harder than you'd think..." > > > > > > > > Hopefully not ;) > > > > > > > I was also thinking about the /dev/iio subfolder while doing this. > > > > > > > I can copy that from /dev/input > > > > > > > They seem to do it already. > > > > > > > I don't know how difficult it would be. But it looks like a good > > > > > > > precedent. > > > > > > All you have to do is return "iio/..." from the devnode() callback. > > > > > I admit I did not look closely into drivers/input/input.c before > > > > > mentioning > > > > > this > > > > > as as good precedent. > > > > > > > > > > But, I looks like /dev/inpput is a class. > > > > > While IIO devices are a bus_type devices. > > > > > Should we start implementing an IIO class? or? > > > > What I should have highlighted [before] with this, is that there is no > > > > devnode() > > > > callback for the bus_type [type]. > > > But there is one in device_type :) > > Many thanks :) > > That worked nicely. > > > > I now have: > > > > root@analog:~# ls /dev/iio/* > > /dev/iio/iio:device0 /dev/iio/iio:device1 > > > > /dev/iio/device3: > > buffer0 buffer1 buffer2 buffer3 > > > > /dev/iio/device4: > > buffer0 > > > > > > It looks like I can shift these around as needed. > > This is just an experiment. > > I managed to move the iio devices under /dev/iio, though probably the IIO > > devices will still be around as /dev/iio:deviceX for legacy reasons. > > > > Two things remain unresolved. > > 1. The name of the IIO buffer device. > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/ > > buffer in_voltage0_test_mode name > > events in_voltage1_test_mode of_node > > iio:buffer:3:0 in_voltage_sampling_frequency power > > iio:buffer:3:1 in_voltage_scale scan_elements > > iio:buffer:3:2 in_voltage_scale_available subsystem > > iio:buffer:3:3 in_voltage_test_mode_available uevent > > > > > > Right now, each buffer device is named 'iio:buffer:X:Y'. > > One suggesttion was 'iio:deviceX:bufferY' > > I'm suspecting the latter is preferred as when you sort the folders, buffers > > come right after the iio:deviceX folders in /sys/bus/iio/devices. > > > > I don't feel it matters much the device name of the IIO buffer if we symlink > > it > > to a shorter form. > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders in > > each > > 'iio:deviceX'? > > I think that would be a bit excessive. Only for the legacy buffer we > need to have a symlink. > > > [...] > > 2. I know this is [still] stupid now; but any suggestions one how to symlink > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ? > > > Does not seem to be possible. Userspace will have to take care of it. > This means we need to keep legacy devices in /dev/ and only new buffers > in /dev/iio/. One thought about this, was that we keep the chardev for the IIO device for this. i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer. This means that for a device with 4 buffers, you get 5 chardevs. This also seems a bit much/excessive. Maybe also in terms of source-code. It would at least mean not moving the event-only chardev to 'industrialio- event.c', OR move it, and have the same chardev in 3 places ['industrialio- event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c' Maybe this sort-of makes sense to have for a few years/kernel-revisions until things clean-up. I guess at this point, the maintainer should have the final say about this. > >
On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote: > [External] > > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote: > > [External] > > > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote: > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote: > > > > [External] > > > > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote: > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: > > > > > > [External] > > > > > > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: > > > > > > > [External] > > > > > > > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200 > > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > > > > > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > > > > > > > > > [...] > > > > > > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 > > > > > > > > > > > (to > > > > > > > > > > > 3). > > > > > > > > > > > This is because the 'buffer->dev.parent = &indio_dev- > > > > > > > > > > > >dev'. > > > > > > > > > > > But I do feel this is correct. > > > > > > > > > > > So, now I don't know whether to leave it like that or > > > > > > > > > > > symlink to > > > > > > > > > > > shorter > > > > > > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > > > > > > > > > > > The reason for naming the IIO buffer devices to > > > > > > > > > > > 'iio:bufferX:Y' > > > > > > > > > > > is > > > > > > > > > > > mostly to make the names unique. It would have looked > > > > > > > > > > > weird > > > > > > > > > > > to > > > > > > > > > > > do > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer devices > > > > > > > > > > > 'bufferX'. > > > > > > > > > > > > > > > > > > > > > > So, now I'm thinking of whether all this is acceptable. > > > > > > > > > > > Or what is acceptable? > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> > > > > > > > > > > > 'iio:device3/buffer0'? > > > > > > > > > > > What else should I consider moving forward? > > > > > > > > > > > What means forward? > > > > > > > > > > > Where did I leave my beer? > > > > > > > > > > Looking at how the /dev/ devices are named I think we can > > > > > > > > > > provide > > > > > > > > > > a > > > > > > > > > > name > > > > > > > > > > that is different from the dev_name() of the device. Have a > > > > > > > > > > look > > > > > > > > > > at > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should be > > > > > > > > > > able > > > > > > > > > > to > > > > > > > > > > provide the name for the chardev through the devnode() > > > > > > > > > > callback. > > > > > > > > > > > > > > > > > > > > While we are at this, do we want to move the new devices > > > > > > > > > > into > > > > > > > > > > an > > > > > > > > > > iio > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > > > > > > > > Possibly on the folder. I can't for the life of me remember > > > > > > > > > why > > > > > > > > > I > > > > > > > > > decided > > > > > > > > > not to do that the first time around - I'll leave it at the > > > > > > > > > mysterious "it may turn out to be harder than you'd think..." > > > > > > > > > Hopefully not ;) > > > > > > > > I was also thinking about the /dev/iio subfolder while doing > > > > > > > > this. > > > > > > > > I can copy that from /dev/input > > > > > > > > They seem to do it already. > > > > > > > > I don't know how difficult it would be. But it looks like a good > > > > > > > > precedent. > > > > > > > All you have to do is return "iio/..." from the devnode() > > > > > > > callback. > > > > > > I admit I did not look closely into drivers/input/input.c before > > > > > > mentioning > > > > > > this > > > > > > as as good precedent. > > > > > > > > > > > > But, I looks like /dev/inpput is a class. > > > > > > While IIO devices are a bus_type devices. > > > > > > Should we start implementing an IIO class? or? > > > > > What I should have highlighted [before] with this, is that there is no > > > > > devnode() > > > > > callback for the bus_type [type]. > > > > But there is one in device_type :) > > > Many thanks :) > > > That worked nicely. > > > > > > I now have: > > > > > > root@analog:~# ls /dev/iio/* > > > /dev/iio/iio:device0 /dev/iio/iio:device1 > > > > > > /dev/iio/device3: > > > buffer0 buffer1 buffer2 buffer3 > > > > > > /dev/iio/device4: > > > buffer0 > > > > > > > > > It looks like I can shift these around as needed. > > > This is just an experiment. > > > I managed to move the iio devices under /dev/iio, though probably the IIO > > > devices will still be around as /dev/iio:deviceX for legacy reasons. > > > > > > Two things remain unresolved. > > > 1. The name of the IIO buffer device. > > > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/ > > > buffer in_voltage0_test_mode name > > > events in_voltage1_test_mode of_node > > > iio:buffer:3:0 in_voltage_sampling_frequency power > > > iio:buffer:3:1 in_voltage_scale scan_elements > > > iio:buffer:3:2 in_voltage_scale_available subsystem > > > iio:buffer:3:3 in_voltage_test_mode_available uevent > > > > > > > > > Right now, each buffer device is named 'iio:buffer:X:Y'. > > > One suggesttion was 'iio:deviceX:bufferY' > > > I'm suspecting the latter is preferred as when you sort the folders, > > > buffers > > > come right after the iio:deviceX folders in /sys/bus/iio/devices. > > > > > > I don't feel it matters much the device name of the IIO buffer if we > > > symlink > > > it > > > to a shorter form. > > > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders in > > > each > > > 'iio:deviceX'? > > > > I think that would be a bit excessive. Only for the legacy buffer we > > need to have a symlink. > > > > > [...] > > > 2. I know this is [still] stupid now; but any suggestions one how to > > > symlink > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ? > > > > > Does not seem to be possible. Userspace will have to take care of it. > > This means we need to keep legacy devices in /dev/ and only new buffers > > in /dev/iio/. > > One thought about this, was that we keep the chardev for the IIO device for > this. > i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer. > This means that for a device with 4 buffers, you get 5 chardevs. > This also seems a bit much/excessive. Maybe also in terms of source-code. > It would at least mean not moving the event-only chardev to 'industrialio- > event.c', OR move it, and have the same chardev in 3 places ['industrialio- > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c' > > Maybe this sort-of makes sense to have for a few years/kernel-revisions until > things clean-up. > > I guess at this point, the maintainer should have the final say about this. Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY' thing a feature for new devices, and leave '/dev/iio:deviceX' devices [for buffers] a thing for current devices. It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name [for this yet]. Over time, people can convert existing drivers to the new IIO-buffer format, if they want to. That also gives them a bit better control over symlinking '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse if they want to]. That may create confusion I guess during a transition period. And it would [ideally] have a mechanism [preferably at build/compile time] to notify users to use the new IIO buffer mechanism [vs the old one] when adding new drivers. Otherwise, there's the risk of people copying the old IIO buffer mechanism. This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying. > > >
On Sat, 16 May 2020 13:08:46 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote: > > [External] > > > > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote: > > > [External] > > > > > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote: > > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote: > > > > > [External] > > > > > > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote: > > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: > > > > > > > [External] > > > > > > > > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: > > > > > > > > [External] > > > > > > > > > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200 > > > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > > > > > > > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > > > > > > > > > > [...] > > > > > > > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 > > > > > > > > > > > > (to > > > > > > > > > > > > 3). > > > > > > > > > > > > This is because the 'buffer->dev.parent = &indio_dev- > > > > > > > > > > > > >dev'. > > > > > > > > > > > > But I do feel this is correct. > > > > > > > > > > > > So, now I don't know whether to leave it like that or > > > > > > > > > > > > symlink to > > > > > > > > > > > > shorter > > > > > > > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. > > > > > > > > > > > > The reason for naming the IIO buffer devices to > > > > > > > > > > > > 'iio:bufferX:Y' > > > > > > > > > > > > is > > > > > > > > > > > > mostly to make the names unique. It would have looked > > > > > > > > > > > > weird > > > > > > > > > > > > to > > > > > > > > > > > > do > > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer devices > > > > > > > > > > > > 'bufferX'. > > > > > > > > > > > > > > > > > > > > > > > > So, now I'm thinking of whether all this is acceptable. > > > > > > > > > > > > Or what is acceptable? > > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> > > > > > > > > > > > > 'iio:device3/buffer0'? > > > > > > > > > > > > What else should I consider moving forward? > > > > > > > > > > > > What means forward? > > > > > > > > > > > > Where did I leave my beer? > > > > > > > > > > > Looking at how the /dev/ devices are named I think we can > > > > > > > > > > > provide > > > > > > > > > > > a > > > > > > > > > > > name > > > > > > > > > > > that is different from the dev_name() of the device. Have a > > > > > > > > > > > look > > > > > > > > > > > at > > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should be > > > > > > > > > > > able > > > > > > > > > > > to > > > > > > > > > > > provide the name for the chardev through the devnode() > > > > > > > > > > > callback. > > > > > > > > > > > > > > > > > > > > > > While we are at this, do we want to move the new devices > > > > > > > > > > > into > > > > > > > > > > > an > > > > > > > > > > > iio > > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > > > > > > > > > Possibly on the folder. I can't for the life of me remember > > > > > > > > > > why > > > > > > > > > > I > > > > > > > > > > decided > > > > > > > > > > not to do that the first time around - I'll leave it at the > > > > > > > > > > mysterious "it may turn out to be harder than you'd think..." > > > > > > > > > > Hopefully not ;) > > > > > > > > > I was also thinking about the /dev/iio subfolder while doing > > > > > > > > > this. > > > > > > > > > I can copy that from /dev/input > > > > > > > > > They seem to do it already. > > > > > > > > > I don't know how difficult it would be. But it looks like a good > > > > > > > > > precedent. > > > > > > > > All you have to do is return "iio/..." from the devnode() > > > > > > > > callback. > > > > > > > I admit I did not look closely into drivers/input/input.c before > > > > > > > mentioning > > > > > > > this > > > > > > > as as good precedent. > > > > > > > > > > > > > > But, I looks like /dev/inpput is a class. > > > > > > > While IIO devices are a bus_type devices. > > > > > > > Should we start implementing an IIO class? or? > > > > > > What I should have highlighted [before] with this, is that there is no > > > > > > devnode() > > > > > > callback for the bus_type [type]. > > > > > But there is one in device_type :) > > > > Many thanks :) > > > > That worked nicely. > > > > > > > > I now have: > > > > > > > > root@analog:~# ls /dev/iio/* > > > > /dev/iio/iio:device0 /dev/iio/iio:device1 > > > > > > > > /dev/iio/device3: > > > > buffer0 buffer1 buffer2 buffer3 > > > > > > > > /dev/iio/device4: > > > > buffer0 > > > > > > > > > > > > It looks like I can shift these around as needed. > > > > This is just an experiment. > > > > I managed to move the iio devices under /dev/iio, though probably the IIO > > > > devices will still be around as /dev/iio:deviceX for legacy reasons. > > > > > > > > Two things remain unresolved. > > > > 1. The name of the IIO buffer device. > > > > > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/ > > > > buffer in_voltage0_test_mode name > > > > events in_voltage1_test_mode of_node > > > > iio:buffer:3:0 in_voltage_sampling_frequency power > > > > iio:buffer:3:1 in_voltage_scale scan_elements > > > > iio:buffer:3:2 in_voltage_scale_available subsystem > > > > iio:buffer:3:3 in_voltage_test_mode_available uevent > > > > > > > > > > > > Right now, each buffer device is named 'iio:buffer:X:Y'. > > > > One suggesttion was 'iio:deviceX:bufferY' > > > > I'm suspecting the latter is preferred as when you sort the folders, > > > > buffers > > > > come right after the iio:deviceX folders in /sys/bus/iio/devices. > > > > > > > > I don't feel it matters much the device name of the IIO buffer if we > > > > symlink > > > > it > > > > to a shorter form. > > > > > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders in > > > > each > > > > 'iio:deviceX'? > > > > > > I think that would be a bit excessive. Only for the legacy buffer we > > > need to have a symlink. > > > > > > > [...] > > > > 2. I know this is [still] stupid now; but any suggestions one how to > > > > symlink > > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ? > > > > > > > Does not seem to be possible. Userspace will have to take care of it. > > > This means we need to keep legacy devices in /dev/ and only new buffers > > > in /dev/iio/. > > > > One thought about this, was that we keep the chardev for the IIO device for > > this. > > i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer. > > This means that for a device with 4 buffers, you get 5 chardevs. > > This also seems a bit much/excessive. Maybe also in terms of source-code. > > It would at least mean not moving the event-only chardev to 'industrialio- > > event.c', OR move it, and have the same chardev in 3 places ['industrialio- > > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c' > > > > Maybe this sort-of makes sense to have for a few years/kernel-revisions until > > things clean-up. > > > > I guess at this point, the maintainer should have the final say about this. > > Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY' thing > a feature for new devices, and leave '/dev/iio:deviceX' devices [for buffers] a > thing for current devices. > It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name [for > this yet]. Definitely a no to that. If we make this transition it needs to be automatic and subsystem wide. At some point we could have a kconfig option to disable the legacy interface subsystem wise as a precursor to eventually dropping it. > > Over time, people can convert existing drivers to the new IIO-buffer format, if > they want to. That also gives them a bit better control over symlinking > '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse if > they want to]. > > That may create confusion I guess during a transition period. > And it would [ideally] have a mechanism [preferably at build/compile time] to > notify users to use the new IIO buffer mechanism [vs the old one] when adding > new drivers. > Otherwise, there's the risk of people copying the old IIO buffer mechanism. > This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying. If we can't do this in a transparent fashion we need to rethink. The existing interface 'has' to remain and do something sensible. Realistically we need to keep it in place for 3-5 years at least. I'm not yet convinced the complexity is worthwhile. We 'could' fallback to the same trick used for events and use an ioctl to access all buffers other than the first one... Then we retain one chardev per iio device and still get the flexibility we need to have multiple buffers. In some ways it is tidier, even if a bit less intuitive... If we can't build the symlinks we were all kind of assuming we could we may need to rethink the overall path. Anyhow, you are doing great work exploring the options! Thanks, Jonathan > > > > > > >
On Sat, 2020-05-16 at 17:24 +0100, Jonathan Cameron wrote: > [External] > > On Sat, 16 May 2020 13:08:46 +0000 > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > > > On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote: > > > [External] > > > > > > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote: > > > > [External] > > > > > > > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote: > > > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote: > > > > > > [External] > > > > > > > > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote: > > > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: > > > > > > > > [External] > > > > > > > > > > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > > > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > > > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200 > > > > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > > > > > > > > > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > > > > > > > > > > > [...] > > > > > > > > > > > > > What I don't like, is that iio:device3 has > > > > > > > > > > > > > iio:buffer3:0 > > > > > > > > > > > > > (to > > > > > > > > > > > > > 3). > > > > > > > > > > > > > This is because the 'buffer->dev.parent = > > > > > > > > > > > > > &indio_dev- > > > > > > > > > > > > > > dev'. > > > > > > > > > > > > > But I do feel this is correct. > > > > > > > > > > > > > So, now I don't know whether to leave it like that or > > > > > > > > > > > > > symlink to > > > > > > > > > > > > > shorter > > > > > > > > > > > > > versions like 'iio:buffer3:Y' -> > > > > > > > > > > > > > 'iio:device3/bufferY'. > > > > > > > > > > > > > The reason for naming the IIO buffer devices to > > > > > > > > > > > > > 'iio:bufferX:Y' > > > > > > > > > > > > > is > > > > > > > > > > > > > mostly to make the names unique. It would have looked > > > > > > > > > > > > > weird > > > > > > > > > > > > > to > > > > > > > > > > > > > do > > > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer > > > > > > > > > > > > > devices > > > > > > > > > > > > > 'bufferX'. > > > > > > > > > > > > > > > > > > > > > > > > > > So, now I'm thinking of whether all this is > > > > > > > > > > > > > acceptable. > > > > > > > > > > > > > Or what is acceptable? > > > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> > > > > > > > > > > > > > 'iio:device3/buffer0'? > > > > > > > > > > > > > What else should I consider moving forward? > > > > > > > > > > > > > What means forward? > > > > > > > > > > > > > Where did I leave my beer? > > > > > > > > > > > > Looking at how the /dev/ devices are named I think we > > > > > > > > > > > > can > > > > > > > > > > > > provide > > > > > > > > > > > > a > > > > > > > > > > > > name > > > > > > > > > > > > that is different from the dev_name() of the device. > > > > > > > > > > > > Have a > > > > > > > > > > > > look > > > > > > > > > > > > at > > > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should > > > > > > > > > > > > be > > > > > > > > > > > > able > > > > > > > > > > > > to > > > > > > > > > > > > provide the name for the chardev through the devnode() > > > > > > > > > > > > callback. > > > > > > > > > > > > > > > > > > > > > > > > While we are at this, do we want to move the new devices > > > > > > > > > > > > into > > > > > > > > > > > > an > > > > > > > > > > > > iio > > > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > > > > > > > > > > Possibly on the folder. I can't for the life of me > > > > > > > > > > > remember > > > > > > > > > > > why > > > > > > > > > > > I > > > > > > > > > > > decided > > > > > > > > > > > not to do that the first time around - I'll leave it at > > > > > > > > > > > the > > > > > > > > > > > mysterious "it may turn out to be harder than you'd > > > > > > > > > > > think..." > > > > > > > > > > > Hopefully not ;) > > > > > > > > > > I was also thinking about the /dev/iio subfolder while doing > > > > > > > > > > this. > > > > > > > > > > I can copy that from /dev/input > > > > > > > > > > They seem to do it already. > > > > > > > > > > I don't know how difficult it would be. But it looks like a > > > > > > > > > > good > > > > > > > > > > precedent. > > > > > > > > > All you have to do is return "iio/..." from the devnode() > > > > > > > > > callback. > > > > > > > > I admit I did not look closely into drivers/input/input.c before > > > > > > > > mentioning > > > > > > > > this > > > > > > > > as as good precedent. > > > > > > > > > > > > > > > > But, I looks like /dev/inpput is a class. > > > > > > > > While IIO devices are a bus_type devices. > > > > > > > > Should we start implementing an IIO class? or? > > > > > > > What I should have highlighted [before] with this, is that there > > > > > > > is no > > > > > > > devnode() > > > > > > > callback for the bus_type [type]. > > > > > > But there is one in device_type :) > > > > > Many thanks :) > > > > > That worked nicely. > > > > > > > > > > I now have: > > > > > > > > > > root@analog:~# ls /dev/iio/* > > > > > /dev/iio/iio:device0 /dev/iio/iio:device1 > > > > > > > > > > /dev/iio/device3: > > > > > buffer0 buffer1 buffer2 buffer3 > > > > > > > > > > /dev/iio/device4: > > > > > buffer0 > > > > > > > > > > > > > > > It looks like I can shift these around as needed. > > > > > This is just an experiment. > > > > > I managed to move the iio devices under /dev/iio, though probably the > > > > > IIO > > > > > devices will still be around as /dev/iio:deviceX for legacy reasons. > > > > > > > > > > Two things remain unresolved. > > > > > 1. The name of the IIO buffer device. > > > > > > > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/ > > > > > buffer in_voltage0_test_mode name > > > > > events in_voltage1_test_mode of_node > > > > > iio:buffer:3:0 in_voltage_sampling_frequency power > > > > > iio:buffer:3:1 in_voltage_scale scan_elements > > > > > iio:buffer:3:2 in_voltage_scale_available subsystem > > > > > iio:buffer:3:3 in_voltage_test_mode_available uevent > > > > > > > > > > > > > > > Right now, each buffer device is named 'iio:buffer:X:Y'. > > > > > One suggesttion was 'iio:deviceX:bufferY' > > > > > I'm suspecting the latter is preferred as when you sort the folders, > > > > > buffers > > > > > come right after the iio:deviceX folders in /sys/bus/iio/devices. > > > > > > > > > > I don't feel it matters much the device name of the IIO buffer if we > > > > > symlink > > > > > it > > > > > to a shorter form. > > > > > > > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders > > > > > in > > > > > each > > > > > 'iio:deviceX'? > > > > > > > > I think that would be a bit excessive. Only for the legacy buffer we > > > > need to have a symlink. > > > > > > > > > [...] > > > > > 2. I know this is [still] stupid now; but any suggestions one how to > > > > > symlink > > > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ? > > > > > > > > > Does not seem to be possible. Userspace will have to take care of it. > > > > This means we need to keep legacy devices in /dev/ and only new buffers > > > > in /dev/iio/. > > > > > > One thought about this, was that we keep the chardev for the IIO device > > > for > > > this. > > > i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer. > > > This means that for a device with 4 buffers, you get 5 chardevs. > > > This also seems a bit much/excessive. Maybe also in terms of source-code. > > > It would at least mean not moving the event-only chardev to 'industrialio- > > > event.c', OR move it, and have the same chardev in 3 places > > > ['industrialio- > > > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c' > > > > > > Maybe this sort-of makes sense to have for a few years/kernel-revisions > > > until > > > things clean-up. > > > > > > I guess at this point, the maintainer should have the final say about > > > this. > > > > Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY' > > thing > > a feature for new devices, and leave '/dev/iio:deviceX' devices [for > > buffers] a > > thing for current devices. > > It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name > > [for > > this yet]. > > Definitely a no to that. If we make this transition it needs to be > automatic and subsystem wide. At some point we could have a kconfig option > to disable the legacy interface subsystem wise as a precursor to eventually > dropping it. > > > Over time, people can convert existing drivers to the new IIO-buffer format, > > if > > they want to. That also gives them a bit better control over symlinking > > '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse > > if > > they want to]. > > > > That may create confusion I guess during a transition period. > > And it would [ideally] have a mechanism [preferably at build/compile time] > > to > > notify users to use the new IIO buffer mechanism [vs the old one] when > > adding > > new drivers. > > Otherwise, there's the risk of people copying the old IIO buffer mechanism. > > This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying. > > If we can't do this in a transparent fashion we need to rethink. > The existing interface 'has' to remain and do something sensible. > Realistically > we need to keep it in place for 3-5 years at least. > > I'm not yet convinced the complexity is worthwhile. We 'could' fallback to > the same trick used for events and use an ioctl to access all buffers > other than the first one... Then we retain one chardev per iio device > and still get the flexibility we need to have multiple buffers. > In some ways it is tidier, even if a bit less intuitive... > If we can't build the symlinks we were all kind of assuming we could > we may need to rethink the overall path. > > Anyhow, you are doing great work exploring the options! I wonder if you got to read the idea about adding more chardevs. The one where /dev/iio:deviceX & /dev/iio/deviceX/buffer0 open the same buffer object. I'm not sure about any race issues here. The bad-part [I feel] is that we get more duplication on chardev file_operations (open, release, ioctl, etc). We need to re-wrap code-paths so that they open the same buffer. And the number of chardevs per IIO device increases by 1 (for a device with 4 buffers == 4 chardevs + 1 legacy) I kinda like the idea of the /dev/iio/ folder. But I'm not strongly opinionated towards it either. This also allows some /dev/iio/deviceX/eventY chardevs. And some other types of chardevs /dev/iio/deviceX/somethingNewY But, if I think about it, the only benefit of this [over anon inodes for chardevs] is that it allows us to do direct access via cat/echo to the actual chardev of the buffer. Then, there's also the fact that adding more chardevs increases complexity to userspace, so it won't matter much. People would probably prefer some userspace IIO library to do the data read/write. I'm getting the feeling now that the final debathe is 'anon inodes or not' Thoughts here? > > Thanks, > > Jonathan > > > > > > > > > > >
On Sun, 17 May 2020 06:26:17 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Sat, 2020-05-16 at 17:24 +0100, Jonathan Cameron wrote: > > [External] > > > > On Sat, 16 May 2020 13:08:46 +0000 > > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > > > > > On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote: > > > > [External] > > > > > > > > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote: > > > > > [External] > > > > > > > > > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote: > > > > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote: > > > > > > > [External] > > > > > > > > > > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote: > > > > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote: > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote: > > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote: > > > > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote: > > > > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200 > > > > > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote: > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > What I don't like, is that iio:device3 has > > > > > > > > > > > > > > iio:buffer3:0 > > > > > > > > > > > > > > (to > > > > > > > > > > > > > > 3). > > > > > > > > > > > > > > This is because the 'buffer->dev.parent = > > > > > > > > > > > > > > &indio_dev- > > > > > > > > > > > > > > > dev'. > > > > > > > > > > > > > > But I do feel this is correct. > > > > > > > > > > > > > > So, now I don't know whether to leave it like that or > > > > > > > > > > > > > > symlink to > > > > > > > > > > > > > > shorter > > > > > > > > > > > > > > versions like 'iio:buffer3:Y' -> > > > > > > > > > > > > > > 'iio:device3/bufferY'. > > > > > > > > > > > > > > The reason for naming the IIO buffer devices to > > > > > > > > > > > > > > 'iio:bufferX:Y' > > > > > > > > > > > > > > is > > > > > > > > > > > > > > mostly to make the names unique. It would have looked > > > > > > > > > > > > > > weird > > > > > > > > > > > > > > to > > > > > > > > > > > > > > do > > > > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer > > > > > > > > > > > > > > devices > > > > > > > > > > > > > > 'bufferX'. > > > > > > > > > > > > > > > > > > > > > > > > > > > > So, now I'm thinking of whether all this is > > > > > > > > > > > > > > acceptable. > > > > > > > > > > > > > > Or what is acceptable? > > > > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> > > > > > > > > > > > > > > 'iio:device3/buffer0'? > > > > > > > > > > > > > > What else should I consider moving forward? > > > > > > > > > > > > > > What means forward? > > > > > > > > > > > > > > Where did I leave my beer? > > > > > > > > > > > > > Looking at how the /dev/ devices are named I think we > > > > > > > > > > > > > can > > > > > > > > > > > > > provide > > > > > > > > > > > > > a > > > > > > > > > > > > > name > > > > > > > > > > > > > that is different from the dev_name() of the device. > > > > > > > > > > > > > Have a > > > > > > > > > > > > > look > > > > > > > > > > > > > at > > > > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should > > > > > > > > > > > > > be > > > > > > > > > > > > > able > > > > > > > > > > > > > to > > > > > > > > > > > > > provide the name for the chardev through the devnode() > > > > > > > > > > > > > callback. > > > > > > > > > > > > > > > > > > > > > > > > > > While we are at this, do we want to move the new devices > > > > > > > > > > > > > into > > > > > > > > > > > > > an > > > > > > > > > > > > > iio > > > > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0? > > > > > > > > > > > > Possibly on the folder. I can't for the life of me > > > > > > > > > > > > remember > > > > > > > > > > > > why > > > > > > > > > > > > I > > > > > > > > > > > > decided > > > > > > > > > > > > not to do that the first time around - I'll leave it at > > > > > > > > > > > > the > > > > > > > > > > > > mysterious "it may turn out to be harder than you'd > > > > > > > > > > > > think..." > > > > > > > > > > > > Hopefully not ;) > > > > > > > > > > > I was also thinking about the /dev/iio subfolder while doing > > > > > > > > > > > this. > > > > > > > > > > > I can copy that from /dev/input > > > > > > > > > > > They seem to do it already. > > > > > > > > > > > I don't know how difficult it would be. But it looks like a > > > > > > > > > > > good > > > > > > > > > > > precedent. > > > > > > > > > > All you have to do is return "iio/..." from the devnode() > > > > > > > > > > callback. > > > > > > > > > I admit I did not look closely into drivers/input/input.c before > > > > > > > > > mentioning > > > > > > > > > this > > > > > > > > > as as good precedent. > > > > > > > > > > > > > > > > > > But, I looks like /dev/inpput is a class. > > > > > > > > > While IIO devices are a bus_type devices. > > > > > > > > > Should we start implementing an IIO class? or? > > > > > > > > What I should have highlighted [before] with this, is that there > > > > > > > > is no > > > > > > > > devnode() > > > > > > > > callback for the bus_type [type]. > > > > > > > But there is one in device_type :) > > > > > > Many thanks :) > > > > > > That worked nicely. > > > > > > > > > > > > I now have: > > > > > > > > > > > > root@analog:~# ls /dev/iio/* > > > > > > /dev/iio/iio:device0 /dev/iio/iio:device1 > > > > > > > > > > > > /dev/iio/device3: > > > > > > buffer0 buffer1 buffer2 buffer3 > > > > > > > > > > > > /dev/iio/device4: > > > > > > buffer0 > > > > > > > > > > > > > > > > > > It looks like I can shift these around as needed. > > > > > > This is just an experiment. > > > > > > I managed to move the iio devices under /dev/iio, though probably the > > > > > > IIO > > > > > > devices will still be around as /dev/iio:deviceX for legacy reasons. > > > > > > > > > > > > Two things remain unresolved. > > > > > > 1. The name of the IIO buffer device. > > > > > > > > > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/ > > > > > > buffer in_voltage0_test_mode name > > > > > > events in_voltage1_test_mode of_node > > > > > > iio:buffer:3:0 in_voltage_sampling_frequency power > > > > > > iio:buffer:3:1 in_voltage_scale scan_elements > > > > > > iio:buffer:3:2 in_voltage_scale_available subsystem > > > > > > iio:buffer:3:3 in_voltage_test_mode_available uevent > > > > > > > > > > > > > > > > > > Right now, each buffer device is named 'iio:buffer:X:Y'. > > > > > > One suggesttion was 'iio:deviceX:bufferY' > > > > > > I'm suspecting the latter is preferred as when you sort the folders, > > > > > > buffers > > > > > > come right after the iio:deviceX folders in /sys/bus/iio/devices. > > > > > > > > > > > > I don't feel it matters much the device name of the IIO buffer if we > > > > > > symlink > > > > > > it > > > > > > to a shorter form. > > > > > > > > > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders > > > > > > in > > > > > > each > > > > > > 'iio:deviceX'? > > > > > > > > > > I think that would be a bit excessive. Only for the legacy buffer we > > > > > need to have a symlink. > > > > > > > > > > > [...] > > > > > > 2. I know this is [still] stupid now; but any suggestions one how to > > > > > > symlink > > > > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ? > > > > > > > > > > > Does not seem to be possible. Userspace will have to take care of it. > > > > > This means we need to keep legacy devices in /dev/ and only new buffers > > > > > in /dev/iio/. > > > > > > > > One thought about this, was that we keep the chardev for the IIO device > > > > for > > > > this. > > > > i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer. > > > > This means that for a device with 4 buffers, you get 5 chardevs. > > > > This also seems a bit much/excessive. Maybe also in terms of source-code. > > > > It would at least mean not moving the event-only chardev to 'industrialio- > > > > event.c', OR move it, and have the same chardev in 3 places > > > > ['industrialio- > > > > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c' > > > > > > > > Maybe this sort-of makes sense to have for a few years/kernel-revisions > > > > until > > > > things clean-up. > > > > > > > > I guess at this point, the maintainer should have the final say about > > > > this. > > > > > > Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY' > > > thing > > > a feature for new devices, and leave '/dev/iio:deviceX' devices [for > > > buffers] a > > > thing for current devices. > > > It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name > > > [for > > > this yet]. > > > > Definitely a no to that. If we make this transition it needs to be > > automatic and subsystem wide. At some point we could have a kconfig option > > to disable the legacy interface subsystem wise as a precursor to eventually > > dropping it. > > > > > Over time, people can convert existing drivers to the new IIO-buffer format, > > > if > > > they want to. That also gives them a bit better control over symlinking > > > '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse > > > if > > > they want to]. > > > > > > That may create confusion I guess during a transition period. > > > And it would [ideally] have a mechanism [preferably at build/compile time] > > > to > > > notify users to use the new IIO buffer mechanism [vs the old one] when > > > adding > > > new drivers. > > > Otherwise, there's the risk of people copying the old IIO buffer mechanism. > > > This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying. > > > > If we can't do this in a transparent fashion we need to rethink. > > The existing interface 'has' to remain and do something sensible. > > Realistically > > we need to keep it in place for 3-5 years at least. > > > > I'm not yet convinced the complexity is worthwhile. We 'could' fallback to > > the same trick used for events and use an ioctl to access all buffers > > other than the first one... Then we retain one chardev per iio device > > and still get the flexibility we need to have multiple buffers. > > In some ways it is tidier, even if a bit less intuitive... > > If we can't build the symlinks we were all kind of assuming we could > > we may need to rethink the overall path. > > > > Anyhow, you are doing great work exploring the options! > > I wonder if you got to read the idea about adding more chardevs. > > The one where /dev/iio:deviceX & /dev/iio/deviceX/buffer0 open the same buffer > object. I'm not sure about any race issues here. > The bad-part [I feel] is that we get more duplication on chardev file_operations > (open, release, ioctl, etc). > We need to re-wrap code-paths so that they open the same buffer. > And the number of chardevs per IIO device increases by 1 (for a device with 4 > buffers == 4 chardevs + 1 legacy) I read that one and it strikes me as a hack. Two interfaces to the same thing is always a bad idea if we can possibly avoid it. I was happy enough with a link if that was doable because then they are the same device with same dev-node etc. Though thinking more on that it will be hard to get all the relevant links in place. > > I kinda like the idea of the /dev/iio/ folder. But I'm not strongly opinionated > towards it either. > This also allows some /dev/iio/deviceX/eventY chardevs. > And some other types of chardevs /dev/iio/deviceX/somethingNewY That expansion of chrdevs is kind of what worries me. We end up with more and more of them. > > But, if I think about it, the only benefit of this [over anon inodes for > chardevs] is that it allows us to do direct access via cat/echo to the actual > chardev of the buffer. > Then, there's also the fact that adding more chardevs increases complexity to > userspace, so it won't matter much. People would probably prefer some userspace > IIO library to do the data read/write. > > I'm getting the feeling now that the final debathe is 'anon inodes or not' Yes, I think that is the big question. Without your work to explore the options we wouldn't really know what the other options were. I'm far from sure what the 'right' answer is to this. So there is another corner case we should think about. What do we do about the devices that current expose multiple buffers by having multiple IIO devices? Usually that occurs for devices that can do weird interleaving in hardware fifos - hence can produce some channels at N times the frequency of others - or where we have one shared stream with tagged data. I suspect it's going to be really hard to map those across to the new interface with any sort of backwards compatibility. They expose complete iio devices with all the infrastructure that entails. Maybe we just leave them alone? J > > Thoughts here? > > > > > > Thanks, > > > > Jonathan > > > > > > > > > > > > > > > >
I blew a few fuses trying to get to this combination. And I still don't like this one, but it works. Some patches here [like Lars' patches] are here just make things easier to apply when moving from the ADI tree to the upstream one. For a final patchset I'll need to re-apply/re-test. A working version of this patchset but also with output buffer support is here: https://github.com/analogdevicesinc/linux/tree/adi-iio-buffers-work [ it's based on 4.19 ] It will take a few more patches to bring output support to this tree, but this should add initial support for multiple IIO buffers per device. I haven't tested this patchset, I've tested the one in the link above. I currently don't have [at hand] any devices that would benefit from using multiple buffers per IIO device. Maybe when I get back to the office. Firstly: I need a bit of help in trying to symlink from the kernel files in the /dev folder. I could not find any examples of how this is done [without cracking open some deep kernel/kernfs code], so maybe it's done via userspace? Any suggestions are welcome. I noticed that symlinking via sysfs_create_link() is fine outside of the fs/ folder, so that's what I used. I think I also partially understand why Lars was initially against doing this 'multiple IIO buffers per IIO device'. I've tried various combinations of doing things, and nothing fits perfect, without feeling like I'm doing some sort of mini-frankenstein, just in order to preserve some backwards compatibility. So, to make this work, what I need to do now, is: ------------ root@analog:~# cd /dev/ root@analog:/dev# ln -s iio\:buffer3\:0 iio\:device3 root@analog:/dev# ln -s iio\:buffer4\:0 iio\:device4 root@analog:/dev# ls | grep iio iio:buffer3:0 iio:buffer3:1 iio:buffer3:2 iio:buffer3:3 iio:buffer4:0 iio:device0 iio:device1 iio:device3 iio:device4 ------------ device3 is an ADC with 4 buffers attached, only the first one is currently being used (in legacy mode); the other 3 aren't being used. The IIO buffer devices are named '/dev/iio:bufferX:Y', X being the ID of the IIO device, and Y being the ID of the buffer of device X. These could be renamed to '/dev/iio:deviceX:Y', but that would be confusing I guess. Then ------------ root@analog:/# ls /sys/bus/iio/devices/ iio:buffer3:0 iio:buffer3:1 iio:buffer3:2 iio:buffer3:3 iio:buffer4:0 iio:device0 iio:device1 iio:device2 iio:device3 iio:device4 ------------ root@analog:/# ls /sys/bus/iio/devices/iio:buffer3:0 data_available dev enable length length_align_bytes power scan_elements subsystem uevent watermark ------------ root@analog:/# ls -la /sys/bus/iio/devices/iio\:device3/ total 0 drwxr-xr-x 8 root root 0 May 8 13:00 . drwxr-xr-x 4 root root 0 May 8 12:53 .. lrwxrwxrwx 1 root root 0 May 8 13:00 buffer -> iio:buffer3:0 drwxrwxrwx 2 root root 0 May 8 12:53 events drwxrwxrwx 4 root root 0 May 8 12:53 iio:buffer3:0 drwxrwxrwx 4 root root 0 May 8 12:53 iio:buffer3:1 drwxrwxrwx 4 root root 0 May 8 12:53 iio:buffer3:2 drwxrwxrwx 4 root root 0 May 8 12:53 iio:buffer3:3 -rw-rw-rw- 1 root root 4096 May 8 12:54 in_voltage0_test_mode -rw-rw-rw- 1 root root 4096 May 8 12:54 in_voltage1_test_mode -rw-rw-rw- 1 root root 4096 May 8 12:54 in_voltage_sampling_frequency -rw-rw-rw- 1 root root 4096 May 8 12:53 in_voltage_scale -rw-rw-rw- 1 root root 4096 May 8 12:53 in_voltage_scale_available -rw-rw-rw- 1 root root 4096 May 8 12:53 in_voltage_test_mode_available -rw-rw-rw- 1 root root 4096 May 8 12:53 name lrwxrwxrwx 1 root root 0 May 8 13:00 of_node -> ../../../../../firmware/devicetree/base/fpga-axi@0/axi-ad9680-hpc@44a10000 drwxrwxrwx 2 root root 0 May 8 12:53 power lrwxrwxrwx 1 root root 0 May 8 13:00 scan_elements -> iio:buffer3:0/scan_elements lrwxrwxrwx 1 root root 0 May 8 13:00 subsystem -> ../../../../../bus/iio -rw-rw-rw- 1 root root 4096 May 8 12:53 uevent ------------ root@analog:/# ls -la /sys/bus/iio/devices/iio:device3/iio:buffer3:0/ total 0 drwxrwxrwx 4 root root 0 May 8 12:53 . drwxr-xr-x 8 root root 0 May 8 13:00 .. -rw-rw-rw- 1 root root 4096 May 8 12:53 data_available -rw-rw-rw- 1 root root 4096 May 8 12:53 dev -rw-rw-rw- 1 root root 4096 May 8 12:53 enable -rw-rw-rw- 1 root root 4096 May 8 12:53 length -rw-rw-rw- 1 root root 4096 May 8 12:53 length_align_bytes drwxrwxrwx 2 root root 0 May 8 12:53 power drwxrwxrwx 2 root root 0 May 8 12:53 scan_elements lrwxrwxrwx 1 root root 0 May 8 12:53 subsystem -> ../../../../../../bus/iio -rw-rw-rw- 1 root root 4096 May 8 12:53 uevent -rw-rw-rw- 1 root root 4096 May 8 12:53 watermark ------------ Using our fancy IIO-oscilloscope[1] app with this format, seems to work. [1] https://github.com/analogdevicesinc/iio-oscilloscope Linking the 'scan_elements' folder from the first buffer looks a bit fishy [the code I mean]; it requires that the buffer device be added first, so that the 'scan_elements' be attached via kobject, so that we can have the kobject (as a symlink target). Again, I did not want to crack open the kernfs. What I don't like, is that iio:device3 has iio:buffer3:0 (to 3). This is because the 'buffer->dev.parent = &indio_dev->dev'. But I do feel this is correct. So, now I don't know whether to leave it like that or symlink to shorter versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'. The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is mostly to make the names unique. It would have looked weird to do '/dev/buffer1' if I would have named the buffer devices 'bufferX'. So, now I'm thinking of whether all this is acceptable. Or what is acceptable? Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'? What else should I consider moving forward? What means forward? Where did I leave my beer? Well, if you've found my beer, it means you got this far :) Thanks for reading all of this [including this bit of quarantine humor]. Alexandru Ardelean (12): iio: buffer: add back-ref from iio_buffer to iio_dev iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put() iio: core: register chardev only if needed iio: buffer,event: duplicate chardev creation for buffers & events iio: core: add simple centralized mechanism for ioctl() handlers iio: core: use new common ioctl() mechanism iio: buffer: split buffer sysfs creation to take buffer as primary arg iio: buffer: remove attrcount_orig var from sysfs creation iio: buffer: add underlying device object and convert buffers to devices iio: buffer: symlink the scan_elements dir back into IIO device's dir iio: unpack all iio buffer attributes correctly iio: buffer: convert single buffer to list of buffers Lars-Peter Clausen (2): iio: Move scan mask management to the core iio: hw_consumer: use new scanmask functions drivers/iio/accel/adxl372.c | 6 +- drivers/iio/accel/bmc150-accel-core.c | 6 +- drivers/iio/buffer/industrialio-buffer-cb.c | 16 +- .../buffer/industrialio-buffer-dmaengine.c | 4 +- drivers/iio/buffer/industrialio-hw-consumer.c | 19 +- drivers/iio/iio_core.h | 40 +- drivers/iio/industrialio-buffer.c | 468 ++++++++++++++---- drivers/iio/industrialio-core.c | 174 +++---- drivers/iio/industrialio-event.c | 100 +++- drivers/iio/inkern.c | 15 + include/linux/iio/buffer.h | 3 + include/linux/iio/buffer_impl.h | 24 +- include/linux/iio/consumer.h | 10 + include/linux/iio/iio.h | 12 +- 14 files changed, 688 insertions(+), 209 deletions(-)