diff mbox series

IIO: hid-sensor-prox: add missing scale attribute

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

Commit Message

Philipp Jungkamp Aug. 6, 2023, 12:59 p.m. UTC
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

Comments

Srinivas Pandruvada Aug. 7, 2023, 11:02 p.m. UTC | #1
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
>
Philipp Jungkamp Aug. 8, 2023, 12:22 p.m. UTC | #2
> > 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
Jonathan Cameron Oct. 14, 2023, 4:52 p.m. UTC | #3
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
Srinivas Pandruvada Oct. 15, 2023, 2:56 a.m. UTC | #4
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
Jonathan Cameron Oct. 15, 2023, 11:04 a.m. UTC | #5
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  
>
Jonathan Cameron Oct. 16, 2023, 7:44 a.m. UTC | #6
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    
> >   
>
Srinivas Pandruvada Oct. 16, 2023, 2:28 p.m. UTC | #7
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    
> > >   
> > 
>
Jonathan Cameron Oct. 17, 2023, 7:11 p.m. UTC | #8
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      
> > > >     
> > >   
> >   
>
Srinivas Pandruvada Oct. 18, 2023, 1:56 a.m. UTC | #9
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      
> > > > >     
> > > >   
> > >   
> > 
>
Jonathan Cameron Oct. 28, 2023, 4:30 p.m. UTC | #10
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 mbox series

Patch

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