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

[PATCH v3 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

Changes in v3:
 - Rename defines for ALS/PS shutdown bits
 - Add local variable for building register value

Changes in v2:
 - Removed unnecessary switch for chip id
 - Guard read/write sequence against potential race
 - Remove confusing boolean operation
 - Use bitmask macros instead of local field shifting
 - Use .read_avail callback instead of using IIO_CONST_ATTR
 - Skip [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040

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 | 185 +++++++++++++++++++++++++++++++++--
 1 file changed, 179 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH v3 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..208539ad50c2 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_SHUTDOWN	BIT(0)
+#define VCNL4040_PS_CONF1_PS_SHUTDOWN	BIT(0)
+
 /* 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_SHUTDOWN;
+	else
+		ret |= VCNL4040_ALS_CONF_ALS_SHUTDOWN;
+
+	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_SHUTDOWN;
+	else
+		ret |= VCNL4040_PS_CONF1_PS_SHUTDOWN;
+
+	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 v3 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 | 131 ++++++++++++++++++++++++++++++++++-
 1 file changed, 128 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 208539ad50c2..2a76b19d1cd4 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_SHUTDOWN	BIT(0)
 #define VCNL4040_PS_CONF1_PS_SHUTDOWN	BIT(0)
+#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,57 @@ 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;
+	u16 regval;
+
+	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;
+
+	regval = (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,
+					regval);
+
+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 +570,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 +949,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 +977,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 +1012,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 v3 0/2] Add ps_it attributes for vcnl4040

[Marten Lindahl]

On Mon, Sep 26, 2022 at 11:18:58AM +0200, Mårten Lindahl wrote:
> 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

Hi!

I suspect this mail may have slipped through unnoticed since there has
not been any comments on it. v2 had some minor comments which I hope I
fixed.

Jonathan?

Kind regards
Mårten

> 
> Changes in v3:
>  - Rename defines for ALS/PS shutdown bits
>  - Add local variable for building register value
> 
> Changes in v2:
>  - Removed unnecessary switch for chip id
>  - Guard read/write sequence against potential race
>  - Remove confusing boolean operation
>  - Use bitmask macros instead of local field shifting
>  - Use .read_avail callback instead of using IIO_CONST_ATTR
>  - Skip [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040
> 
> 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 | 185 +++++++++++++++++++++++++++++++++--
>  1 file changed, 179 insertions(+), 6 deletions(-)
> 
> -- 
> 2.30.2
> 

Re: [PATCH v3 0/2] Add ps_it attributes for vcnl4040

[Jonathan Cameron]

On Mon, 7 Nov 2022 08:47:54 +0100
Marten Lindahl <martenli@axis.com> wrote:

> On Mon, Sep 26, 2022 at 11:18:58AM +0200, Mårten Lindahl wrote:
> > 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  
> 
> Hi!
> 
> I suspect this mail may have slipped through unnoticed since there has
> not been any comments on it. v2 had some minor comments which I hope I
> fixed.
> 
> Jonathan?

It's queued up. Not sure why I failed to send an email though - I'll probably
find it's in my outbox on the other laptop or something equally silly.

https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/drivers/iio/light/vcnl4000.c?h=testing&id=1a09d52bf0cab10580ab52f04f8b182f279be455

Jonathan

> 
> Kind regards
> Mårten
> 
> > 
> > Changes in v3:
> >  - Rename defines for ALS/PS shutdown bits
> >  - Add local variable for building register value
> > 
> > Changes in v2:
> >  - Removed unnecessary switch for chip id
> >  - Guard read/write sequence against potential race
> >  - Remove confusing boolean operation
> >  - Use bitmask macros instead of local field shifting
> >  - Use .read_avail callback instead of using IIO_CONST_ATTR
> >  - Skip [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040
> > 
> > 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 | 185 +++++++++++++++++++++++++++++++++--
> >  1 file changed, 179 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.30.2
> >   

Re: [PATCH v3 0/2] Add ps_it attributes for vcnl4040

[Marten Lindahl]

On Mon, Nov 07, 2022 at 05:44:31PM +0100, Jonathan Cameron wrote:
> On Mon, 7 Nov 2022 08:47:54 +0100
> Marten Lindahl <martenli@axis.com> wrote:
> 
> > On Mon, Sep 26, 2022 at 11:18:58AM +0200, Mårten Lindahl wrote:
> > > 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  
> > 
> > Hi!
> > 
> > I suspect this mail may have slipped through unnoticed since there has
> > not been any comments on it. v2 had some minor comments which I hope I
> > fixed.
> > 
> > Jonathan?
> 
> It's queued up. Not sure why I failed to send an email though - I'll probably
> find it's in my outbox on the other laptop or something equally silly.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/drivers/iio/light/vcnl4000.c?h=testing&id=1a09d52bf0cab10580ab52f04f8b182f279be455
> 
> Jonathan

Ok, then I know. Thanks!
Kind regards
Mårten
> 
> > 
> > Kind regards
> > Mårten
> > 
> > > 
> > > Changes in v3:
> > >  - Rename defines for ALS/PS shutdown bits
> > >  - Add local variable for building register value
> > > 
> > > Changes in v2:
> > >  - Removed unnecessary switch for chip id
> > >  - Guard read/write sequence against potential race
> > >  - Remove confusing boolean operation
> > >  - Use bitmask macros instead of local field shifting
> > >  - Use .read_avail callback instead of using IIO_CONST_ATTR
> > >  - Skip [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040
> > > 
> > > 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 | 185 +++++++++++++++++++++++++++++++++--
> > >  1 file changed, 179 insertions(+), 6 deletions(-)
> > > 
> > > -- 
> > > 2.30.2
> > >   
>