[PATCH 0/3] Add basic attributes for vcnl4040

[PATCH 0/3] Add basic attributes for vcnl4040

[Mårten Lindahl]

Hi!

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 basic control of the sensors such as enable/disable
and setting 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 switching
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 (3):
  iio: light: vcnl4000: Preserve conf bits when toggle power
  iio: light: vcnl4000: Add enable attributes for vcnl4040
  iio: light: vcnl4000: Add ps_it attributes for vcnl4040

 drivers/iio/light/vcnl4000.c | 205 ++++++++++++++++++++++++++++++++++-
 1 file changed, 200 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH 1/3] 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 | 53 ++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 3db4e26731bb..0b226c684957 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,62 @@ 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, int val)
+{
+	int ret;
+
+	switch (data->id) {
+	case VCNL4040:
+	case VCNL4200:
+		ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
+		if (ret < 0)
+			return ret;
+
+		if (val)
+			ret &= ~VCNL4040_ALS_CONF_ALS_SD;
+		else
+			ret |= VCNL4040_ALS_CONF_ALS_SD;
+
+		return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF,
+						 ret);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, int val)
+{
+	int ret;
+
+	switch (data->id) {
+	case VCNL4040:
+	case VCNL4200:
+		ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
+		if (ret < 0)
+			return ret;
+
+		if (val)
+			ret &= ~VCNL4040_PS_CONF1_PS_SD;
+		else
+			ret |= VCNL4040_PS_CONF1_PS_SD;
+
+		return i2c_smbus_write_word_data(data->client,
+						 VCNL4200_PS_CONF1, ret);
+	default:
+		return -EINVAL;
+	}
+}
+
 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, !val);
 	if (ret < 0)
 		return ret;
 
-	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
+	ret = vcnl4000_write_ps_enable(data, !val);
 	if (ret < 0)
 		return ret;
 
-- 
2.30.2

[PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040

[Mårten Lindahl]

Add channel attribute in_illuminance_en and in_proximity_en with
read/write access for vcnl4040. If automatic runtime power management is
turned off (power/control = on), both sensors can be kept on or off by
userspace.

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

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 0b226c684957..9838f0868372 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -125,6 +125,9 @@ struct vcnl4000_data {
 	enum vcnl4000_device_ids id;
 	int rev;
 	int al_scale;
+	bool als_enable;
+	bool ps_enable;
+
 	const struct vcnl4000_chip_spec *chip_spec;
 	struct mutex vcnl4000_lock;
 	struct vcnl4200_channel vcnl4200_al;
@@ -202,10 +205,13 @@ static ssize_t vcnl4000_write_als_enable(struct vcnl4000_data *data, int val)
 		if (ret < 0)
 			return ret;
 
-		if (val)
+		if (val) {
 			ret &= ~VCNL4040_ALS_CONF_ALS_SD;
-		else
+			data->als_enable = true;
+		} else {
 			ret |= VCNL4040_ALS_CONF_ALS_SD;
+			data->als_enable = false;
+		}
 
 		return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF,
 						 ret);
@@ -225,10 +231,13 @@ static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, int val)
 		if (ret < 0)
 			return ret;
 
-		if (val)
+		if (val) {
 			ret &= ~VCNL4040_PS_CONF1_PS_SD;
-		else
+			data->ps_enable = true;
+		} else {
 			ret |= VCNL4040_PS_CONF1_PS_SD;
+			data->ps_enable = false;
+		}
 
 		return i2c_smbus_write_word_data(data->client,
 						 VCNL4200_PS_CONF1, ret);
@@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data *data)
 	dev_dbg(&data->client->dev, "device id 0x%x", id);
 
 	data->rev = (ret >> 8) & 0xf;
+	data->als_enable = false;
+	data->ps_enable = false;
 
 	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
 	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
@@ -459,8 +470,12 @@ static bool vcnl4010_is_in_periodic_mode(struct vcnl4000_data *data)
 static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on)
 {
 	struct device *dev = &data->client->dev;
+	struct iio_dev *indio_dev = i2c_get_clientdata(data->client);
 	int ret;
 
+	if (!indio_dev->dev.power.runtime_auto)
+		return 0;
+
 	if (on) {
 		ret = pm_runtime_resume_and_get(dev);
 	} else {
@@ -507,6 +522,38 @@ 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_ENABLE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			*val = data->als_enable;
+			return IIO_VAL_INT;
+		case IIO_PROXIMITY:
+			*val = data->ps_enable;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	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_ENABLE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			return vcnl4000_write_als_enable(data, val);
+		case IIO_PROXIMITY:
+			return vcnl4000_write_ps_enable(data, val);
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -845,6 +892,19 @@ 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) | BIT(IIO_CHAN_INFO_ENABLE),
+	}, {
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_ENABLE),
+		.ext_info = vcnl4000_ext_info,
+	}
+};
+
 static const struct iio_info vcnl4000_info = {
 	.read_raw = vcnl4000_read_raw,
 };
@@ -859,6 +919,11 @@ 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,
+};
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
@@ -888,9 +953,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

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

[Mårten Lindahl]

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

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

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 9838f0868372..7a207e48335d 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -76,6 +76,8 @@
 
 #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 \
+	(BIT(3) | BIT(2) | BIT(1)) /* Proximity integration time */
 
 /* Bit masks for interrupt registers. */
 #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
@@ -103,6 +105,16 @@ static const int vcnl4010_prox_sampling_frequency[][2] = {
 	{125, 0},
 	{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 */
 
@@ -486,6 +498,49 @@ 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 = (ret & VCNL4040_PS_CONF2_PS_IT) >> 1;
+
+	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;
+
+	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
+	if (ret < 0)
+		return ret;
+
+	ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) | (index << 1);
+
+	return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, ret);
+}
+
 static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 				struct iio_chan_spec const *chan,
 				int *val, int *val2, long mask)
