Re: [PATCH] iio: light: vcnl4000: Set ps high definition for 4040/4200

[Mårten Lindahl]

On 12/26/23 17:19, Jonathan Cameron wrote:
> On Thu, 21 Dec 2023 17:33:09 +0100
> Mårten Lindahl<marten.lindahl@xxxxxxxx>  wrote:
>
> > The vcnl4040/vcnl4200 proximity sensor defaults to 12 bit data
> > resolution, but the chip also supports 16 bit data resolution, which is
> > called proximity high definition (PS_HD).
> >
> > Make the vcnl4040/vcnl4200 proximity sensor use the high definition for
> > all data readings.
> >
> > Signed-off-by: Mårten Lindahl<marten.lindahl@xxxxxxxx>
Hi Jonathan!
> Hmm. Was about to apply this and had a nasty thought.  Whilst proximity isn't
> 'scaled' as such because there is no absolute scale applied, I assume this change
> divides the effective scale (so what someone may be applying in userspace) by 16?
>
> So this might cause someone a visible userspace regression?
>
> If so we may have to report it IIO_VAL_FRACTIONAL with the bottom set to 16
> so we end up with a suitable fixed point value from sysfs.
>
> Jonathan

Yes, your assumption is correct. And I found that this can easily pass 
unnoticed, at least on our HW. To get full 16 bit data width the sensor 
needs higher proximity integration time, current, and sample rate, or 
else it will give a raw output very close to the 12 bit steps with a 
maximum around 4150 compared to 12 bit max 4095. With our HW only few 
would notice the difference.

Increasing integration time/current/sample rate is already supported 
from sysfs, and to get the proper scaling I can use the 
IIO_VAL_FRACTIONAL as you suggest, but then I also need to set these 
higher values from the beginning (where PS_HD is set) or else sysfs 
in_proximity_raw will give output "259.375000000" for 4150, instead of 
"4095.937500000" for 65535. If not, userspace will have to change the 
values manually before first read can be done.

Regarding the changed output format, is it ok to change it from "4095" 
to "4095.937500000", without making users upset?

Kind regards

Mårten

> > ---
> >   drivers/iio/light/vcnl4000.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index fdf763a04b0b..4846f3b698b5 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -90,6 +90,7 @@
> >   #define VCNL4040_PS_CONF1_PS_SHUTDOWN	BIT(0)
> >   #define VCNL4040_PS_CONF2_PS_IT	GENMASK(3, 1) /* Proximity integration time */
> >   #define VCNL4040_CONF1_PS_PERS	GENMASK(5, 4) /* Proximity interrupt persistence setting */
> > +#define VCNL4040_PS_CONF2_PS_HD		BIT(11)	/* Proximity high definition */
> >   #define VCNL4040_PS_CONF2_PS_INT	GENMASK(9, 8) /* Proximity interrupt mode */
> >   #define VCNL4040_PS_CONF3_MPS		GENMASK(6, 5) /* Proximity multi pulse number */
> >   #define VCNL4040_PS_MS_LED_I		GENMASK(10, 8) /* Proximity current */
> > @@ -345,6 +346,7 @@ static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on)
> >   static int vcnl4200_init(struct vcnl4000_data *data)
> >   {
> >   	int ret, id;
> > +	u16 regval;
> >   
> >   	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
> >   	if (ret < 0)
> > @@ -389,6 +391,17 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> >   	mutex_init(&data->vcnl4200_al.lock);
> >   	mutex_init(&data->vcnl4200_ps.lock);
> >   
> > +	/* Use 16 bits proximity sensor readings */
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> > +	if (ret >= 0) {
> > +		regval = (ret | VCNL4040_PS_CONF2_PS_HD);
> > +		ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
> > +						regval);
> > +	}
> > +
> > +	if (ret < 0)
> > +		dev_info(&data->client->dev, "Default to 12 bits sensor data");
> > +
> >   	ret = data->chip_spec->set_power_state(data, true);
> >   	if (ret < 0)
> >   		return ret;
> >
> > ---
> > base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
> > change-id: 20231221-vcnl4000-ps-hd-863f4f8fcea7
> >
> > Best regards,