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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 24 Oct 2024 13:29:07 +0000
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Egis620 supports 3 channels: presense, proximity and attention.

It's not obvious to me that these should necessarily be represented
as proximity channels.

Presence and proximity perhaps though I'm confused as to why
both make sense on a device, but maybe there are two forms of sensor.

Attention is an oddity and not the same as proximity from the definition
(see patch 1 review).

So for that we probably need a new channel type.

Jonathan


> 
> 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 | 161 ++++++++++++++++++++----------------
>  1 file changed, 89 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
> index d38564fe22df..97550d0d21a9 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c
> @@ -13,16 +13,26 @@
>  #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,
> +};
Use an enum so that you can set these as entries you can later index.

[HID_HUMAN_PRESENCE] = HID_USAGE_...
etc

> +
> +#define MAX_USAGE ARRAY_SIZE(prox_usage_ids)
Name that something more specific or just use
the ARRAY_SIZE inline.

>  
>  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_USAGE];
> +	struct iio_chan_spec channels[MAX_USAGE];
> +	u32 channel2usage[MAX_USAGE];
> +	u32 human_presence[MAX_USAGE];
>  	int scale_pre_decml;
>  	int scale_post_decml;
>  	int scale_precision;
> +	unsigned long scan_mask[2];

Perhaps add a comment that this is one entry plus a terminator.

> +	int num_channels;
>  };
>  
>  static const u32 prox_sensitivity_addresses[] = {
> @@ -30,17 +40,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,
> +#define PROX_CHANNEL(_indexed, _channel) \
> +	{\
> +		.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),\
> +		.indexed = _indexed,\
> +		.channel = _channel,\
>  	}
> +
> +/* Channel definitions (same order as prox_usage_ids) */
> +static const struct iio_chan_spec prox_channels[] = {
> +	PROX_CHANNEL(false, 0), // PRESENCE
With a suitable enum as suggested above can use
[HID_HUMAN_PRESENCE] = PROX_CHANNEL(false, 0) etc

Combining index and not is unusual. If we have to do this
I think we should consider the technical ABI breakage of adding
an index to the first one.  It 'shouldn't' break code using the ABI
correctly but it is a risk.

> +	PROX_CHANNEL(true, 1), // PROXIMITY
> +	PROX_CHANNEL(true, 2), // ATTENTION
>  };
>  
>  /* Adjust channel real bits based on report descriptor */
> @@ -62,7 +78,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;
> @@ -71,29 +87,22 @@ 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 {
> +		if (chan->scan_index >= prox_state->num_channels) {
>  			*val = 0;
No need to set this in an error path. Not sure why original code did.

>  			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;

> @@ -178,48 +187,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;
> -		}
> +	if (chan == prox_state->num_channels)
> +		return -EINVAL;
> +
> +	switch (raw_len) {
> +	case 1:
> +		prox_state->human_presence[chan] = *(u8 *)raw_data;
> +		break;
Might as well return here.
> +	case 4:
> +		prox_state->human_presence[chan] = *(u32 *)raw_data;
>  		break;
and here.

>  	}
>  
> -	return ret;
> +	return 0;
>  }
>
>