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

From: Mårten Lindahl
Date: Mon Jan 15 2024 - 05:12:46 EST

On 1/13/24 16:07, Jonathan Cameron wrote:
On Wed, 10 Jan 2024 13:58:07 +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. Please note that in order to preserve the 12 bit
integer part of the in_proximity_raw output, the format is changed from
integer to fixed point.

Signed-off-by: Mårten Lindahl <marten.lindahl@xxxxxxxx>
Hi Mårten,

I don't understand why we'd try to carry on with using the device if
the additional register accesses fail. Just fail the probe, the hardware
is broken.

Jonathan

Hi Jonathan!

Fair point. I'll fail the probe if the register accesses fail. It's unlikely that the device would work if these fail.

Kind regards

Mårten


---
Changes in v2:
- Update registers for sample rate to align it with 16 bit sensor readings.
- Change sysfs output from IIO_VAL_INT to IIO_VAL_FRACTIONAL and scale by 16.
- Link to v1: https://lore.kernel.org/r/20231221-vcnl4000-ps-hd-v1-1-a024bfb28896@xxxxxxxx
---
drivers/iio/light/vcnl4000.c | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index fdf763a04b0b..a078c573131e 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 */
@@ -114,6 +115,13 @@
#define VCNL4010_INT_DRDY \
(BIT(VCNL4010_INT_PROXIMITY) | BIT(VCNL4010_INT_ALS))
+#define VCNL4040_CONF3_PS_MPS_16BITS 3 /* 8 multi pulses */
+#define VCNL4040_CONF3_PS_LED_I_16BITS 3 /* 120 mA */
+
+#define VCNL4040_CONF3_PS_SAMPLE_16BITS \
+ (FIELD_PREP(VCNL4040_PS_CONF3_MPS, VCNL4040_CONF3_PS_MPS_16BITS) | \
+ FIELD_PREP(VCNL4040_PS_MS_LED_I, VCNL4040_CONF3_PS_LED_I_16BITS))
+
static const int vcnl4010_prox_sampling_frequency[][2] = {
{1, 950000},
{3, 906250},
@@ -195,6 +203,7 @@ struct vcnl4000_data {
enum vcnl4000_device_ids id;
int rev;
int al_scale;
+ int ps_scale;
u8 ps_int; /* proximity interrupt mode */
u8 als_int; /* ambient light interrupt mode*/
const struct vcnl4000_chip_spec *chip_spec;
@@ -345,6 +354,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)
@@ -386,9 +396,36 @@ static int vcnl4200_init(struct vcnl4000_data *data)
break;
}
data->al_scale = data->chip_spec->ulux_step;
+ data->ps_scale = 16;
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);
Trivial but those brackets are unnecessary.

+ 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");
Silly question - how can we get here? If it's a case of a read or a write failing
then that's a critical failure and we have very little idea what the state is.
If that happens I'd just fail to probe the driver rather than carrying on with
12 bit assumed.

+ data->ps_scale = 1;
+ goto out;
+ }
+
+ /* Align proximity sensor sample rate to 16 bits data width */
+ ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF3);
+ if (ret >= 0) {
If the read fails, something is very wrong. Fail the probe.

+ regval = (ret | VCNL4040_CONF3_PS_SAMPLE_16BITS);
+ ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF3,
+ regval);
+ }
+
+ if (ret < 0)
+ dev_warn(&data->client->dev, "Sample rate may not be accurate");
If this fails, fail the probe. Don't try to carry on anyway.


+
+out:
ret = data->chip_spec->set_power_state(data, true);
if (ret < 0)
return ret;
@@ -901,8 +938,9 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
break;
case IIO_PROXIMITY:
ret = data->chip_spec->measure_proximity(data, val);
+ *val2 = data->ps_scale;
if (!ret)
- ret = IIO_VAL_INT;
+ ret = IIO_VAL_FRACTIONAL;
break;
default:
ret = -EINVAL;

---
base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
change-id: 20231221-vcnl4000-ps-hd-863f4f8fcea7

Best regards,