Message-ID: <4a04fa3f-c056-4443-a55a-e8622feb1c2a@baylibre.com>
Date: Sun, 18 Jan 2026 14:33:18 -0600
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt1@...il.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: jic23@...nel.org, Jonathan.Cameron@...wei.com, nuno.sa@...log.com,
 andy@...nel.org
Subject: Re: [RFC PATCH v1 0/9] iio: Expand IIO event interface for real-world
 unit handling

On 1/18/26 12:18 PM, Marcelo Schmitt wrote:
> This patch set adjusts and complements the IIO event ABI docs making them
> coherent with the fact that not all threshold value attributes had a _raw/_input
> indicator set in their names. In addition that, the latter patches on this
> series update the IIO event infrastructure to actually enable drivers to provide
> _input threshold value attributes.

I generally like the idea. There were a number of times recently when
adding events where we had to do complex calculation to convert to/from
raw units for the event value because it needed to match the in_..._raw
scaling. In these cases it would have been much more convenient to be able
to use SI units for the event value while keeping raw+scale for the actual
sensor value.

> 
> I'm actually proposing this set just for the sake of solving the event ABI
> naming inconsistency. I recall the event ABI was a bit puzzling to understand
> when I was having a closer look at that a while ago [1].
> [1]: https://lore.kernel.org/linux-iio/cover.1703013352.git.marcelo.schmitt1@gmail.com/
> 
> Also, I'm doing this set entirely on my spare time and just because I wanted
> to have the ABI naming inconsistency fixed. This is also a late reply to [2].
> [2]: https://lore.kernel.org/linux-iio/20251109164312.781de64c@jic23-huawei/
> 
> I've tested this with ad7091r8 driver and result is:
> 
> Before event extension (current IIO teting branch):
> root@...alhost:~# ls /sys/bus/iio/devices/iio:device0/events
> in_voltage0_thresh_either_hysteresis  in_voltage4_thresh_either_hysteresis
> in_voltage0_thresh_falling_en         in_voltage4_thresh_falling_en
> in_voltage0_thresh_falling_value      in_voltage4_thresh_falling_value
> in_voltage0_thresh_rising_en          in_voltage4_thresh_rising_en
> in_voltage0_thresh_rising_value       in_voltage4_thresh_rising_value
> in_voltage1_thresh_either_hysteresis  in_voltage5_thresh_either_hysteresis
> in_voltage1_thresh_falling_en         in_voltage5_thresh_falling_en
> in_voltage1_thresh_falling_value      in_voltage5_thresh_falling_value
> in_voltage1_thresh_rising_en          in_voltage5_thresh_rising_en
> in_voltage1_thresh_rising_value       in_voltage5_thresh_rising_value
> in_voltage2_thresh_either_hysteresis  in_voltage6_thresh_either_hysteresis
> in_voltage2_thresh_falling_en         in_voltage6_thresh_falling_en
> in_voltage2_thresh_falling_value      in_voltage6_thresh_falling_value
> in_voltage2_thresh_rising_en          in_voltage6_thresh_rising_en
> in_voltage2_thresh_rising_value       in_voltage6_thresh_rising_value
> in_voltage3_thresh_either_hysteresis  in_voltage7_thresh_either_hysteresis
> in_voltage3_thresh_falling_en         in_voltage7_thresh_falling_en
> in_voltage3_thresh_falling_value      in_voltage7_thresh_falling_value
> in_voltage3_thresh_rising_en          in_voltage7_thresh_rising_en
> in_voltage3_thresh_rising_value       in_voltage7_thresh_rising_value
> 
> After event extension:
> root@...alhost:~# ls /sys/bus/iio/devices/iio:device0/events
> in_voltage0_raw_thresh_either_hysteresis  in_voltage4_raw_thresh_either_hysteresis
> in_voltage0_raw_thresh_falling_value      in_voltage4_raw_thresh_falling_value
> in_voltage0_raw_thresh_rising_value       in_voltage4_raw_thresh_rising_value
> in_voltage0_thresh_falling_en             in_voltage4_thresh_falling_en
> in_voltage0_thresh_rising_en              in_voltage4_thresh_rising_en
> in_voltage1_raw_thresh_either_hysteresis  in_voltage5_raw_thresh_either_hysteresis
> in_voltage1_raw_thresh_falling_value      in_voltage5_raw_thresh_falling_value
> in_voltage1_raw_thresh_rising_value       in_voltage5_raw_thresh_rising_value
> in_voltage1_thresh_falling_en             in_voltage5_thresh_falling_en
> in_voltage1_thresh_rising_en              in_voltage5_thresh_rising_en
> in_voltage2_raw_thresh_either_hysteresis  in_voltage6_raw_thresh_either_hysteresis
> in_voltage2_raw_thresh_falling_value      in_voltage6_raw_thresh_falling_value
> in_voltage2_raw_thresh_rising_value       in_voltage6_raw_thresh_rising_value
> in_voltage2_thresh_falling_en             in_voltage6_thresh_falling_en
> in_voltage2_thresh_rising_en              in_voltage6_thresh_rising_en
> in_voltage3_raw_thresh_either_hysteresis  in_voltage7_raw_thresh_either_hysteresis
> in_voltage3_raw_thresh_falling_value      in_voltage7_raw_thresh_falling_value
> in_voltage3_raw_thresh_rising_value       in_voltage7_raw_thresh_rising_value
> in_voltage3_thresh_falling_en             in_voltage7_thresh_falling_en
> in_voltage3_thresh_rising_en              in_voltage7_thresh_rising_en

