[PATCH 0/2] iio: events.h: add event identifier macros for differential channel

[PATCH 0/2] iio: events.h: add event identifier macros for differential channel

[Julien Stephan]

Hello,

This series adds a a new helper macro, IIO_DIFF_EVENT_CODE, to
specifically create event identifiers for differential channel.

Indeed, currently, there are 3 helper macros in iio/events.h to create
event identifiers:
- IIO_EVENT_CODE : create generic event identifier for differential and non
  differential channels
- IIO_MOD_EVENT_CODE : create event identifier for modified (non
  differential) channels
- IIO_UNMOD_EVENT_CODE : create event identifier for unmodified (non
  differential) channels

For differential channels, drivers are expected to use IIO_EVENT_CODE.
However, only one driver in drivers/iio currently uses it correctly,
leading to inconsistent event identifiers for differential channels that
don’t match the intended attributes (such as max1363.c that supports
differential channels, but only uses IIO_UNMOD_EVENT_CODE).

Adding this new macro to prevent such issues in future drivers.
Only one helper is needed for differential channels since they cannot have
modifiers.

Additionally, the descriptions for IIO_MOD_EVENT_CODE and
IIO_UNMOD_EVENT_CODE have been updated to clarify that they are intended
for non-differential channels,

This series also fix ad7280a driver to use the new helper.
However, the current implementation in ad7280a incorrectly sets both
chan1 and chan2 to 0. To maintain compatibility and avoid breaking
existing user space applications, this behavior is preserved for now.

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
Julien Stephan (2):
      iio: events.h: add event identifier macros for differential channel
      iio: adc: ad7280a: use IIO_DIFF_EVENT_CODE macro helper

 drivers/iio/adc/ad7280a.c  | 14 ++++++--------
 include/linux/iio/events.h | 18 ++++++++++++++++--
 2 files changed, 22 insertions(+), 10 deletions(-)
---
base-commit: 9090ececac9ff1e22fb7e042f3c886990a8fb090
change-id: 20241024-iio-add-macro-for-even-identifier-for-differential-channels-1bd8afdacf42

Best regards,
-- 
Julien Stephan <jstephan@baylibre.com>

[PATCH 1/2] iio: events.h: add event identifier macros for differential channel

[Julien Stephan]

Currently, there are 3 helper macros in iio/events.h to create event
identifiers:
- IIO_EVENT_CODE : create generic event identifier for differential and non
  differential channels
- IIO_MOD_EVENT_CODE : create event identifier for modified (non
  differential) channels
- IIO_UNMOD_EVENT_CODE : create event identifier for unmodified (non
  differential) channels

For differential channels, drivers are expected to use IIO_EVENT_CODE.
However, only one driver in drivers/iio currently uses it correctly,
leading to inconsistent event identifiers for differential channels that
don’t match the intended attributes (such as max1363.c that supports
differential channels, but only uses IIO_UNMOD_EVENT_CODE).

To prevent such issues in future drivers, a new helper macro,
IIO_DIFF_EVENT_CODE, is introduced to specifically create event identifiers
for differential channels. Only one helper is needed for differential
channels since they cannot have modifiers.

