[PATCH 0/2] Introduce new iio resolution standard attribute

[PATCH 0/2] Introduce new iio resolution standard attribute

[Mårten Lindahl]

This patch introduces a new IIO standard attribute to set the bit
resolution of the data *_raw readings dynamically using sysfs.

The VCNL4040/4200 proximity/ambient light sensors support 12-bit
(default) and 16-bit ADC resolutions. This can be dynamically changed,
so to support this with the standard iio channel configuration a new iio
attribute should be added.

The VCNL4040 devices will use this for setting proximity high definition
(16-bit resolution).

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
Mårten Lindahl (2):
      iio: core: Introduce resolution standard attribute
      iio: light: vcnl4000: Add ps high definition for vcnl4040

 drivers/iio/industrialio-core.c |  1 +
 drivers/iio/light/vcnl4000.c    | 87 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/iio/types.h       |  1 +
 3 files changed, 87 insertions(+), 2 deletions(-)
---
base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
change-id: 20231212-vcnl4000-ps-hd-38d42abf9095

Best regards,
-- 
Mårten Lindahl <marten.lindahl@axis.com>

[PATCH 1/2] iio: core: Introduce resolution standard attribute

[Mårten Lindahl]

Introduce a new IIO standard attribute to set bit resolution of the
data *_raw readings dynamically using sysfs.

Needed to set 16 bit data width (high definition) with the VCNL4040
driver.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/iio/industrialio-core.c | 1 +
 include/linux/iio/types.h       | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c77745b594bd..4316e0290404 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -182,6 +182,7 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
 	[IIO_CHAN_INFO_CALIBAMBIENT] = "calibambient",
 	[IIO_CHAN_INFO_ZEROPOINT] = "zeropoint",
+	[IIO_CHAN_INFO_RESOLUTION] = "resolution",
 };
 /**
  * iio_device_id() - query the unique ID for the device
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 117bde7d6ad7..7de4d83ca105 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -68,6 +68,7 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_THERMOCOUPLE_TYPE,
 	IIO_CHAN_INFO_CALIBAMBIENT,
 	IIO_CHAN_INFO_ZEROPOINT,
+	IIO_CHAN_INFO_RESOLUTION,
 };
 
 #endif /* _IIO_TYPES_H_ */

-- 
2.30.2

[PATCH 2/2] iio: light: vcnl4000: Add ps high definition for vcnl4040

[Mårten Lindahl]

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 <marten.lindahl@axis.com>
---
 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

Re: [PATCH 0/2] Introduce new iio resolution standard attribute

[Jonathan Cameron]

On Fri, 15 Dec 2023 13:43:03 +0100
Mårten Lindahl <marten.lindahl@axis.com> wrote:

> This patch introduces a new IIO standard attribute to set the bit
> resolution of the data *_raw readings dynamically using sysfs.
> 
> The VCNL4040/4200 proximity/ambient light sensors support 12-bit
> (default) and 16-bit ADC resolutions. This can be dynamically changed,
> so to support this with the standard iio channel configuration a new iio
> attribute should be added.
> 
> The VCNL4040 devices will use this for setting proximity high definition
> (16-bit resolution).
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>

Hi Mårten,

What is the use case?  We've had lots of devices capable of doing this
sort of resolution change, but never yet come up with a reason to do so for
the sysfs interfaces on the basis the overhead of the sysfs interfaces is
high enough the best bet is almost always to use the highest available resolution
and don't worry that the read takes a little longer.

Jonathan

> ---
> Mårten Lindahl (2):
>       iio: core: Introduce resolution standard attribute
>       iio: light: vcnl4000: Add ps high definition for vcnl4040
> 
>  drivers/iio/industrialio-core.c |  1 +
>  drivers/iio/light/vcnl4000.c    | 87 ++++++++++++++++++++++++++++++++++++++++-
>  include/linux/iio/types.h       |  1 +
>  3 files changed, 87 insertions(+), 2 deletions(-)
> ---
> base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
> change-id: 20231212-vcnl4000-ps-hd-38d42abf9095
> 
> Best regards,

Re: [PATCH 2/2] iio: light: vcnl4000: Add ps high definition for vcnl4040

[Jonathan Cameron]

On Fri, 15 Dec 2023 13:43:05 +0100
Mårten Lindahl <marten.lindahl@axis.com> 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 <marten.lindahl@axis.com>
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),
> 

Re: [PATCH 0/2] Introduce new iio resolution standard attribute

[Mårten Lindahl]

On 12/17/23 15:10, Jonathan Cameron wrote:
> On Fri, 15 Dec 2023 13:43:03 +0100
> Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
>> This patch introduces a new IIO standard attribute to set the bit
>> resolution of the data *_raw readings dynamically using sysfs.
>>
>> The VCNL4040/4200 proximity/ambient light sensors support 12-bit
>> (default) and 16-bit ADC resolutions. This can be dynamically changed,
>> so to support this with the standard iio channel configuration a new iio
>> attribute should be added.
>>
>> The VCNL4040 devices will use this for setting proximity high definition
>> (16-bit resolution).
>>
>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> Hi Mårten,
>
> What is the use case?  We've had lots of devices capable of doing this
> sort of resolution change, but never yet come up with a reason to do so for
> the sysfs interfaces on the basis the overhead of the sysfs interfaces is
> high enough the best bet is almost always to use the highest available resolution
> and don't worry that the read takes a little longer.
>
> Jonathan

Hi Jonathan!

My use case probably does not differ from others, in that 12 bits does 
not give enough precision. So it's just a dynamic feature that the 
sensor has, but as you suggest to hard code this to the highest works 
fine for me. I just didn't feel confident enough to do that :)

I'll make a single patch for this change instead. Thanks!

Kind regards

Mårten

>
>> ---
>> Mårten Lindahl (2):
>>        iio: core: Introduce resolution standard attribute
>>        iio: light: vcnl4000: Add ps high definition for vcnl4040
>>
>>   drivers/iio/industrialio-core.c |  1 +
>>   drivers/iio/light/vcnl4000.c    | 87 ++++++++++++++++++++++++++++++++++++++++-
>>   include/linux/iio/types.h       |  1 +
>>   3 files changed, 87 insertions(+), 2 deletions(-)
>> ---
>> base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
>> change-id: 20231212-vcnl4000-ps-hd-38d42abf9095
>>
>> Best regards,

Re: [PATCH 2/2] iio: light: vcnl4000: Add ps high definition for vcnl4040

[Mårten Lindahl]

On 12/17/23 15:15, Jonathan Cameron wrote:
> On Fri, 15 Dec 2023 13:43:05 +0100
> Mårten Lindahl <marten.lindahl@axis.com> 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 <marten.lindahl@axis.com>
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),

Re: [PATCH 2/2] iio: light: vcnl4000: Add ps high definition for vcnl4040

[Mårten Lindahl]

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 <marten.lindahl@axis.com> 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 <marten.lindahl@axis.com>
> 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),

Re: [PATCH 2/2] iio: light: vcnl4000: Add ps high definition for vcnl4040

[Jonathan Cameron]

On Mon, 18 Dec 2023 17:00:10 +0100
Mårten Lindahl <martenli@axis.com> 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 <marten.lindahl@axis.com> 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 <marten.lindahl@axis.com>  
> > 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),