On Thu, 17 Nov 2022 00:19:47 +0100
Philipp Jungkamp <p.jungkamp@xxxxxxx> wrote:
> Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_PROX to
> allow this driver to drive the Lenvo custom proximity sensor, which is
> registered under a 'custom' usage and not HID_USAGE_SENSOR_PROX.
>
> Check for raw_len to accomodate the ReportSize(8) field used on the
> Lenovo Yoga 9 14IAP7.
>
> Add the Lenovo Intelligent Sensing Solution (LISS) human presence sensor
> to the platform device ids.
>
> Signed-off-by: Philipp Jungkamp <p.jungkamp@xxxxxxx>
As with previous - prefer this split into a noop patch then one that
adds the new use case.
It's small enough I'm not that bothered though if you'd rather not.
A few comments inline.
Thanks,
Jonathan
> ---
> drivers/hid/hid-sensor-custom.c | 10 ++++++++
> drivers/iio/light/hid-sensor-prox.c | 39 +++++++++++++++++++++--------
> 2 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index 124493b8abaf..ec49dc80f074 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -795,6 +795,16 @@ static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
> DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> }
> },
> + { /* human presence */
> + .tag = "LISS",
> + .luid = "0226000171AC0081",
> + .model = "VL53L1_HOD Sensor",
> + .manufacturer = "ST_MICRO",
> + .check_dmi = true,
> + .dmi.matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> + }
> + },
> {}
> };
>
> diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
> index f10fa2abfe72..cca6b75c44d5 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c
> @@ -61,6 +61,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
> long mask)
> {
> struct prox_state *prox_state = iio_priv(indio_dev);
> + struct hid_sensor_hub_device *hsdev;
> int report_id = -1;
> u32 address;
> int ret_type;
> @@ -75,6 +76,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
> 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;
> @@ -84,8 +86,9 @@ static int prox_read_raw(struct iio_dev *indio_dev,
> hid_sensor_power_state(&prox_state->common_attributes,
> true);
> *val = sensor_hub_input_attr_get_raw_value(
> - prox_state->common_attributes.hsdev,
> - HID_USAGE_SENSOR_PROX, address,
> + hsdev,
> + hsdev->usage,
> + address,
Wrapping is already odd here but this patch can avoid making it worse
by trying to get multiple params on one line < 80 chars.
> report_id,
> SENSOR_HUB_SYNC,
> min < 0);
> @@ -191,10 +194,16 @@ static int prox_capture_sample(struct hid_sensor_hub_device *hsdev,
>
> switch (usage_id) {
> case HID_USAGE_SENSOR_HUMAN_PRESENCE:
> - prox_state->human_presence = *(u32 *)raw_data;
> - ret = 0;
> - break;
> - default:
> + switch (raw_len) {
> + case 1:
> + prox_state->human_presence = *(u8 *)raw_data;
> + ret = 0;
No obvious reason not to use direct returns here as they will be slightly
more readable as well as shortening the code a little.
return 0;
> + break;
> + case 4:
> + prox_state->human_presence = *(u32 *)raw_data;
> + ret = 0;
> + break;
Likely to generate warnings if a compiler can't see far enough to tell this
only ever gets 1 or 4 in raw_len.
Avoid that by adding a default.
> + }
> break;
> }
>
> @@ -244,7 +253,8 @@ static int hid_prox_probe(struct platform_device *pdev)
> prox_state->common_attributes.hsdev = hsdev;
> prox_state->common_attributes.pdev = pdev;
>
> - ret = hid_sensor_parse_common_attributes(hsdev, HID_USAGE_SENSOR_PROX,
> + ret = hid_sensor_parse_common_attributes(hsdev,
> + hsdev->usage,
> &prox_state->common_attributes,
> prox_sensitivity_addresses,
> ARRAY_SIZE(prox_sensitivity_addresses));
> @@ -262,7 +272,8 @@ static int hid_prox_probe(struct platform_device *pdev)
>
> ret = prox_parse_report(pdev, hsdev,
> (struct iio_chan_spec *)indio_dev->channels,
> - HID_USAGE_SENSOR_PROX, prox_state);
> + hsdev->usage,
> + prox_state);
As below.
> if (ret) {
> dev_err(&pdev->dev, "failed to setup attributes\n");
> return ret;
> @@ -291,8 +302,9 @@ static int hid_prox_probe(struct platform_device *pdev)
> prox_state->callbacks.send_event = prox_proc_event;
> prox_state->callbacks.capture_sample = prox_capture_sample;
> prox_state->callbacks.pdev = pdev;
> - ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_PROX,
> - &prox_state->callbacks);
> + ret = sensor_hub_register_callback(hsdev,
> + hsdev->usage,
> + &prox_state->callbacks);
As below.
> if (ret < 0) {
> dev_err(&pdev->dev, "callback reg failed\n");
> goto error_iio_unreg;
> @@ -314,7 +326,8 @@ static int hid_prox_remove(struct platform_device *pdev)
> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> struct prox_state *prox_state = iio_priv(indio_dev);
>
> - sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PROX);
> + sensor_hub_remove_callback(hsdev,
> + hsdev->usage);
Similar to previous patch. Line wrap is too short.
> iio_device_unregister(indio_dev);
> hid_sensor_remove_trigger(indio_dev, &prox_state->common_attributes);
>
> @@ -326,6 +339,10 @@ static const struct platform_device_id hid_prox_ids[] = {
> /* Format: HID-SENSOR-usage_id_in_hex_lowercase */
> .name = "HID-SENSOR-200011",
> },
> + {
> + /* Format: HID-SENSOR-tag-usage_id_in_hex_lowercase */
> + .name = "HID-SENSOR-LISS-0226",
> + },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(platform, hid_prox_ids);
> --
> 2.38.1
>