@@ -533,6 +588,13 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	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;
 	}
@@ -554,6 +616,12 @@ static int vcnl4040_write_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	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;
 	}
@@ -900,11 +968,23 @@ static const struct iio_chan_spec vcnl4040_channels[] = {
 	}, {
 		.type = IIO_PROXIMITY,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_ENABLE),
+			BIT(IIO_CHAN_INFO_ENABLE) | BIT(IIO_CHAN_INFO_INT_TIME),
 		.ext_info = vcnl4000_ext_info,
 	}
 };
 
+static IIO_CONST_ATTR(in_proximity_integration_time_available,
+	"0.000100 0.000150 0.000200 0.000250 0.000300 0.000350 0.000400 0.000800");
+
+static struct attribute *vcnl4040_attributes[] = {
+	&iio_const_attr_in_proximity_integration_time_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group vcnl4040_attribute_group = {
+	.attrs = vcnl4040_attributes,
+};
+
 static const struct iio_info vcnl4000_info = {
 	.read_raw = vcnl4000_read_raw,
 };
@@ -922,6 +1002,7 @@ static const struct iio_info vcnl4010_info = {
 static const struct iio_info vcnl4040_info = {
 	.read_raw = vcnl4000_read_raw,
 	.write_raw = vcnl4040_write_raw,
+	.attrs = &vcnl4040_attribute_group,
 };
 
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
-- 
2.30.2

Re: [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040

[Paul Cercueil]

Hi Mårten,

Le mar., sept. 20 2022 at 20:09:57 +0200, Mårten Lindahl 
<marten.lindahl@axis.com> a écrit :
> Add channel attribute in_illuminance_en and in_proximity_en with
> read/write access for vcnl4040. If automatic runtime power management 
> is
> turned off (power/control = on), both sensors can be kept on or off by
> userspace.

I don't really understand this. If automatic runtime power management 
is turned OFF, I would expect both sensors to be kept ON always.

It's not userspace's job to do power management of the chip. Why are 
these channel attributes needed?

Cheers,
-Paul

> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
>  drivers/iio/light/vcnl4000.c | 79 
> ++++++++++++++++++++++++++++++++----
>  1 file changed, 72 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c 
> b/drivers/iio/light/vcnl4000.c
> index 0b226c684957..9838f0868372 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -125,6 +125,9 @@ struct vcnl4000_data {
>  	enum vcnl4000_device_ids id;
>  	int rev;
>  	int al_scale;
> +	bool als_enable;
> +	bool ps_enable;
> +
>  	const struct vcnl4000_chip_spec *chip_spec;
>  	struct mutex vcnl4000_lock;
>  	struct vcnl4200_channel vcnl4200_al;
> @@ -202,10 +205,13 @@ static ssize_t vcnl4000_write_als_enable(struct 
> vcnl4000_data *data, int val)
>  		if (ret < 0)
>  			return ret;
> 
> -		if (val)
> +		if (val) {
>  			ret &= ~VCNL4040_ALS_CONF_ALS_SD;
> -		else
> +			data->als_enable = true;
> +		} else {
>  			ret |= VCNL4040_ALS_CONF_ALS_SD;
> +			data->als_enable = false;
> +		}
> 
>  		return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF,
>  						 ret);
> @@ -225,10 +231,13 @@ static ssize_t vcnl4000_write_ps_enable(struct 
> vcnl4000_data *data, int val)
>  		if (ret < 0)
>  			return ret;
> 
> -		if (val)
> +		if (val) {
>  			ret &= ~VCNL4040_PS_CONF1_PS_SD;
> -		else
> +			data->ps_enable = true;
> +		} else {
>  			ret |= VCNL4040_PS_CONF1_PS_SD;
> +			data->ps_enable = false;
> +		}
> 
>  		return i2c_smbus_write_word_data(data->client,
>  						 VCNL4200_PS_CONF1, ret);
> @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data 
> *data)
>  	dev_dbg(&data->client->dev, "device id 0x%x", id);
> 
>  	data->rev = (ret >> 8) & 0xf;
> +	data->als_enable = false;
> +	data->ps_enable = false;
> 
>  	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
>  	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
> @@ -459,8 +470,12 @@ static bool vcnl4010_is_in_periodic_mode(struct 
> vcnl4000_data *data)
>  static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, 
> bool on)
>  {
>  	struct device *dev = &data->client->dev;
> +	struct iio_dev *indio_dev = i2c_get_clientdata(data->client);
>  	int ret;
> 
> +	if (!indio_dev->dev.power.runtime_auto)
> +		return 0;
> +
>  	if (on) {
>  		ret = pm_runtime_resume_and_get(dev);
>  	} else {
> @@ -507,6 +522,38 @@ 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_ENABLE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			*val = data->als_enable;
> +			return IIO_VAL_INT;
> +		case IIO_PROXIMITY:
> +			*val = data->ps_enable;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	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_ENABLE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			return vcnl4000_write_als_enable(data, val);
> +		case IIO_PROXIMITY:
> +			return vcnl4000_write_ps_enable(data, val);
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -845,6 +892,19 @@ 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) | BIT(IIO_CHAN_INFO_ENABLE),
> +	}, {
> +		.type = IIO_PROXIMITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_ENABLE),
> +		.ext_info = vcnl4000_ext_info,
> +	}
> +};
> +
>  static const struct iio_info vcnl4000_info = {
>  	.read_raw = vcnl4000_read_raw,
>  };
> @@ -859,6 +919,11 @@ 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,
> +};
> +
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.prod = "VCNL4000",
> @@ -888,9 +953,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 3/3] iio: light: vcnl4000: Add ps_it attributes for vcnl4040

[Paul Cercueil]

Hi Mårten,

