[PATCH v2 0/2] Add ps_it attributes for vcnl4040

[PATCH v2 0/2] Add ps_it attributes for vcnl4040

[Mårten Lindahl]

Currently there is no way for userspace to make any configuration of
the VCNL4040 sensors, but only the sensor readings are exported in
sysfs. To support configuration for proximity integration time value,
sysfs attributes for this needs to be exported.

To begin with the runtime power management turns both sensors (ALS, and
PS) on before reading the sensor register values and then switches
them off again. But when doing so it writes the whole register instead
of just switching the power on/off bit. This needs to be fixed in order
to make other persistent configurations.

Kind regards
Mårten Lindahl

Mårten Lindahl (2):
  iio: light: vcnl4000: Preserve conf bits when toggle power
  iio: light: vcnl4000: Add ps_it attributes for vcnl4040

 drivers/iio/light/vcnl4000.c | 183 +++++++++++++++++++++++++++++++++--
 1 file changed, 177 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] iio: light: vcnl4000: Preserve conf bits when toggle power

[Mårten Lindahl]

As the vcnl4040 and vcnl4200 chip uses runtime power management for
turning the ambient light and proximity sensors on/off, it overwrites
the entire register each time. In ALS_CONF register bit fields ALS_IT,
ALS_PERS, ALS_INT_EN are overwritten. In PS_CONF1 register bit fields
PS_DUTY, PS_PERS, PS_IT, PS_HD, and PS_INT are overwritten.

Add functions for preserving the affected bit fields when changing power
state.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/iio/light/vcnl4000.c | 54 ++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 3db4e26731bb..b2ecf8af1aa5 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -74,6 +74,9 @@
 #define VCNL4000_PROX_EN	BIT(1) /* start proximity measurement */
 #define VCNL4000_SELF_TIMED_EN	BIT(0) /* start self-timed measurement */
 
+#define VCNL4040_ALS_CONF_ALS_SD	BIT(0) /* Enable ambient light sensor */
+#define VCNL4040_PS_CONF1_PS_SD	BIT(0) /* Enable proximity sensor */
+
 /* Bit masks for interrupt registers. */
 #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
 #define VCNL4010_INT_THR_EN	BIT(1) /* Threshold interrupt type */
@@ -188,16 +191,61 @@ static int vcnl4000_init(struct vcnl4000_data *data)
 	return data->chip_spec->set_power_state(data, true);
 };
 
+static ssize_t vcnl4000_write_als_enable(struct vcnl4000_data *data, bool en)
+{
+	int ret;
+
+	mutex_lock(&data->vcnl4000_lock);
+
+	ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
+	if (ret < 0)
+		goto out;
+
+	if (en)
+		ret &= ~VCNL4040_ALS_CONF_ALS_SD;
+	else
+		ret |= VCNL4040_ALS_CONF_ALS_SD;
+
+	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, ret);
+
+out:
+	mutex_unlock(&data->vcnl4000_lock);
+
+	return ret;
+}
+
+static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, bool en)
+{
+	int ret;
+
+	mutex_lock(&data->vcnl4000_lock);
+
+	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
+	if (ret < 0)
+		goto out;
+
+	if (en)
+		ret &= ~VCNL4040_PS_CONF1_PS_SD;
+	else
+		ret |= VCNL4040_PS_CONF1_PS_SD;
+
+	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret);
+
+out:
+	mutex_unlock(&data->vcnl4000_lock);
+
+	return ret;
+}
+
 static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on)
 {
-	u16 val = on ? 0 /* power on */ : 1 /* shut down */;
 	int ret;
 
-	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, val);
+	ret = vcnl4000_write_als_enable(data, on);
 	if (ret < 0)
 		return ret;
 
-	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
+	ret = vcnl4000_write_ps_enable(data, on);
 	if (ret < 0)
 		return ret;
 
-- 
2.30.2

[PATCH v2 2/2] iio: light: vcnl4000: Add ps_it attributes for vcnl4040

[Mårten Lindahl]

Add read/write attribute for proximity integration time, and read
attribute for available proximity integration times for the vcnl4040
chip.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/iio/light/vcnl4000.c | 129 ++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index b2ecf8af1aa5..056079b592c6 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -17,6 +17,7 @@
  *   interrupts (VCNL4040, VCNL4200)
  */
 
