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 | 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,
+};
+
+#define MAX_USAGE ARRAY_SIZE(prox_usage_ids)
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];
+ 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
+ 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;
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:
@@ -103,7 +112,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:
@@ -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;
+ case 4:
+ prox_state->human_presence[chan] = *(u32 *)raw_data;
break;
}
- return ret;
+ return 0;
}
/* 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_USAGE; 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 */
@@ -250,22 +274,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-googOn 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;
> }
>
>
© 2016 - 2024 Red Hat, Inc.