Le mar., sept. 20 2022 at 20:09:58 +0200, Mårten Lindahl 
<marten.lindahl@axis.com> a écrit :
> Add read/write attribute for proximity integration time, and a read
> attribute for available integration times for the vcnl4040 chip.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
>  drivers/iio/light/vcnl4000.c | 83 
> +++++++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c 
> b/drivers/iio/light/vcnl4000.c
> index 9838f0868372..7a207e48335d 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -76,6 +76,8 @@
> 
>  #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 \
> +	(BIT(3) | BIT(2) | BIT(1)) /* Proximity integration time */

Use the GENMASK() macro.

> 
>  /* Bit masks for interrupt registers. */
>  #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt 
> source */
> @@ -103,6 +105,16 @@ static const int 
> vcnl4010_prox_sampling_frequency[][2] = {
>  	{125, 0},
>  	{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 */
> 
> @@ -486,6 +498,49 @@ 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 = (ret & VCNL4040_PS_CONF2_PS_IT) >> 1;

Use the FIELD_GET() macro.

> +
> +	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;
> +
> +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) | (index << 1);

Use the FIELD_PREP() macro.

> +
> +	return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, 
> ret);
> +}
> +
>  static int vcnl4000_read_raw(struct iio_dev *indio_dev,
>  				struct iio_chan_spec const *chan,
>  				int *val, int *val2, long mask)
> @@ -533,6 +588,13 @@ static int vcnl4000_read_raw(struct iio_dev 
> *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	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;
>  	}
> @@ -554,6 +616,12 @@ static int vcnl4040_write_raw(struct iio_dev 
> *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	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;
>  	}
> @@ -900,11 +968,23 @@ static const struct iio_chan_spec 
> vcnl4040_channels[] = {
>  	}, {
>  		.type = IIO_PROXIMITY,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -			BIT(IIO_CHAN_INFO_ENABLE),
> +			BIT(IIO_CHAN_INFO_ENABLE) | BIT(IIO_CHAN_INFO_INT_TIME),
>  		.ext_info = vcnl4000_ext_info,
>  	}
>  };
> 
> +static IIO_CONST_ATTR(in_proximity_integration_time_available,
> +	"0.000100 0.000150 0.000200 0.000250 0.000300 0.000350 0.000400 
> 0.000800");

Hmm, this is not how you define a "_available" attribute.

You need to add a
.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME)

to your iio_chan_spec, then implement the .read_avail callback in your 
iio_info structure.

Cheers,
-Paul