+#include <linux/bitfield.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/err.h>
@@ -76,6 +77,7 @@
 
 #define VCNL4040_ALS_CONF_ALS_SD	BIT(0) /* Enable ambient light sensor */
 #define VCNL4040_PS_CONF1_PS_SD	BIT(0) /* Enable proximity sensor */
+#define VCNL4040_PS_CONF2_PS_IT	GENMASK(3, 1) /* Proximity integration time */
 
 /* Bit masks for interrupt registers. */
 #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
@@ -104,6 +106,17 @@ static const int vcnl4010_prox_sampling_frequency[][2] = {
 	{250, 0},
 };
 
+static const int vcnl4040_ps_it_times[][2] = {
+	{0, 100},
+	{0, 150},
+	{0, 200},
+	{0, 250},
+	{0, 300},
+	{0, 350},
+	{0, 400},
+	{0, 800},
+};
+
 #define VCNL4000_SLEEP_DELAY_MS	2000 /* before we enter pm_runtime_suspend */
 
 enum vcnl4000_device_ids {
@@ -470,6 +483,55 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
 	return ret;
 }
 
+static int vcnl4040_read_ps_it(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_IT, ret);
+
+	if (ret >= ARRAY_SIZE(vcnl4040_ps_it_times))
+		return -EINVAL;
+
+	*val = vcnl4040_ps_it_times[ret][0];
+	*val2 = vcnl4040_ps_it_times[ret][1];
+
+	return 0;
+}
+
+static ssize_t vcnl4040_write_ps_it(struct vcnl4000_data *data, int val)
+{
+	unsigned int i;
+	int ret, index = -1;
+
+	for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_it_times); i++) {
+		if (val == vcnl4040_ps_it_times[i][1]) {
+			index = i;
+			break;
+		}
+	}
+
+	if (index < 0)
+		return -EINVAL;
+
+	mutex_lock(&data->vcnl4000_lock);
+
+	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
+	if (ret < 0)
+		goto out;
+
+	ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) |
+	    FIELD_PREP(VCNL4040_PS_CONF2_PS_IT, index);
+	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret);
+
+out:
+	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)
@@ -506,6 +568,47 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 		*val = 0;
 		*val2 = data->al_scale;
 		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_INT_TIME:
+		if (chan->type != IIO_PROXIMITY)
+			return -EINVAL;
+		ret = vcnl4040_read_ps_it(data, val, val2);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl4040_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val != 0)
+			return -EINVAL;
+		if (chan->type != IIO_PROXIMITY)
+			return -EINVAL;
+		return vcnl4040_write_ps_it(data, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl4040_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*vals = (int *)vcnl4040_ps_it_times;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = 2 * ARRAY_SIZE(vcnl4040_ps_it_times);
+		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}
@@ -844,6 +947,20 @@ static const struct iio_chan_spec vcnl4010_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(1),
 };
 
+static const struct iio_chan_spec vcnl4040_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+	}, {
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.ext_info = vcnl4000_ext_info,
+	}
+};
+
 static const struct iio_info vcnl4000_info = {
 	.read_raw = vcnl4000_read_raw,
 };
@@ -858,6 +975,12 @@ static const struct iio_info vcnl4010_info = {
 	.write_event_config = vcnl4010_write_event_config,
 };
 
+static const struct iio_info vcnl4040_info = {
+	.read_raw = vcnl4000_read_raw,
+	.write_raw = vcnl4040_write_raw,
+	.read_avail = vcnl4040_read_avail,
+};
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
@@ -887,9 +1010,9 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.measure_light = vcnl4200_measure_light,
 		.measure_proximity = vcnl4200_measure_proximity,
 		.set_power_state = vcnl4200_set_power_state,