Additionally, the descriptions for IIO_MOD_EVENT_CODE and
IIO_UNMOD_EVENT_CODE have been updated to clarify that they are intended
for non-differential channels,

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 include/linux/iio/events.h | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/linux/iio/events.h b/include/linux/iio/events.h
index a4558c45a548834e33702927609ca9ad447c67de..eeaba5e1525e44fd3b51985ffa99837efc6cdd00 100644
--- a/include/linux/iio/events.h
+++ b/include/linux/iio/events.h
@@ -30,7 +30,8 @@
 
 
 /**
- * IIO_MOD_EVENT_CODE() - create event identifier for modified channels
+ * IIO_MOD_EVENT_CODE() - create event identifier for modified (non
+ * differential) channels
  * @chan_type:	Type of the channel. Should be one of enum iio_chan_type.
  * @number:	Channel number.
  * @modifier:	Modifier for the channel. Should be one of enum iio_modifier.
@@ -43,7 +44,8 @@
 	IIO_EVENT_CODE(chan_type, 0, modifier, direction, type, number, 0, 0)
 
 /**
- * IIO_UNMOD_EVENT_CODE() - create event identifier for unmodified channels
+ * IIO_UNMOD_EVENT_CODE() - create event identifier for unmodified (non
+ * differential) channels
  * @chan_type:	Type of the channel. Should be one of enum iio_chan_type.
  * @number:	Channel number.
  * @type:	Type of the event. Should be one of enum iio_event_type.
@@ -53,4 +55,16 @@
 #define IIO_UNMOD_EVENT_CODE(chan_type, number, type, direction)	\
 	IIO_EVENT_CODE(chan_type, 0, 0, direction, type, number, 0, 0)
 
+/**
+ * IIO_DIFF_EVENT_CODE() - create event identifier for differential channels
+ * @chan_type:	Type of the channel. Should be one of enum iio_chan_type.
+ * @chan1:	First channel number for differential channels.
+ * @chan2:	Second channel number for differential channels.
+ * @type:	Type of the event. Should be one of enum iio_event_type.
+ * @direction:	Direction of the event. One of enum iio_event_direction.
+ */
+
+#define IIO_DIFF_EVENT_CODE(chan_type, chan1, chan2, type, direction)	\
+	IIO_EVENT_CODE(chan_type, 1, 0, direction, type, 0, chan1, chan2)
+
 #endif

-- 
2.47.0

[PATCH 2/2] iio: adc: ad7280a: use IIO_DIFF_EVENT_CODE macro helper

[Julien Stephan]

The IIO_DIFF_EVENT_CODE macro helper was introduced to provide a more
specific alternative to the generic IIO_EVENT_CODE macro for handling
differential channels. This commit updates the code to use
IIO_DIFF_EVENT_CODE for better clarity and maintainability.

