Hi Jonathan, jic23@xxxxxxxxxx wrote on Sat, 25 Sep 2021 17:08:50 +0100: > On Wed, 22 Sep 2021 11:22:31 +0200 > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote: > > > Hello IIO folks, > > > > I am currently writing a blog post trying to explain the situation that > > I had with the max1027 driver [1] and possibly try to explain the logic > > behind the changes that Jonathan/Nuno requested. As part of this work, I > > tried to understand (and explain?) the meaning of the mode definitions > > and if they were needed or not, but just looking at the code was not > > enough for me to really understand. > > > > While digging into the IIO core, I realized that many definitions and > > helpers had no comments explaining their use. I could not find any > > documentation about the kernel API in general neither (while the > > userspace side is well documented). > > > > I asked yesterday on #linux-iio but got no answers so I am also asking > > here in case there are knowledgeable people willing to explain what > > each of these definition actually mean and how they should be used: > > https://elixir.bootlin.com/linux/latest/source/include/linux/iio/iio.h#L319 > > > > I am ready to send a patch upstream to add the necessary comment > > so that these explanations do not stay on the mailing list only. > > I'll have a go, but definitely would like others to sanity check my explanations > before we add them to the docs. Thanks for offering to do that btw and blog > sounds like a great plan. > > This stuff looks to have bitrotted pretty badly. > > > /* Device operating modes */ > > #define INDIO_DIRECT_MODE 0x01 > > This one simply means there is a sysfs poll mode. Would "sysfs read mode" work for you here? In my mind "poll" means that this is a continuous effort while here I believe we are mainly talking about single shots. > We don't actually check anywhere if it is set for a driver so it's possible there > are drivers out there that should have this set and don't. > That might change in future if we have a reason to start checking. > > It is used internally in the state management to indicate that we are not > currently in one of the buffered modes. That's really just a place holder > though for 'not one one of the other options' Perhaps it would be good to explain these two fields first: * iio_dev->modes This is the list of supported modes by the IIO device. This list should be initialized before registering the IIO device. This list can be filled up by the IIO core depending on the features advertised by the driver during other steps of the registration. * iio_dev->currentmode This is the mode currently in use. It may be checked by device drivers but should be considered read-only by individual drivers, only the core updates this field. > > #define INDIO_BUFFER_TRIGGERED 0x02 > > This is the most common mode when dealing with buffers. It indicates there > is an explicit trigger being used and so we should attach a poll func when > enabling the buffer. Here by "the buffer" I assume you mean "any type of buffer, including a software buffer, eg. an array kmalloc'ed by the core to store the samples"? So here I understand that "_TRIGGERED" in the name means that the core needs to attach a poll function to the trigger handler in order to fill a buffer. > There is a subtlety on this one. If a device supports > both this and BUFFER_SOFTWARE (which typically means there is a FIFO or similar > and hence no explicit per scan trigger is exposed) then the mode setting will > use this if a trigger has been set, but otherwise use the INDIO_BUFFER_HARDWARE > or INDIO_BUFFER_SOFTWARE as appropriate (which normally means INDIO_BUFFER_SOFTWARE) > > > #define INDIO_BUFFER_SOFTWARE 0x04 > > So this case is (now) misleadingly named. It's typically used for > FIFO cases. (Naming is because it used to reflect the presence of a KFIFO as > opposed to directly accessing a hardware FIFO). Basically this means we > get the data in a kfifo but not one scan at a time if watermarks set > to greater than 1. Is this what we usually refer to "continuous mode" in devices datasheets? So the difference with BUFFER_TRIGGERED is that the _TRIG* mode can be enabled at some point in time by an external event while the _SOFT* mode does it more or less continuously and reading the FIFO only gives the last samples that have not yet been overwritten? > > #define INDIO_BUFFER_HARDWARE 0x08 > > An odd corner case similar to INDIO_BUFFER_SOFTWARE but with more restrictions > that exists for some unusual devices in which we are representing a flow of > data we can't actually see from software. Used for some cases were a consumer > driver is actually accessing the data via some back channel. I think we also > use it for DMA buffers because they have very similar restrictions (no DEMUX > in software). I think I get the rough idea but this is still pretty obscure. I believe this one should might not be considered as a real mode, I feel it's more like something that should be advertised by an additional flag because in the end it is a sub-case of INDIO_BUFFER_SOFT*. But I guess we can flag it as being special and not meant to be used in most cases. > What doesn't help is the meaning of this changed over time. A long time back > IIRC it reflected a mode in which userspace was reading directly from fifos > on a device. These days we always route through a kfifo to give us more > flexibility. > > > #define INDIO_EVENT_TRIGGERED 0x10 > There are devices (well 1 anyway) that use triggers to drive capture of > data that only results in events. In that particular case it's a threshold > detector. It uses some of the same intrastructure as INDIO_BUFFER_TRIGGERED. The explanation is clear to me, the name definitely is not :) > > #define INDIO_HARDWARE_TRIGGERED 0x20 > Ah, this one I'd forgotten about. Its stm32 only (so far) and that has > a lot of internal data paths. So we can have triggers that result in data > capture and IIRC can be routed to multiple components. So we wire it > up in a similar fashion to a IIO trigger but none of the management of > interrupts / pollfuncs etc is relevant as it's all hardware mediated and > distributed. I am not sure how relevant it is to have this specific mode in the 'public' definitions list. Most of them are tied to the core's constraints but this one really only is used in the following files: $ git grep INDIO_HARDWARE_TRIGG drivers/iio/adc/stm32-adc.c: indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED; drivers/iio/adc/stm32-dfsdm-adc.c: indio_dev->modes |= INDIO_HARDWARE_TRIGGERED; drivers/iio/trigger/stm32-lptimer-trigger.c: if (indio_dev->modes & INDIO_HARDWARE_TRIGGERED) drivers/iio/trigger/stm32-timer-trigger.c: indio_dev->modes = INDIO_HARDWARE_TRIGGERED; include/linux/iio/iio.h:#define INDIO_HARDWARE_TRIGGERED 0x20 include/linux/iio/iio.h: | INDIO_HARDWARE_TRIGGERED) Maybe we should kill this mode and just use an internal flag specific to the STM32 devices? (or otherwise flat it "STM32" in order to clearly communicate with developers that "this mode is not suitable for your driver"). This is not a mandatory step but while discussing the definition of these fields I feel that maybe we could as well reword a bit the definitions and try to convey their meaning more easily. Of course this is highly possible that I missed some of the key concepts there and I may be proposing something completely wrong. /* Device operating modes */ -#define INDIO_DIRECT_MODE 0x01 -#define INDIO_BUFFER_TRIGGERED 0x02 -#define INDIO_BUFFER_SOFTWARE 0x04 -#define INDIO_BUFFER_HARDWARE 0x08 -#define INDIO_EVENT_TRIGGERED 0x10 +#define INDIO_SINGLE_READ_MODE 0x01 +#define INDIO_SINGLE_TRIGGER_MODE 0x02 +#define INDIO_CONTINUOUS_CAPTURE_MODE 0x04 +#define INDIO_INDIRECT_CONTINUOUS_CAPTURE_MODE 0x08 +#define INDIO_NON_CAPTURE_TRIGGER_MODE 0x10 -#define INDIO_HARDWARE_TRIGGERED 0x20 > I'm not sure how easy it will be to distill that into brief documentation. > > If others want to contribute and point out what I got wrong above that would > be great. This was very much an aspect that evolved rather than conformed > to an original plan unfortunately and so is perhaps less than ideal, but hard > to unwind. I understand. I am regularly working in the MTD subsystem, everything there stayed for legacy and historical reasons, but that's not a reason to keep it that way :-) Thanks, Miquèl