-		.channels = vcnl4000_channels,
-		.num_channels = ARRAY_SIZE(vcnl4000_channels),
-		.info = &vcnl4000_info,
+		.channels = vcnl4040_channels,
+		.num_channels = ARRAY_SIZE(vcnl4040_channels),
+		.info = &vcnl4040_info,
 		.irq_support = false,
 	},
 	[VCNL4200] = {
-- 
2.30.2

Re: [PATCH v2 1/2] iio: light: vcnl4000: Preserve conf bits when toggle power

[Jonathan Cameron]

On Fri, 23 Sep 2022 13:40:30 +0200
Mårten Lindahl <marten.lindahl@axis.com> wrote:

> As the vcnl4040 and vcnl4200 chip uses runtime power management for
> turning the ambient light and proximity sensors on/off, it overwrites
> the entire register each time. In ALS_CONF register bit fields ALS_IT,
> ALS_PERS, ALS_INT_EN are overwritten. In PS_CONF1 register bit fields
> PS_DUTY, PS_PERS, PS_IT, PS_HD, and PS_INT are overwritten.
> 
> Add functions for preserving the affected bit fields when changing power
> state.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
See inline.
Otherwise looks good to me.

Jonathan

> ---
>  drivers/iio/light/vcnl4000.c | 54 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 3db4e26731bb..b2ecf8af1aa5 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -74,6 +74,9 @@
>  #define VCNL4000_PROX_EN	BIT(1) /* start proximity measurement */
>  #define VCNL4000_SELF_TIMED_EN	BIT(0) /* start self-timed measurement */
>  
> +#define VCNL4040_ALS_CONF_ALS_SD	BIT(0) /* Enable ambient light sensor */

Comment seems inverted. Bit being set is 'shut down'  I would expand the
name to
VCNL4040_ALS_CONF_ALS_SHUTDOWN 
then drop the comment as the name is self explanatory

> +#define VCNL4040_PS_CONF1_PS_SD	BIT(0) /* Enable proximity sensor */
> +
>  /* Bit masks for interrupt registers. */
>  #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
>  #define VCNL4010_INT_THR_EN	BIT(1) /* Threshold interrupt type */
> @@ -188,16 +191,61 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>  	return data->chip_spec->set_power_state(data, true);
>  };
>  
> +static ssize_t vcnl4000_write_als_enable(struct vcnl4000_data *data, bool en)
> +{
> +	int ret;
> +
> +	mutex_lock(&data->vcnl4000_lock);
> +
> +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (en)
> +		ret &= ~VCNL4040_ALS_CONF_ALS_SD;
> +	else
> +		ret |= VCNL4040_ALS_CONF_ALS_SD;
> +
> +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, ret);
> +
> +out:
> +	mutex_unlock(&data->vcnl4000_lock);
> +
> +	return ret;
> +}
> +
> +static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, bool en)
> +{
> +	int ret;
> +
> +	mutex_lock(&data->vcnl4000_lock);
> +
> +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (en)
> +		ret &= ~VCNL4040_PS_CONF1_PS_SD;
> +	else
> +		ret |= VCNL4040_PS_CONF1_PS_SD;
> +
> +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret);
> +
> +out:
> +	mutex_unlock(&data->vcnl4000_lock);
> +
> +	return ret;
> +}
> +
>  static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on)
>  {
> -	u16 val = on ? 0 /* power on */ : 1 /* shut down */;
>  	int ret;
>  
> -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, val);
> +	ret = vcnl4000_write_als_enable(data, on);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
> +	ret = vcnl4000_write_ps_enable(data, on);
>  	if (ret < 0)
>  		return ret;
>  

Re: [PATCH v2 2/2] iio: light: vcnl4000: Add ps_it attributes for vcnl4040

[Jonathan Cameron]

On Fri, 23 Sep 2022 13:40:31 +0200
Mårten Lindahl <marten.lindahl@axis.com> wrote:

> Add read/write attribute for proximity integration time, and read
> attribute for available proximity integration times for the vcnl4040
> chip.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
Hi Mårten,

One minor comment inline given I've asked for changes that mean you'll
probably be doing a v3 anyway.

Thanks,

Jonathan