> +
> +static struct attribute *vcnl4040_attributes[] = {
> +	&iio_const_attr_in_proximity_integration_time_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group vcnl4040_attribute_group = {
> +	.attrs = vcnl4040_attributes,
> +};
> +
>  static const struct iio_info vcnl4000_info = {
>  	.read_raw = vcnl4000_read_raw,
>  };
> @@ -922,6 +1002,7 @@ static const struct iio_info vcnl4010_info = {
>  static const struct iio_info vcnl4040_info = {
>  	.read_raw = vcnl4000_read_raw,
>  	.write_raw = vcnl4040_write_raw,
> +	.attrs = &vcnl4040_attribute_group,
>  };
> 
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> --
> 2.30.2
> 

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

[Paul Cercueil]

Hi Mårten,

Le mar., sept. 20 2022 at 20:09:56 +0200, Mårten Lindahl 
<marten.lindahl@axis.com> a écrit :
> 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 | 53 
> ++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c 
> b/drivers/iio/light/vcnl4000.c
> index 3db4e26731bb..0b226c684957 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,62 @@ 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, 
> int val)
> +{
> +	int ret;
> +
> +	switch (data->id) {
> +	case VCNL4040:
> +	case VCNL4200:

The switch isn't needed, since vcnl4200_set_power_state() is only 
called for compatible chips.

> +		ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (val)
> +			ret &= ~VCNL4040_ALS_CONF_ALS_SD;
> +		else
> +			ret |= VCNL4040_ALS_CONF_ALS_SD;
> +
> +		return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF,
> +						 ret);

Are you sure a race cannot happen here?

I would assume it cannot, but that's still a bit fishy.

This driver would benefit from a regmap conversion, but it's probably a 
bit too much to ask...

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, 
> int val)
> +{
> +	int ret;
> +
> +	switch (data->id) {
> +	case VCNL4040:
> +	case VCNL4200:
> +		ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (val)
> +			ret &= ~VCNL4040_PS_CONF1_PS_SD;
> +		else
> +			ret |= VCNL4040_PS_CONF1_PS_SD;
> +
> +		return i2c_smbus_write_word_data(data->client,
> +						 VCNL4200_PS_CONF1, ret);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  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, !val);

You set "val" to 0 if "on", and to 1 if "!on", then pass "!val", this 
is very confusing. You could just pass "on" here and below.

Cheers,
-Paul

>  	if (ret < 0)
>  		return ret;
> 
> -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, 
> val);
> +	ret = vcnl4000_write_ps_enable(data, !val);
>  	if (ret < 0)
>  		return ret;
> 
> --
> 2.30.2
> 

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

[Marten Lindahl]

On Wed, Sep 21, 2022 at 12:23:26AM +0200, Paul Cercueil wrote:
> Hi Mårten,
> 
> Le mar., sept. 20 2022 at 20:09:56 +0200, Mårten Lindahl 
> <marten.lindahl@axis.com> a écrit :
> > 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 | 53 
> > ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 51 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c 
> > b/drivers/iio/light/vcnl4000.c
> > index 3db4e26731bb..0b226c684957 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,62 @@ 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, 
> > int val)
> > +{
> > +	int ret;
> > +
> > +	switch (data->id) {
> > +	case VCNL4040:
> > +	case VCNL4200:

Hi Paul!
Thanks for looking at this!
> 
> The switch isn't needed, since vcnl4200_set_power_state() is only 
> called for compatible chips.

Yes, your're right. I will remove the switch.
> 
> > +		ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (val)
> > +			ret &= ~VCNL4040_ALS_CONF_ALS_SD;
> > +		else
> > +			ret |= VCNL4040_ALS_CONF_ALS_SD;
> > +
> > +		return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF,
> > +						 ret);
> 
> Are you sure a race cannot happen here?
> 
> I would assume it cannot, but that's still a bit fishy.

Ok, I see what you mean. I cannot tell for sure, but I can guard the
read and write with the mutex.

> 
> This driver would benefit from a regmap conversion, but it's probably a 
> bit too much to ask...

I can try to do this in a separate patch after these ones if that's ok?

> 
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static ssize_t vcnl4000_write_ps_enable(struct vcnl4000_data *data, 
> > int val)
> > +{
> > +	int ret;
> > +
> > +	switch (data->id) {
> > +	case VCNL4040:
> > +	case VCNL4200:
> > +		ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (val)
> > +			ret &= ~VCNL4040_PS_CONF1_PS_SD;
> > +		else
> > +			ret |= VCNL4040_PS_CONF1_PS_SD;
> > +
> > +		return i2c_smbus_write_word_data(data->client,
> > +						 VCNL4200_PS_CONF1, ret);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  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, !val);
> 
> You set "val" to 0 if "on", and to 1 if "!on", then pass "!val", this 
> is very confusing. You could just pass "on" here and below.

I agree! I will change it to only use the boolean 'on'.

Kind reagards
Mårten
> 
> Cheers,
> -Paul
> 
> >  	if (ret < 0)
> >  		return ret;
> > 
> > -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, 
> > val);
> > +	ret = vcnl4000_write_ps_enable(data, !val);
> >  	if (ret < 0)
> >  		return ret;
> > 
> > --
> > 2.30.2
> > 
> 
> 

Re: [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040

[Marten Lindahl]

On Wed, Sep 21, 2022 at 12:01:24AM +0200, Paul Cercueil wrote:
> Hi Mårten,
> 
> Le mar., sept. 20 2022 at 20:09:57 +0200, Mårten Lindahl 
> <marten.lindahl@axis.com> a écrit :
> > Add channel attribute in_illuminance_en and in_proximity_en with
> > read/write access for vcnl4040. If automatic runtime power management 
> > is
> > turned off (power/control = on), both sensors can be kept on or off by
> > userspace.
>

Hi Paul!

> I don't really understand this. If automatic runtime power management 
> is turned OFF, I would expect both sensors to be kept ON always.
> 
> It's not userspace's job to do power management of the chip. Why are 
> these channel attributes needed?

I think I understand the problem here. I added the *_en attributes
because I couldn't see any way to turn the sensors on without forcing it
on during the *_raw read operation (with vcnl4000_set_pm_runtime_state(true))
after which it is turned off again (false).

Even if the power/control is set to 'on', there will be no callback for
changing the state to active.

It seems this does not work in this driver (with or without my patches) and I
was confused by how it was supposed to work. But after some digging I suspect
there could be a bug in the driver since the sysfs control/* nodes seems to
operate on the indio_dev->dev and not on the driver dev, which is used for
the vcnl4000 driver pm_runtime operations.

Setting the power/control to 'on' invokes the rpm_resume function which
checks the dev.power.disable_depth attribute before it calls the
resume_callback for setting the active state on the driver. But if the
dev.power.disable_depth == 1 (which is the init value) the callback will not
be called. And nothing happens. And I suspect the reason why the
dev.power.disable_depth is 1 may be that it is not the vcnl4000 dev
object that is being checked, but the indio_dev->dev object, which has not
been configured for pm_runtime operations in the driver.

Sorry for a long reply to your question, but I suspect that if the
automatic pm_runtime for this driver can be disabled by the sysfs
power/control, the *_en attributes wont be needed.

I will look into why it does not work.

Kind regards
Mårten

> 
> Cheers,
> -Paul
> 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >  drivers/iio/light/vcnl4000.c | 79 
> > ++++++++++++++++++++++++++++++++----
> >  1 file changed, 72 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c 
> > b/drivers/iio/light/vcnl4000.c
> > index 0b226c684957..9838f0868372 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -125,6 +125,9 @@ struct vcnl4000_data {
> >  	enum vcnl4000_device_ids id;
> >  	int rev;
> >  	int al_scale;
> > +	bool als_enable;
> > +	bool ps_enable;
> > +
> >  	const struct vcnl4000_chip_spec *chip_spec;
> >  	struct mutex vcnl4000_lock;
> >  	struct vcnl4200_channel vcnl4200_al;
> > @@ -202,10 +205,13 @@ static ssize_t vcnl4000_write_als_enable(struct 
> > vcnl4000_data *data, int val)
> >  		if (ret < 0)
> >  			return ret;
> > 
> > -		if (val)
> > +		if (val) {
> >  			ret &= ~VCNL4040_ALS_CONF_ALS_SD;
> > -		else
> > +			data->als_enable = true;
> > +		} else {
> >  			ret |= VCNL4040_ALS_CONF_ALS_SD;
> > +			data->als_enable = false;
> > +		}
> > 
> >  		return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF,
> >  						 ret);
> > @@ -225,10 +231,13 @@ static ssize_t vcnl4000_write_ps_enable(struct 
> > vcnl4000_data *data, int val)
> >  		if (ret < 0)
> >  			return ret;
> > 
> > -		if (val)
> > +		if (val) {
> >  			ret &= ~VCNL4040_PS_CONF1_PS_SD;
> > -		else
> > +			data->ps_enable = true;
> > +		} else {
> >  			ret |= VCNL4040_PS_CONF1_PS_SD;
> > +			data->ps_enable = false;
> > +		}
> > 
> >  		return i2c_smbus_write_word_data(data->client,
> >  						 VCNL4200_PS_CONF1, ret);
> > @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data 
> > *data)
> >  	dev_dbg(&data->client->dev, "device id 0x%x", id);
> > 
> >  	data->rev = (ret >> 8) & 0xf;
> > +	data->als_enable = false;
> > +	data->ps_enable = false;
> > 
> >  	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
> >  	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
> > @@ -459,8 +470,12 @@ static bool vcnl4010_is_in_periodic_mode(struct 
> > vcnl4000_data *data)
> >  static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, 
> > bool on)
> >  {
> >  	struct device *dev = &data->client->dev;
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(data->client);
> >  	int ret;
> > 
> > +	if (!indio_dev->dev.power.runtime_auto)
> > +		return 0;
> > +
> >  	if (on) {
> >  		ret = pm_runtime_resume_and_get(dev);
> >  	} else {
> > @@ -507,6 +522,38 @@ 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_ENABLE:
> > +		switch (chan->type) {
> > +		case IIO_LIGHT:
> > +			*val = data->als_enable;
> > +			return IIO_VAL_INT;
> > +		case IIO_PROXIMITY:
> > +			*val = data->ps_enable;
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	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_ENABLE:
> > +		switch (chan->type) {
> > +		case IIO_LIGHT:
> > +			return vcnl4000_write_als_enable(data, val);
> > +		case IIO_PROXIMITY:
> > +			return vcnl4000_write_ps_enable(data, val);
> > +		default:
> > +			return -EINVAL;
> > +		}
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -845,6 +892,19 @@ 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) | BIT(IIO_CHAN_INFO_ENABLE),
> > +	}, {
> > +		.type = IIO_PROXIMITY,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +			BIT(IIO_CHAN_INFO_ENABLE),
> > +		.ext_info = vcnl4000_ext_info,
> > +	}
> > +};
> > +
> >  static const struct iio_info vcnl4000_info = {
> >  	.read_raw = vcnl4000_read_raw,
> >  };
> > @@ -859,6 +919,11 @@ 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,
> > +};
> > +
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.prod = "VCNL4000",
> > @@ -888,9 +953,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 3/3] iio: light: vcnl4000: Add ps_it attributes for vcnl4040

[Marten Lindahl]

On Wed, Sep 21, 2022 at 12:12:27AM +0200, Paul Cercueil wrote:
> Hi Mårten,
> 
> Le mar., sept. 20 2022 at 20:09:58 +0200, Mårten Lindahl 
> <marten.lindahl@axis.com> a écrit :
> > Add read/write attribute for proximity integration time, and a read
> > attribute for available integration times for the vcnl4040 chip.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >  drivers/iio/light/vcnl4000.c | 83 
> > +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 82 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c 
> > b/drivers/iio/light/vcnl4000.c
> > index 9838f0868372..7a207e48335d 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -76,6 +76,8 @@
> > 
> >  #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 \
> > +	(BIT(3) | BIT(2) | BIT(1)) /* Proximity integration time */

Hi Paul!
> 
> Use the GENMASK() macro.
>
Will do!

> > 
> >  /* Bit masks for interrupt registers. */
> >  #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt 
> > source */
> > @@ -103,6 +105,16 @@ static const int 
> > vcnl4010_prox_sampling_frequency[][2] = {
> >  	{125, 0},
> >  	{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 */
> > 
> > @@ -486,6 +498,49 @@ 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 = (ret & VCNL4040_PS_CONF2_PS_IT) >> 1;
> 
> Use the FIELD_GET() macro.
> 
Will do!

> > +
> > +	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;
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = (ret & ~VCNL4040_PS_CONF2_PS_IT) | (index << 1);
> 
> Use the FIELD_PREP() macro.
> 
Will do!

> > +
> > +	return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, 
> > ret);
> > +}
> > +
> >  static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> >  				struct iio_chan_spec const *chan,
> >  				int *val, int *val2, long mask)
> > @@ -533,6 +588,13 @@ static int vcnl4000_read_raw(struct iio_dev 
> > *indio_dev,
> >  		default:
> >  			return -EINVAL;
> >  		}
> > +	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;
> >  	}
> > @@ -554,6 +616,12 @@ static int vcnl4040_write_raw(struct iio_dev 
> > *indio_dev,
> >  		default:
> >  			return -EINVAL;
> >  		}
> > +	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;
> >  	}
> > @@ -900,11 +968,23 @@ static const struct iio_chan_spec 
> > vcnl4040_channels[] = {
> >  	}, {
> >  		.type = IIO_PROXIMITY,
> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > -			BIT(IIO_CHAN_INFO_ENABLE),
> > +			BIT(IIO_CHAN_INFO_ENABLE) | BIT(IIO_CHAN_INFO_INT_TIME),
> >  		.ext_info = vcnl4000_ext_info,
> >  	}
> >  };
> > 
> > +static IIO_CONST_ATTR(in_proximity_integration_time_available,
> > +	"0.000100 0.000150 0.000200 0.000250 0.000300 0.000350 0.000400 
> > 0.000800");
> 
> Hmm, this is not how you define a "_available" attribute.
> 
> You need to add a
> .info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME)
> 
> to your iio_chan_spec, then implement the .read_avail callback in your 
> iio_info structure.

