| Message ID | 20230806130558.89812-2-p.jungkamp@gmx.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | IIO: hid-sensor-prox: add missing scale attribute | expand |
On Sun, 2023-08-06 at 14:59 +0200, Philipp Jungkamp wrote: > The hid-sensor-prox returned an empty string on sysfs > in_proximity_scale > read. This is due to the the driver's scale never being initialized. What is scale value reporting here? Is it 1. Thanks, Srinivas > > Try to query the scale of the HID sensor using > hid_sensor_format_scale. > > Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net> > --- > Hello, > > up until now I've used the sensor directly through the buffered IIO > interface, > ignoring the in_proximity_scale attribute. But while integrating it > with > iio-sensor-proxy I noticed that a read on scale return an empty > string, > breaking the code there. > > Looking at the code in `hid-sensor-prox.c` it is fairly obvious that > the scale > just wasn't being initialized. I now added the > hid_sensor_format_scale call > similar to the ones found in e.g. `hid-sensor-als.c`. > > Regards, > Philipp Jungkamp > > drivers/iio/light/hid-sensor-prox.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/iio/light/hid-sensor-prox.c > b/drivers/iio/light/hid-sensor-prox.c > index a47591e1bad9..aaf2ce6ed52c 100644 > --- a/drivers/iio/light/hid-sensor-prox.c > +++ b/drivers/iio/light/hid-sensor-prox.c > @@ -227,6 +227,9 @@ static int prox_parse_report(struct > platform_device *pdev, > dev_dbg(&pdev->dev, "prox %x:%x\n", st->prox_attr.index, > st->prox_attr.report_id); > > + st->scale_precision = hid_sensor_format_scale(usage_id, &st- > >prox_attr, > + &st->scale_pre_decml, &st- > >scale_post_decml); > + > return ret; > } > > -- > 2.41.0 >
> > The hid-sensor-prox returned an empty string on sysfs > > in_proximity_scale > > read. This is due to the the driver's scale never being initialized. > > What is scale value reporting here? Is it 1. > > Thanks, > Srinivas Calling `read` on the sysfs file `in_proximity_scale` returns 0, thus an empty string. Adding the hid_format_scale call makes that a '1.000000'. Regards, Philipp
On Tue, 8 Aug 2023 14:22:10 +0200 Philipp Jungkamp <p.jungkamp@gmx.net> wrote: > > > The hid-sensor-prox returned an empty string on sysfs > > > in_proximity_scale > > > read. This is due to the the driver's scale never being initialized. > > > > What is scale value reporting here? Is it 1. > > > > Thanks, > > Srinivas > > Calling `read` on the sysfs file `in_proximity_scale` returns 0, thus an empty string. > Adding the hid_format_scale call makes that a '1.000000'. > > Regards, > Philipp Srinivas - I was kind of waiting for a reply to say if you are happy with the explanation. All good? Phillipp - this sounds like a fix to me. Fixes tag? Thanks, Jonathan
On Sat, 2023-10-14 at 17:52 +0100, Jonathan Cameron wrote: > On Tue, 8 Aug 2023 14:22:10 +0200 > Philipp Jungkamp <p.jungkamp@gmx.net> wrote: > > > > > The hid-sensor-prox returned an empty string on sysfs > > > > in_proximity_scale > > > > read. This is due to the the driver's scale never being > > > > initialized. > > > > > > What is scale value reporting here? Is it 1. > > > > > > Thanks, > > > Srinivas > > > > Calling `read` on the sysfs file `in_proximity_scale` returns 0, > > thus an empty string. > > Adding the hid_format_scale call makes that a '1.000000'. > > > > Regards, > > Philipp > > Srinivas - I was kind of waiting for a reply to say if you are happy > with the explanation. > All good? All good. Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com> > > Phillipp - this sounds like a fix to me. Fixes tag? > > Thanks, > > Jonathan
On Sat, 14 Oct 2023 19:56:26 -0700 srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On Sat, 2023-10-14 at 17:52 +0100, Jonathan Cameron wrote: > > On Tue, 8 Aug 2023 14:22:10 +0200 > > Philipp Jungkamp <p.jungkamp@gmx.net> wrote: > > > > > > > The hid-sensor-prox returned an empty string on sysfs > > > > > in_proximity_scale > > > > > read. This is due to the the driver's scale never being > > > > > initialized. > > > > > > > > What is scale value reporting here? Is it 1. > > > > > > > > Thanks, > > > > Srinivas > > > > > > Calling `read` on the sysfs file `in_proximity_scale` returns 0, > > > thus an empty string. > > > Adding the hid_format_scale call makes that a '1.000000'. > > > > > > Regards, > > > Philipp > > > > Srinivas - I was kind of waiting for a reply to say if you are happy > > with the explanation. > > All good? > All good. > > Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com> > Given we are late in the cycle and my next fixes pull will probably end up going in during the merge window anyway, I've applied this to the togreg branch of iio.git (so the slow path). Phillipp, if a backport makes sense you can request that after this goes upstream. Thanks, Jonathan > > > > Phillipp - this sounds like a fix to me. Fixes tag? > > > > Thanks, > > > > Jonathan >
On Sun, 15 Oct 2023 12:04:48 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 14 Oct 2023 19:56:26 -0700 > srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > > On Sat, 2023-10-14 at 17:52 +0100, Jonathan Cameron wrote: > > > On Tue, 8 Aug 2023 14:22:10 +0200 > > > Philipp Jungkamp <p.jungkamp@gmx.net> wrote: > > > > > > > > > The hid-sensor-prox returned an empty string on sysfs > > > > > > in_proximity_scale > > > > > > read. This is due to the the driver's scale never being > > > > > > initialized. > > > > > > > > > > What is scale value reporting here? Is it 1. > > > > > > > > > > Thanks, > > > > > Srinivas > > > > > > > > Calling `read` on the sysfs file `in_proximity_scale` returns 0, > > > > thus an empty string. > > > > Adding the hid_format_scale call makes that a '1.000000'. > > > > > > > > Regards, > > > > Philipp > > > > > > Srinivas - I was kind of waiting for a reply to say if you are happy > > > with the explanation. > > > All good? > > All good. > > > > Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com> > > > Given we are late in the cycle and my next fixes pull will probably end up > going in during the merge window anyway, I've applied this to the togreg > branch of iio.git (so the slow path). > > Phillipp, if a backport makes sense you can request that after this > goes upstream. Whilst typing up a pull request I saw this again and thought a bit more on it. This fix is probably wrong approach. Proximity sensors are often scale free because they depend on reflectance off something or a capacitance changing etc so we don't know the scaling. So the right response then is not to return a scale value of 1.0 but to not provide the attribute at all. Is that something that could be easily done here? For now I'm dropping the patch. Sorry I wasn't paying enough attention to notice this was a proximity sensor. Jonathan > > Thanks, > > Jonathan > > > > > > > Phillipp - this sounds like a fix to me. Fixes tag? > > > > > > Thanks, > > > > > > Jonathan > > >
On Mon, 2023-10-16 at 08:44 +0100, Jonathan Cameron wrote: > On Sun, 15 Oct 2023 12:04:48 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Sat, 14 Oct 2023 19:56:26 -0700 > > srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > > > > On Sat, 2023-10-14 at 17:52 +0100, Jonathan Cameron wrote: > > > > > [...] > > Phillipp, if a backport makes sense you can request that after this > > goes upstream. > Whilst typing up a pull request I saw this again and thought a bit > more on it. > > This fix is probably wrong approach. Proximity sensors are often > scale free > because they depend on reflectance off something or a capacitance > changing etc > so we don't know the scaling. So the right response then is not to > return a scale > value of 1.0 but to not provide the attribute at all. Is that > something that > could be easily done here? I think so. But hope that iio-sensor-proxy can handle absence of scale attribute. git diff drivers/iio/light/hid-sensor-prox.c diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c index a47591e1bad9..e4b81fa948f5 100644 --- a/drivers/iio/light/hid-sensor-prox.c +++ b/drivers/iio/light/hid-sensor-prox.c @@ -36,7 +36,6 @@ 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, Thanks, Srinivas > > For now I'm dropping the patch. Sorry I wasn't paying enough > attention to notice > this was a proximity sensor. > > Jonathan > > > > > Thanks, > > > > Jonathan > > > > > > > > > > Phillipp - this sounds like a fix to me. Fixes tag? > > > > > > > > Thanks, > > > > > > > > Jonathan > > > > > >
On Mon, 16 Oct 2023 07:28:36 -0700 srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On Mon, 2023-10-16 at 08:44 +0100, Jonathan Cameron wrote: > > On Sun, 15 Oct 2023 12:04:48 +0100 > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > On Sat, 14 Oct 2023 19:56:26 -0700 > > > srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > > > > > > On Sat, 2023-10-14 at 17:52 +0100, Jonathan Cameron wrote: > > > > > > > > > [...] > > > > Phillipp, if a backport makes sense you can request that after this > > > goes upstream. > > Whilst typing up a pull request I saw this again and thought a bit > > more on it. > > > > This fix is probably wrong approach. Proximity sensors are often > > scale free > > because they depend on reflectance off something or a capacitance > > changing etc > > so we don't know the scaling. So the right response then is not to > > return a scale > > value of 1.0 but to not provide the attribute at all. Is that > > something that > > could be easily done here? > > I think so. But hope that iio-sensor-proxy can handle absence of scale > attribute. > > git diff drivers/iio/light/hid-sensor-prox.c > diff --git a/drivers/iio/light/hid-sensor-prox.c > b/drivers/iio/light/hid-sensor-prox.c > index a47591e1bad9..e4b81fa948f5 100644 > --- a/drivers/iio/light/hid-sensor-prox.c > +++ b/drivers/iio/light/hid-sensor-prox.c > @@ -36,7 +36,6 @@ 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, > > Thanks, > Srinivas Just to check. Are we guaranteed that there is never a scale parameter? Some proximity sensors do have absolute units (time of flight sensors for example). Jonathan > > > > > For now I'm dropping the patch. Sorry I wasn't paying enough > > attention to notice > > this was a proximity sensor. > > > > Jonathan > > > > > > > > Thanks, > > > > > > Jonathan > > > > > > > > > > > > > Phillipp - this sounds like a fix to me. Fixes tag? > > > > > > > > > > Thanks, > > > > > > > > > > Jonathan > > > > > > > > > >
On Tue, 2023-10-17 at 20:11 +0100, Jonathan Cameron wrote: > On Mon, 16 Oct 2023 07:28:36 -0700 > srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > > On Mon, 2023-10-16 at 08:44 +0100, Jonathan Cameron wrote: > > > On Sun, 15 Oct 2023 12:04:48 +0100 > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > On Sat, 14 Oct 2023 19:56:26 -0700 > > > > srinivas pandruvada <srinivas.pandruvada@linux.intel.com> > > > > wrote: > > > > > > > > > On Sat, 2023-10-14 at 17:52 +0100, Jonathan Cameron wrote: > > > > > > > > > > > > > [...] > > > > > > Phillipp, if a backport makes sense you can request that after > > > > this > > > > goes upstream. > > > Whilst typing up a pull request I saw this again and thought a > > > bit > > > more on it. > > > > > > This fix is probably wrong approach. Proximity sensors are often > > > scale free > > > because they depend on reflectance off something or a capacitance > > > changing etc > > > so we don't know the scaling. So the right response then is not > > > to > > > return a scale > > > value of 1.0 but to not provide the attribute at all. Is that > > > something that > > > could be easily done here? > > > > I think so. But hope that iio-sensor-proxy can handle absence of > > scale > > attribute. > > > > git diff drivers/iio/light/hid-sensor-prox.c > > diff --git a/drivers/iio/light/hid-sensor-prox.c > > b/drivers/iio/light/hid-sensor-prox.c > > index a47591e1bad9..e4b81fa948f5 100644 > > --- a/drivers/iio/light/hid-sensor-prox.c > > +++ b/drivers/iio/light/hid-sensor-prox.c > > @@ -36,7 +36,6 @@ 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, > > > > Thanks, > > Srinivas > > Just to check. Are we guaranteed that there is never a scale > parameter? > Some proximity sensors do have absolute units (time of flight sensors > for example). > This driver is implementing: Biometric: Human Presence (Usage ID 0x11) from HID sensor hub specification. "Biometric: Human PresenceCA,CP – An application-level or physical collection that identifies a device that detects human presence (Boolean yes or no)." It is not implementing Biometric: Human Proximity (Usage ID 0x12). This has range of values, then unit will be applicable. Thanks, Srinivas > Jonathan > > > > > > > > > For now I'm dropping the patch. Sorry I wasn't paying enough > > > attention to notice > > > this was a proximity sensor. > > > > > > Jonathan > > > > > > > > > > > Thanks, > > > > > > > > Jonathan > > > > > > > > > > > > > > > > Phillipp - this sounds like a fix to me. Fixes tag? > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > > >
On Tue, 17 Oct 2023 18:56:38 -0700 srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On Tue, 2023-10-17 at 20:11 +0100, Jonathan Cameron wrote: > > On Mon, 16 Oct 2023 07:28:36 -0700 > > srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > > > > On Mon, 2023-10-16 at 08:44 +0100, Jonathan Cameron wrote: > > > > On Sun, 15 Oct 2023 12:04:48 +0100 > > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > > On Sat, 14 Oct 2023 19:56:26 -0700 > > > > > srinivas pandruvada <srinivas.pandruvada@linux.intel.com> > > > > > wrote: > > > > > > > > > > > On Sat, 2023-10-14 at 17:52 +0100, Jonathan Cameron wrote: > > > > > > > > > > > > > > > > > [...] > > > > > > > > Phillipp, if a backport makes sense you can request that after > > > > > this > > > > > goes upstream. > > > > Whilst typing up a pull request I saw this again and thought a > > > > bit > > > > more on it. > > > > > > > > This fix is probably wrong approach. Proximity sensors are often > > > > scale free > > > > because they depend on reflectance off something or a capacitance > > > > changing etc > > > > so we don't know the scaling. So the right response then is not > > > > to > > > > return a scale > > > > value of 1.0 but to not provide the attribute at all. Is that > > > > something that > > > > could be easily done here? > > > > > > I think so. But hope that iio-sensor-proxy can handle absence of > > > scale > > > attribute. > > > > > > git diff drivers/iio/light/hid-sensor-prox.c > > > diff --git a/drivers/iio/light/hid-sensor-prox.c > > > b/drivers/iio/light/hid-sensor-prox.c > > > index a47591e1bad9..e4b81fa948f5 100644 > > > --- a/drivers/iio/light/hid-sensor-prox.c > > > +++ b/drivers/iio/light/hid-sensor-prox.c > > > @@ -36,7 +36,6 @@ 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, > > > > > > Thanks, > > > Srinivas > > > > Just to check. Are we guaranteed that there is never a scale > > parameter? > > Some proximity sensors do have absolute units (time of flight sensors > > for example). > > > This driver is implementing: > Biometric: Human Presence (Usage ID 0x11) from HID sensor hub > specification. > > "Biometric: Human > PresenceCA,CP – An application-level or physical collection that > identifies > a device that detects human presence (Boolean yes or no)." > > > It is not implementing Biometric: Human Proximity (Usage ID 0x12). > This has range of values, then unit will be applicable. Makes sense. In that case, formal version of the patch above? Thanks, Jonathan > > Thanks, > Srinivas > > > Jonathan > > > > > > > > > > > > > For now I'm dropping the patch. Sorry I wasn't paying enough > > > > attention to notice > > > > this was a proximity sensor. > > > > > > > > Jonathan > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Jonathan > > > > > > > > > > > > > > > > > > > Phillipp - this sounds like a fix to me. Fixes tag? > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > > > > > > > > >
diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c index a47591e1bad9..aaf2ce6ed52c 100644 --- a/drivers/iio/light/hid-sensor-prox.c +++ b/drivers/iio/light/hid-sensor-prox.c @@ -227,6 +227,9 @@ static int prox_parse_report(struct platform_device *pdev, dev_dbg(&pdev->dev, "prox %x:%x\n", st->prox_attr.index, st->prox_attr.report_id); + st->scale_precision = hid_sensor_format_scale(usage_id, &st->prox_attr, + &st->scale_pre_decml, &st->scale_post_decml); + return ret; }
The hid-sensor-prox returned an empty string on sysfs in_proximity_scale read. This is due to the the driver's scale never being initialized. Try to query the scale of the HID sensor using hid_sensor_format_scale. Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net> --- Hello, up until now I've used the sensor directly through the buffered IIO interface, ignoring the in_proximity_scale attribute. But while integrating it with iio-sensor-proxy I noticed that a read on scale return an empty string, breaking the code there. Looking at the code in `hid-sensor-prox.c` it is fairly obvious that the scale just wasn't being initialized. I now added the hid_sensor_format_scale call similar to the ones found in e.g. `hid-sensor-als.c`. Regards, Philipp Jungkamp drivers/iio/light/hid-sensor-prox.c | 3 +++ 1 file changed, 3 insertions(+) -- 2.41.0