> ---
>  drivers/iio/light/vcnl4000.c | 129 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 126 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index b2ecf8af1aa5..056079b592c6 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -17,6 +17,7 @@
>   *   interrupts (VCNL4040, VCNL4200)
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/err.h>
> @@ -76,6 +77,7 @@
>  
>  #define VCNL4040_ALS_CONF_ALS_SD	BIT(0) /* Enable ambient light sensor */
>  #define VCNL4040_PS_CONF1_PS_SD	BIT(0) /* Enable proximity sensor */
> +#define VCNL4040_PS_CONF2_PS_IT	GENMASK(3, 1) /* Proximity integration time */
>  
>  /* Bit masks for interrupt registers. */
>  #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
> @@ -104,6 +106,17 @@ static const int vcnl4010_prox_sampling_frequency[][2] = {
>  	{250, 0},
>  };
>  
> +static const int vcnl4040_ps_it_times[][2] = {
> +	{0, 100},
> +	{0, 150},
> +	{0, 200},
> +	{0, 250},
> +	{0, 300},
> +	{0, 350},
> +	{0, 400},
> +	{0, 800},
> +};
> +
>  #define VCNL4000_SLEEP_DELAY_MS	2000 /* before we enter pm_runtime_suspend */
>  
>  enum vcnl4000_device_ids {
> @@ -470,6 +483,55 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
>  	return ret;
>  }
>  
> +static int vcnl4040_read_ps_it(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_IT, ret);
> +
> +	if (ret >= ARRAY_SIZE(vcnl4040_ps_it_times))
> +		return -EINVAL;
> +
> +	*val = vcnl4040_ps_it_times[ret][0];
> +	*val2 = vcnl4040_ps_it_times[ret][1];
> +
> +	return 0;
> +}
> +
> +static ssize_t vcnl4040_write_ps_it(struct vcnl4000_data *data, int val)
> +{
> +	unsigned int i;
> +	int ret, index = -1;
> +
> +	for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_it_times); i++) {
> +		if (val == vcnl4040_ps_it_times[i][1]) {
> +			index = i;
> +			break;
> +		}
> +	}
> +
> +	if (index < 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->vcnl4000_lock);
> +
> +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) |
> +	    FIELD_PREP(VCNL4040_PS_CONF2_PS_IT, index);

It can be confusing to read ret both as a temporary to build value ad for the
return code. I would introduce a
u16 val
and build the value in that.

> +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret);
> +
> +out:
> +	mutex_unlock(&data->vcnl4000_lock);
> +	return ret;
> +}
> +

Re: [PATCH v2 1/2] iio: light: vcnl4000: Preserve conf bits when toggle power

[Marten Lindahl]

On Sat, Sep 24, 2022 at 06:37:24PM +0200, Jonathan Cameron wrote:
> On Fri, 23 Sep 2022 13:40:30 +0200
> Mårten Lindahl <marten.lindahl@axis.com> wrote:
> 
> > As the vcnl4040 and vcnl4200 chip uses runtime power management for
> > turning the ambient light and proximity sensors on/off, it overwrites
> > the entire register each time. In ALS_CONF register bit fields ALS_IT,
> > ALS_PERS, ALS_INT_EN are overwritten. In PS_CONF1 register bit fields
> > PS_DUTY, PS_PERS, PS_IT, PS_HD, and PS_INT are overwritten.
> > 
> > Add functions for preserving the affected bit fields when changing power
> > state.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> See inline.
> Otherwise looks good to me.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/light/vcnl4000.c | 54 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 51 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 3db4e26731bb..b2ecf8af1aa5 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -74,6 +74,9 @@
> >  #define VCNL4000_PROX_EN	BIT(1) /* start proximity measurement */
> >  #define VCNL4000_SELF_TIMED_EN	BIT(0) /* start self-timed measurement */
> >  
> > +#define VCNL4040_ALS_CONF_ALS_SD	BIT(0) /* Enable ambient light sensor */
>

Hi Jonathan!

> Comment seems inverted. Bit being set is 'shut down'  I would expand the
> name to
> VCNL4040_ALS_CONF_ALS_SHUTDOWN 
> then drop the comment as the name is self explanatory

Ok, I'll do so. Thanks!

Kind regards
Mårten

> 
> > +#define VCNL4040_PS_CONF1_PS_SD	BIT(0) /* Enable proximity sensor */
> > +
> >  /* Bit masks for interrupt registers. */
> >  #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
> >  #define VCNL4010_INT_THR_EN	BIT(1) /* Threshold interrupt type */
> > @@ -188,16 +191,61 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> >  	return data->chip_spec->set_power_state(data, true);
> >  };
> >  
> > +static ssize_t vcnl4000_write_als_enable(struct vcnl4000_data *data, bool en)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&data->vcnl4000_lock);
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (en)
> > +		ret &= ~VCNL4040_ALS_CONF_ALS_SD;
> > +	else
> > +		ret |= VCNL4040_ALS_CONF_ALS_SD;
> > +
> > +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, ret);
> > +
> > +out:
> > +	mutex_unlock(&data->vcnl4000_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, bool en)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&data->vcnl4000_lock);
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (en)
> > +		ret &= ~VCNL4040_PS_CONF1_PS_SD;
> > +	else
> > +		ret |= VCNL4040_PS_CONF1_PS_SD;
> > +
> > +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret);
> > +
> > +out:
> > +	mutex_unlock(&data->vcnl4000_lock);
> > +
> > +	return ret;
> > +}
> > +
> >  static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on)
> >  {
> > -	u16 val = on ? 0 /* power on */ : 1 /* shut down */;
> >  	int ret;
> >  
> > -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, val);
> > +	ret = vcnl4000_write_als_enable(data, on);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
> > +	ret = vcnl4000_write_ps_enable(data, on);
> >  	if (ret < 0)
> >  		return ret;
> >  
> 

