[PATCH v2 0/5] iio: hid-sensors-prox: Add support for more channels

[PATCH v2 0/5] iio: hid-sensors-prox: Add support for more channels

[Ricardo Ribalda]

EgisVision 620 provides two additional channels:
- proximity
- attention

Add support for them.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v2 (Thanks Jonathan):
- Create new attention channel type.
- Improve documentation for HID usages.
- Link to v1: https://lore.kernel.org/r/20241024-hpd-v1-0-2a125882f1f8@chromium.org

---
Ricardo Ribalda (5):
      iio: hid-sensors: Add proximity and attention IDs
      iio: hid-sensors-prox: Factor-in hid_sensor_push_data
      iio: Add channel type for attention
      iio: hid-sensors-prox: Make proximity channel indexed
      iio: hid-sensor-prox: Add support for more channels

 Documentation/ABI/testing/sysfs-bus-iio |   7 ++
 drivers/iio/industrialio-core.c         |   1 +
 drivers/iio/light/hid-sensor-prox.c     | 186 +++++++++++++++++---------------
 include/linux/hid-sensor-ids.h          |   2 +
 include/uapi/linux/iio/types.h          |   1 +
 tools/iio/iio_event_monitor.c           |   2 +
 6 files changed, 112 insertions(+), 87 deletions(-)
---
base-commit: c2ee9f594da826bea183ed14f2cc029c719bf4da
change-id: 20241023-hpd-edeb37f1ffc4

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v2 1/5] iio: hid-sensors: Add proximity and attention IDs

[Ricardo Ribalda]

The HID Usage Table at https://usb.org/sites/default/files/hut1_5.pdf
reserves:

- 0x4b2 for Human Proximity Range
Distance between a human and the computer. Default unit of
measure is meters;
https://www.usb.org/sites/default/files/hutrr39b_0.pdf

- 0x4bd for Human Attention Detected
Human-Presence sensors detect the presence of humans in the sensor’s
field-of-view using diverse and evolving technologies. Some presence
sensors are implemented with low resolution video cameras, which can
additionally track a subject’s attention (i.e. if the
user is ‘looking’ at the system with the integrated sensor).
A Human-Presence sensor, providing a Host with the user’s attention
state, allows the Host to optimize its behavior. For example, to
brighten/dim the system display, based on the user’s attention to the
system (potentially prolonging battery life). Default unit is
true/false;
https://www.usb.org/sites/default/files/hutrr107-humanpresenceattention_1.pdf

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 include/linux/hid-sensor-ids.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index 6730ee900ee1..8a03d9696b1c 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -30,6 +30,8 @@
 #define HID_USAGE_SENSOR_PROX                                   0x200011
 #define HID_USAGE_SENSOR_DATA_PRESENCE                          0x2004b0
 #define HID_USAGE_SENSOR_HUMAN_PRESENCE                         0x2004b1
+#define HID_USAGE_SENSOR_HUMAN_PROXIMITY                        0x2004b2
+#define HID_USAGE_SENSOR_HUMAN_ATTENTION                        0x2004bd
 
 /* Pressure (200031) */
 #define HID_USAGE_SENSOR_PRESSURE                               0x200031

-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH v2 2/5] iio: hid-sensors-prox: Factor-in hid_sensor_push_data

[Ricardo Ribalda]

The function is only called from one place and it is a one-liner.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/iio/light/hid-sensor-prox.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
index 26c481d2998c..d38564fe22df 100644
--- a/drivers/iio/light/hid-sensor-prox.c
+++ b/drivers/iio/light/hid-sensor-prox.c
@@ -153,14 +153,6 @@ static const struct iio_info prox_info = {
 	.write_raw = &prox_write_raw,
 };
 
-/* Function to push data to buffer */
-static void hid_sensor_push_data(struct iio_dev *indio_dev, const void *data,
-					int len)
-{
-	dev_dbg(&indio_dev->dev, "hid_sensor_push_data\n");
-	iio_push_to_buffers(indio_dev, data);
-}
-
 /* Callback handler to send event after all samples are received and captured */
 static int prox_proc_event(struct hid_sensor_hub_device *hsdev,
 				unsigned usage_id,
@@ -170,10 +162,10 @@ static int prox_proc_event(struct hid_sensor_hub_device *hsdev,
 	struct prox_state *prox_state = iio_priv(indio_dev);
 
 	dev_dbg(&indio_dev->dev, "prox_proc_event\n");
-	if (atomic_read(&prox_state->common_attributes.data_ready))
-		hid_sensor_push_data(indio_dev,
-				&prox_state->human_presence,
-				sizeof(prox_state->human_presence));
+	if (atomic_read(&prox_state->common_attributes.data_ready)) {
+		dev_dbg(&indio_dev->dev, "hid_sensor_push_data\n");
+		iio_push_to_buffers(indio_dev, &prox_state->human_presence);
+	}
 
 	return 0;
 }

-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH v2 3/5] iio: Add channel type for attention