However, the current implementation incorrectly sets both chan1 and
chan2 to 0. To maintain compatibility and avoid breaking existing
user space applications, this behavior is preserved for now.

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 drivers/iio/adc/ad7280a.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7280a.c b/drivers/iio/adc/ad7280a.c
index 35aa39fe4bde62290996abd3076ac350eedf1052..f9f32737db8079a5b96847442a64fea811438363 100644
--- a/drivers/iio/adc/ad7280a.c
+++ b/drivers/iio/adc/ad7280a.c
@@ -822,17 +822,15 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 		if (FIELD_GET(AD7280A_TRANS_READ_CONV_CHANADDR_MSK, channels[i]) <=
 		    AD7280A_CELL_VOLTAGE_6_REG) {
 			if (val >= st->cell_threshhigh) {
-				u64 tmp = IIO_EVENT_CODE(IIO_VOLTAGE, 1, 0,
-							 IIO_EV_DIR_RISING,
-							 IIO_EV_TYPE_THRESH,
-							 0, 0, 0);
+				u64 tmp = IIO_DIFF_EVENT_CODE(IIO_VOLTAGE, 0, 0,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_RISING);
 				iio_push_event(indio_dev, tmp,
 					       iio_get_time_ns(indio_dev));
 			} else if (val <= st->cell_threshlow) {
-				u64 tmp = IIO_EVENT_CODE(IIO_VOLTAGE, 1, 0,
-							 IIO_EV_DIR_FALLING,
-							 IIO_EV_TYPE_THRESH,
-							 0, 0, 0);
+				u64 tmp = IIO_DIFF_EVENT_CODE(IIO_VOLTAGE, 0, 0,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_FALLING);
 				iio_push_event(indio_dev, tmp,
 					       iio_get_time_ns(indio_dev));
 			}

-- 
2.47.0

Re: [PATCH 0/2] iio: events.h: add event identifier macros for differential channel

[David Lechner]

On 10/28/24 11:38 AM, Julien Stephan wrote:
> Hello,
> 
> This series adds a a new helper macro, IIO_DIFF_EVENT_CODE, to
> specifically create event identifiers for differential channel.
> 
Reviewed-by: David Lechner <dlechner@baylibre.com>

Re: [PATCH 1/2] iio: events.h: add event identifier macros for differential channel

[Jonathan Cameron]

On Mon, 28 Oct 2024 17:38:11 +0100
Julien Stephan <jstephan@baylibre.com> wrote:

> Currently, there are 3 helper macros in iio/events.h to create event
> identifiers:
> - IIO_EVENT_CODE : create generic event identifier for differential and non
>   differential channels
> - IIO_MOD_EVENT_CODE : create event identifier for modified (non
>   differential) channels
> - IIO_UNMOD_EVENT_CODE : create event identifier for unmodified (non
>   differential) channels
> 
> For differential channels, drivers are expected to use IIO_EVENT_CODE.
> However, only one driver in drivers/iio currently uses it correctly,
> leading to inconsistent event identifiers for differential channels that
> don’t match the intended attributes (such as max1363.c that supports
> differential channels, but only uses IIO_UNMOD_EVENT_CODE).

The max1363 is a weird beast IIRC. It's only been about 15 years since I implemented
events :(
When events are enabled it is really fiddly to read the data, so we never
bothered. Mind you, it does indeed seem to set up the differential mode
but not return differential events. oops.

> 
> To prevent such issues in future drivers, a new helper macro,
> IIO_DIFF_EVENT_CODE, is introduced to specifically create event identifiers
> for differential channels. Only one helper is needed for differential
> channels since they cannot have modifiers.
> 
> Additionally, the descriptions for IIO_MOD_EVENT_CODE and
> IIO_UNMOD_EVENT_CODE have been updated to clarify that they are intended
> for non-differential channels,
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
Given comment below doesn't really matter, series applied.

I'm tempted to just say break ABI and fix these. It's a bug
even if a long standing one so a valid reason to cause people
problems if they are checking for wrong event. 

Thanks,

Jonathan

> ---
>  include/linux/iio/events.h | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/iio/events.h b/include/linux/iio/events.h
> index a4558c45a548834e33702927609ca9ad447c67de..eeaba5e1525e44fd3b51985ffa99837efc6cdd00 100644
> --- a/include/linux/iio/events.h
> +++ b/include/linux/iio/events.h
> @@ -30,7 +30,8 @@
>  
>  
>  /**
> - * IIO_MOD_EVENT_CODE() - create event identifier for modified channels
> + * IIO_MOD_EVENT_CODE() - create event identifier for modified (non
> + * differential) channels

Whilst subtle and maybe ok to state here, there is no such thing as a modified
differential channel (because they both use chan2).

So we could not mention it, but I guess it is harmless addition.

>   * @chan_type:	Type of the channel. Should be one of enum iio_chan_type.
>   * @number:	Channel number.
>   * @modifier:	Modifier for the channel. Should be one of enum iio_modifier.
> @@ -43,7 +44,8 @@
>  	IIO_EVENT_CODE(chan_type, 0, modifier, direction, type, number, 0, 0)
>  
>  /**
> - * IIO_UNMOD_EVENT_CODE() - create event identifier for unmodified channels
> + * IIO_UNMOD_EVENT_CODE() - create event identifier for unmodified (non
> + * differential) channels
>   * @chan_type:	Type of the channel. Should be one of enum iio_chan_type.
>   * @number:	Channel number.
>   * @type:	Type of the event. Should be one of enum iio_event_type.
> @@ -53,4 +55,16 @@
>  #define IIO_UNMOD_EVENT_CODE(chan_type, number, type, direction)	\
>  	IIO_EVENT_CODE(chan_type, 0, 0, direction, type, number, 0, 0)
>  
> +/**
> + * IIO_DIFF_EVENT_CODE() - create event identifier for differential channels
> + * @chan_type:	Type of the channel. Should be one of enum iio_chan_type.
> + * @chan1:	First channel number for differential channels.
> + * @chan2:	Second channel number for differential channels.
> + * @type:	Type of the event. Should be one of enum iio_event_type.
> + * @direction:	Direction of the event. One of enum iio_event_direction.
> + */
> +
> +#define IIO_DIFF_EVENT_CODE(chan_type, chan1, chan2, type, direction)	\
> +	IIO_EVENT_CODE(chan_type, 1, 0, direction, type, 0, chan1, chan2)
> +
>  #endif
>