Ok, I didn't notice that possibility. Thanks, I will do so!

Kind regards
Mårten

> 
> Cheers,
> -Paul
> 
> > +
> > +static struct attribute *vcnl4040_attributes[] = {
> > +	&iio_const_attr_in_proximity_integration_time_available.dev_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group vcnl4040_attribute_group = {
> > +	.attrs = vcnl4040_attributes,
> > +};
> > +
> >  static const struct iio_info vcnl4000_info = {
> >  	.read_raw = vcnl4000_read_raw,
> >  };
> > @@ -922,6 +1002,7 @@ static const struct iio_info vcnl4010_info = {
> >  static const struct iio_info vcnl4040_info = {
> >  	.read_raw = vcnl4000_read_raw,
> >  	.write_raw = vcnl4040_write_raw,
> > +	.attrs = &vcnl4040_attribute_group,
> >  };
> > 
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > --
> > 2.30.2
> > 
> 
> 

Re: [PATCH 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040

[Paul Cercueil]

Le jeu., sept. 22 2022 at 15:04:47 +0200, Marten Lindahl 
<martenli@axis.com> a écrit :
> On Wed, Sep 21, 2022 at 12:01:24AM +0200, Paul Cercueil wrote:
>>  Hi Mårten,
>> 
>>  Le mar., sept. 20 2022 at 20:09:57 +0200, Mårten Lindahl
>>  <marten.lindahl@axis.com> a écrit :
>>  > Add channel attribute in_illuminance_en and in_proximity_en with
>>  > read/write access for vcnl4040. If automatic runtime power 
>> management
>>  > is
>>  > turned off (power/control = on), both sensors can be kept on or 
>> off by
>>  > userspace.
>> 
> 
> Hi Paul!
> 
>>  I don't really understand this. If automatic runtime power 
>> management
>>  is turned OFF, I would expect both sensors to be kept ON always.
>> 
>>  It's not userspace's job to do power management of the chip. Why are
>>  these channel attributes needed?
> 
> I think I understand the problem here. I added the *_en attributes
> because I couldn't see any way to turn the sensors on without forcing 
> it
> on during the *_raw read operation (with 
> vcnl4000_set_pm_runtime_state(true))
> after which it is turned off again (false).

What's wrong with doing that?

> Even if the power/control is set to 'on', there will be no callback 
> for
> changing the state to active.
> 
> It seems this does not work in this driver (with or without my 
> patches) and I
> was confused by how it was supposed to work. But after some digging I 
> suspect
> there could be a bug in the driver since the sysfs control/* nodes 
> seems to
> operate on the indio_dev->dev and not on the driver dev, which is 
> used for
> the vcnl4000 driver pm_runtime operations.

I believe this is normal. The devm_iio_device_alloc() creates a new 
device, whose parent is your i2c_client->dev.

> Setting the power/control to 'on' invokes the rpm_resume function 
> which
> checks the dev.power.disable_depth attribute before it calls the
> resume_callback for setting the active state on the driver. But if the
> dev.power.disable_depth == 1 (which is the init value) the callback 
> will not
> be called. And nothing happens. And I suspect the reason why the
> dev.power.disable_depth is 1 may be that it is not the vcnl4000 dev
> object that is being checked, but the indio_dev->dev object, which 
> has not
> been configured for pm_runtime operations in the driver.
> 
> Sorry for a long reply to your question, but I suspect that if the
> automatic pm_runtime for this driver can be disabled by the sysfs
> power/control, the *_en attributes wont be needed.
> 
> I will look into why it does not work.

I still don't understand. Why do you *need* to disable runtime PM?

-Paul

> Kind regards
> Mårten
> 
>> 
>>  Cheers,
>>  -Paul
>> 
>>  > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>>  > ---
>>  >  drivers/iio/light/vcnl4000.c | 79
>>  > ++++++++++++++++++++++++++++++++----
>>  >  1 file changed, 72 insertions(+), 7 deletions(-)
>>  >
>>  > diff --git a/drivers/iio/light/vcnl4000.c
>>  > b/drivers/iio/light/vcnl4000.c
>>  > index 0b226c684957..9838f0868372 100644
>>  > --- a/drivers/iio/light/vcnl4000.c
>>  > +++ b/drivers/iio/light/vcnl4000.c
>>  > @@ -125,6 +125,9 @@ struct vcnl4000_data {
>>  >  	enum vcnl4000_device_ids id;
>>  >  	int rev;
>>  >  	int al_scale;
>>  > +	bool als_enable;
>>  > +	bool ps_enable;
>>  > +
>>  >  	const struct vcnl4000_chip_spec *chip_spec;
>>  >  	struct mutex vcnl4000_lock;
>>  >  	struct vcnl4200_channel vcnl4200_al;
>>  > @@ -202,10 +205,13 @@ static ssize_t 
>> vcnl4000_write_als_enable(struct
>>  > vcnl4000_data *data, int val)
>>  >  		if (ret < 0)
>>  >  			return ret;
>>  >
>>  > -		if (val)
>>  > +		if (val) {
>>  >  			ret &= ~VCNL4040_ALS_CONF_ALS_SD;
>>  > -		else
>>  > +			data->als_enable = true;
>>  > +		} else {
>>  >  			ret |= VCNL4040_ALS_CONF_ALS_SD;
>>  > +			data->als_enable = false;
>>  > +		}
>>  >
>>  >  		return i2c_smbus_write_word_data(data->client, 
>> VCNL4200_AL_CONF,
>>  >  						 ret);
>>  > @@ -225,10 +231,13 @@ static ssize_t 
>> vcnl4000_write_ps_enable(struct
>>  > vcnl4000_data *data, int val)
>>  >  		if (ret < 0)
>>  >  			return ret;
>>  >
>>  > -		if (val)
>>  > +		if (val) {
>>  >  			ret &= ~VCNL4040_PS_CONF1_PS_SD;
>>  > -		else
>>  > +			data->ps_enable = true;
>>  > +		} else {
>>  >  			ret |= VCNL4040_PS_CONF1_PS_SD;
>>  > +			data->ps_enable = false;
>>  > +		}
>>  >
>>  >  		return i2c_smbus_write_word_data(data->client,
>>  >  						 VCNL4200_PS_CONF1, ret);
>>  > @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data
>>  > *data)
>>  >  	dev_dbg(&data->client->dev, "device id 0x%x", id);
>>  >
>>  >  	data->rev = (ret >> 8) & 0xf;
>>  > +	data->als_enable = false;
>>  > +	data->ps_enable = false;
>>  >
>>  >  	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
>>  >  	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
>>  > @@ -459,8 +470,12 @@ static bool 
>> vcnl4010_is_in_periodic_mode(struct
>>  > vcnl4000_data *data)
>>  >  static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data 
>> *data,
>>  > bool on)
>>  >  {
>>  >  	struct device *dev = &data->client->dev;
>>  > +	struct iio_dev *indio_dev = i2c_get_clientdata(data->client);
>>  >  	int ret;
>>  >
>>  > +	if (!indio_dev->dev.power.runtime_auto)
>>  > +		return 0;
>>  > +
>>  >  	if (on) {
>>  >  		ret = pm_runtime_resume_and_get(dev);
>>  >  	} else {
>>  > @@ -507,6 +522,38 @@ 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_ENABLE:
>>  > +		switch (chan->type) {
>>  > +		case IIO_LIGHT:
>>  > +			*val = data->als_enable;
>>  > +			return IIO_VAL_INT;
>>  > +		case IIO_PROXIMITY:
>>  > +			*val = data->ps_enable;
>>  > +			return IIO_VAL_INT;
>>  > +		default:
>>  > +			return -EINVAL;
>>  > +		}
>>  > +	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_ENABLE:
>>  > +		switch (chan->type) {
>>  > +		case IIO_LIGHT:
>>  > +			return vcnl4000_write_als_enable(data, val);
>>  > +		case IIO_PROXIMITY:
>>  > +			return vcnl4000_write_ps_enable(data, val);
>>  > +		default:
>>  > +			return -EINVAL;
>>  > +		}
>>  >  	default:
>>  >  		return -EINVAL;
>>  >  	}
>>  > @@ -845,6 +892,19 @@ 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) | BIT(IIO_CHAN_INFO_ENABLE),
>>  > +	}, {
>>  > +		.type = IIO_PROXIMITY,
>>  > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  > +			BIT(IIO_CHAN_INFO_ENABLE),
>>  > +		.ext_info = vcnl4000_ext_info,
>>  > +	}
>>  > +};
>>  > +
>>  >  static const struct iio_info vcnl4000_info = {
>>  >  	.read_raw = vcnl4000_read_raw,
>>  >  };
>>  > @@ -859,6 +919,11 @@ 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,
>>  > +};
>>  > +
>>  >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] 
>> = {
>>  >  	[VCNL4000] = {
>>  >  		.prod = "VCNL4000",
>>  > @@ -888,9 +953,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 2/3] iio: light: vcnl4000: Add enable attributes for vcnl4040

[Marten Lindahl]

On Thu, Sep 22, 2022 at 04:10:54PM +0200, Paul Cercueil wrote:
> 
> 
> Le jeu., sept. 22 2022 at 15:04:47 +0200, Marten Lindahl 
> <martenli@axis.com> a écrit :
> > On Wed, Sep 21, 2022 at 12:01:24AM +0200, Paul Cercueil wrote:
> >>  Hi Mårten,
> >> 
> >>  Le mar., sept. 20 2022 at 20:09:57 +0200, Mårten Lindahl
> >>  <marten.lindahl@axis.com> a écrit :
> >>  > Add channel attribute in_illuminance_en and in_proximity_en with
> >>  > read/write access for vcnl4040. If automatic runtime power 
> >> management
> >>  > is
> >>  > turned off (power/control = on), both sensors can be kept on or 
> >> off by
> >>  > userspace.
> >> 
> > 
> > Hi Paul!
> > 
> >>  I don't really understand this. If automatic runtime power 
> >> management
> >>  is turned OFF, I would expect both sensors to be kept ON always.
> >> 
> >>  It's not userspace's job to do power management of the chip. Why are
> >>  these channel attributes needed?
> > 
> > I think I understand the problem here. I added the *_en attributes
> > because I couldn't see any way to turn the sensors on without forcing 
> > it
> > on during the *_raw read operation (with 
> > vcnl4000_set_pm_runtime_state(true))
> > after which it is turned off again (false).
> 
> What's wrong with doing that?

Hi!

No, nothing is wrong with that. I was just confused by the fact that
power/control=on didn't have any effect on the device. 

> 
> > Even if the power/control is set to 'on', there will be no callback 
> > for
> > changing the state to active.
> > 
> > It seems this does not work in this driver (with or without my 
> > patches) and I
> > was confused by how it was supposed to work. But after some digging I 
> > suspect
> > there could be a bug in the driver since the sysfs control/* nodes 
> > seems to
> > operate on the indio_dev->dev and not on the driver dev, which is 
> > used for
> > the vcnl4000 driver pm_runtime operations.
> 
> I believe this is normal. The devm_iio_device_alloc() creates a new 
> device, whose parent is your i2c_client->dev.

Ok
> 
> > Setting the power/control to 'on' invokes the rpm_resume function 
> > which
> > checks the dev.power.disable_depth attribute before it calls the
> > resume_callback for setting the active state on the driver. But if the
> > dev.power.disable_depth == 1 (which is the init value) the callback 
> > will not
> > be called. And nothing happens. And I suspect the reason why the
> > dev.power.disable_depth is 1 may be that it is not the vcnl4000 dev
> > object that is being checked, but the indio_dev->dev object, which 
> > has not
> > been configured for pm_runtime operations in the driver.
> > 
> > Sorry for a long reply to your question, but I suspect that if the
> > automatic pm_runtime for this driver can be disabled by the sysfs
> > power/control, the *_en attributes wont be needed.
> > 
> > I will look into why it does not work.
> 
> I still don't understand. Why do you *need* to disable runtime PM?

There is no special reason to disable runtime PM for the usecases we
have. The original reason behind this patch is a local patch from the
4.19 kernel when runtime PM did not exist in the driver. The local
patch added *_en attributes for turning the sensors on/off.

I will gladly skip this patch, since it has come clear to me that it
doesn't bring any value to the current version of the driver. I will
probably look at a fix for the power/control=on problem, but any fix
for that will be a separate patch.

Kind regards
Mårten

> 
> -Paul
> 
> > Kind regards
> > Mårten
> > 
> >> 
> >>  Cheers,
> >>  -Paul
> >> 
> >>  > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> >>  > ---
> >>  >  drivers/iio/light/vcnl4000.c | 79
> >>  > ++++++++++++++++++++++++++++++++----
> >>  >  1 file changed, 72 insertions(+), 7 deletions(-)
> >>  >
> >>  > diff --git a/drivers/iio/light/vcnl4000.c
> >>  > b/drivers/iio/light/vcnl4000.c
> >>  > index 0b226c684957..9838f0868372 100644
> >>  > --- a/drivers/iio/light/vcnl4000.c
> >>  > +++ b/drivers/iio/light/vcnl4000.c
> >>  > @@ -125,6 +125,9 @@ struct vcnl4000_data {
> >>  >  	enum vcnl4000_device_ids id;
> >>  >  	int rev;
> >>  >  	int al_scale;
> >>  > +	bool als_enable;
> >>  > +	bool ps_enable;
> >>  > +
> >>  >  	const struct vcnl4000_chip_spec *chip_spec;
> >>  >  	struct mutex vcnl4000_lock;
> >>  >  	struct vcnl4200_channel vcnl4200_al;
> >>  > @@ -202,10 +205,13 @@ static ssize_t 
> >> vcnl4000_write_als_enable(struct
> >>  > vcnl4000_data *data, int val)
> >>  >  		if (ret < 0)
> >>  >  			return ret;
> >>  >
> >>  > -		if (val)
> >>  > +		if (val) {
> >>  >  			ret &= ~VCNL4040_ALS_CONF_ALS_SD;
> >>  > -		else
> >>  > +			data->als_enable = true;
> >>  > +		} else {
> >>  >  			ret |= VCNL4040_ALS_CONF_ALS_SD;
> >>  > +			data->als_enable = false;
> >>  > +		}
> >>  >
> >>  >  		return i2c_smbus_write_word_data(data->client, 
> >> VCNL4200_AL_CONF,
> >>  >  						 ret);
> >>  > @@ -225,10 +231,13 @@ static ssize_t 
> >> vcnl4000_write_ps_enable(struct
> >>  > vcnl4000_data *data, int val)
> >>  >  		if (ret < 0)
> >>  >  			return ret;
> >>  >
> >>  > -		if (val)
> >>  > +		if (val) {
> >>  >  			ret &= ~VCNL4040_PS_CONF1_PS_SD;
> >>  > -		else
> >>  > +			data->ps_enable = true;
> >>  > +		} else {
> >>  >  			ret |= VCNL4040_PS_CONF1_PS_SD;
> >>  > +			data->ps_enable = false;
> >>  > +		}
> >>  >
> >>  >  		return i2c_smbus_write_word_data(data->client,
> >>  >  						 VCNL4200_PS_CONF1, ret);
> >>  > @@ -283,6 +292,8 @@ static int vcnl4200_init(struct vcnl4000_data
> >>  > *data)
> >>  >  	dev_dbg(&data->client->dev, "device id 0x%x", id);
> >>  >
> >>  >  	data->rev = (ret >> 8) & 0xf;
> >>  > +	data->als_enable = false;
> >>  > +	data->ps_enable = false;
> >>  >
> >>  >  	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
> >>  >  	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
> >>  > @@ -459,8 +470,12 @@ static bool 
> >> vcnl4010_is_in_periodic_mode(struct
> >>  > vcnl4000_data *data)
> >>  >  static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data 
> >> *data,
> >>  > bool on)
> >>  >  {
> >>  >  	struct device *dev = &data->client->dev;
> >>  > +	struct iio_dev *indio_dev = i2c_get_clientdata(data->client);
> >>  >  	int ret;
> >>  >
> >>  > +	if (!indio_dev->dev.power.runtime_auto)
> >>  > +		return 0;
> >>  > +
> >>  >  	if (on) {
> >>  >  		ret = pm_runtime_resume_and_get(dev);
> >>  >  	} else {
> >>  > @@ -507,6 +522,38 @@ 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_ENABLE:
> >>  > +		switch (chan->type) {
> >>  > +		case IIO_LIGHT:
> >>  > +			*val = data->als_enable;
> >>  > +			return IIO_VAL_INT;
> >>  > +		case IIO_PROXIMITY:
> >>  > +			*val = data->ps_enable;
> >>  > +			return IIO_VAL_INT;
> >>  > +		default:
> >>  > +			return -EINVAL;
> >>  > +		}
> >>  > +	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_ENABLE:
> >>  > +		switch (chan->type) {
> >>  > +		case IIO_LIGHT:
> >>  > +			return vcnl4000_write_als_enable(data, val);
> >>  > +		case IIO_PROXIMITY:
> >>  > +			return vcnl4000_write_ps_enable(data, val);
> >>  > +		default:
> >>  > +			return -EINVAL;
> >>  > +		}
> >>  >  	default:
> >>  >  		return -EINVAL;
> >>  >  	}
> >>  > @@ -845,6 +892,19 @@ 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) | BIT(IIO_CHAN_INFO_ENABLE),
> >>  > +	}, {
> >>  > +		.type = IIO_PROXIMITY,
> >>  > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >>  > +			BIT(IIO_CHAN_INFO_ENABLE),
> >>  > +		.ext_info = vcnl4000_ext_info,
> >>  > +	}
> >>  > +};
> >>  > +
> >>  >  static const struct iio_info vcnl4000_info = {
> >>  >  	.read_raw = vcnl4000_read_raw,
> >>  >  };
> >>  > @@ -859,6 +919,11 @@ 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,
> >>  > +};
> >>  > +
> >>  >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] 
> >> = {
> >>  >  	[VCNL4000] = {
> >>  >  		.prod = "VCNL4000",
> >>  > @@ -888,9 +953,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
> >>  >
> >> 
> >> 
> 
>