[Ricardo Ribalda]

Add a new channel type representing if the user's attention state to the
the system. This usually means if the user is looking at the screen or
not.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
 drivers/iio/industrialio-core.c         | 1 +
 include/uapi/linux/iio/types.h          | 1 +
 tools/iio/iio_event_monitor.c           | 2 ++
 4 files changed, 11 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 89943c2d54e8..d5a2f93bd051 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2339,3 +2339,10 @@ KernelVersion:	6.10
 Contact:	linux-iio@vger.kernel.org
 Description:
 		The value of current sense resistor in Ohms.
+
+What:		/sys/.../iio:deviceX/in_attention_raw
+KernelVersion:	6.13
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Boolean value representing the user's attention to the system.
+		This usually means if the user is looking at the screen or not.
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 6a6568d4a2cb..bdfb51275b68 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -95,6 +95,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_DELTA_VELOCITY] = "deltavelocity",
 	[IIO_COLORTEMP] = "colortemp",
 	[IIO_CHROMATICITY] = "chromaticity",
+	[IIO_ATTENTION] = "attention",
 };
 
 static const char * const iio_modifier_names[] = {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index f2e0b2d50e6b..12886d4465e4 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -51,6 +51,7 @@ enum iio_chan_type {
 	IIO_DELTA_VELOCITY,
 	IIO_COLORTEMP,
 	IIO_CHROMATICITY,
+	IIO_ATTENTION,
 };
 
 enum iio_modifier {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index 8073c9e4fe46..ed9a677f1028 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -63,6 +63,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_DELTA_VELOCITY] = "deltavelocity",
 	[IIO_COLORTEMP] = "colortemp",
 	[IIO_CHROMATICITY] = "chromaticity",
+	[IIO_ATTENTION] = "attention",
 };
 
 static const char * const iio_ev_type_text[] = {
@@ -183,6 +184,7 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_DELTA_VELOCITY:
 	case IIO_COLORTEMP:
 	case IIO_CHROMATICITY:
+	case IIO_ATTENTION:
 		break;
 	default:
 		return false;

-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH v2 3/5] iio: Add channel type for attention

[Jonathan Cameron]

On Mon, 28 Oct 2024 10:12:23 +0000
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Add a new channel type representing if the user's attention state to the
> the system. This usually means if the user is looking at the screen or
> not.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
>  drivers/iio/industrialio-core.c         | 1 +
>  include/uapi/linux/iio/types.h          | 1 +
>  tools/iio/iio_event_monitor.c           | 2 ++
>  4 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 89943c2d54e8..d5a2f93bd051 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -2339,3 +2339,10 @@ KernelVersion:	6.10
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		The value of current sense resistor in Ohms.
> +
> +What:		/sys/.../iio:deviceX/in_attention_raw
> +KernelVersion:	6.13
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Boolean value representing the user's attention to the system.
> +		This usually means if the user is looking at the screen or not.

Hmm. I should have thought of this when I replied to suggest a new channel type.
The question is 'units' for a decision.

Last time we hit something like this where processing is used to make a decision
we decided to at least allow for the concept of 'certainty'.

The idea being that smarter sensors would tell us something about how sure they
are that the attention is on the device.
The analogy being with activity detection. See in_activity_walking_input
in Documentation/ABI/testing/sysfs-bus-iio

Do you think that would be appropriate here as well?  For this device
it would take the values 0 and 100 rather than 0 and 1.


> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 6a6568d4a2cb..bdfb51275b68 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -95,6 +95,7 @@ static const char * const iio_chan_type_name_spec[] = {
>  	[IIO_DELTA_VELOCITY] = "deltavelocity",
>  	[IIO_COLORTEMP] = "colortemp",
>  	[IIO_CHROMATICITY] = "chromaticity",
> +	[IIO_ATTENTION] = "attention",
>  };
>  
>  static const char * const iio_modifier_names[] = {
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index f2e0b2d50e6b..12886d4465e4 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -51,6 +51,7 @@ enum iio_chan_type {
>  	IIO_DELTA_VELOCITY,
>  	IIO_COLORTEMP,
>  	IIO_CHROMATICITY,
> +	IIO_ATTENTION,
>  };
>  
>  enum iio_modifier {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index 8073c9e4fe46..ed9a677f1028 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -63,6 +63,7 @@ static const char * const iio_chan_type_name_spec[] = {
>  	[IIO_DELTA_VELOCITY] = "deltavelocity",
>  	[IIO_COLORTEMP] = "colortemp",
>  	[IIO_CHROMATICITY] = "chromaticity",
> +	[IIO_ATTENTION] = "attention",
>  };
>  
>  static const char * const iio_ev_type_text[] = {
> @@ -183,6 +184,7 @@ static bool event_is_known(struct iio_event_data *event)
>  	case IIO_DELTA_VELOCITY:
>  	case IIO_COLORTEMP:
>  	case IIO_CHROMATICITY:
> +	case IIO_ATTENTION:
>  		break;
>  	default:
>  		return false;
> 

Re: [PATCH v2 3/5] iio: Add channel type for attention

[Ricardo Ribalda]

Hi Jonathan

On Mon, 28 Oct 2024 at 21:34, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 28 Oct 2024 10:12:23 +0000
> Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> > Add a new channel type representing if the user's attention state to the
> > the system. This usually means if the user is looking at the screen or
> > not.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
> >  drivers/iio/industrialio-core.c         | 1 +
> >  include/uapi/linux/iio/types.h          | 1 +
> >  tools/iio/iio_event_monitor.c           | 2 ++
> >  4 files changed, 11 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index 89943c2d54e8..d5a2f93bd051 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -2339,3 +2339,10 @@ KernelVersion: 6.10
> >  Contact:     linux-iio@vger.kernel.org
> >  Description:
> >               The value of current sense resistor in Ohms.
> > +
> > +What:                /sys/.../iio:deviceX/in_attention_raw
> > +KernelVersion:       6.13
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Boolean value representing the user's attention to the system.
> > +             This usually means if the user is looking at the screen or not.
>
> Hmm. I should have thought of this when I replied to suggest a new channel type.
> The question is 'units' for a decision.
>
> Last time we hit something like this where processing is used to make a decision
> we decided to at least allow for the concept of 'certainty'.
>
> The idea being that smarter sensors would tell us something about how sure they
> are that the attention is on the device.
> The analogy being with activity detection. See in_activity_walking_input
> in Documentation/ABI/testing/sysfs-bus-iio
>
> Do you think that would be appropriate here as well?  For this device
> it would take the values 0 and 100 rather than 0 and 1.

For the particular device that I want to support, they are giving me a
value of 1 and 0, and the example from usb.org seems to work the same
way (Logical Maximum of 1)
https://www.usb.org/sites/default/files/hutrr107-humanpresenceattention_1.pdf

I have no problem multiplying my value by 100 if you think there will
be a use case for that. It will not have a major performance impact on
the driver.

You decide ;)

>
>
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 6a6568d4a2cb..bdfb51275b68 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -95,6 +95,7 @@ static const char * const iio_chan_type_name_spec[] = {
> >       [IIO_DELTA_VELOCITY] = "deltavelocity",
> >       [IIO_COLORTEMP] = "colortemp",
> >       [IIO_CHROMATICITY] = "chromaticity",
> > +     [IIO_ATTENTION] = "attention",
> >  };
> >
> >  static const char * const iio_modifier_names[] = {
> > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > index f2e0b2d50e6b..12886d4465e4 100644
> > --- a/include/uapi/linux/iio/types.h
> > +++ b/include/uapi/linux/iio/types.h
> > @@ -51,6 +51,7 @@ enum iio_chan_type {
> >       IIO_DELTA_VELOCITY,
> >       IIO_COLORTEMP,
> >       IIO_CHROMATICITY,
> > +     IIO_ATTENTION,
> >  };
> >
> >  enum iio_modifier {
> > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > index 8073c9e4fe46..ed9a677f1028 100644
> > --- a/tools/iio/iio_event_monitor.c
> > +++ b/tools/iio/iio_event_monitor.c
> > @@ -63,6 +63,7 @@ static const char * const iio_chan_type_name_spec[] = {
> >       [IIO_DELTA_VELOCITY] = "deltavelocity",
> >       [IIO_COLORTEMP] = "colortemp",
> >       [IIO_CHROMATICITY] = "chromaticity",
> > +     [IIO_ATTENTION] = "attention",
> >  };
> >
> >  static const char * const iio_ev_type_text[] = {
> > @@ -183,6 +184,7 @@ static bool event_is_known(struct iio_event_data *event)
> >       case IIO_DELTA_VELOCITY:
> >       case IIO_COLORTEMP:
> >       case IIO_CHROMATICITY:
> > +     case IIO_ATTENTION:
> >               break;
> >       default:
> >               return false;
> >
>


-- 
Ricardo Ribalda

Re: [PATCH v2 3/5] iio: Add channel type for attention

[Jonathan Cameron]

On Tue, 29 Oct 2024 13:20:06 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Hi Jonathan
> 
> On Mon, 28 Oct 2024 at 21:34, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 28 Oct 2024 10:12:23 +0000
> > Ricardo Ribalda <ribalda@chromium.org> wrote:
> >  
> > > Add a new channel type representing if the user's attention state to the
> > > the system. This usually means if the user is looking at the screen or
> > > not.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
> > >  drivers/iio/industrialio-core.c         | 1 +
> > >  include/uapi/linux/iio/types.h          | 1 +
> > >  tools/iio/iio_event_monitor.c           | 2 ++
> > >  4 files changed, 11 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index 89943c2d54e8..d5a2f93bd051 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -2339,3 +2339,10 @@ KernelVersion: 6.10
> > >  Contact:     linux-iio@vger.kernel.org
> > >  Description:
> > >               The value of current sense resistor in Ohms.
> > > +
> > > +What:                /sys/.../iio:deviceX/in_attention_raw
> > > +KernelVersion:       6.13
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Boolean value representing the user's attention to the system.
> > > +             This usually means if the user is looking at the screen or not.  
> >
> > Hmm. I should have thought of this when I replied to suggest a new channel type.
> > The question is 'units' for a decision.
> >
> > Last time we hit something like this where processing is used to make a decision
> > we decided to at least allow for the concept of 'certainty'.
> >
> > The idea being that smarter sensors would tell us something about how sure they
> > are that the attention is on the device.
> > The analogy being with activity detection. See in_activity_walking_input
> > in Documentation/ABI/testing/sysfs-bus-iio
> >
> > Do you think that would be appropriate here as well?  For this device
> > it would take the values 0 and 100 rather than 0 and 1.  
> 
> For the particular device that I want to support, they are giving me a
> value of 1 and 0, and the example from usb.org seems to work the same
> way (Logical Maximum of 1)
> https://www.usb.org/sites/default/files/hutrr107-humanpresenceattention_1.pdf
> 
> I have no problem multiplying my value by 100 if you think there will
> be a use case for that. It will not have a major performance impact on
> the driver.
Same was true (0 or 1) for the activity classification but I'm not
keen on certainty :)  So lets' copy that precedence and *100


> You decide ;)
> 
> >
> >  
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > index 6a6568d4a2cb..bdfb51275b68 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -95,6 +95,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > >       [IIO_DELTA_VELOCITY] = "deltavelocity",
> > >       [IIO_COLORTEMP] = "colortemp",
> > >       [IIO_CHROMATICITY] = "chromaticity",
> > > +     [IIO_ATTENTION] = "attention",
> > >  };
> > >
> > >  static const char * const iio_modifier_names[] = {
> > > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > > index f2e0b2d50e6b..12886d4465e4 100644
> > > --- a/include/uapi/linux/iio/types.h
> > > +++ b/include/uapi/linux/iio/types.h
> > > @@ -51,6 +51,7 @@ enum iio_chan_type {
> > >       IIO_DELTA_VELOCITY,
> > >       IIO_COLORTEMP,
> > >       IIO_CHROMATICITY,
> > > +     IIO_ATTENTION,
> > >  };
> > >
> > >  enum iio_modifier {
> > > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > > index 8073c9e4fe46..ed9a677f1028 100644
> > > --- a/tools/iio/iio_event_monitor.c
> > > +++ b/tools/iio/iio_event_monitor.c
> > > @@ -63,6 +63,7 @@ static const char * const iio_chan_type_name_spec[] = {
> > >       [IIO_DELTA_VELOCITY] = "deltavelocity",
> > >       [IIO_COLORTEMP] = "colortemp",
> > >       [IIO_CHROMATICITY] = "chromaticity",
> > > +     [IIO_ATTENTION] = "attention",
> > >  };
> > >
> > >  static const char * const iio_ev_type_text[] = {
> > > @@ -183,6 +184,7 @@ static bool event_is_known(struct iio_event_data *event)
> > >       case IIO_DELTA_VELOCITY:
> > >       case IIO_COLORTEMP:
> > >       case IIO_CHROMATICITY:
> > > +     case IIO_ATTENTION:
> > >               break;
> > >       default:
> > >               return false;
> > >  
> >  
> 
> 

Re: [PATCH v2 3/5] iio: Add channel type for attention

[David Lechner]

On 10/29/24 9:38 AM, Jonathan Cameron wrote:
> On Tue, 29 Oct 2024 13:20:06 +0100
> Ricardo Ribalda <ribalda@chromium.org> wrote:
> 
>> Hi Jonathan
>>
>> On Mon, 28 Oct 2024 at 21:34, Jonathan Cameron <jic23@kernel.org> wrote:
>>>
>>> On Mon, 28 Oct 2024 10:12:23 +0000
>>> Ricardo Ribalda <ribalda@chromium.org> wrote:
>>>  
>>>> Add a new channel type representing if the user's attention state to the
>>>> the system. This usually means if the user is looking at the screen or
>>>> not.
>>>>
>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>> ---
>>>>  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
>>>>  drivers/iio/industrialio-core.c         | 1 +
>>>>  include/uapi/linux/iio/types.h          | 1 +
>>>>  tools/iio/iio_event_monitor.c           | 2 ++
>>>>  4 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>>> index 89943c2d54e8..d5a2f93bd051 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>>> @@ -2339,3 +2339,10 @@ KernelVersion: 6.10
>>>>  Contact:     linux-iio@vger.kernel.org
>>>>  Description:
>>>>               The value of current sense resistor in Ohms.
>>>> +
>>>> +What:                /sys/.../iio:deviceX/in_attention_raw
>>>> +KernelVersion:       6.13
>>>> +Contact:     linux-iio@vger.kernel.org
>>>> +Description:
>>>> +             Boolean value representing the user's attention to the system.
>>>> +             This usually means if the user is looking at the screen or not.  
>>>
>>> Hmm. I should have thought of this when I replied to suggest a new channel type.
>>> The question is 'units' for a decision.
>>>
>>> Last time we hit something like this where processing is used to make a decision
>>> we decided to at least allow for the concept of 'certainty'.
>>>
>>> The idea being that smarter sensors would tell us something about how sure they
>>> are that the attention is on the device.
>>> The analogy being with activity detection. See in_activity_walking_input
>>> in Documentation/ABI/testing/sysfs-bus-iio
>>>
>>> Do you think that would be appropriate here as well?  For this device
>>> it would take the values 0 and 100 rather than 0 and 1.  
>>
>> For the particular device that I want to support, they are giving me a
>> value of 1 and 0, and the example from usb.org seems to work the same
>> way (Logical Maximum of 1)
>> https://www.usb.org/sites/default/files/hutrr107-humanpresenceattention_1.pdf
>>
>> I have no problem multiplying my value by 100 if you think there will
>> be a use case for that. It will not have a major performance impact on
>> the driver.
> Same was true (0 or 1) for the activity classification but I'm not
> keen on certainty :)  So lets' copy that precedence and *100
> 
> 
And I assume we would want this to be in_attention_input (processed),
not in_attention_raw.

Re: [PATCH v2 3/5] iio: Add channel type for attention

[Jonathan Cameron]

On Tue, 29 Oct 2024 17:26:06 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 10/29/24 9:38 AM, Jonathan Cameron wrote:
> > On Tue, 29 Oct 2024 13:20:06 +0100
> > Ricardo Ribalda <ribalda@chromium.org> wrote:
> >   
> >> Hi Jonathan
> >>
> >> On Mon, 28 Oct 2024 at 21:34, Jonathan Cameron <jic23@kernel.org> wrote:  
> >>>
> >>> On Mon, 28 Oct 2024 10:12:23 +0000
> >>> Ricardo Ribalda <ribalda@chromium.org> wrote:
> >>>    
> >>>> Add a new channel type representing if the user's attention state to the
> >>>> the system. This usually means if the user is looking at the screen or
> >>>> not.
> >>>>
> >>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>>> ---
> >>>>  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
> >>>>  drivers/iio/industrialio-core.c         | 1 +
> >>>>  include/uapi/linux/iio/types.h          | 1 +
> >>>>  tools/iio/iio_event_monitor.c           | 2 ++
> >>>>  4 files changed, 11 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> >>>> index 89943c2d54e8..d5a2f93bd051 100644
> >>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
> >>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> >>>> @@ -2339,3 +2339,10 @@ KernelVersion: 6.10
> >>>>  Contact:     linux-iio@vger.kernel.org
> >>>>  Description:
> >>>>               The value of current sense resistor in Ohms.
> >>>> +
> >>>> +What:                /sys/.../iio:deviceX/in_attention_raw
> >>>> +KernelVersion:       6.13
> >>>> +Contact:     linux-iio@vger.kernel.org
> >>>> +Description:
> >>>> +             Boolean value representing the user's attention to the system.
> >>>> +             This usually means if the user is looking at the screen or not.    
> >>>
> >>> Hmm. I should have thought of this when I replied to suggest a new channel type.
> >>> The question is 'units' for a decision.
> >>>
> >>> Last time we hit something like this where processing is used to make a decision
> >>> we decided to at least allow for the concept of 'certainty'.
> >>>
> >>> The idea being that smarter sensors would tell us something about how sure they
> >>> are that the attention is on the device.
> >>> The analogy being with activity detection. See in_activity_walking_input
> >>> in Documentation/ABI/testing/sysfs-bus-iio
> >>>
> >>> Do you think that would be appropriate here as well?  For this device
> >>> it would take the values 0 and 100 rather than 0 and 1.    
> >>
> >> For the particular device that I want to support, they are giving me a
> >> value of 1 and 0, and the example from usb.org seems to work the same
> >> way (Logical Maximum of 1)
> >> https://www.usb.org/sites/default/files/hutrr107-humanpresenceattention_1.pdf
> >>
> >> I have no problem multiplying my value by 100 if you think there will
> >> be a use case for that. It will not have a major performance impact on
> >> the driver.  
> > Same was true (0 or 1) for the activity classification but I'm not
> > keen on certainty :)  So lets' copy that precedence and *100
> > 
> >   
> And I assume we would want this to be in_attention_input (processed),
> not in_attention_raw.
Good point. Yes.
> 

[PATCH v2 4/5] iio: hid-sensors-prox: Make proximity channel indexed

[Ricardo Ribalda]

We are going to introduce more proximity channels. Make proximity a
indexed channel now, in a simple patch, so the change can be easily
bisected if there are any issues.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/iio/light/hid-sensor-prox.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
index d38564fe22df..0f12a8a83790 100644
--- a/drivers/iio/light/hid-sensor-prox.c
+++ b/drivers/iio/light/hid-sensor-prox.c
@@ -40,6 +40,7 @@ static const struct iio_chan_spec prox_channels[] = {
 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 		BIT(IIO_CHAN_INFO_HYSTERESIS),
 		.scan_index = CHANNEL_SCAN_INDEX_PRESENCE,
+		.indexed = true,
 	}
 };
 

-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH v2 5/5] iio: hid-sensor-prox: Add support for more channels

[Ricardo Ribalda]

Egis620 supports 3 channels: presense, proximity and attention.

Modify the driver so it can read those channels as well.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/iio/light/hid-sensor-prox.c | 171 ++++++++++++++++++++----------------
 1 file changed, 95 insertions(+), 76 deletions(-)

diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
index 0f12a8a83790..e6f81d40a492 100644
--- a/drivers/iio/light/hid-sensor-prox.c
+++ b/drivers/iio/light/hid-sensor-prox.c
@@ -13,16 +13,31 @@
 #include <linux/iio/buffer.h>
 #include "../common/hid-sensors/hid-sensor-trigger.h"
 
-#define CHANNEL_SCAN_INDEX_PRESENCE 0
+static const u32 prox_usage_ids[] = {
+	HID_USAGE_SENSOR_HUMAN_PRESENCE,
+	HID_USAGE_SENSOR_HUMAN_PROXIMITY,
+	HID_USAGE_SENSOR_HUMAN_ATTENTION,
+};
+#define MAX_CHANNELS ARRAY_SIZE(prox_usage_ids)
+
+enum {
+	HID_HUMAN_PRESENCE,
+	HID_HUMAN_PROXIMITY,
+	HID_HUMAN_ATTENTION,
+};
 
 struct prox_state {
 	struct hid_sensor_hub_callbacks callbacks;
 	struct hid_sensor_common common_attributes;
-	struct hid_sensor_hub_attribute_info prox_attr;
-	u32 human_presence;
+	struct hid_sensor_hub_attribute_info prox_attr[MAX_CHANNELS];
+	struct iio_chan_spec channels[MAX_CHANNELS];
+	u32 channel2usage[MAX_CHANNELS];
+	u32 human_presence[MAX_CHANNELS];
 	int scale_pre_decml;
 	int scale_post_decml;
 	int scale_precision;
+	unsigned long scan_mask[2]; // One entry plus one terminator.
+	int num_channels;
 };
 
 static const u32 prox_sensitivity_addresses[] = {
@@ -30,18 +45,23 @@ static const u32 prox_sensitivity_addresses[] = {
 	HID_USAGE_SENSOR_DATA_PRESENCE,
 };
 
-/* Channel definitions */
-static const struct iio_chan_spec prox_channels[] = {
-	{
-		.type = IIO_PROXIMITY,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
-		BIT(IIO_CHAN_INFO_SCALE) |
-		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
-		BIT(IIO_CHAN_INFO_HYSTERESIS),
-		.scan_index = CHANNEL_SCAN_INDEX_PRESENCE,
-		.indexed = true,
+#define PROX_CHANNEL(_type, _indexed, _channel) \
+	{\
+		.type = _type,\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |\
+		BIT(IIO_CHAN_INFO_SCALE) |\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ) |\
+		BIT(IIO_CHAN_INFO_HYSTERESIS),\
+		.indexed = _indexed,\
+		.channel = _channel,\
 	}
+
+/* Channel definitions (same order as prox_usage_ids) */
+static const struct iio_chan_spec prox_channels[] = {
+	PROX_CHANNEL(IIO_PROXIMITY, true, HID_HUMAN_PRESENCE),
+	PROX_CHANNEL(IIO_PROXIMITY, true, HID_HUMAN_PROXIMITY),
+	PROX_CHANNEL(IIO_ATTENTION, false, 0),
 };
 
 /* Adjust channel real bits based on report descriptor */
@@ -63,7 +83,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 {
 	struct prox_state *prox_state = iio_priv(indio_dev);
 	struct hid_sensor_hub_device *hsdev;
-	int report_id = -1;
+	int report_id;
 	u32 address;
 	int ret_type;
 	s32 min;
@@ -72,29 +92,20 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 	*val2 = 0;
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		switch (chan->scan_index) {
-		case  CHANNEL_SCAN_INDEX_PRESENCE:
-			report_id = prox_state->prox_attr.report_id;
-			min = prox_state->prox_attr.logical_minimum;
-			address = HID_USAGE_SENSOR_HUMAN_PRESENCE;
-			hsdev = prox_state->common_attributes.hsdev;
-			break;
-		default:
-			report_id = -1;
-			break;
-		}
-		if (report_id >= 0) {
-			hid_sensor_power_state(&prox_state->common_attributes,
-						true);
-			*val = sensor_hub_input_attr_get_raw_value(
-				hsdev, hsdev->usage, address, report_id,
-				SENSOR_HUB_SYNC, min < 0);
-			hid_sensor_power_state(&prox_state->common_attributes,
-						false);
-		} else {
-			*val = 0;
+		if (chan->scan_index >= prox_state->num_channels)
 			return -EINVAL;
-		}
+		address = prox_state->channel2usage[chan->scan_index];
+		report_id = prox_state->prox_attr[chan->scan_index].report_id;
+		hsdev = prox_state->common_attributes.hsdev;
+		min = prox_state->prox_attr[chan->scan_index].logical_minimum;
+		hid_sensor_power_state(&prox_state->common_attributes, true);
+		*val = sensor_hub_input_attr_get_raw_value(hsdev,
+							   hsdev->usage,
+							   address,
+							   report_id,
+							   SENSOR_HUB_SYNC,
+							   min < 0);
+		hid_sensor_power_state(&prox_state->common_attributes, false);
 		ret_type = IIO_VAL_INT;
 		break;
 	case IIO_CHAN_INFO_SCALE:
@@ -104,7 +115,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 		break;
 	case IIO_CHAN_INFO_OFFSET:
 		*val = hid_sensor_convert_exponent(
-				prox_state->prox_attr.unit_expo);
+			prox_state->prox_attr[chan->scan_index].unit_expo);
 		ret_type = IIO_VAL_INT;
 		break;
 	case IIO_CHAN_INFO_SAMP_FREQ:
@@ -179,48 +190,63 @@ static int prox_capture_sample(struct hid_sensor_hub_device *hsdev,
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(priv);
 	struct prox_state *prox_state = iio_priv(indio_dev);
-	int ret = -EINVAL;
-
-	switch (usage_id) {
-	case HID_USAGE_SENSOR_HUMAN_PRESENCE:
-		switch (raw_len) {
-		case 1:
-			prox_state->human_presence = *(u8 *)raw_data;
-			return 0;
-		case 4:
-			prox_state->human_presence = *(u32 *)raw_data;
-			return 0;
-		default:
+	int chan;
+
+	for (chan = 0; chan < prox_state->num_channels; chan++)
+		if (prox_state->channel2usage[chan] == usage_id)
 			break;
-		}
-		break;
+	if (chan == prox_state->num_channels)
+		return -EINVAL;
+
+	switch (raw_len) {
+	case 1:
+		prox_state->human_presence[chan] = *(u8 *)raw_data;
+		return 0;
+	case 4:
+		prox_state->human_presence[chan] = *(u32 *)raw_data;
+		return 0;
 	}
 
-	return ret;
+	return -EINVAL;
 }
 
 /* Parse report which is specific to an usage id*/
 static int prox_parse_report(struct platform_device *pdev,
 				struct hid_sensor_hub_device *hsdev,
-				struct iio_chan_spec *channels,
-				unsigned usage_id,
 				struct prox_state *st)
 {
+	struct iio_chan_spec *channels = st->channels;
+	int index = 0;
 	int ret;
+	int i;
+
+	for (i = 0; i < MAX_CHANNELS; i++) {
+		u32 usage_id = prox_usage_ids[i];
+
+		ret = sensor_hub_input_get_attribute_info(hsdev,
+							  HID_INPUT_REPORT,
+							  hsdev->usage,
+							  usage_id,
+							  &st->prox_attr[index]);
+		if (ret < 0)
+			continue;
+		st->channel2usage[index] = usage_id;
+		st->scan_mask[0] |= BIT(index);
+		channels[index] = prox_channels[i];
+		channels[index].scan_index = index;
+		prox_adjust_channel_bit_mask(channels, index,
+					     st->prox_attr[index].size);
+		dev_dbg(&pdev->dev, "prox %x:%x\n", st->prox_attr[index].index,
+			st->prox_attr[index].report_id);
+		index++;
+	}
 
-	ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT,
-			usage_id,
-			HID_USAGE_SENSOR_HUMAN_PRESENCE,
-			&st->prox_attr);
-	if (ret < 0)
+	if (!index)
 		return ret;
-	prox_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_PRESENCE,
-					st->prox_attr.size);
 
-	dev_dbg(&pdev->dev, "prox %x:%x\n", st->prox_attr.index,
-			st->prox_attr.report_id);
+	st->num_channels = index;
 
-	return ret;
+	return 0;
 }
 
 /* Function to initialize the processing for usage id */
@@ -251,22 +277,15 @@ static int hid_prox_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	indio_dev->channels = devm_kmemdup(&pdev->dev, prox_channels,
-					   sizeof(prox_channels), GFP_KERNEL);
-	if (!indio_dev->channels) {
-		dev_err(&pdev->dev, "failed to duplicate channels\n");
-		return -ENOMEM;
-	}
-
-	ret = prox_parse_report(pdev, hsdev,
-				(struct iio_chan_spec *)indio_dev->channels,
-				hsdev->usage, prox_state);
+	ret = prox_parse_report(pdev, hsdev, prox_state);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to setup attributes\n");
 		return ret;
 	}
 
-	indio_dev->num_channels = ARRAY_SIZE(prox_channels);
+	indio_dev->num_channels = prox_state->num_channels;
+	indio_dev->channels = prox_state->channels;
+	indio_dev->available_scan_masks = prox_state->scan_mask;
 	indio_dev->info = &prox_info;
 	indio_dev->name = name;
 	indio_dev->modes = INDIO_DIRECT_MODE;

-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH v2 5/5] iio: hid-sensor-prox: Add support for more channels

