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@xxxxxxxx> 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@xxxxxxxx>
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),