Re: [PATCH v2 2/2] iio: light: vcnl4000: Add ps_it attributes for vcnl4040

[Marten Lindahl]

On Sat, Sep 24, 2022 at 06:40:44PM +0200, Jonathan Cameron wrote:
> On Fri, 23 Sep 2022 13:40:31 +0200
> Mårten Lindahl <marten.lindahl@axis.com> wrote:
> 
> > Add read/write attribute for proximity integration time, and read
> > attribute for available proximity integration times for the vcnl4040
> > chip.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> Hi Mårten,
> 
> One minor comment inline given I've asked for changes that mean you'll
> probably be doing a v3 anyway.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/light/vcnl4000.c | 129 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 126 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index b2ecf8af1aa5..056079b592c6 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -17,6 +17,7 @@
> >   *   interrupts (VCNL4040, VCNL4200)
> >   */
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/module.h>
> >  #include <linux/i2c.h>
> >  #include <linux/err.h>
> > @@ -76,6 +77,7 @@
> >  
> >  #define VCNL4040_ALS_CONF_ALS_SD	BIT(0) /* Enable ambient light sensor */
> >  #define VCNL4040_PS_CONF1_PS_SD	BIT(0) /* Enable proximity sensor */
> > +#define VCNL4040_PS_CONF2_PS_IT	GENMASK(3, 1) /* Proximity integration time */
> >  
> >  /* Bit masks for interrupt registers. */
> >  #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
> > @@ -104,6 +106,17 @@ static const int vcnl4010_prox_sampling_frequency[][2] = {
> >  	{250, 0},
> >  };
> >  
> > +static const int vcnl4040_ps_it_times[][2] = {
> > +	{0, 100},
> > +	{0, 150},
> > +	{0, 200},
> > +	{0, 250},
> > +	{0, 300},
> > +	{0, 350},
> > +	{0, 400},
> > +	{0, 800},
> > +};
> > +
> >  #define VCNL4000_SLEEP_DELAY_MS	2000 /* before we enter pm_runtime_suspend */
> >  
> >  enum vcnl4000_device_ids {
> > @@ -470,6 +483,55 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
> >  	return ret;
> >  }
> >  
> > +static int vcnl4040_read_ps_it(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_IT, ret);
> > +
> > +	if (ret >= ARRAY_SIZE(vcnl4040_ps_it_times))
> > +		return -EINVAL;
> > +
> > +	*val = vcnl4040_ps_it_times[ret][0];
> > +	*val2 = vcnl4040_ps_it_times[ret][1];
> > +
> > +	return 0;
> > +}
> > +
> > +static ssize_t vcnl4040_write_ps_it(struct vcnl4000_data *data, int val)
> > +{
> > +	unsigned int i;
> > +	int ret, index = -1;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(vcnl4040_ps_it_times); i++) {
> > +		if (val == vcnl4040_ps_it_times[i][1]) {
> > +			index = i;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (index < 0)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&data->vcnl4000_lock);
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) |
> > +	    FIELD_PREP(VCNL4040_PS_CONF2_PS_IT, index);
> 
> It can be confusing to read ret both as a temporary to build value ad for the
> return code. I would introduce a
> u16 val
> and build the value in that.

Hi Jonathan!

Thanks. I'll do so. As there already is a 'val' I will call it 'regval'.

Kind regards
Mårten
> 
> > +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret);
> > +
> > +out:
> > +	mutex_unlock(&data->vcnl4000_lock);
> > +	return ret;
> > +}
> > +