[Jonathan Cameron]

On Mon, 28 Oct 2024 10:12:25 +0000
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Egis620 supports 3 channels: presense, proximity and attention.
> 
> Modify the driver so it can read those channels as well.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Hi Ricardo, one comment inline that I'm fine fixing up if nothing else
comes up. 

Given this series is introducing a new channel type, I'd like to leave
it on the list for a little while to see if there are any comments on that
definition beyond the one I had...

Jonathan

> ---
>  drivers/iio/light/hid-sensor-prox.c | 171 ++++++++++++++++++++----------------
>  1 file changed, 95 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
> index 0f12a8a83790..e6f81d40a492 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c
> @@ -13,16 +13,31 @@
>  #include <linux/iio/buffer.h>
>  #include "../common/hid-sensors/hid-sensor-trigger.h"
>  
> -#define CHANNEL_SCAN_INDEX_PRESENCE 0
> +static const u32 prox_usage_ids[] = {
> +	HID_USAGE_SENSOR_HUMAN_PRESENCE,
> +	HID_USAGE_SENSOR_HUMAN_PROXIMITY,
> +	HID_USAGE_SENSOR_HUMAN_ATTENTION,
> +};
> +#define MAX_CHANNELS ARRAY_SIZE(prox_usage_ids)
> +
> +enum {
> +	HID_HUMAN_PRESENCE,
> +	HID_HUMAN_PROXIMITY,
> +	HID_HUMAN_ATTENTION,
> +};
>  
>  struct prox_state {
>  	struct hid_sensor_hub_callbacks callbacks;
>  	struct hid_sensor_common common_attributes;
> -	struct hid_sensor_hub_attribute_info prox_attr;
> -	u32 human_presence;
> +	struct hid_sensor_hub_attribute_info prox_attr[MAX_CHANNELS];
> +	struct iio_chan_spec channels[MAX_CHANNELS];
> +	u32 channel2usage[MAX_CHANNELS];
> +	u32 human_presence[MAX_CHANNELS];
>  	int scale_pre_decml;
>  	int scale_post_decml;
>  	int scale_precision;
> +	unsigned long scan_mask[2]; // One entry plus one terminator.
Really trivial, but in IIO and most of the kernel comment are still
'c style' /* One entry plus one terminator */

> +	int num_channels;
>  };
>  
>  static const u32 prox_sensitivity_addresses[] = {
> @@ -30,18 +45,23 @@ static const u32 prox_sensitivity_addresses[] = {
>  	HID_USAGE_SENSOR_DATA_PRESENCE,
>  };

>