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

[Jonathan Cameron]

On Tue, 9 Jan 2024 19:25:25 +0100
Mårten Lindahl <martenli@xxxxxxxx> wrote:

> 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.

Ok. Then I think we are fine.

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

Should be fine.  Generic userspace code already has to deal with
fixed point values (treating them as if they were fp) as other drivers will
produce this anyway.   We 'might' break a flakey script, but this is
in the category of hoping no one notices!

Jonathan

> 
> 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/vcnl4000c
> >> 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,  
>