The vcnl4040 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).
Add read/write attribute for proximity resolution, and read attribute
for available proximity resolution values for the vcnl4040 chip.
Signed-off-by: Mårten Lindahl <[email protected]>
---
drivers/iio/light/vcnl4000.c | 87 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 85 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index fdf763a04b0b..2addff635b79 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 */
@@ -170,6 +171,11 @@ static const int vcnl4040_ps_calibbias_ua[][2] = {
{0, 200000},
};
+static const int vcnl4040_ps_resolutions[2] = {
+ 12,
+ 16
+};
+
static const int vcnl4040_als_persistence[] = {1, 2, 4, 8};
static const int vcnl4040_ps_persistence[] = {1, 2, 3, 4};
static const int vcnl4040_ps_oversampling_ratio[] = {1, 2, 4, 8};
@@ -880,6 +886,54 @@ static ssize_t vcnl4040_write_ps_calibbias(struct vcnl4000_data *data, int val)
return ret;
}
+static ssize_t vcnl4040_read_ps_resolution(struct vcnl4000_data *data, int *val, int *val2)
+{
+ int ret;
+
+ ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
+ if (ret < 0)
+ return ret;
+
+ ret = FIELD_GET(VCNL4040_PS_CONF2_PS_HD, ret);
+ if (ret >= ARRAY_SIZE(vcnl4040_ps_resolutions))
+ return -EINVAL;
+
+ *val = vcnl4040_ps_resolutions[ret];
+ *val2 = 0;
+
+ return ret;
+}
+
+static ssize_t vcnl4040_write_ps_resolution(struct vcnl4000_data *data, int val)
+{
+ unsigned int i;
+ int ret;
+ u16 regval;
+
+ for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_resolutions); i++) {
+ if (val == vcnl4040_ps_resolutions[i])
+ break;
+ }
+
+ if (i >= ARRAY_SIZE(vcnl4040_ps_resolutions))
+ return -EINVAL;
+
+ mutex_lock(&data->vcnl4000_lock);
+
+ ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
+ if (ret < 0)
+ goto out_unlock;
+
+ regval = (ret & ~VCNL4040_PS_CONF2_PS_HD);
+ regval |= FIELD_PREP(VCNL4040_PS_CONF2_PS_HD, i);
+ ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
+ regval);
+
+out_unlock:
+ mutex_unlock(&data->vcnl4000_lock);
+ return ret;
+}
+
static int vcnl4000_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -950,6 +1004,16 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_CHAN_INFO_RESOLUTION:
+ switch (chan->type) {
+ case IIO_PROXIMITY:
+ ret = vcnl4040_read_ps_resolution(data, val, val2);
+ if (ret < 0)
+ return ret;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
@@ -987,6 +1051,13 @@ static int vcnl4040_write_raw(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_CHAN_INFO_RESOLUTION:
+ switch (chan->type) {
+ case IIO_PROXIMITY:
+ return vcnl4040_write_ps_resolution(data, val);
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
@@ -1035,6 +1106,16 @@ static int vcnl4040_read_avail(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_CHAN_INFO_RESOLUTION:
+ switch (chan->type) {
+ case IIO_PROXIMITY:
+ *vals = (int *)vcnl4040_ps_resolutions;
+ *length = ARRAY_SIZE(vcnl4040_ps_resolutions);
+ *type = IIO_VAL_INT;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
@@ -1808,10 +1889,12 @@ static const struct iio_chan_spec vcnl4040_channels[] = {
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_INT_TIME) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
- BIT(IIO_CHAN_INFO_CALIBBIAS),
+ BIT(IIO_CHAN_INFO_CALIBBIAS) |
+ BIT(IIO_CHAN_INFO_RESOLUTION),
.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
- BIT(IIO_CHAN_INFO_CALIBBIAS),
+ BIT(IIO_CHAN_INFO_CALIBBIAS) |
+ BIT(IIO_CHAN_INFO_RESOLUTION),
.ext_info = vcnl4000_ext_info,
.event_spec = vcnl4040_event_spec,
.num_event_specs = ARRAY_SIZE(vcnl4040_event_spec),
--
2.30.2
On Fri, 15 Dec 2023 13:43:05 +0100
Mårten Lindahl <[email protected]> wrote:
> The vcnl4040 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).
>
> Add read/write attribute for proximity resolution, and read attribute
> for available proximity resolution values for the vcnl4040 chip.
>
> Signed-off-by: Mårten Lindahl <[email protected]>
I'll review this on basis the usecase is clear (see reply to cover letter)
The manipulation of the CONF1 and CONF2 registers in a pair is rather odd given you
only want to change one bit here.
Why is that done?
> ---
> drivers/iio/light/vcnl4000.c | 87 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 85 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index fdf763a04b0b..2addff635b79 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 */
> @@ -170,6 +171,11 @@ static const int vcnl4040_ps_calibbias_ua[][2] = {
> {0, 200000},
> };
>
> +static const int vcnl4040_ps_resolutions[2] = {
> + 12,
> + 16
> +};
> +
> static const int vcnl4040_als_persistence[] = {1, 2, 4, 8};
> static const int vcnl4040_ps_persistence[] = {1, 2, 3, 4};
> static const int vcnl4040_ps_oversampling_ratio[] = {1, 2, 4, 8};
> @@ -880,6 +886,54 @@ static ssize_t vcnl4040_write_ps_calibbias(struct vcnl4000_data *data, int val)
> return ret;
> }
>
> +static ssize_t vcnl4040_read_ps_resolution(struct vcnl4000_data *data, int *val, int *val2)
> +{
> + int ret;
> +
> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
The field seems to be in PS_CONF2. So you are reading a word and I guess that
gets you two registers. Can we not do a byte read to get just CONF2?
> + if (ret < 0)
> + return ret;
> +
> + ret = FIELD_GET(VCNL4040_PS_CONF2_PS_HD, ret);
> + if (ret >= ARRAY_SIZE(vcnl4040_ps_resolutions))
> + return -EINVAL;
> +
> + *val = vcnl4040_ps_resolutions[ret];
> + *val2 = 0;
> +
> + return ret;
> +}
> +
> +static ssize_t vcnl4040_write_ps_resolution(struct vcnl4000_data *data, int val)
> +{
> + unsigned int i;
> + int ret;
> + u16 regval;
> +
> + for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_resolutions); i++) {
> + if (val == vcnl4040_ps_resolutions[i])
> + break;
> + }
> +
> + if (i >= ARRAY_SIZE(vcnl4040_ps_resolutions))
> + return -EINVAL;
> +
> + mutex_lock(&data->vcnl4000_lock);
> +
> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> + if (ret < 0)
> + goto out_unlock;
> +
> + regval = (ret & ~VCNL4040_PS_CONF2_PS_HD);
> + regval |= FIELD_PREP(VCNL4040_PS_CONF2_PS_HD, i);
> + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
> + regval);
> +
> +out_unlock:
> + mutex_unlock(&data->vcnl4000_lock);
> + return ret;
> +}
c),
>
On 12/17/23 15:15, Jonathan Cameron wrote:
> On Fri, 15 Dec 2023 13:43:05 +0100
> Mårten Lindahl <[email protected]> wrote:
>
>> The vcnl4040 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).
>>
>> Add read/write attribute for proximity resolution, and read attribute
>> for available proximity resolution values for the vcnl4040 chip.
>>
>> Signed-off-by: Mårten Lindahl <[email protected]>
Hi Jonathan!
> I'll review this on basis the usecase is clear (see reply to cover letter)
I'll skip this patch (see reply to cover letter comment)
Your are right about the paired register manipulation. Better to
read/write byte instead of word.
Kind regards
Mårten
>
> The manipulation of the CONF1 and CONF2 registers in a pair is rather odd given you
> only want to change one bit here.
>
> Why is that done?
>> ---
>> drivers/iio/light/vcnl4000.c | 87 +++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 85 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
>> index fdf763a04b0b..2addff635b79 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 */
>> @@ -170,6 +171,11 @@ static const int vcnl4040_ps_calibbias_ua[][2] = {
>> {0, 200000},
>> };
>>
>> +static const int vcnl4040_ps_resolutions[2] = {
>> + 12,
>> + 16
>> +};
>> +
>> static const int vcnl4040_als_persistence[] = {1, 2, 4, 8};
>> static const int vcnl4040_ps_persistence[] = {1, 2, 3, 4};
>> static const int vcnl4040_ps_oversampling_ratio[] = {1, 2, 4, 8};
>> @@ -880,6 +886,54 @@ static ssize_t vcnl4040_write_ps_calibbias(struct vcnl4000_data *data, int val)
>> return ret;
>> }
>>
>> +static ssize_t vcnl4040_read_ps_resolution(struct vcnl4000_data *data, int *val, int *val2)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> The field seems to be in PS_CONF2. So you are reading a word and I guess that
> gets you two registers. Can we not do a byte read to get just CONF2?
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = FIELD_GET(VCNL4040_PS_CONF2_PS_HD, ret);
>> + if (ret >= ARRAY_SIZE(vcnl4040_ps_resolutions))
>> + return -EINVAL;
>> +
>> + *val = vcnl4040_ps_resolutions[ret];
>> + *val2 = 0;
>> +
>> + return ret;
>> +}
>> +
>> +static ssize_t vcnl4040_write_ps_resolution(struct vcnl4000_data *data, int val)
>> +{
>> + unsigned int i;
>> + int ret;
>> + u16 regval;
>> +
>> + for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_resolutions); i++) {
>> + if (val == vcnl4040_ps_resolutions[i])
>> + break;
>> + }
>> +
>> + if (i >= ARRAY_SIZE(vcnl4040_ps_resolutions))
>> + return -EINVAL;
>> +
>> + mutex_lock(&data->vcnl4000_lock);
>> +
>> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
>> + if (ret < 0)
>> + goto out_unlock;
>> +
>> + regval = (ret & ~VCNL4040_PS_CONF2_PS_HD);
>> + regval |= FIELD_PREP(VCNL4040_PS_CONF2_PS_HD, i);
>> + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
>> + regval);
>> +
>> +out_unlock:
>> + mutex_unlock(&data->vcnl4000_lock);
>> + return ret;
>> +}
> c),
On 12/18/23 16:19, Mårten Lindahl wrote:
> On 12/17/23 15:15, Jonathan Cameron wrote:
>> On Fri, 15 Dec 2023 13:43:05 +0100
>> Mårten Lindahl <[email protected]> wrote:
>>
>>> The vcnl4040 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).
>>>
>>> Add read/write attribute for proximity resolution, and read attribute
>>> for available proximity resolution values for the vcnl4040 chip.
>>>
>>> Signed-off-by: Mårten Lindahl <[email protected]>
> Hi Jonathan!
>> I'll review this on basis the usecase is clear (see reply to cover
>> letter)
>
> I'll skip this patch (see reply to cover letter comment)
>
> Your are right about the paired register manipulation. Better to
> read/write byte instead of word.
Hi Jonathan!
I now remember why the register is read as a word (CONF1/CONF2). It is
addressed as one 16 bit register where CONF1 is the low 8 bit field and
CONF2 is the high bit field. It is the same address/code for both:
Register PS_CONF1 - COMMAND CODE: 0x03_L (0x03 DATA BYTE LOW)
Register PS_CONF2 - COMMAND CODE: 0x03_H (0x03 DATA BYTE HIGH)
Where in my case (PS_CONF2->PS_HD[3] is the same as PS_CONF1[11])
Kind regards
Mårten
>
> Kind regards
>
> Mårten
>
>>
>> The manipulation of the CONF1 and CONF2 registers in a pair is rather
>> odd given you
>> only want to change one bit here.
>>
>> Why is that done?
>>> ---
>>> drivers/iio/light/vcnl4000.c | 87
>>> +++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 85 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/vcnl4000.c
>>> b/drivers/iio/light/vcnl4000.c
>>> index fdf763a04b0b..2addff635b79 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 */
>>> @@ -170,6 +171,11 @@ static const int vcnl4040_ps_calibbias_ua[][2] = {
>>> {0, 200000},
>>> };
>>> +static const int vcnl4040_ps_resolutions[2] = {
>>> + 12,
>>> + 16
>>> +};
>>> +
>>> static const int vcnl4040_als_persistence[] = {1, 2, 4, 8};
>>> static const int vcnl4040_ps_persistence[] = {1, 2, 3, 4};
>>> static const int vcnl4040_ps_oversampling_ratio[] = {1, 2, 4, 8};
>>> @@ -880,6 +886,54 @@ static ssize_t
>>> vcnl4040_write_ps_calibbias(struct vcnl4000_data *data, int val)
>>> return ret;
>>> }
>>> +static ssize_t vcnl4040_read_ps_resolution(struct vcnl4000_data
>>> *data, int *val, int *val2)
>>> +{
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
>> The field seems to be in PS_CONF2. So you are reading a word and I
>> guess that
>> gets you two registers. Can we not do a byte read to get just CONF2?
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = FIELD_GET(VCNL4040_PS_CONF2_PS_HD, ret);
>>> + if (ret >= ARRAY_SIZE(vcnl4040_ps_resolutions))
>>> + return -EINVAL;
>>> +
>>> + *val = vcnl4040_ps_resolutions[ret];
>>> + *val2 = 0;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static ssize_t vcnl4040_write_ps_resolution(struct vcnl4000_data
>>> *data, int val)
>>> +{
>>> + unsigned int i;
>>> + int ret;
>>> + u16 regval;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_resolutions); i++) {
>>> + if (val == vcnl4040_ps_resolutions[i])
>>> + break;
>>> + }
>>> +
>>> + if (i >= ARRAY_SIZE(vcnl4040_ps_resolutions))
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&data->vcnl4000_lock);
>>> +
>>> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
>>> + if (ret < 0)
>>> + goto out_unlock;
>>> +
>>> + regval = (ret & ~VCNL4040_PS_CONF2_PS_HD);
>>> + regval |= FIELD_PREP(VCNL4040_PS_CONF2_PS_HD, i);
>>> + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
>>> + regval);
>>> +
>>> +out_unlock:
>>> + mutex_unlock(&data->vcnl4000_lock);
>>> + return ret;
>>> +}
>> c),
On Mon, 18 Dec 2023 17:00:10 +0100
Mårten Lindahl <[email protected]> wrote:
> On 12/18/23 16:19, Mårten Lindahl wrote:
> > On 12/17/23 15:15, Jonathan Cameron wrote:
> >> On Fri, 15 Dec 2023 13:43:05 +0100
> >> Mårten Lindahl <[email protected]> wrote:
> >>
> >>> The vcnl4040 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).
> >>>
> >>> Add read/write attribute for proximity resolution, and read attribute
> >>> for available proximity resolution values for the vcnl4040 chip.
> >>>
> >>> Signed-off-by: Mårten Lindahl <[email protected]>
> > Hi Jonathan!
> >> I'll review this on basis the usecase is clear (see reply to cover
> >> letter)
> >
> > I'll skip this patch (see reply to cover letter comment)
> >
> > Your are right about the paired register manipulation. Better to
> > read/write byte instead of word.
>
> Hi Jonathan!
>
> I now remember why the register is read as a word (CONF1/CONF2). It is
> addressed as one 16 bit register where CONF1 is the low 8 bit field and
> CONF2 is the high bit field. It is the same address/code for both:
>
> Register PS_CONF1 - COMMAND CODE: 0x03_L (0x03 DATA BYTE LOW)
>
> Register PS_CONF2 - COMMAND CODE: 0x03_H (0x03 DATA BYTE HIGH)
>
> Where in my case (PS_CONF2->PS_HD[3] is the same as PS_CONF1[11])
Ouch. That's a horrible way to define a register map.
Jonathan
>
> Kind regards
>
> Mårten
>
> >
> > Kind regards
> >
> > Mårten
> >
> >>
> >> The manipulation of the CONF1 and CONF2 registers in a pair is rather
> >> odd given you
> >> only want to change one bit here.
> >>
> >> Why is that done?
> >>> ---
> >>> drivers/iio/light/vcnl4000.c | 87
> >>> +++++++++++++++++++++++++++++++++++++++++++-
> >>> 1 file changed, 85 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/light/vcnl4000.c
> >>> b/drivers/iio/light/vcnl4000.c
> >>> index fdf763a04b0b..2addff635b79 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 */
> >>> @@ -170,6 +171,11 @@ static const int vcnl4040_ps_calibbias_ua[][2] = {
> >>> {0, 200000},
> >>> };
> >>> +static const int vcnl4040_ps_resolutions[2] = {
> >>> + 12,
> >>> + 16
> >>> +};
> >>> +
> >>> static const int vcnl4040_als_persistence[] = {1, 2, 4, 8};
> >>> static const int vcnl4040_ps_persistence[] = {1, 2, 3, 4};
> >>> static const int vcnl4040_ps_oversampling_ratio[] = {1, 2, 4, 8};
> >>> @@ -880,6 +886,54 @@ static ssize_t
> >>> vcnl4040_write_ps_calibbias(struct vcnl4000_data *data, int val)
> >>> return ret;
> >>> }
> >>> +static ssize_t vcnl4040_read_ps_resolution(struct vcnl4000_data
> >>> *data, int *val, int *val2)
> >>> +{
> >>> + int ret;
> >>> +
> >>> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> >> The field seems to be in PS_CONF2. So you are reading a word and I
> >> guess that
> >> gets you two registers. Can we not do a byte read to get just CONF2?
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + ret = FIELD_GET(VCNL4040_PS_CONF2_PS_HD, ret);
> >>> + if (ret >= ARRAY_SIZE(vcnl4040_ps_resolutions))
> >>> + return -EINVAL;
> >>> +
> >>> + *val = vcnl4040_ps_resolutions[ret];
> >>> + *val2 = 0;
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static ssize_t vcnl4040_write_ps_resolution(struct vcnl4000_data
> >>> *data, int val)
> >>> +{
> >>> + unsigned int i;
> >>> + int ret;
> >>> + u16 regval;
> >>> +
> >>> + for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_resolutions); i++) {
> >>> + if (val == vcnl4040_ps_resolutions[i])
> >>> + break;
> >>> + }
> >>> +
> >>> + if (i >= ARRAY_SIZE(vcnl4040_ps_resolutions))
> >>> + return -EINVAL;
> >>> +
> >>> + mutex_lock(&data->vcnl4000_lock);
> >>> +
> >>> + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> >>> + if (ret < 0)
> >>> + goto out_unlock;
> >>> +
> >>> + regval = (ret & ~VCNL4040_PS_CONF2_PS_HD);
> >>> + regval |= FIELD_PREP(VCNL4040_PS_CONF2_PS_HD, i);
> >>> + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
> >>> + regval);
> >>> +
> >>> +out_unlock:
> >>> + mutex_unlock(&data->vcnl4000_lock);
> >>> + return ret;
> >>> +}
> >> c),