We can't be breaking ABI like this. There are some alternatives though. We
could register duplicate attributes with both the old and the new name (or can
we make the old name a symlink to the new?). Or we could add a 3rd option to the
unit of IIO_EV_UNIT_NONE and use that on existing drivers so that they don't
change the attribute name.

> 
> The difference is the '_raw' element in thresh_(rising|falling|either) attributes.
> 
> Why posting it as an RFC?
> 1) ABI changes a sensitive topic.
> 2) There are 77 drivers that will go through collateral evolution with the event
> interface update. A 77+ patch set is probably not a good idea? I recall the
> claim_direct stuff was split into 2 or more patch sets. This might need a
> similar approach (if accepted).
> 3) My coccinelle semantic patch does a nice job in updating the vast majority
> of the drivers but, it produces diffs longer than needed. E.g.
> 
> @@ -844,7 +844,8 @@ static int adxl313_read_event_value(stru
>  				    enum iio_event_type type,
>  				    enum iio_event_direction dir,
>  				    enum iio_event_info info,
> -				    int *val, int *val2)
> +				    enum iio_event_unit unit, int *val,
> +				    int *val2)
> 
> could be
> 
> @@ -844,6 +844,7 @@ static int adxl313_read_event_value(struct iio_dev *indio_dev,
>  				    enum iio_event_type type,
>  				    enum iio_event_direction dir,
>  				    enum iio_event_info info,
> +				    enum iio_event_unit unit,
>  				    int *val, int *val2)
> 
> I'll try to figure out how to make it generate smaller diffs, or do the changes
> by hand if needed.
> 

Just throwing out an idea here without thinking about it too much...

Instead of adding a new field/parameter for units, could we extend
enum iio_event_info to add IIO_EV_INFO_VALUE_RAW and IIO_EV_INFO_VALUE_INPUT
(and same for HYSTERESIS). Really, the units only make sense for these
two info types anyway.

This would work like my suggestion above that existing drivers would continue
to use the old enum value, but we would encourage using the new enum values
in new code. And it would eliminate the churn of having to touch every existing
user.

And if someone really wants to take advantage of the new naming for a driver
with existing events, we could do the duplicate attribute thing I mentioned
above still.

Powered by blists - more mailing lists