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

Mårten Lindahl
2024-01-09T18:25:57Z

On 12/26/23 17:19, Jonathan Cameron wrote:
> On Thu, 21 Dec 2023 17:33:09 +0100
> Mårten Lindahl<marten.lindahl@axis.com> 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@axis.com>
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,