[PATCH 0/5] iio: light: vcnl4000: make some settings configurable

[PATCH 0/5] iio: light: vcnl4000: make some settings configurable

[Tomas Novotny]

Hi,

series adds enable, integration time, duty ratio and multi pulse
settings for vcnl4040 and vcnl4200. Only enable is also for ambient
light sensor, the rest is for proximity sensor.

The second patch allows you to switch sensors on or off. This is
required for the rest of patches, as the settings should be changed on
disabled sensor.

The duty ratio (the fourth patch) is available as extended channel enum.
I've chosen this approach as it is more straightforward from the
sensor's perspective. It will be a bit tricky, but it can be
exported as sampling frequency attribute. The sampling frequency depends
on the integration time and 20% of part to part tolerance needs to be
added. In this case, only the available frequencies for currently
selected integration time should be listed in the available attribute?
Please note that the sampling frequency -> duty ratio mapping is
ambiguous. There are more frequencies which map to different integration
and duty ratio pairs.

The whole series is tested on vcnl4020 and vcnl4200.

The series is based on fixes-togreg (e19ac9d9a978) and two patches
already posted to iio mailing lists (series 'iio: light: vcnl4000:
update sampling rates' [1]).

[1] https://lore.kernel.org/linux-iio/20200108155852.32702-1-tomas@novotny.cz/

Tomas Novotny (5):
  iio: light: vcnl4000: convert to regmap
  iio: light: vcnl4000: add enable attributes for vcnl4040/4200
  iio: light: vcnl4000: add proximity integration time for vcnl4040/4200
  iio: light: vcnl4000: add control of duty ratio
  iio: light: vcnl4000: add control of multi pulse

 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 |  39 ++
 drivers/iio/light/Kconfig                        |   1 +
 drivers/iio/light/vcnl4000.c                     | 545 +++++++++++++++++++++--
 3 files changed, 549 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000

-- 
2.16.4

[PATCH 1/5] iio: light: vcnl4000: convert to regmap

[Tomas Novotny]

It will be easier to configure various device registers.

No functional change.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/light/Kconfig    |  1 +
 drivers/iio/light/vcnl4000.c | 59 ++++++++++++++++++++++++++++++--------------
 2 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 4a1a883dc061..ae2b9dafb9f6 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -475,6 +475,7 @@ config US5182D
 config VCNL4000
 	tristate "VCNL4000/4010/4020/4200 combined ALS and proximity sensor"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  Say Y here if you want to build a driver for the Vishay VCNL4000,
 	  VCNL4010, VCNL4020, VCNL4200 combined ambient light and proximity
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index e5b00a6611ac..9f673e89084a 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -22,6 +22,7 @@
 #include <linux/i2c.h>
 #include <linux/err.h>
 #include <linux/delay.h>
+#include <linux/regmap.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -73,6 +74,7 @@ struct vcnl4200_channel {
 
 struct vcnl4000_data {
 	struct i2c_client *client;
+	struct regmap *regmap;
 	enum vcnl4000_device_ids id;
 	int rev;
 	int al_scale;
@@ -84,6 +86,7 @@ struct vcnl4000_data {
 
 struct vcnl4000_chip_spec {
 	const char *prod;
+	const struct regmap_config *regmap_config;
 	int (*init)(struct vcnl4000_data *data);
 	int (*measure_light)(struct vcnl4000_data *data, int *val);
 	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
@@ -99,15 +102,27 @@ static const struct i2c_device_id vcnl4000_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
 
+static const struct regmap_config vcnl4000_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct regmap_config vcnl4200_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
 static int vcnl4000_init(struct vcnl4000_data *data)
 {
 	int ret, prod_id;
+	unsigned int val;
 
-	ret = i2c_smbus_read_byte_data(data->client, VCNL4000_PROD_REV);
+	ret = regmap_read(data->regmap, VCNL4000_PROD_REV, &val);
 	if (ret < 0)
 		return ret;
 
-	prod_id = ret >> 4;
+	prod_id = val >> 4;
 	switch (prod_id) {
 	case VCNL4000_PROD_ID:
 		if (data->id != VCNL4000)
@@ -123,7 +138,7 @@ static int vcnl4000_init(struct vcnl4000_data *data)
 		return -ENODEV;
 	}
 
-	data->rev = ret & 0xf;
+	data->rev = val & 0xf;
 	data->al_scale = 250000;
 	mutex_init(&data->vcnl4000_lock);
 
@@ -133,19 +148,20 @@ static int vcnl4000_init(struct vcnl4000_data *data)
 static int vcnl4200_init(struct vcnl4000_data *data)
 {
 	int ret, id;
+	unsigned int val;
 
-	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
+	ret = regmap_read(data->regmap, VCNL4200_DEV_ID, &val);
 	if (ret < 0)
 		return ret;
 
-	id = ret & 0xff;
+	id = val & 0xff;
 
 	if (id != VCNL4200_PROD_ID) {
-		ret = i2c_smbus_read_word_data(data->client, VCNL4040_DEV_ID);
+		ret = regmap_read(data->regmap, VCNL4040_DEV_ID, &val);
 		if (ret < 0)
 			return ret;
 
-		id = ret & 0xff;
+		id = val & 0xff;
 
 		if (id != VCNL4040_PROD_ID)
 			return -ENODEV;
@@ -156,10 +172,10 @@ static int vcnl4200_init(struct vcnl4000_data *data)
 	data->rev = (ret >> 8) & 0xf;
 
 	/* Set defaults and enable both channels */
-	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, 0);
+	ret = regmap_write(data->regmap, VCNL4200_AL_CONF, 0);
 	if (ret < 0)
 		return ret;
-	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, 0);
+	ret = regmap_write(data->regmap, VCNL4200_PS_CONF1, 0);
 	if (ret < 0)
 		return ret;
 
@@ -193,22 +209,22 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 				u8 rdy_mask, u8 data_reg, int *val)
 {
 	int tries = 20;
+	unsigned int status;
 	__be16 buf;
 	int ret;
 
 	mutex_lock(&data->vcnl4000_lock);
 
-	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
-					req_mask);
+	ret = regmap_write(data->regmap, VCNL4000_COMMAND, req_mask);
 	if (ret < 0)
 		goto fail;
 
 	/* wait for data to become ready */
 	while (tries--) {
-		ret = i2c_smbus_read_byte_data(data->client, VCNL4000_COMMAND);
+		ret = regmap_read(data->regmap, VCNL4000_COMMAND, &status);
 		if (ret < 0)
 			goto fail;
-		if (ret & rdy_mask)
+		if (status & rdy_mask)
 			break;
 		msleep(20); /* measurement takes up to 100 ms */
 	}
@@ -220,8 +236,8 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 		goto fail;
 	}
 
-	ret = i2c_smbus_read_i2c_block_data(data->client,
-		data_reg, sizeof(buf), (u8 *) &buf);
+	ret = regmap_bulk_read(data->regmap,
+			data_reg, (u8 *) &buf, sizeof(buf));
 	if (ret < 0)
 		goto fail;
 
@@ -253,12 +269,10 @@ static int vcnl4200_measure(struct vcnl4000_data *data,
 
 	mutex_unlock(&chan->lock);
 
-	ret = i2c_smbus_read_word_data(data->client, chan->reg);
+	ret = regmap_read(data->regmap, chan->reg, val);
 	if (ret < 0)
 		return ret;
 
-	*val = ret;
-
 	return 0;
 }
 
@@ -289,24 +303,28 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
+		.regmap_config = &vcnl4000_regmap_config,
 		.init = vcnl4000_init,
 		.measure_light = vcnl4000_measure_light,
 		.measure_proximity = vcnl4000_measure_proximity,
 	},
 	[VCNL4010] = {
 		.prod = "VCNL4010/4020",
+		.regmap_config = &vcnl4000_regmap_config,
 		.init = vcnl4000_init,
 		.measure_light = vcnl4000_measure_light,
 		.measure_proximity = vcnl4000_measure_proximity,
 	},
 	[VCNL4040] = {
 		.prod = "VCNL4040",
+		.regmap_config = &vcnl4200_regmap_config,
 		.init = vcnl4200_init,
 		.measure_light = vcnl4200_measure_light,
 		.measure_proximity = vcnl4200_measure_proximity,
 	},
 	[VCNL4200] = {
 		.prod = "VCNL4200",
+		.regmap_config = &vcnl4200_regmap_config,
 		.init = vcnl4200_init,
 		.measure_light = vcnl4200_measure_light,
 		.measure_proximity = vcnl4200_measure_proximity,
@@ -380,6 +398,11 @@ static int vcnl4000_probe(struct i2c_client *client,
 	data->id = id->driver_data;
 	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
 
+	data->regmap = devm_regmap_init_i2c(client,
+			data->chip_spec->regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
 	ret = data->chip_spec->init(data);
 	if (ret < 0)
 		return ret;
-- 
2.16.4

Re: [PATCH 1/5] iio: light: vcnl4000: convert to regmap

[Marco Felsch]

Hi Tomas,

thanks for the patche =) Pls. check my comments I made inline.

On 20-02-05 11:16, Tomas Novotny wrote:
> It will be easier to configure various device registers.
> 
> No functional change.

Yeah.. should we be more verbose here? Regmap also abstracts the locking
mechanism, provides caches and more..

> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> ---
>  drivers/iio/light/Kconfig    |  1 +
>  drivers/iio/light/vcnl4000.c | 59 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 4a1a883dc061..ae2b9dafb9f6 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -475,6 +475,7 @@ config US5182D
>  config VCNL4000
>  	tristate "VCNL4000/4010/4020/4200 combined ALS and proximity sensor"
>  	depends on I2C
> +	select REGMAP_I2C

We can drop the 'depends on I2C' here.

>  	help
>  	  Say Y here if you want to build a driver for the Vishay VCNL4000,
>  	  VCNL4010, VCNL4020, VCNL4200 combined ambient light and proximity
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index e5b00a6611ac..9f673e89084a 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -22,6 +22,7 @@
>  #include <linux/i2c.h>
>  #include <linux/err.h>
>  #include <linux/delay.h>
> +#include <linux/regmap.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -73,6 +74,7 @@ struct vcnl4200_channel {
>  
>  struct vcnl4000_data {
>  	struct i2c_client *client;

Can we drop the 'struct i2c_client' here?

> +	struct regmap *regmap;
>  	enum vcnl4000_device_ids id;
>  	int rev;
>  	int al_scale;
> @@ -84,6 +86,7 @@ struct vcnl4000_data {
>  
>  struct vcnl4000_chip_spec {
>  	const char *prod;
> +	const struct regmap_config *regmap_config;
>  	int (*init)(struct vcnl4000_data *data);
>  	int (*measure_light)(struct vcnl4000_data *data, int *val);
>  	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
> @@ -99,15 +102,27 @@ static const struct i2c_device_id vcnl4000_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
>  
> +static const struct regmap_config vcnl4000_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct regmap_config vcnl4200_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +};

We should be more precise here, e.g. add write/reable ranges, specify
cache mechanism and so on..

>  static int vcnl4000_init(struct vcnl4000_data *data)
>  {
>  	int ret, prod_id;
> +	unsigned int val;
>  
> -	ret = i2c_smbus_read_byte_data(data->client, VCNL4000_PROD_REV);
> +	ret = regmap_read(data->regmap, VCNL4000_PROD_REV, &val);

Should we use the reg_field mechanism here so we can avoid the shifting
and so on.

>  	if (ret < 0)
>  		return ret;
>  
> -	prod_id = ret >> 4;
> +	prod_id = val >> 4;
>  	switch (prod_id) {
>  	case VCNL4000_PROD_ID:
>  		if (data->id != VCNL4000)
> @@ -123,7 +138,7 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>  		return -ENODEV;
>  	}
>  
> -	data->rev = ret & 0xf;
> +	data->rev = val & 0xf;
>  	data->al_scale = 250000;
>  	mutex_init(&data->vcnl4000_lock);

We can remove the lock if it is used to protect the hw-access.

> @@ -133,19 +148,20 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>  static int vcnl4200_init(struct vcnl4000_data *data)
>  {
>  	int ret, id;
> +	unsigned int val;
>  
> -	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
> +	ret = regmap_read(data->regmap, VCNL4200_DEV_ID, &val);

Same here, we can use the reg_field to avoid bit ops later on.

Regards,
  Marco

>  	if (ret < 0)
>  		return ret;
>  
> -	id = ret & 0xff;
> +	id = val & 0xff;
>  
>  	if (id != VCNL4200_PROD_ID) {
> -		ret = i2c_smbus_read_word_data(data->client, VCNL4040_DEV_ID);
> +		ret = regmap_read(data->regmap, VCNL4040_DEV_ID, &val);
>  		if (ret < 0)
>  			return ret;
>  
> -		id = ret & 0xff;
> +		id = val & 0xff;
>  
>  		if (id != VCNL4040_PROD_ID)
>  			return -ENODEV;
> @@ -156,10 +172,10 @@ static int vcnl4200_init(struct vcnl4000_data *data)
>  	data->rev = (ret >> 8) & 0xf;
>  
>  	/* Set defaults and enable both channels */
> -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, 0);
> +	ret = regmap_write(data->regmap, VCNL4200_AL_CONF, 0);
>  	if (ret < 0)
>  		return ret;
> -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, 0);
> +	ret = regmap_write(data->regmap, VCNL4200_PS_CONF1, 0);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -193,22 +209,22 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  				u8 rdy_mask, u8 data_reg, int *val)
>  {
>  	int tries = 20;
> +	unsigned int status;
>  	__be16 buf;
>  	int ret;
>  
>  	mutex_lock(&data->vcnl4000_lock);
>  
> -	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
> -					req_mask);
> +	ret = regmap_write(data->regmap, VCNL4000_COMMAND, req_mask);
>  	if (ret < 0)
>  		goto fail;
>  
>  	/* wait for data to become ready */
>  	while (tries--) {
> -		ret = i2c_smbus_read_byte_data(data->client, VCNL4000_COMMAND);
> +		ret = regmap_read(data->regmap, VCNL4000_COMMAND, &status);
>  		if (ret < 0)
>  			goto fail;
> -		if (ret & rdy_mask)
> +		if (status & rdy_mask)
>  			break;
>  		msleep(20); /* measurement takes up to 100 ms */
>  	}
> @@ -220,8 +236,8 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  		goto fail;
>  	}
>  
> -	ret = i2c_smbus_read_i2c_block_data(data->client,
> -		data_reg, sizeof(buf), (u8 *) &buf);
> +	ret = regmap_bulk_read(data->regmap,
> +			data_reg, (u8 *) &buf, sizeof(buf));
>  	if (ret < 0)
>  		goto fail;
>  
> @@ -253,12 +269,10 @@ static int vcnl4200_measure(struct vcnl4000_data *data,
>  
>  	mutex_unlock(&chan->lock);
>  
> -	ret = i2c_smbus_read_word_data(data->client, chan->reg);
> +	ret = regmap_read(data->regmap, chan->reg, val);
>  	if (ret < 0)
>  		return ret;
>  
> -	*val = ret;
> -
>  	return 0;
>  }
>  
> @@ -289,24 +303,28 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.prod = "VCNL4000",
> +		.regmap_config = &vcnl4000_regmap_config,
>  		.init = vcnl4000_init,
>  		.measure_light = vcnl4000_measure_light,
>  		.measure_proximity = vcnl4000_measure_proximity,
>  	},
>  	[VCNL4010] = {
>  		.prod = "VCNL4010/4020",
> +		.regmap_config = &vcnl4000_regmap_config,
>  		.init = vcnl4000_init,
>  		.measure_light = vcnl4000_measure_light,
>  		.measure_proximity = vcnl4000_measure_proximity,
>  	},
>  	[VCNL4040] = {
>  		.prod = "VCNL4040",
> +		.regmap_config = &vcnl4200_regmap_config,
>  		.init = vcnl4200_init,
>  		.measure_light = vcnl4200_measure_light,
>  		.measure_proximity = vcnl4200_measure_proximity,
>  	},
>  	[VCNL4200] = {
>  		.prod = "VCNL4200",
> +		.regmap_config = &vcnl4200_regmap_config,
>  		.init = vcnl4200_init,
>  		.measure_light = vcnl4200_measure_light,
>  		.measure_proximity = vcnl4200_measure_proximity,
> @@ -380,6 +398,11 @@ static int vcnl4000_probe(struct i2c_client *client,
>  	data->id = id->driver_data;
>  	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
>  
> +	data->regmap = devm_regmap_init_i2c(client,
> +			data->chip_spec->regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
>  	ret = data->chip_spec->init(data);
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.16.4

Re: [PATCH 1/5] iio: light: vcnl4000: convert to regmap

[Tomas Novotny]

Hi Marco,

On Wed, 5 Feb 2020 12:46:07 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> Hi Tomas,
> 
> thanks for the patche =) Pls. check my comments I made inline.
> 
> On 20-02-05 11:16, Tomas Novotny wrote:
> > It will be easier to configure various device registers.
> > 
> > No functional change.  
> 
> Yeah.. should we be more verbose here? Regmap also abstracts the locking
> mechanism, provides caches and more..

I used a bit misleading wording. I meant that there are no other features
except the regmap (which should be implicit). Also the bare minimum is
configured in regmap. I will change in v2.

> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > ---
> >  drivers/iio/light/Kconfig    |  1 +
> >  drivers/iio/light/vcnl4000.c | 59 ++++++++++++++++++++++++++++++--------------
> >  2 files changed, 42 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 4a1a883dc061..ae2b9dafb9f6 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -475,6 +475,7 @@ config US5182D
> >  config VCNL4000
> >  	tristate "VCNL4000/4010/4020/4200 combined ALS and proximity sensor"
> >  	depends on I2C
> > +	select REGMAP_I2C  
> 
> We can drop the 'depends on I2C' here.

I'm not sure. I'd say that it protects the situation when I2C isn't selected
(thus REGMAP_I2C alone would be wrongly selected). Besides of that, every
other IIO driver in the ligth/ has it, so I would let it as is because of
consistency (or fix all in a separated change).

> >  	help
> >  	  Say Y here if you want to build a driver for the Vishay VCNL4000,
> >  	  VCNL4010, VCNL4020, VCNL4200 combined ambient light and proximity
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index e5b00a6611ac..9f673e89084a 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/err.h>
> >  #include <linux/delay.h>
> > +#include <linux/regmap.h>
> >  
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -73,6 +74,7 @@ struct vcnl4200_channel {
> >  
> >  struct vcnl4000_data {
> >  	struct i2c_client *client;  
> 
> Can we drop the 'struct i2c_client' here?

Nice catch. I can get the device struct for dev_* functions from regmap. So
yes, I will drop it.

> > +	struct regmap *regmap;
> >  	enum vcnl4000_device_ids id;
> >  	int rev;
> >  	int al_scale;
> > @@ -84,6 +86,7 @@ struct vcnl4000_data {
> >  
> >  struct vcnl4000_chip_spec {
> >  	const char *prod;
> > +	const struct regmap_config *regmap_config;
> >  	int (*init)(struct vcnl4000_data *data);
> >  	int (*measure_light)(struct vcnl4000_data *data, int *val);
> >  	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
> > @@ -99,15 +102,27 @@ static const struct i2c_device_id vcnl4000_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> >  
> > +static const struct regmap_config vcnl4000_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +};
> > +
> > +static const struct regmap_config vcnl4200_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 16,
> > +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> > +};  
> 
> We should be more precise here, e.g. add write/reable ranges, specify
> cache mechanism and so on..

That's a point I was also thinking of. I've selected limited usage as the
regmap is used solely in this file and the addresses are fixed. The cache is
not selected because the vast majority of accesses would bypass the cache
(the sensor readings). There would be only a few cache hits during
configuration.

That may change in the near future (I saw that PM was already added), so I
would extend when needed. What do you think?

> >  static int vcnl4000_init(struct vcnl4000_data *data)
> >  {
> >  	int ret, prod_id;
> > +	unsigned int val;
> >  
> > -	ret = i2c_smbus_read_byte_data(data->client, VCNL4000_PROD_REV);
> > +	ret = regmap_read(data->regmap, VCNL4000_PROD_REV, &val);  
> 
> Should we use the reg_field mechanism here so we can avoid the shifting
> and so on.

Yes, you are right, I will use it in v2.

> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	prod_id = ret >> 4;
> > +	prod_id = val >> 4;
> >  	switch (prod_id) {
> >  	case VCNL4000_PROD_ID:
> >  		if (data->id != VCNL4000)
> > @@ -123,7 +138,7 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> >  		return -ENODEV;
> >  	}
> >  
> > -	data->rev = ret & 0xf;
> > +	data->rev = val & 0xf;
> >  	data->al_scale = 250000;
> >  	mutex_init(&data->vcnl4000_lock);  
> 
> We can remove the lock if it is used to protect the hw-access.

That lock protects the whole reading (i.e. establish, wait for result and
then return), so it is still necessary.

> > @@ -133,19 +148,20 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> >  static int vcnl4200_init(struct vcnl4000_data *data)
> >  {
> >  	int ret, id;
> > +	unsigned int val;
> >  
> > -	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
> > +	ret = regmap_read(data->regmap, VCNL4200_DEV_ID, &val);  
> 
> Same here, we can use the reg_field to avoid bit ops later on.

Yes.

Thanks a lot for the review,

Tomas

> Regards,
>   Marco
> 
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	id = ret & 0xff;
> > +	id = val & 0xff;
> >  
> >  	if (id != VCNL4200_PROD_ID) {
> > -		ret = i2c_smbus_read_word_data(data->client, VCNL4040_DEV_ID);
> > +		ret = regmap_read(data->regmap, VCNL4040_DEV_ID, &val);
> >  		if (ret < 0)
> >  			return ret;
> >  
> > -		id = ret & 0xff;
> > +		id = val & 0xff;
> >  
> >  		if (id != VCNL4040_PROD_ID)
> >  			return -ENODEV;
> > @@ -156,10 +172,10 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> >  	data->rev = (ret >> 8) & 0xf;
> >  
> >  	/* Set defaults and enable both channels */
> > -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, 0);
> > +	ret = regmap_write(data->regmap, VCNL4200_AL_CONF, 0);
> >  	if (ret < 0)
> >  		return ret;
> > -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, 0);
> > +	ret = regmap_write(data->regmap, VCNL4200_PS_CONF1, 0);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -193,22 +209,22 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> >  				u8 rdy_mask, u8 data_reg, int *val)
> >  {
> >  	int tries = 20;
> > +	unsigned int status;
> >  	__be16 buf;
> >  	int ret;
> >  
> >  	mutex_lock(&data->vcnl4000_lock);
> >  
> > -	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
> > -					req_mask);
> > +	ret = regmap_write(data->regmap, VCNL4000_COMMAND, req_mask);
> >  	if (ret < 0)
> >  		goto fail;
> >  
> >  	/* wait for data to become ready */
> >  	while (tries--) {
> > -		ret = i2c_smbus_read_byte_data(data->client, VCNL4000_COMMAND);
> > +		ret = regmap_read(data->regmap, VCNL4000_COMMAND, &status);
> >  		if (ret < 0)
> >  			goto fail;
> > -		if (ret & rdy_mask)
> > +		if (status & rdy_mask)
> >  			break;
> >  		msleep(20); /* measurement takes up to 100 ms */
> >  	}
> > @@ -220,8 +236,8 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> >  		goto fail;
> >  	}
> >  
> > -	ret = i2c_smbus_read_i2c_block_data(data->client,
> > -		data_reg, sizeof(buf), (u8 *) &buf);
> > +	ret = regmap_bulk_read(data->regmap,
> > +			data_reg, (u8 *) &buf, sizeof(buf));
> >  	if (ret < 0)
> >  		goto fail;
> >  
> > @@ -253,12 +269,10 @@ static int vcnl4200_measure(struct vcnl4000_data *data,
> >  
> >  	mutex_unlock(&chan->lock);
> >  
> > -	ret = i2c_smbus_read_word_data(data->client, chan->reg);
> > +	ret = regmap_read(data->regmap, chan->reg, val);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	*val = ret;
> > -
> >  	return 0;
> >  }
> >  
> > @@ -289,24 +303,28 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.prod = "VCNL4000",
> > +		.regmap_config = &vcnl4000_regmap_config,
> >  		.init = vcnl4000_init,
> >  		.measure_light = vcnl4000_measure_light,
> >  		.measure_proximity = vcnl4000_measure_proximity,
> >  	},
> >  	[VCNL4010] = {
> >  		.prod = "VCNL4010/4020",
> > +		.regmap_config = &vcnl4000_regmap_config,
> >  		.init = vcnl4000_init,
> >  		.measure_light = vcnl4000_measure_light,
> >  		.measure_proximity = vcnl4000_measure_proximity,
> >  	},
> >  	[VCNL4040] = {
> >  		.prod = "VCNL4040",
> > +		.regmap_config = &vcnl4200_regmap_config,
> >  		.init = vcnl4200_init,
> >  		.measure_light = vcnl4200_measure_light,
> >  		.measure_proximity = vcnl4200_measure_proximity,
> >  	},
> >  	[VCNL4200] = {
> >  		.prod = "VCNL4200",
> > +		.regmap_config = &vcnl4200_regmap_config,
> >  		.init = vcnl4200_init,
> >  		.measure_light = vcnl4200_measure_light,
> >  		.measure_proximity = vcnl4200_measure_proximity,
> > @@ -380,6 +398,11 @@ static int vcnl4000_probe(struct i2c_client *client,
> >  	data->id = id->driver_data;
> >  	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
> >  
> > +	data->regmap = devm_regmap_init_i2c(client,
> > +			data->chip_spec->regmap_config);
> > +	if (IS_ERR(data->regmap))
> > +		return PTR_ERR(data->regmap);
> > +
> >  	ret = data->chip_spec->init(data);
> >  	if (ret < 0)
> >  		return ret;
> > -- 
> > 2.16.4  
> 

[PATCH 2/5] iio: light: vcnl4000: add enable attributes for vcnl4040/4200

[Tomas Novotny]

Both vcnl4040 and vcnl4200 chips start with ambient light and proximity
channels disabled. The driver enables both channels during
initialization. The channels may be enabled or disabled with this
change.

Disabled ambient light channel returns the last measured value on read.
This differs from proximity, which returns 0. Channels return these
values with the configured sampling rate.

The driver doesn't configure some defaults during probe now. Only the
enable bit is handled.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/light/vcnl4000.c | 118 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 102 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 9f673e89084a..f351b100a165 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -58,6 +58,10 @@
 #define VCNL4000_AL_OD		BIT(4) /* start on-demand ALS measurement */
 #define VCNL4000_PS_OD		BIT(3) /* start on-demand proximity measurement */
 
+/* Bit masks for various configuration registers */
+#define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
+#define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
+
 enum vcnl4000_device_ids {
 	VCNL4000,
 	VCNL4010,
@@ -86,10 +90,16 @@ struct vcnl4000_data {
 
 struct vcnl4000_chip_spec {
 	const char *prod;
+	const struct iio_chan_spec *channels;
+	size_t num_channels;
 	const struct regmap_config *regmap_config;
 	int (*init)(struct vcnl4000_data *data);
 	int (*measure_light)(struct vcnl4000_data *data, int *val);
 	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
+	int (*is_enabled)(struct vcnl4000_data *data, enum iio_chan_type type,
+			int *val);
+	int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
+			bool val);
 };
 
 static const struct i2c_device_id vcnl4000_id[] = {
@@ -102,6 +112,30 @@ static const struct i2c_device_id vcnl4000_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
 
+static const struct iio_chan_spec vcnl4000_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),
+	}
+};
+
+static const struct iio_chan_spec vcnl4200_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),
+	}
+};
+
 static const struct regmap_config vcnl4000_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -171,11 +205,10 @@ static int vcnl4200_init(struct vcnl4000_data *data)
 
 	data->rev = (ret >> 8) & 0xf;
 
-	/* Set defaults and enable both channels */
-	ret = regmap_write(data->regmap, VCNL4200_AL_CONF, 0);
+	ret = data->chip_spec->enable(data, IIO_LIGHT, true);
 	if (ret < 0)
 		return ret;
-	ret = regmap_write(data->regmap, VCNL4200_PS_CONF1, 0);
+	ret = data->chip_spec->enable(data, IIO_PROXIMITY, true);
 	if (ret < 0)
 		return ret;
 
@@ -300,9 +333,46 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
 	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
 }
 
+static int vcnl4200_is_enabled(struct vcnl4000_data *data,
+			       enum iio_chan_type type, int *val)
+{
+	int ret;
+
+	switch (type) {
+	case IIO_LIGHT:
+		ret = regmap_read(data->regmap, VCNL4200_AL_CONF, val);
+		*val = !(*val & VCNL4200_AL_SD);
+		break;
+	case IIO_PROXIMITY:
+		ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, val);
+		*val = !(*val & VCNL4200_PS_SD);
+		break;
+	default:
+		return -EINVAL;
+	}
+	return ret < 0 ? ret : IIO_VAL_INT;
+}
+
+static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
+			   bool val)
+{
+	switch (type) {
+	case IIO_LIGHT:
+		return regmap_update_bits(data->regmap, VCNL4200_AL_CONF,
+				VCNL4200_AL_SD, !val);
+	case IIO_PROXIMITY:
+		return regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
+				VCNL4200_PS_SD, !val);
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
+		.channels = vcnl4000_channels,
+		.num_channels = ARRAY_SIZE(vcnl4000_channels),
 		.regmap_config = &vcnl4000_regmap_config,
 		.init = vcnl4000_init,
 		.measure_light = vcnl4000_measure_light,
@@ -310,6 +380,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	},
 	[VCNL4010] = {
 		.prod = "VCNL4010/4020",
+		.channels = vcnl4000_channels,
+		.num_channels = ARRAY_SIZE(vcnl4000_channels),
 		.regmap_config = &vcnl4000_regmap_config,
 		.init = vcnl4000_init,
 		.measure_light = vcnl4000_measure_light,
@@ -317,31 +389,28 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	},
 	[VCNL4040] = {
 		.prod = "VCNL4040",
+		.channels = vcnl4200_channels,
+		.num_channels = ARRAY_SIZE(vcnl4200_channels),
 		.regmap_config = &vcnl4200_regmap_config,
 		.init = vcnl4200_init,
 		.measure_light = vcnl4200_measure_light,
 		.measure_proximity = vcnl4200_measure_proximity,
+		.is_enabled = vcnl4200_is_enabled,
+		.enable = vcnl4200_enable,
 	},
 	[VCNL4200] = {
 		.prod = "VCNL4200",
+		.channels = vcnl4200_channels,
+		.num_channels = ARRAY_SIZE(vcnl4200_channels),
 		.regmap_config = &vcnl4200_regmap_config,
 		.init = vcnl4200_init,
 		.measure_light = vcnl4200_measure_light,
 		.measure_proximity = vcnl4200_measure_proximity,
+		.is_enabled = vcnl4200_is_enabled,
+		.enable = vcnl4200_enable,
 	},
 };
 
-static const struct iio_chan_spec vcnl4000_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),
-	}
-};
-
 static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 				struct iio_chan_spec const *chan,
 				int *val, int *val2, long mask)
@@ -372,6 +441,22 @@ 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:
+		return data->chip_spec->is_enabled(data, chan->type, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl4000_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:
+		return data->chip_spec->enable(data, chan->type, val);
 	default:
 		return -EINVAL;
 	}
@@ -379,6 +464,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 
 static const struct iio_info vcnl4000_info = {
 	.read_raw = vcnl4000_read_raw,
+	.write_raw = vcnl4000_write_raw,
 };
 
 static int vcnl4000_probe(struct i2c_client *client,
@@ -412,8 +498,8 @@ static int vcnl4000_probe(struct i2c_client *client,
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &vcnl4000_info;
-	indio_dev->channels = vcnl4000_channels;
-	indio_dev->num_channels = ARRAY_SIZE(vcnl4000_channels);
+	indio_dev->channels = data->chip_spec->channels;
+	indio_dev->num_channels = data->chip_spec->num_channels;
 	indio_dev->name = VCNL4000_DRV_NAME;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-- 
2.16.4

Re: [PATCH 2/5] iio: light: vcnl4000: add enable attributes for vcnl4040/4200

[Jonathan Cameron]

On Wed,  5 Feb 2020 11:16:52 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Both vcnl4040 and vcnl4200 chips start with ambient light and proximity
> channels disabled. The driver enables both channels during
> initialization. The channels may be enabled or disabled with this
> change.
> 
> Disabled ambient light channel returns the last measured value on read.
> This differs from proximity, which returns 0. Channels return these
> values with the configured sampling rate.
> 
> The driver doesn't configure some defaults during probe now. Only the
> enable bit is handled.
> 
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>

As a general rule we don't do this.  The enable controls for input devices are
only there for things like step counters that will count from point of being
enabled until you read them.

For light sensors etc it's a userspace control that no standard code knows
how to use.  So if it make sense to enable / disable under normal conditions
it should either be done for every reading with increased delays, or you
should look at doing it with runtime power management.  Typical choice
is to turn off if no one has read from the sensor for N seconds.

runtime pm reduces the exposed complexity of the interfaces from userspace
and also leads to power savings when a program is taking readings, but not
that often.

Thanks,

Jonathan

> ---
>  drivers/iio/light/vcnl4000.c | 118 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 102 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 9f673e89084a..f351b100a165 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -58,6 +58,10 @@
>  #define VCNL4000_AL_OD		BIT(4) /* start on-demand ALS measurement */
>  #define VCNL4000_PS_OD		BIT(3) /* start on-demand proximity measurement */
>  
> +/* Bit masks for various configuration registers */
> +#define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
> +#define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> +
>  enum vcnl4000_device_ids {
>  	VCNL4000,
>  	VCNL4010,
> @@ -86,10 +90,16 @@ struct vcnl4000_data {
>  
>  struct vcnl4000_chip_spec {
>  	const char *prod;
> +	const struct iio_chan_spec *channels;
> +	size_t num_channels;
>  	const struct regmap_config *regmap_config;
>  	int (*init)(struct vcnl4000_data *data);
>  	int (*measure_light)(struct vcnl4000_data *data, int *val);
>  	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
> +	int (*is_enabled)(struct vcnl4000_data *data, enum iio_chan_type type,
> +			int *val);
> +	int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
> +			bool val);
>  };
>  
>  static const struct i2c_device_id vcnl4000_id[] = {
> @@ -102,6 +112,30 @@ static const struct i2c_device_id vcnl4000_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
>  
> +static const struct iio_chan_spec vcnl4000_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),
> +	}
> +};
> +
> +static const struct iio_chan_spec vcnl4200_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),
> +	}
> +};
> +
>  static const struct regmap_config vcnl4000_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -171,11 +205,10 @@ static int vcnl4200_init(struct vcnl4000_data *data)
>  
>  	data->rev = (ret >> 8) & 0xf;
>  
> -	/* Set defaults and enable both channels */
> -	ret = regmap_write(data->regmap, VCNL4200_AL_CONF, 0);
> +	ret = data->chip_spec->enable(data, IIO_LIGHT, true);
>  	if (ret < 0)
>  		return ret;
> -	ret = regmap_write(data->regmap, VCNL4200_PS_CONF1, 0);
> +	ret = data->chip_spec->enable(data, IIO_PROXIMITY, true);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -300,9 +333,46 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
>  	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
>  }
>  
> +static int vcnl4200_is_enabled(struct vcnl4000_data *data,
> +			       enum iio_chan_type type, int *val)
> +{
> +	int ret;
> +
> +	switch (type) {
> +	case IIO_LIGHT:
> +		ret = regmap_read(data->regmap, VCNL4200_AL_CONF, val);
> +		*val = !(*val & VCNL4200_AL_SD);
> +		break;
> +	case IIO_PROXIMITY:
> +		ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, val);
> +		*val = !(*val & VCNL4200_PS_SD);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret < 0 ? ret : IIO_VAL_INT;
> +}
> +
> +static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
> +			   bool val)
> +{
> +	switch (type) {
> +	case IIO_LIGHT:
> +		return regmap_update_bits(data->regmap, VCNL4200_AL_CONF,
> +				VCNL4200_AL_SD, !val);
> +	case IIO_PROXIMITY:
> +		return regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
> +				VCNL4200_PS_SD, !val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.prod = "VCNL4000",
> +		.channels = vcnl4000_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
>  		.regmap_config = &vcnl4000_regmap_config,
>  		.init = vcnl4000_init,
>  		.measure_light = vcnl4000_measure_light,
> @@ -310,6 +380,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	},
>  	[VCNL4010] = {
>  		.prod = "VCNL4010/4020",
> +		.channels = vcnl4000_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
>  		.regmap_config = &vcnl4000_regmap_config,
>  		.init = vcnl4000_init,
>  		.measure_light = vcnl4000_measure_light,
> @@ -317,31 +389,28 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	},
>  	[VCNL4040] = {
>  		.prod = "VCNL4040",
> +		.channels = vcnl4200_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4200_channels),
>  		.regmap_config = &vcnl4200_regmap_config,
>  		.init = vcnl4200_init,
>  		.measure_light = vcnl4200_measure_light,
>  		.measure_proximity = vcnl4200_measure_proximity,
> +		.is_enabled = vcnl4200_is_enabled,
> +		.enable = vcnl4200_enable,
>  	},
>  	[VCNL4200] = {
>  		.prod = "VCNL4200",
> +		.channels = vcnl4200_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4200_channels),
>  		.regmap_config = &vcnl4200_regmap_config,
>  		.init = vcnl4200_init,
>  		.measure_light = vcnl4200_measure_light,
>  		.measure_proximity = vcnl4200_measure_proximity,
> +		.is_enabled = vcnl4200_is_enabled,
> +		.enable = vcnl4200_enable,
>  	},
>  };
>  
> -static const struct iio_chan_spec vcnl4000_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),
> -	}
> -};
> -
>  static int vcnl4000_read_raw(struct iio_dev *indio_dev,
>  				struct iio_chan_spec const *chan,
>  				int *val, int *val2, long mask)
> @@ -372,6 +441,22 @@ 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:
> +		return data->chip_spec->is_enabled(data, chan->type, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int vcnl4000_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:
> +		return data->chip_spec->enable(data, chan->type, val);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -379,6 +464,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
>  
>  static const struct iio_info vcnl4000_info = {
>  	.read_raw = vcnl4000_read_raw,
> +	.write_raw = vcnl4000_write_raw,
>  };
>  
>  static int vcnl4000_probe(struct i2c_client *client,
> @@ -412,8 +498,8 @@ static int vcnl4000_probe(struct i2c_client *client,
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &vcnl4000_info;
> -	indio_dev->channels = vcnl4000_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(vcnl4000_channels);
> +	indio_dev->channels = data->chip_spec->channels;
> +	indio_dev->num_channels = data->chip_spec->num_channels;
>  	indio_dev->name = VCNL4000_DRV_NAME;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  

Re: [PATCH 2/5] iio: light: vcnl4000: add enable attributes for vcnl4040/4200

[Tomas Novotny]

Hi Jonathan,

On Sat, 8 Feb 2020 14:53:54 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed,  5 Feb 2020 11:16:52 +0100
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > Both vcnl4040 and vcnl4200 chips start with ambient light and proximity
> > channels disabled. The driver enables both channels during
> > initialization. The channels may be enabled or disabled with this
> > change.
> > 
> > Disabled ambient light channel returns the last measured value on read.
> > This differs from proximity, which returns 0. Channels return these
> > values with the configured sampling rate.
> > 
> > The driver doesn't configure some defaults during probe now. Only the
> > enable bit is handled.
> > 
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>  
> 
> As a general rule we don't do this.  The enable controls for input devices are
> only there for things like step counters that will count from point of being
> enabled until you read them.

ok.

> For light sensors etc it's a userspace control that no standard code knows
> how to use.  So if it make sense to enable / disable under normal conditions
> it should either be done for every reading with increased delays, or you
> should look at doing it with runtime power management.  Typical choice
> is to turn off if no one has read from the sensor for N seconds.
> 
> runtime pm reduces the exposed complexity of the interfaces from userspace
> and also leads to power savings when a program is taking readings, but not
> that often.

yes, the PM way is much neater and I will drop this patch in favour of it.

By the way, what about disabled CONFIG_PM? Although it is not my case, is
there a way how to disable the sensor?

Thanks,

Tomas

> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/light/vcnl4000.c | 118 +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 102 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 9f673e89084a..f351b100a165 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -58,6 +58,10 @@
> >  #define VCNL4000_AL_OD		BIT(4) /* start on-demand ALS measurement */
> >  #define VCNL4000_PS_OD		BIT(3) /* start on-demand proximity measurement */
> >  
> > +/* Bit masks for various configuration registers */
> > +#define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
> > +#define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> > +
> >  enum vcnl4000_device_ids {
> >  	VCNL4000,
> >  	VCNL4010,
> > @@ -86,10 +90,16 @@ struct vcnl4000_data {
> >  
> >  struct vcnl4000_chip_spec {
> >  	const char *prod;
> > +	const struct iio_chan_spec *channels;
> > +	size_t num_channels;
> >  	const struct regmap_config *regmap_config;
> >  	int (*init)(struct vcnl4000_data *data);
> >  	int (*measure_light)(struct vcnl4000_data *data, int *val);
> >  	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
> > +	int (*is_enabled)(struct vcnl4000_data *data, enum iio_chan_type type,
> > +			int *val);
> > +	int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
> > +			bool val);
> >  };
> >  
> >  static const struct i2c_device_id vcnl4000_id[] = {
> > @@ -102,6 +112,30 @@ static const struct i2c_device_id vcnl4000_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> >  
> > +static const struct iio_chan_spec vcnl4000_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),
> > +	}
> > +};
> > +
> > +static const struct iio_chan_spec vcnl4200_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),
> > +	}
> > +};
> > +
> >  static const struct regmap_config vcnl4000_regmap_config = {
> >  	.reg_bits = 8,
> >  	.val_bits = 8,
> > @@ -171,11 +205,10 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> >  
> >  	data->rev = (ret >> 8) & 0xf;
> >  
> > -	/* Set defaults and enable both channels */
> > -	ret = regmap_write(data->regmap, VCNL4200_AL_CONF, 0);
> > +	ret = data->chip_spec->enable(data, IIO_LIGHT, true);
> >  	if (ret < 0)
> >  		return ret;
> > -	ret = regmap_write(data->regmap, VCNL4200_PS_CONF1, 0);
> > +	ret = data->chip_spec->enable(data, IIO_PROXIMITY, true);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -300,9 +333,46 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
> >  	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
> >  }
> >  
> > +static int vcnl4200_is_enabled(struct vcnl4000_data *data,
> > +			       enum iio_chan_type type, int *val)
> > +{
> > +	int ret;
> > +
> > +	switch (type) {
> > +	case IIO_LIGHT:
> > +		ret = regmap_read(data->regmap, VCNL4200_AL_CONF, val);
> > +		*val = !(*val & VCNL4200_AL_SD);
> > +		break;
> > +	case IIO_PROXIMITY:
> > +		ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, val);
> > +		*val = !(*val & VCNL4200_PS_SD);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return ret < 0 ? ret : IIO_VAL_INT;
> > +}
> > +
> > +static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
> > +			   bool val)
> > +{
> > +	switch (type) {
> > +	case IIO_LIGHT:
> > +		return regmap_update_bits(data->regmap, VCNL4200_AL_CONF,
> > +				VCNL4200_AL_SD, !val);
> > +	case IIO_PROXIMITY:
> > +		return regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
> > +				VCNL4200_PS_SD, !val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.prod = "VCNL4000",
> > +		.channels = vcnl4000_channels,
> > +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> >  		.regmap_config = &vcnl4000_regmap_config,
> >  		.init = vcnl4000_init,
> >  		.measure_light = vcnl4000_measure_light,
> > @@ -310,6 +380,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	},
> >  	[VCNL4010] = {
> >  		.prod = "VCNL4010/4020",
> > +		.channels = vcnl4000_channels,
> > +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> >  		.regmap_config = &vcnl4000_regmap_config,
> >  		.init = vcnl4000_init,
> >  		.measure_light = vcnl4000_measure_light,
> > @@ -317,31 +389,28 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	},
> >  	[VCNL4040] = {
> >  		.prod = "VCNL4040",
> > +		.channels = vcnl4200_channels,
> > +		.num_channels = ARRAY_SIZE(vcnl4200_channels),
> >  		.regmap_config = &vcnl4200_regmap_config,
> >  		.init = vcnl4200_init,
> >  		.measure_light = vcnl4200_measure_light,
> >  		.measure_proximity = vcnl4200_measure_proximity,
> > +		.is_enabled = vcnl4200_is_enabled,
> > +		.enable = vcnl4200_enable,
> >  	},
> >  	[VCNL4200] = {
> >  		.prod = "VCNL4200",
> > +		.channels = vcnl4200_channels,
> > +		.num_channels = ARRAY_SIZE(vcnl4200_channels),
> >  		.regmap_config = &vcnl4200_regmap_config,
> >  		.init = vcnl4200_init,
> >  		.measure_light = vcnl4200_measure_light,
> >  		.measure_proximity = vcnl4200_measure_proximity,
> > +		.is_enabled = vcnl4200_is_enabled,
> > +		.enable = vcnl4200_enable,
> >  	},
> >  };
> >  
> > -static const struct iio_chan_spec vcnl4000_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),
> > -	}
> > -};
> > -
> >  static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> >  				struct iio_chan_spec const *chan,
> >  				int *val, int *val2, long mask)
> > @@ -372,6 +441,22 @@ 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:
> > +		return data->chip_spec->is_enabled(data, chan->type, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int vcnl4000_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:
> > +		return data->chip_spec->enable(data, chan->type, val);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -379,6 +464,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> >  
> >  static const struct iio_info vcnl4000_info = {
> >  	.read_raw = vcnl4000_read_raw,
> > +	.write_raw = vcnl4000_write_raw,
> >  };
> >  
> >  static int vcnl4000_probe(struct i2c_client *client,
> > @@ -412,8 +498,8 @@ static int vcnl4000_probe(struct i2c_client *client,
> >  
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->info = &vcnl4000_info;
> > -	indio_dev->channels = vcnl4000_channels;
> > -	indio_dev->num_channels = ARRAY_SIZE(vcnl4000_channels);
> > +	indio_dev->channels = data->chip_spec->channels;
> > +	indio_dev->num_channels = data->chip_spec->num_channels;
> >  	indio_dev->name = VCNL4000_DRV_NAME;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >    
> 

Re: [PATCH 2/5] iio: light: vcnl4000: add enable attributes for vcnl4040/4200

[Jonathan Cameron]

On Tue, 11 Feb 2020 16:37:59 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Hi Jonathan,
> 
> On Sat, 8 Feb 2020 14:53:54 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Wed,  5 Feb 2020 11:16:52 +0100
> > Tomas Novotny <tomas@novotny.cz> wrote:
> >   
> > > Both vcnl4040 and vcnl4200 chips start with ambient light and proximity
> > > channels disabled. The driver enables both channels during
> > > initialization. The channels may be enabled or disabled with this
> > > change.
> > > 
> > > Disabled ambient light channel returns the last measured value on read.
> > > This differs from proximity, which returns 0. Channels return these
> > > values with the configured sampling rate.
> > > 
> > > The driver doesn't configure some defaults during probe now. Only the
> > > enable bit is handled.
> > > 
> > > Signed-off-by: Tomas Novotny <tomas@novotny.cz>    
> > 
> > As a general rule we don't do this.  The enable controls for input devices are
> > only there for things like step counters that will count from point of being
> > enabled until you read them.  
> 
> ok.
> 
> > For light sensors etc it's a userspace control that no standard code knows
> > how to use.  So if it make sense to enable / disable under normal conditions
> > it should either be done for every reading with increased delays, or you
> > should look at doing it with runtime power management.  Typical choice
> > is to turn off if no one has read from the sensor for N seconds.
> > 
> > runtime pm reduces the exposed complexity of the interfaces from userspace
> > and also leads to power savings when a program is taking readings, but not
> > that often.  
> 
> yes, the PM way is much neater and I will drop this patch in favour of it.
> 
> By the way, what about disabled CONFIG_PM? Although it is not my case, is
> there a way how to disable the sensor?

Nope.  If people are running without power management, then they can't
really expect anything other than everything being turned on all the time.


> 
> Thanks,
> 
> Tomas
> 
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > >  drivers/iio/light/vcnl4000.c | 118 +++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 102 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > > index 9f673e89084a..f351b100a165 100644
> > > --- a/drivers/iio/light/vcnl4000.c
> > > +++ b/drivers/iio/light/vcnl4000.c
> > > @@ -58,6 +58,10 @@
> > >  #define VCNL4000_AL_OD		BIT(4) /* start on-demand ALS measurement */
> > >  #define VCNL4000_PS_OD		BIT(3) /* start on-demand proximity measurement */
> > >  
> > > +/* Bit masks for various configuration registers */
> > > +#define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
> > > +#define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> > > +
> > >  enum vcnl4000_device_ids {
> > >  	VCNL4000,
> > >  	VCNL4010,
> > > @@ -86,10 +90,16 @@ struct vcnl4000_data {
> > >  
> > >  struct vcnl4000_chip_spec {
> > >  	const char *prod;
> > > +	const struct iio_chan_spec *channels;
> > > +	size_t num_channels;
> > >  	const struct regmap_config *regmap_config;
> > >  	int (*init)(struct vcnl4000_data *data);
> > >  	int (*measure_light)(struct vcnl4000_data *data, int *val);
> > >  	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
> > > +	int (*is_enabled)(struct vcnl4000_data *data, enum iio_chan_type type,
> > > +			int *val);
> > > +	int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
> > > +			bool val);
> > >  };
> > >  
> > >  static const struct i2c_device_id vcnl4000_id[] = {
> > > @@ -102,6 +112,30 @@ static const struct i2c_device_id vcnl4000_id[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> > >  
> > > +static const struct iio_chan_spec vcnl4000_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),
> > > +	}
> > > +};
> > > +
> > > +static const struct iio_chan_spec vcnl4200_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),
> > > +	}
> > > +};
> > > +
> > >  static const struct regmap_config vcnl4000_regmap_config = {
> > >  	.reg_bits = 8,
> > >  	.val_bits = 8,
> > > @@ -171,11 +205,10 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> > >  
> > >  	data->rev = (ret >> 8) & 0xf;
> > >  
> > > -	/* Set defaults and enable both channels */
> > > -	ret = regmap_write(data->regmap, VCNL4200_AL_CONF, 0);
> > > +	ret = data->chip_spec->enable(data, IIO_LIGHT, true);
> > >  	if (ret < 0)
> > >  		return ret;
> > > -	ret = regmap_write(data->regmap, VCNL4200_PS_CONF1, 0);
> > > +	ret = data->chip_spec->enable(data, IIO_PROXIMITY, true);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > @@ -300,9 +333,46 @@ static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
> > >  	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
> > >  }
> > >  
> > > +static int vcnl4200_is_enabled(struct vcnl4000_data *data,
> > > +			       enum iio_chan_type type, int *val)
> > > +{
> > > +	int ret;
> > > +
> > > +	switch (type) {
> > > +	case IIO_LIGHT:
> > > +		ret = regmap_read(data->regmap, VCNL4200_AL_CONF, val);
> > > +		*val = !(*val & VCNL4200_AL_SD);
> > > +		break;
> > > +	case IIO_PROXIMITY:
> > > +		ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, val);
> > > +		*val = !(*val & VCNL4200_PS_SD);
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +	return ret < 0 ? ret : IIO_VAL_INT;
> > > +}
> > > +
> > > +static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
> > > +			   bool val)
> > > +{
> > > +	switch (type) {
> > > +	case IIO_LIGHT:
> > > +		return regmap_update_bits(data->regmap, VCNL4200_AL_CONF,
> > > +				VCNL4200_AL_SD, !val);
> > > +	case IIO_PROXIMITY:
> > > +		return regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
> > > +				VCNL4200_PS_SD, !val);
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  	[VCNL4000] = {
> > >  		.prod = "VCNL4000",
> > > +		.channels = vcnl4000_channels,
> > > +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> > >  		.regmap_config = &vcnl4000_regmap_config,
> > >  		.init = vcnl4000_init,
> > >  		.measure_light = vcnl4000_measure_light,
> > > @@ -310,6 +380,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  	},
> > >  	[VCNL4010] = {
> > >  		.prod = "VCNL4010/4020",
> > > +		.channels = vcnl4000_channels,
> > > +		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> > >  		.regmap_config = &vcnl4000_regmap_config,
> > >  		.init = vcnl4000_init,
> > >  		.measure_light = vcnl4000_measure_light,
> > > @@ -317,31 +389,28 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  	},
> > >  	[VCNL4040] = {
> > >  		.prod = "VCNL4040",
> > > +		.channels = vcnl4200_channels,
> > > +		.num_channels = ARRAY_SIZE(vcnl4200_channels),
> > >  		.regmap_config = &vcnl4200_regmap_config,
> > >  		.init = vcnl4200_init,
> > >  		.measure_light = vcnl4200_measure_light,
> > >  		.measure_proximity = vcnl4200_measure_proximity,
> > > +		.is_enabled = vcnl4200_is_enabled,
> > > +		.enable = vcnl4200_enable,
> > >  	},
> > >  	[VCNL4200] = {
> > >  		.prod = "VCNL4200",
> > > +		.channels = vcnl4200_channels,
> > > +		.num_channels = ARRAY_SIZE(vcnl4200_channels),
> > >  		.regmap_config = &vcnl4200_regmap_config,
> > >  		.init = vcnl4200_init,
> > >  		.measure_light = vcnl4200_measure_light,
> > >  		.measure_proximity = vcnl4200_measure_proximity,
> > > +		.is_enabled = vcnl4200_is_enabled,
> > > +		.enable = vcnl4200_enable,
> > >  	},
> > >  };
> > >  
> > > -static const struct iio_chan_spec vcnl4000_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),
> > > -	}
> > > -};
> > > -
> > >  static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> > >  				struct iio_chan_spec const *chan,
> > >  				int *val, int *val2, long mask)
> > > @@ -372,6 +441,22 @@ 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:
> > > +		return data->chip_spec->is_enabled(data, chan->type, val);
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static int vcnl4000_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:
> > > +		return data->chip_spec->enable(data, chan->type, val);
> > >  	default:
> > >  		return -EINVAL;
> > >  	}
> > > @@ -379,6 +464,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> > >  
> > >  static const struct iio_info vcnl4000_info = {
> > >  	.read_raw = vcnl4000_read_raw,
> > > +	.write_raw = vcnl4000_write_raw,
> > >  };
> > >  
> > >  static int vcnl4000_probe(struct i2c_client *client,
> > > @@ -412,8 +498,8 @@ static int vcnl4000_probe(struct i2c_client *client,
> > >  
> > >  	indio_dev->dev.parent = &client->dev;
> > >  	indio_dev->info = &vcnl4000_info;
> > > -	indio_dev->channels = vcnl4000_channels;
> > > -	indio_dev->num_channels = ARRAY_SIZE(vcnl4000_channels);
> > > +	indio_dev->channels = data->chip_spec->channels;
> > > +	indio_dev->num_channels = data->chip_spec->num_channels;
> > >  	indio_dev->name = VCNL4000_DRV_NAME;
> > >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > >      
> >   

[PATCH 3/5] iio: light: vcnl4000: add proximity integration time for vcnl4040/4200

[Tomas Novotny]

Proximity integration time handling and available values are added. The
integration time affects sampling rate, so it is computed now. The
settings should be changed only on the disabled channel. The check is
performed and user is notified in case of enabled channel. The helper
function is added as it will be used also for other settings.

Integration time values are taken from the Vishay's documents "Designing
the VCNL4200 Into an Application" from Oct-2019 and "Designing the
VCNL4040 Into an Application" from Nov-2019.

Duty ratio will be made configurable in the next patch.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/light/vcnl4000.c | 188 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 183 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index f351b100a165..0bad082d762d 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -60,6 +60,8 @@
 
 /* Bit masks for various configuration registers */
 #define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
+#define VCNL4200_PS_IT_MASK	GENMASK(3, 1) /* Proximity integration time */
+#define VCNL4200_PS_IT_SHIFT	1
 #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
 
 enum vcnl4000_device_ids {
@@ -74,6 +76,9 @@ struct vcnl4200_channel {
 	ktime_t last_measurement;
 	ktime_t sampling_rate;
 	struct mutex lock;
+	const int *int_time;
+	size_t int_time_size;
+	int ps_duty_ratio;	/* Proximity specific */
 };
 
 struct vcnl4000_data {
@@ -100,6 +105,10 @@ struct vcnl4000_chip_spec {
 			int *val);
 	int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
 			bool val);
+	int (*get_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
+			    int *val, int *val2);
+	int (*set_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
+			    int val, int val2);
 };
 
 static const struct i2c_device_id vcnl4000_id[] = {
@@ -132,10 +141,36 @@ static const struct iio_chan_spec vcnl4200_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),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
 	}
 };
 
+/* Values are directly mapped to register values. */
+/* Integration time in us; 1T, 1.5T, 2T, 2.5T, 3T, 3.5T, 4T, 8T */
+static const int vcnl4040_ps_int_time[] = {
+	0, 125,
+	0, 188, /* 187.5 */
+	0, 250,
+	0, 313, /* 312.5 */
+	0, 375,
+	0, 438, /* 437.5 */
+	0, 500,
+	0, 1000
+};
+
+/* Values are directly mapped to register values. */
+/* Integration time in us; 1T, 1.5T, 2T, 4T, 8T, 9T */
+static const int vcnl4200_ps_int_time[] = {
+	0, 30,
+	0, 45,
+	0, 60,
+	0, 120,
+	0, 240,
+	0, 270
+};
+
 static const struct regmap_config vcnl4000_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -179,6 +214,34 @@ static int vcnl4000_init(struct vcnl4000_data *data)
 	return 0;
 };
 
+static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
+				  enum iio_chan_type type)
+{
+	int ret;
+	int it_val, it_val2;
+	int duty_ratio;
+
+	switch (type) {
+	case IIO_PROXIMITY:
+		ret = data->chip_spec->get_int_time(data, IIO_PROXIMITY,
+						    &it_val, &it_val2);
+		if (ret < 0)
+			return ret;
+
+		duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
+		/*
+		 * Integration time multiplied by duty ratio.
+		 * Add 20% of part to part tolerance.
+		 */
+		data->vcnl4200_ps.sampling_rate =
+			ktime_set(((it_val * duty_ratio) * 6) / 5,
+				  (((it_val2 * duty_ratio) * 6) / 5) * 1000);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int vcnl4200_init(struct vcnl4000_data *data)
 {
 	int ret, id;
@@ -218,18 +281,25 @@ static int vcnl4200_init(struct vcnl4000_data *data)
 	case VCNL4200_PROD_ID:
 		/* Default wait time is 50ms, add 20% tolerance. */
 		data->vcnl4200_al.sampling_rate = ktime_set(0, 60000 * 1000);
-		/* Default wait time is 4.8ms, add 20% tolerance. */
-		data->vcnl4200_ps.sampling_rate = ktime_set(0, 5760 * 1000);
+		data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
+		data->vcnl4200_ps.int_time_size =
+			ARRAY_SIZE(vcnl4200_ps_int_time);
+		data->vcnl4200_ps.ps_duty_ratio = 160;
 		data->al_scale = 24000;
 		break;
 	case VCNL4040_PROD_ID:
 		/* Default wait time is 80ms, add 20% tolerance. */
 		data->vcnl4200_al.sampling_rate = ktime_set(0, 96000 * 1000);
-		/* Default wait time is 5ms, add 20% tolerance. */
-		data->vcnl4200_ps.sampling_rate = ktime_set(0, 6000 * 1000);
+		data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
+		data->vcnl4200_ps.int_time_size =
+			ARRAY_SIZE(vcnl4040_ps_int_time);
+		data->vcnl4200_ps.ps_duty_ratio = 40;
 		data->al_scale = 120000;
 		break;
 	}
+	ret = vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
+	if (ret < 0)
+		return ret;
 	data->vcnl4200_al.last_measurement = ktime_set(0, 0);
 	data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
 	mutex_init(&data->vcnl4200_al.lock);
@@ -368,6 +438,80 @@ static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
 	}
 }
 
+static int vcnl4200_check_enabled(struct vcnl4000_data *data,
+				  enum iio_chan_type type)
+{
+	int ret, val;
+
+	ret = data->chip_spec->is_enabled(data, type, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val) {
+		dev_warn(&data->client->dev,
+			 "Channel is enabled. Parameter cannot be changed.\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int vcnl4200_get_int_time(struct vcnl4000_data *data,
+				 enum iio_chan_type type, int *val, int *val2)
+{
+	int ret;
+	unsigned int reg;
+
+	switch (type) {
+	case IIO_PROXIMITY:
+		ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, &reg);
+		if (ret < 0)
+			return ret;
+
+		reg &= VCNL4200_PS_IT_MASK;
+		reg >>= VCNL4200_PS_IT_SHIFT;
+
+		*val = data->vcnl4200_ps.int_time[reg * 2];
+		*val2 = data->vcnl4200_ps.int_time[reg * 2 + 1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+
+static int vcnl4200_set_int_time(struct vcnl4000_data *data,
+				 enum iio_chan_type type, int val, int val2)
+{
+	int i, ret;
+
+	ret = vcnl4200_check_enabled(data, type);
+	if (ret < 0)
+		return ret;
+
+	switch (type) {
+	case IIO_PROXIMITY:
+		for (i = 0; i < data->vcnl4200_ps.int_time_size; i += 2) {
+			if (val == data->vcnl4200_ps.int_time[i] &&
+			    data->vcnl4200_ps.int_time[i + 1] == val2) {
+				ret = regmap_update_bits(data->regmap,
+							 VCNL4200_PS_CONF1,
+							 VCNL4200_PS_IT_MASK,
+							 (i / 2) <<
+							 VCNL4200_PS_IT_SHIFT);
+				if (ret < 0)
+					return ret;
+				return vcnl4200_set_samp_rate(data,
+							      IIO_PROXIMITY);
+			}
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
@@ -397,6 +541,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.measure_proximity = vcnl4200_measure_proximity,
 		.is_enabled = vcnl4200_is_enabled,
 		.enable = vcnl4200_enable,
+		.get_int_time = vcnl4200_get_int_time,
+		.set_int_time = vcnl4200_set_int_time,
 	},
 	[VCNL4200] = {
 		.prod = "VCNL4200",
@@ -408,6 +554,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.measure_proximity = vcnl4200_measure_proximity,
 		.is_enabled = vcnl4200_is_enabled,
 		.enable = vcnl4200_enable,
+		.get_int_time = vcnl4200_get_int_time,
+		.set_int_time = vcnl4200_set_int_time,
 	},
 };
 
@@ -443,6 +591,32 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_ENABLE:
 		return data->chip_spec->is_enabled(data, chan->type, val);
+	case IIO_CHAN_INFO_INT_TIME:
+		return data->chip_spec->get_int_time(data, chan->type,
+						     val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl4000_read_avail(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				const int **vals, int *type, int *length,
+				long mask)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			*vals = data->vcnl4200_ps.int_time;
+			*length = data->vcnl4200_ps.int_time_size;
+			*type =  IIO_VAL_INT_PLUS_MICRO;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -457,6 +631,9 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_ENABLE:
 		return data->chip_spec->enable(data, chan->type, val);
+	case IIO_CHAN_INFO_INT_TIME:
+		return data->chip_spec->set_int_time(data, chan->type,
+						     val, val2);
 	default:
 		return -EINVAL;
 	}
@@ -464,6 +641,7 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
 
 static const struct iio_info vcnl4000_info = {
 	.read_raw = vcnl4000_read_raw,
+	.read_avail = vcnl4000_read_avail,
 	.write_raw = vcnl4000_write_raw,
 };
 
-- 
2.16.4

Re: [PATCH 3/5] iio: light: vcnl4000: add proximity integration time for vcnl4040/4200

[Jonathan Cameron]

On Wed,  5 Feb 2020 11:16:53 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Proximity integration time handling and available values are added. The
> integration time affects sampling rate, so it is computed now. The
> settings should be changed only on the disabled channel. The check is
> performed and user is notified in case of enabled channel. The helper
> function is added as it will be used also for other settings.
> 
> Integration time values are taken from the Vishay's documents "Designing
> the VCNL4200 Into an Application" from Oct-2019 and "Designing the
> VCNL4040 Into an Application" from Nov-2019.
> 
> Duty ratio will be made configurable in the next patch.
A few comments inline.

Jonathan

> 
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> ---
>  drivers/iio/light/vcnl4000.c | 188 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 183 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index f351b100a165..0bad082d762d 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -60,6 +60,8 @@
>  
>  /* Bit masks for various configuration registers */
>  #define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
> +#define VCNL4200_PS_IT_MASK	GENMASK(3, 1) /* Proximity integration time */
> +#define VCNL4200_PS_IT_SHIFT	1
>  #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
>  
>  enum vcnl4000_device_ids {
> @@ -74,6 +76,9 @@ struct vcnl4200_channel {
>  	ktime_t last_measurement;
>  	ktime_t sampling_rate;
>  	struct mutex lock;
> +	const int *int_time;
> +	size_t int_time_size;
> +	int ps_duty_ratio;	/* Proximity specific */
>  };
>  
>  struct vcnl4000_data {
> @@ -100,6 +105,10 @@ struct vcnl4000_chip_spec {
>  			int *val);
>  	int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
>  			bool val);
> +	int (*get_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
> +			    int *val, int *val2);
> +	int (*set_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
> +			    int val, int val2);
>  };
>  
>  static const struct i2c_device_id vcnl4000_id[] = {
> @@ -132,10 +141,36 @@ static const struct iio_chan_spec vcnl4200_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),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
>  	}
>  };
>  
> +/* Values are directly mapped to register values. */
> +/* Integration time in us; 1T, 1.5T, 2T, 2.5T, 3T, 3.5T, 4T, 8T */
> +static const int vcnl4040_ps_int_time[] = {
> +	0, 125,
> +	0, 188, /* 187.5 */
> +	0, 250,
> +	0, 313, /* 312.5 */
> +	0, 375,
> +	0, 438, /* 437.5 */
> +	0, 500,
> +	0, 1000
> +};
> +
> +/* Values are directly mapped to register values. */
> +/* Integration time in us; 1T, 1.5T, 2T, 4T, 8T, 9T */
> +static const int vcnl4200_ps_int_time[] = {
> +	0, 30,
> +	0, 45,
> +	0, 60,
> +	0, 120,
> +	0, 240,
> +	0, 270
> +};
> +
>  static const struct regmap_config vcnl4000_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -179,6 +214,34 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>  	return 0;
>  };
>  
> +static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> +				  enum iio_chan_type type)
> +{
> +	int ret;
> +	int it_val, it_val2;
> +	int duty_ratio;
> +
> +	switch (type) {
> +	case IIO_PROXIMITY:
> +		ret = data->chip_spec->get_int_time(data, IIO_PROXIMITY,
> +						    &it_val, &it_val2);
> +		if (ret < 0)
> +			return ret;
> +
> +		duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
> +		/*
> +		 * Integration time multiplied by duty ratio.
> +		 * Add 20% of part to part tolerance.
> +		 */
> +		data->vcnl4200_ps.sampling_rate =

sampling_period perhaps?  It's a time rather than a rate (so many per second).

> +			ktime_set(((it_val * duty_ratio) * 6) / 5,
> +				  (((it_val2 * duty_ratio) * 6) / 5) * 1000);
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int vcnl4200_init(struct vcnl4000_data *data)
>  {
>  	int ret, id;
> @@ -218,18 +281,25 @@ static int vcnl4200_init(struct vcnl4000_data *data)
>  	case VCNL4200_PROD_ID:
>  		/* Default wait time is 50ms, add 20% tolerance. */
>  		data->vcnl4200_al.sampling_rate = ktime_set(0, 60000 * 1000);
> -		/* Default wait time is 4.8ms, add 20% tolerance. */
> -		data->vcnl4200_ps.sampling_rate = ktime_set(0, 5760 * 1000);
> +		data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
> +		data->vcnl4200_ps.int_time_size =
> +			ARRAY_SIZE(vcnl4200_ps_int_time);
> +		data->vcnl4200_ps.ps_duty_ratio = 160;
>  		data->al_scale = 24000;
>  		break;
>  	case VCNL4040_PROD_ID:
>  		/* Default wait time is 80ms, add 20% tolerance. */
>  		data->vcnl4200_al.sampling_rate = ktime_set(0, 96000 * 1000);
> -		/* Default wait time is 5ms, add 20% tolerance. */
> -		data->vcnl4200_ps.sampling_rate = ktime_set(0, 6000 * 1000);
> +		data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
> +		data->vcnl4200_ps.int_time_size =
> +			ARRAY_SIZE(vcnl4040_ps_int_time);
> +		data->vcnl4200_ps.ps_duty_ratio = 40;
>  		data->al_scale = 120000;
>  		break;
>  	}
> +	ret = vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> +	if (ret < 0)
> +		return ret;
>  	data->vcnl4200_al.last_measurement = ktime_set(0, 0);
>  	data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
>  	mutex_init(&data->vcnl4200_al.lock);
> @@ -368,6 +438,80 @@ static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
>  	}
>  }
>  
> +static int vcnl4200_check_enabled(struct vcnl4000_data *data,
> +				  enum iio_chan_type type)
> +{
> +	int ret, val;
> +
> +	ret = data->chip_spec->is_enabled(data, type, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val) {
> +		dev_warn(&data->client->dev,
> +			 "Channel is enabled. Parameter cannot be changed.\n");

For a basic parameter like this, userspace isn't going to typically
expect that it can't be changed live.  Hence you should be looking
to go through a disable / modify / enable sequence.

> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vcnl4200_get_int_time(struct vcnl4000_data *data,
> +				 enum iio_chan_type type, int *val, int *val2)
> +{
> +	int ret;
> +	unsigned int reg;
> +
> +	switch (type) {
> +	case IIO_PROXIMITY:
> +		ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, &reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		reg &= VCNL4200_PS_IT_MASK;
> +		reg >>= VCNL4200_PS_IT_SHIFT;
> +
> +		*val = data->vcnl4200_ps.int_time[reg * 2];
> +		*val2 = data->vcnl4200_ps.int_time[reg * 2 + 1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +
> +static int vcnl4200_set_int_time(struct vcnl4000_data *data,
> +				 enum iio_chan_type type, int val, int val2)
> +{
> +	int i, ret;
> +
> +	ret = vcnl4200_check_enabled(data, type);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (type) {

This check is rather paranoid as it shouldn't be possible to get here
without this being IIO_PROXIMITY.

> +	case IIO_PROXIMITY:
> +		for (i = 0; i < data->vcnl4200_ps.int_time_size; i += 2) {
> +			if (val == data->vcnl4200_ps.int_time[i] &&
> +			    data->vcnl4200_ps.int_time[i + 1] == val2) {

> +				ret = regmap_update_bits(data->regmap,
> +							 VCNL4200_PS_CONF1,
> +							 VCNL4200_PS_IT_MASK,
> +							 (i / 2) <<
> +							 VCNL4200_PS_IT_SHIFT);
> +				if (ret < 0)
> +					return ret;
> +				return vcnl4200_set_samp_rate(data,
> +							      IIO_PROXIMITY);
> +			}
> +		}
> +		break;

return -EINVAL here and no need for break or handling below.

> +	default:
> +		return -EINVAL;
> +	}
> +	return -EINVAL;
> +}
> +
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.prod = "VCNL4000",
> @@ -397,6 +541,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.measure_proximity = vcnl4200_measure_proximity,
>  		.is_enabled = vcnl4200_is_enabled,
>  		.enable = vcnl4200_enable,
> +		.get_int_time = vcnl4200_get_int_time,
> +		.set_int_time = vcnl4200_set_int_time,
>  	},

We now support more devices in this driver.

>  	[VCNL4200] = {
>  		.prod = "VCNL4200",
> @@ -408,6 +554,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.measure_proximity = vcnl4200_measure_proximity,
>  		.is_enabled = vcnl4200_is_enabled,
>  		.enable = vcnl4200_enable,
> +		.get_int_time = vcnl4200_get_int_time,
> +		.set_int_time = vcnl4200_set_int_time,
>  	},
>  };
>  
> @@ -443,6 +591,32 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_ENABLE:
>  		return data->chip_spec->is_enabled(data, chan->type, val);
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return data->chip_spec->get_int_time(data, chan->type,
> +						     val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int vcnl4000_read_avail(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				const int **vals, int *type, int *length,
> +				long mask)
> +{
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			*vals = data->vcnl4200_ps.int_time;
> +			*length = data->vcnl4200_ps.int_time_size;
> +			*type =  IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -457,6 +631,9 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_ENABLE:
>  		return data->chip_spec->enable(data, chan->type, val);
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return data->chip_spec->set_int_time(data, chan->type,
> +						     val, val2);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -464,6 +641,7 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
>  
>  static const struct iio_info vcnl4000_info = {
>  	.read_raw = vcnl4000_read_raw,
> +	.read_avail = vcnl4000_read_avail,
>  	.write_raw = vcnl4000_write_raw,
>  };
>  

Re: [PATCH 3/5] iio: light: vcnl4000: add proximity integration time for vcnl4040/4200

[Tomas Novotny]

Hi Jonathan,

On Sat, 8 Feb 2020 15:03:08 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed,  5 Feb 2020 11:16:53 +0100
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > Proximity integration time handling and available values are added. The
> > integration time affects sampling rate, so it is computed now. The
> > settings should be changed only on the disabled channel. The check is
> > performed and user is notified in case of enabled channel. The helper
> > function is added as it will be used also for other settings.
> > 
> > Integration time values are taken from the Vishay's documents "Designing
> > the VCNL4200 Into an Application" from Oct-2019 and "Designing the
> > VCNL4040 Into an Application" from Nov-2019.
> > 
> > Duty ratio will be made configurable in the next patch.  
> A few comments inline.
> 
> Jonathan
> 
> > 
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > ---
> >  drivers/iio/light/vcnl4000.c | 188 +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 183 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index f351b100a165..0bad082d762d 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -60,6 +60,8 @@
> >  
> >  /* Bit masks for various configuration registers */
> >  #define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
> > +#define VCNL4200_PS_IT_MASK	GENMASK(3, 1) /* Proximity integration time */
> > +#define VCNL4200_PS_IT_SHIFT	1
> >  #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> >  
> >  enum vcnl4000_device_ids {
> > @@ -74,6 +76,9 @@ struct vcnl4200_channel {
> >  	ktime_t last_measurement;
> >  	ktime_t sampling_rate;
> >  	struct mutex lock;
> > +	const int *int_time;
> > +	size_t int_time_size;
> > +	int ps_duty_ratio;	/* Proximity specific */
> >  };
> >  
> >  struct vcnl4000_data {
> > @@ -100,6 +105,10 @@ struct vcnl4000_chip_spec {
> >  			int *val);
> >  	int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
> >  			bool val);
> > +	int (*get_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
> > +			    int *val, int *val2);
> > +	int (*set_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
> > +			    int val, int val2);
> >  };
> >  
> >  static const struct i2c_device_id vcnl4000_id[] = {
> > @@ -132,10 +141,36 @@ static const struct iio_chan_spec vcnl4200_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),
> > +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> >  	}
> >  };
> >  
> > +/* Values are directly mapped to register values. */
> > +/* Integration time in us; 1T, 1.5T, 2T, 2.5T, 3T, 3.5T, 4T, 8T */
> > +static const int vcnl4040_ps_int_time[] = {
> > +	0, 125,
> > +	0, 188, /* 187.5 */
> > +	0, 250,
> > +	0, 313, /* 312.5 */
> > +	0, 375,
> > +	0, 438, /* 437.5 */
> > +	0, 500,
> > +	0, 1000
> > +};
> > +
> > +/* Values are directly mapped to register values. */
> > +/* Integration time in us; 1T, 1.5T, 2T, 4T, 8T, 9T */
> > +static const int vcnl4200_ps_int_time[] = {
> > +	0, 30,
> > +	0, 45,
> > +	0, 60,
> > +	0, 120,
> > +	0, 240,
> > +	0, 270
> > +};
> > +
> >  static const struct regmap_config vcnl4000_regmap_config = {
> >  	.reg_bits = 8,
> >  	.val_bits = 8,
> > @@ -179,6 +214,34 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> >  	return 0;
> >  };
> >  
> > +static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> > +				  enum iio_chan_type type)
> > +{
> > +	int ret;
> > +	int it_val, it_val2;
> > +	int duty_ratio;
> > +
> > +	switch (type) {
> > +	case IIO_PROXIMITY:
> > +		ret = data->chip_spec->get_int_time(data, IIO_PROXIMITY,
> > +						    &it_val, &it_val2);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
> > +		/*
> > +		 * Integration time multiplied by duty ratio.
> > +		 * Add 20% of part to part tolerance.
> > +		 */
> > +		data->vcnl4200_ps.sampling_rate =  
> 
> sampling_period perhaps?  It's a time rather than a rate (so many per second).

well, yes. This attribute was introduced in the past. Should I prepend
cleanup patch at the beginning of this series?

> > +			ktime_set(((it_val * duty_ratio) * 6) / 5,
> > +				  (((it_val2 * duty_ratio) * 6) / 5) * 1000);
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static int vcnl4200_init(struct vcnl4000_data *data)
> >  {
> >  	int ret, id;
> > @@ -218,18 +281,25 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> >  	case VCNL4200_PROD_ID:
> >  		/* Default wait time is 50ms, add 20% tolerance. */
> >  		data->vcnl4200_al.sampling_rate = ktime_set(0, 60000 * 1000);
> > -		/* Default wait time is 4.8ms, add 20% tolerance. */
> > -		data->vcnl4200_ps.sampling_rate = ktime_set(0, 5760 * 1000);
> > +		data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
> > +		data->vcnl4200_ps.int_time_size =
> > +			ARRAY_SIZE(vcnl4200_ps_int_time);
> > +		data->vcnl4200_ps.ps_duty_ratio = 160;
> >  		data->al_scale = 24000;
> >  		break;
> >  	case VCNL4040_PROD_ID:
> >  		/* Default wait time is 80ms, add 20% tolerance. */
> >  		data->vcnl4200_al.sampling_rate = ktime_set(0, 96000 * 1000);
> > -		/* Default wait time is 5ms, add 20% tolerance. */
> > -		data->vcnl4200_ps.sampling_rate = ktime_set(0, 6000 * 1000);
> > +		data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
> > +		data->vcnl4200_ps.int_time_size =
> > +			ARRAY_SIZE(vcnl4040_ps_int_time);
> > +		data->vcnl4200_ps.ps_duty_ratio = 40;
> >  		data->al_scale = 120000;
> >  		break;
> >  	}
> > +	ret = vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> > +	if (ret < 0)
> > +		return ret;
> >  	data->vcnl4200_al.last_measurement = ktime_set(0, 0);
> >  	data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
> >  	mutex_init(&data->vcnl4200_al.lock);
> > @@ -368,6 +438,80 @@ static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
> >  	}
> >  }
> >  
> > +static int vcnl4200_check_enabled(struct vcnl4000_data *data,
> > +				  enum iio_chan_type type)
> > +{
> > +	int ret, val;
> > +
> > +	ret = data->chip_spec->is_enabled(data, type, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (val) {
> > +		dev_warn(&data->client->dev,
> > +			 "Channel is enabled. Parameter cannot be changed.\n");  
> 
> For a basic parameter like this, userspace isn't going to typically
> expect that it can't be changed live.  Hence you should be looking
> to go through a disable / modify / enable sequence.

ok, I will handle it.

> > +		return -EBUSY;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int vcnl4200_get_int_time(struct vcnl4000_data *data,
> > +				 enum iio_chan_type type, int *val, int *val2)
> > +{
> > +	int ret;
> > +	unsigned int reg;
> > +
> > +	switch (type) {
> > +	case IIO_PROXIMITY:
> > +		ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, &reg);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		reg &= VCNL4200_PS_IT_MASK;
> > +		reg >>= VCNL4200_PS_IT_SHIFT;
> > +
> > +		*val = data->vcnl4200_ps.int_time[reg * 2];
> > +		*val2 = data->vcnl4200_ps.int_time[reg * 2 + 1];
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +
> > +static int vcnl4200_set_int_time(struct vcnl4000_data *data,
> > +				 enum iio_chan_type type, int val, int val2)
> > +{
> > +	int i, ret;
> > +
> > +	ret = vcnl4200_check_enabled(data, type);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	switch (type) {  
> 
> This check is rather paranoid as it shouldn't be possible to get here
> without this being IIO_PROXIMITY.

I made it confusing. The switch was added as a skeleton because of int. time
for ambient light. I will drop it completely (including the "check effect").

> > +	case IIO_PROXIMITY:
> > +		for (i = 0; i < data->vcnl4200_ps.int_time_size; i += 2) {
> > +			if (val == data->vcnl4200_ps.int_time[i] &&
> > +			    data->vcnl4200_ps.int_time[i + 1] == val2) {  
> 
> > +				ret = regmap_update_bits(data->regmap,
> > +							 VCNL4200_PS_CONF1,
> > +							 VCNL4200_PS_IT_MASK,
> > +							 (i / 2) <<
> > +							 VCNL4200_PS_IT_SHIFT);
> > +				if (ret < 0)
> > +					return ret;
> > +				return vcnl4200_set_samp_rate(data,
> > +							      IIO_PROXIMITY);
> > +			}
> > +		}
> > +		break;  
> 
> return -EINVAL here and no need for break or handling below.
> 
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.prod = "VCNL4000",
> > @@ -397,6 +541,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  		.measure_proximity = vcnl4200_measure_proximity,
> >  		.is_enabled = vcnl4200_is_enabled,
> >  		.enable = vcnl4200_enable,
> > +		.get_int_time = vcnl4200_get_int_time,
> > +		.set_int_time = vcnl4200_set_int_time,
> >  	},  
> 
> We now support more devices in this driver.

You mean that vcnl4000 - vcnl4020 are missing it? There is no integration
time for them. It is guarded by different iio_chan_spec.

Thanks,

Tomas

> >  	[VCNL4200] = {
> >  		.prod = "VCNL4200",
> > @@ -408,6 +554,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  		.measure_proximity = vcnl4200_measure_proximity,
> >  		.is_enabled = vcnl4200_is_enabled,
> >  		.enable = vcnl4200_enable,
> > +		.get_int_time = vcnl4200_get_int_time,
> > +		.set_int_time = vcnl4200_set_int_time,
> >  	},
> >  };
> >  
> > @@ -443,6 +591,32 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> >  		return IIO_VAL_INT_PLUS_MICRO;
> >  	case IIO_CHAN_INFO_ENABLE:
> >  		return data->chip_spec->is_enabled(data, chan->type, val);
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		return data->chip_spec->get_int_time(data, chan->type,
> > +						     val, val2);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int vcnl4000_read_avail(struct iio_dev *indio_dev,
> > +				struct iio_chan_spec const *chan,
> > +				const int **vals, int *type, int *length,
> > +				long mask)
> > +{
> > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			*vals = data->vcnl4200_ps.int_time;
> > +			*length = data->vcnl4200_ps.int_time_size;
> > +			*type =  IIO_VAL_INT_PLUS_MICRO;
> > +			return IIO_AVAIL_LIST;
> > +		default:
> > +			return -EINVAL;
> > +		}
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -457,6 +631,9 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_ENABLE:
> >  		return data->chip_spec->enable(data, chan->type, val);
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		return data->chip_spec->set_int_time(data, chan->type,
> > +						     val, val2);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -464,6 +641,7 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
> >  
> >  static const struct iio_info vcnl4000_info = {
> >  	.read_raw = vcnl4000_read_raw,
> > +	.read_avail = vcnl4000_read_avail,
> >  	.write_raw = vcnl4000_write_raw,
> >  };
> >    
> 

Re: [PATCH 3/5] iio: light: vcnl4000: add proximity integration time for vcnl4040/4200

[Jonathan Cameron]

On Tue, 11 Feb 2020 17:25:52 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Hi Jonathan,
> 
> On Sat, 8 Feb 2020 15:03:08 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Wed,  5 Feb 2020 11:16:53 +0100
> > Tomas Novotny <tomas@novotny.cz> wrote:
> >   
> > > Proximity integration time handling and available values are added. The
> > > integration time affects sampling rate, so it is computed now. The
> > > settings should be changed only on the disabled channel. The check is
> > > performed and user is notified in case of enabled channel. The helper
> > > function is added as it will be used also for other settings.
> > > 
> > > Integration time values are taken from the Vishay's documents "Designing
> > > the VCNL4200 Into an Application" from Oct-2019 and "Designing the
> > > VCNL4040 Into an Application" from Nov-2019.
> > > 
> > > Duty ratio will be made configurable in the next patch.    
> > A few comments inline.
> > 
> > Jonathan
> >   
> > > 
> > > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > > ---
> > >  drivers/iio/light/vcnl4000.c | 188 +++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 183 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > > index f351b100a165..0bad082d762d 100644
> > > --- a/drivers/iio/light/vcnl4000.c
> > > +++ b/drivers/iio/light/vcnl4000.c
> > > @@ -60,6 +60,8 @@
> > >  
> > >  /* Bit masks for various configuration registers */
> > >  #define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
> > > +#define VCNL4200_PS_IT_MASK	GENMASK(3, 1) /* Proximity integration time */
> > > +#define VCNL4200_PS_IT_SHIFT	1
> > >  #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> > >  
> > >  enum vcnl4000_device_ids {
> > > @@ -74,6 +76,9 @@ struct vcnl4200_channel {
> > >  	ktime_t last_measurement;
> > >  	ktime_t sampling_rate;
> > >  	struct mutex lock;
> > > +	const int *int_time;
> > > +	size_t int_time_size;
> > > +	int ps_duty_ratio;	/* Proximity specific */
> > >  };
> > >  
> > >  struct vcnl4000_data {
> > > @@ -100,6 +105,10 @@ struct vcnl4000_chip_spec {
> > >  			int *val);
> > >  	int (*enable)(struct vcnl4000_data *data, enum iio_chan_type type,
> > >  			bool val);
> > > +	int (*get_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
> > > +			    int *val, int *val2);
> > > +	int (*set_int_time)(struct vcnl4000_data *data, enum iio_chan_type type,
> > > +			    int val, int val2);
> > >  };
> > >  
> > >  static const struct i2c_device_id vcnl4000_id[] = {
> > > @@ -132,10 +141,36 @@ static const struct iio_chan_spec vcnl4200_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),
> > > +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> > >  	}
> > >  };
> > >  
> > > +/* Values are directly mapped to register values. */
> > > +/* Integration time in us; 1T, 1.5T, 2T, 2.5T, 3T, 3.5T, 4T, 8T */
> > > +static const int vcnl4040_ps_int_time[] = {
> > > +	0, 125,
> > > +	0, 188, /* 187.5 */
> > > +	0, 250,
> > > +	0, 313, /* 312.5 */
> > > +	0, 375,
> > > +	0, 438, /* 437.5 */
> > > +	0, 500,
> > > +	0, 1000
> > > +};
> > > +
> > > +/* Values are directly mapped to register values. */
> > > +/* Integration time in us; 1T, 1.5T, 2T, 4T, 8T, 9T */
> > > +static const int vcnl4200_ps_int_time[] = {
> > > +	0, 30,
> > > +	0, 45,
> > > +	0, 60,
> > > +	0, 120,
> > > +	0, 240,
> > > +	0, 270
> > > +};
> > > +
> > >  static const struct regmap_config vcnl4000_regmap_config = {
> > >  	.reg_bits = 8,
> > >  	.val_bits = 8,
> > > @@ -179,6 +214,34 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> > >  	return 0;
> > >  };
> > >  
> > > +static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> > > +				  enum iio_chan_type type)
> > > +{
> > > +	int ret;
> > > +	int it_val, it_val2;
> > > +	int duty_ratio;
> > > +
> > > +	switch (type) {
> > > +	case IIO_PROXIMITY:
> > > +		ret = data->chip_spec->get_int_time(data, IIO_PROXIMITY,
> > > +						    &it_val, &it_val2);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
> > > +		/*
> > > +		 * Integration time multiplied by duty ratio.
> > > +		 * Add 20% of part to part tolerance.
> > > +		 */
> > > +		data->vcnl4200_ps.sampling_rate =    
> > 
> > sampling_period perhaps?  It's a time rather than a rate (so many per second).  
> 
> well, yes. This attribute was introduced in the past. Should I prepend
> cleanup patch at the beginning of this series?

That would be great.  If you have already sent out a v2, then it can
always be a follow up tidy up sometime later.

> 
> > > +			ktime_set(((it_val * duty_ratio) * 6) / 5,
> > > +				  (((it_val2 * duty_ratio) * 6) / 5) * 1000);
> > > +		return 0;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > >  static int vcnl4200_init(struct vcnl4000_data *data)
> > >  {
> > >  	int ret, id;
> > > @@ -218,18 +281,25 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> > >  	case VCNL4200_PROD_ID:
> > >  		/* Default wait time is 50ms, add 20% tolerance. */
> > >  		data->vcnl4200_al.sampling_rate = ktime_set(0, 60000 * 1000);
> > > -		/* Default wait time is 4.8ms, add 20% tolerance. */
> > > -		data->vcnl4200_ps.sampling_rate = ktime_set(0, 5760 * 1000);
> > > +		data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
> > > +		data->vcnl4200_ps.int_time_size =
> > > +			ARRAY_SIZE(vcnl4200_ps_int_time);
> > > +		data->vcnl4200_ps.ps_duty_ratio = 160;
> > >  		data->al_scale = 24000;
> > >  		break;
> > >  	case VCNL4040_PROD_ID:
> > >  		/* Default wait time is 80ms, add 20% tolerance. */
> > >  		data->vcnl4200_al.sampling_rate = ktime_set(0, 96000 * 1000);
> > > -		/* Default wait time is 5ms, add 20% tolerance. */
> > > -		data->vcnl4200_ps.sampling_rate = ktime_set(0, 6000 * 1000);
> > > +		data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
> > > +		data->vcnl4200_ps.int_time_size =
> > > +			ARRAY_SIZE(vcnl4040_ps_int_time);
> > > +		data->vcnl4200_ps.ps_duty_ratio = 40;
> > >  		data->al_scale = 120000;
> > >  		break;
> > >  	}
> > > +	ret = vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> > > +	if (ret < 0)
> > > +		return ret;
> > >  	data->vcnl4200_al.last_measurement = ktime_set(0, 0);
> > >  	data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
> > >  	mutex_init(&data->vcnl4200_al.lock);
> > > @@ -368,6 +438,80 @@ static int vcnl4200_enable(struct vcnl4000_data *data, enum iio_chan_type type,
> > >  	}
> > >  }
> > >  
> > > +static int vcnl4200_check_enabled(struct vcnl4000_data *data,
> > > +				  enum iio_chan_type type)
> > > +{
> > > +	int ret, val;
> > > +
> > > +	ret = data->chip_spec->is_enabled(data, type, &val);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (val) {
> > > +		dev_warn(&data->client->dev,
> > > +			 "Channel is enabled. Parameter cannot be changed.\n");    
> > 
> > For a basic parameter like this, userspace isn't going to typically
> > expect that it can't be changed live.  Hence you should be looking
> > to go through a disable / modify / enable sequence.  
> 
> ok, I will handle it.
> 
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int vcnl4200_get_int_time(struct vcnl4000_data *data,
> > > +				 enum iio_chan_type type, int *val, int *val2)
> > > +{
> > > +	int ret;
> > > +	unsigned int reg;
> > > +
> > > +	switch (type) {
> > > +	case IIO_PROXIMITY:
> > > +		ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, &reg);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		reg &= VCNL4200_PS_IT_MASK;
> > > +		reg >>= VCNL4200_PS_IT_SHIFT;
> > > +
> > > +		*val = data->vcnl4200_ps.int_time[reg * 2];
> > > +		*val2 = data->vcnl4200_ps.int_time[reg * 2 + 1];
> > > +		return IIO_VAL_INT_PLUS_MICRO;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +
> > > +static int vcnl4200_set_int_time(struct vcnl4000_data *data,
> > > +				 enum iio_chan_type type, int val, int val2)
> > > +{
> > > +	int i, ret;
> > > +
> > > +	ret = vcnl4200_check_enabled(data, type);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	switch (type) {    
> > 
> > This check is rather paranoid as it shouldn't be possible to get here
> > without this being IIO_PROXIMITY.  
> 
> I made it confusing. The switch was added as a skeleton because of int. time
> for ambient light. I will drop it completely (including the "check effect").

Great.

> 
> > > +	case IIO_PROXIMITY:
> > > +		for (i = 0; i < data->vcnl4200_ps.int_time_size; i += 2) {
> > > +			if (val == data->vcnl4200_ps.int_time[i] &&
> > > +			    data->vcnl4200_ps.int_time[i + 1] == val2) {    
> >   
> > > +				ret = regmap_update_bits(data->regmap,
> > > +							 VCNL4200_PS_CONF1,
> > > +							 VCNL4200_PS_IT_MASK,
> > > +							 (i / 2) <<
> > > +							 VCNL4200_PS_IT_SHIFT);
> > > +				if (ret < 0)
> > > +					return ret;
> > > +				return vcnl4200_set_samp_rate(data,
> > > +							      IIO_PROXIMITY);
> > > +			}
> > > +		}
> > > +		break;    
> > 
> > return -EINVAL here and no need for break or handling below.
> >   
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  	[VCNL4000] = {
> > >  		.prod = "VCNL4000",
> > > @@ -397,6 +541,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  		.measure_proximity = vcnl4200_measure_proximity,
> > >  		.is_enabled = vcnl4200_is_enabled,
> > >  		.enable = vcnl4200_enable,
> > > +		.get_int_time = vcnl4200_get_int_time,
> > > +		.set_int_time = vcnl4200_set_int_time,
> > >  	},    
> > 
> > We now support more devices in this driver.  
> 
> You mean that vcnl4000 - vcnl4020 are missing it? There is no integration
> time for them. It is guarded by different iio_chan_spec.

Ah. My confusion.  Thanks.

> 
> Thanks,
> 
> Tomas
> 
> > >  	[VCNL4200] = {
> > >  		.prod = "VCNL4200",
> > > @@ -408,6 +554,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  		.measure_proximity = vcnl4200_measure_proximity,
> > >  		.is_enabled = vcnl4200_is_enabled,
> > >  		.enable = vcnl4200_enable,
> > > +		.get_int_time = vcnl4200_get_int_time,
> > > +		.set_int_time = vcnl4200_set_int_time,
> > >  	},
> > >  };
> > >  
> > > @@ -443,6 +591,32 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
> > >  		return IIO_VAL_INT_PLUS_MICRO;
> > >  	case IIO_CHAN_INFO_ENABLE:
> > >  		return data->chip_spec->is_enabled(data, chan->type, val);
> > > +	case IIO_CHAN_INFO_INT_TIME:
> > > +		return data->chip_spec->get_int_time(data, chan->type,
> > > +						     val, val2);
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static int vcnl4000_read_avail(struct iio_dev *indio_dev,
> > > +				struct iio_chan_spec const *chan,
> > > +				const int **vals, int *type, int *length,
> > > +				long mask)
> > > +{
> > > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_INT_TIME:
> > > +		switch (chan->type) {
> > > +		case IIO_PROXIMITY:
> > > +			*vals = data->vcnl4200_ps.int_time;
> > > +			*length = data->vcnl4200_ps.int_time_size;
> > > +			*type =  IIO_VAL_INT_PLUS_MICRO;
> > > +			return IIO_AVAIL_LIST;
> > > +		default:
> > > +			return -EINVAL;
> > > +		}
> > >  	default:
> > >  		return -EINVAL;
> > >  	}
> > > @@ -457,6 +631,9 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
> > >  	switch (mask) {
> > >  	case IIO_CHAN_INFO_ENABLE:
> > >  		return data->chip_spec->enable(data, chan->type, val);
> > > +	case IIO_CHAN_INFO_INT_TIME:
> > > +		return data->chip_spec->set_int_time(data, chan->type,
> > > +						     val, val2);
> > >  	default:
> > >  		return -EINVAL;
> > >  	}
> > > @@ -464,6 +641,7 @@ static int vcnl4000_write_raw(struct iio_dev *indio_dev,
> > >  
> > >  static const struct iio_info vcnl4000_info = {
> > >  	.read_raw = vcnl4000_read_raw,
> > > +	.read_avail = vcnl4000_read_avail,
> > >  	.write_raw = vcnl4000_write_raw,
> > >  };
> > >      
> >   

[PATCH 4/5] iio: light: vcnl4000: add control of duty ratio

[Tomas Novotny]

Duty ratio controls the proximity sensor response time. More information
is available in the added documentation.

Duty ratio is specific only for proximity sensor. Only the vcnl4040 and
vcnl4200 hardware supports this settings.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 |  18 +++
 drivers/iio/light/vcnl4000.c                     | 138 ++++++++++++++++++++++-
 2 files changed, 150 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
new file mode 100644
index 000000000000..4860f409dbf0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
@@ -0,0 +1,18 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio
+Date:		February 2020
+KernelVersion:	5.7
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Duty ratio controls the proximity sensor response time. It is a
+		control of on/off led current ratio. The final period depends
+		also on integration time. The formula is simple: integration
+		time * duty ratio off part. The settings cannot be changed when
+		the proximity channel is enabled.  Valid values are in the
+		respective '_available' attribute.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio_available
+Date:		February 2020
+KernelVersion:	5.7
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The on/off available duty ratios.
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 0bad082d762d..a8c2ce1056a6 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -62,6 +62,8 @@
 #define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
 #define VCNL4200_PS_IT_MASK	GENMASK(3, 1) /* Proximity integration time */
 #define VCNL4200_PS_IT_SHIFT	1
+#define VCNL4200_PS_DUTY_MASK	GENMASK(7, 6) /* Proximity duty ratio */
+#define VCNL4200_PS_DUTY_SHIFT	6
 #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
 
 enum vcnl4000_device_ids {
@@ -78,7 +80,7 @@ struct vcnl4200_channel {
 	struct mutex lock;
 	const int *int_time;
 	size_t int_time_size;
-	int ps_duty_ratio;	/* Proximity specific */
+	const int *ps_duty_ratio;	/* Proximity specific */
 };
 
 struct vcnl4000_data {
@@ -132,6 +134,25 @@ static const struct iio_chan_spec vcnl4000_channels[] = {
 	}
 };
 
+static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[];
+
+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) |
+			BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.ext_info = vcnl4040_ps_ext_info,
+	}
+};
+static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[];
+
 static const struct iio_chan_spec vcnl4200_channels[] = {
 	{
 		.type = IIO_LIGHT,
@@ -144,6 +165,7 @@ static const struct iio_chan_spec vcnl4200_channels[] = {
 			BIT(IIO_CHAN_INFO_ENABLE) |
 			BIT(IIO_CHAN_INFO_INT_TIME),
 		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.ext_info = vcnl4200_ps_ext_info,
 	}
 };
 
@@ -171,6 +193,68 @@ static const int vcnl4200_ps_int_time[] = {
 	0, 270
 };
 
+/* Values are directly mapped to register values. */
+static const int vcnl4040_ps_duty_ratio[] = {
+	40,
+	80,
+	160,
+	320
+};
+
+static const char * const vcnl4040_ps_duty_ratio_items[] = {
+	"1/40",
+	"1/80",
+	"1/160",
+	"1/320"
+};
+
+/* Values are directly mapped to register values. */
+static const int vcnl4200_ps_duty_ratio[] = {
+	160,
+	320,
+	640,
+	1280
+};
+
+static const char * const vcnl4200_ps_duty_ratio_items[] = {
+	"1/160",
+	"1/320",
+	"1/640",
+	"1/1280"
+};
+
+static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan);
+static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      unsigned int mode);
+
+static const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
+	.items = vcnl4040_ps_duty_ratio_items,
+	.num_items = ARRAY_SIZE(vcnl4040_ps_duty_ratio_items),
+	.get = vcnl4200_get_ps_duty_ratio,
+	.set = vcnl4200_set_ps_duty_ratio,
+};
+
+static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
+	.items = vcnl4200_ps_duty_ratio_items,
+	.num_items = ARRAY_SIZE(vcnl4200_ps_duty_ratio_items),
+	.get = vcnl4200_get_ps_duty_ratio,
+	.set = vcnl4200_set_ps_duty_ratio,
+};
+
+static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[] = {
+	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4040_ps_duty_ratio_enum),
+	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4040_ps_duty_ratio_enum),
+	{ },
+};
+
+static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[] = {
+	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4200_ps_duty_ratio_enum),
+	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4200_ps_duty_ratio_enum),
+	{ },
+};
+
 static const struct regmap_config vcnl4000_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -228,7 +312,11 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
 		if (ret < 0)
 			return ret;
 
-		duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
+		ret = vcnl4200_get_ps_duty_ratio(iio_priv_to_dev(data), NULL);
+		if (ret < 0)
+			return ret;
+		duty_ratio = data->vcnl4200_ps.ps_duty_ratio[ret];
+
 		/*
 		 * Integration time multiplied by duty ratio.
 		 * Add 20% of part to part tolerance.
@@ -236,6 +324,7 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
 		data->vcnl4200_ps.sampling_rate =
 			ktime_set(((it_val * duty_ratio) * 6) / 5,
 				  (((it_val2 * duty_ratio) * 6) / 5) * 1000);
+
 		return 0;
 	default:
 		return -EINVAL;
@@ -284,7 +373,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
 		data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
 		data->vcnl4200_ps.int_time_size =
 			ARRAY_SIZE(vcnl4200_ps_int_time);
-		data->vcnl4200_ps.ps_duty_ratio = 160;
+		data->vcnl4200_ps.ps_duty_ratio = vcnl4200_ps_duty_ratio;
 		data->al_scale = 24000;
 		break;
 	case VCNL4040_PROD_ID:
@@ -293,7 +382,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
 		data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
 		data->vcnl4200_ps.int_time_size =
 			ARRAY_SIZE(vcnl4040_ps_int_time);
-		data->vcnl4200_ps.ps_duty_ratio = 40;
+		data->vcnl4200_ps.ps_duty_ratio = vcnl4040_ps_duty_ratio;
 		data->al_scale = 120000;
 		break;
 	}
@@ -512,6 +601,43 @@ static int vcnl4200_set_int_time(struct vcnl4000_data *data,
 	return -EINVAL;
 }
 
+static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan)
+{
+	int ret;
+	unsigned int val;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, &val);
+	if (ret < 0)
+		return ret;
+
+	val &= VCNL4200_PS_DUTY_MASK;
+	val >>= VCNL4200_PS_DUTY_SHIFT;
+
+	return val;
+};
+
+static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      unsigned int mode)
+{
+	int ret;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	ret = vcnl4200_check_enabled(data, IIO_PROXIMITY);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
+				 VCNL4200_PS_DUTY_MASK,
+				 mode << VCNL4200_PS_DUTY_SHIFT);
+	if (ret < 0)
+		return ret;
+
+	return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
+};
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
@@ -533,8 +659,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	},
 	[VCNL4040] = {
 		.prod = "VCNL4040",
-		.channels = vcnl4200_channels,
-		.num_channels = ARRAY_SIZE(vcnl4200_channels),
+		.channels = vcnl4040_channels,
+		.num_channels = ARRAY_SIZE(vcnl4040_channels),
 		.regmap_config = &vcnl4200_regmap_config,
 		.init = vcnl4200_init,
 		.measure_light = vcnl4200_measure_light,
-- 
2.16.4

Re: [PATCH 4/5] iio: light: vcnl4000: add control of duty ratio

[Jonathan Cameron]

On Wed,  5 Feb 2020 11:16:54 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Duty ratio controls the proximity sensor response time. More information
> is available in the added documentation.

As always there is significant burden to defining custom ABI for
a device.  Generic userspace will have no idea what to do with it.
In this particular case I can't really think of a straight forward
way of mapping this to existing ABI so I guess we will have to
go with custom.  May be better to make it explicit what it is a
duty ratio of.  I initially thought it was of sampling for the
proximity sensor.

Perhaps something in_proximity_led_duty_cycle

A few comments inline.



> 
> Duty ratio is specific only for proximity sensor. Only the vcnl4040 and
> vcnl4200 hardware supports this settings.
> 
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 |  18 +++
>  drivers/iio/light/vcnl4000.c                     | 138 ++++++++++++++++++++++-
>  2 files changed, 150 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> new file mode 100644
> index 000000000000..4860f409dbf0
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> @@ -0,0 +1,18 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio
> +Date:		February 2020
> +KernelVersion:	5.7
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Duty ratio controls the proximity sensor response time. It is a
> +		control of on/off led current ratio. The final period depends
> +		also on integration time. The formula is simple: integration
> +		time * duty ratio off part. The settings cannot be changed when
> +		the proximity channel is enabled.  Valid values are in the
> +		respective '_available' attribute.

Fix the cannot be changed, by a disable / modify / enable cycle unless there
is a very strong reason not to do that.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio_available
> +Date:		February 2020
> +KernelVersion:	5.7
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The on/off available duty ratios.
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 0bad082d762d..a8c2ce1056a6 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -62,6 +62,8 @@
>  #define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
>  #define VCNL4200_PS_IT_MASK	GENMASK(3, 1) /* Proximity integration time */
>  #define VCNL4200_PS_IT_SHIFT	1
> +#define VCNL4200_PS_DUTY_MASK	GENMASK(7, 6) /* Proximity duty ratio */
> +#define VCNL4200_PS_DUTY_SHIFT	6
>  #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
>  
>  enum vcnl4000_device_ids {
> @@ -78,7 +80,7 @@ struct vcnl4200_channel {
>  	struct mutex lock;
>  	const int *int_time;
>  	size_t int_time_size;
> -	int ps_duty_ratio;	/* Proximity specific */
> +	const int *ps_duty_ratio;	/* Proximity specific */
>  };
>  
>  struct vcnl4000_data {
> @@ -132,6 +134,25 @@ static const struct iio_chan_spec vcnl4000_channels[] = {
>  	}
>  };
>  
> +static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[];
> +
> +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) |
> +			BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.ext_info = vcnl4040_ps_ext_info,
> +	}
> +};
> +static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[];
> +
>  static const struct iio_chan_spec vcnl4200_channels[] = {
>  	{
>  		.type = IIO_LIGHT,
> @@ -144,6 +165,7 @@ static const struct iio_chan_spec vcnl4200_channels[] = {
>  			BIT(IIO_CHAN_INFO_ENABLE) |
>  			BIT(IIO_CHAN_INFO_INT_TIME),
>  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.ext_info = vcnl4200_ps_ext_info,
>  	}
>  };
>  
> @@ -171,6 +193,68 @@ static const int vcnl4200_ps_int_time[] = {
>  	0, 270
>  };
>  
> +/* Values are directly mapped to register values. */
> +static const int vcnl4040_ps_duty_ratio[] = {
> +	40,
> +	80,
> +	160,
> +	320
> +};
> +
> +static const char * const vcnl4040_ps_duty_ratio_items[] = {
> +	"1/40",
> +	"1/80",
> +	"1/160",
> +	"1/320"
> +};
> +
> +/* Values are directly mapped to register values. */
> +static const int vcnl4200_ps_duty_ratio[] = {
> +	160,
> +	320,
> +	640,
> +	1280
> +};
> +
> +static const char * const vcnl4200_ps_duty_ratio_items[] = {
> +	"1/160",

We don't have any interfaces expressed as a fraction.
Please use decimal, even if it is less compact.

> +	"1/320",
> +	"1/640",
> +	"1/1280"
> +};
> +
> +static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan);
> +static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      unsigned int mode);
> +
> +static const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
> +	.items = vcnl4040_ps_duty_ratio_items,
> +	.num_items = ARRAY_SIZE(vcnl4040_ps_duty_ratio_items),
> +	.get = vcnl4200_get_ps_duty_ratio,
> +	.set = vcnl4200_set_ps_duty_ratio,
> +};
> +
> +static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
> +	.items = vcnl4200_ps_duty_ratio_items,
> +	.num_items = ARRAY_SIZE(vcnl4200_ps_duty_ratio_items),
> +	.get = vcnl4200_get_ps_duty_ratio,
> +	.set = vcnl4200_set_ps_duty_ratio,
> +};
> +
> +static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[] = {
> +	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4040_ps_duty_ratio_enum),
> +	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4040_ps_duty_ratio_enum),
> +	{ },
> +};
> +
> +static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[] = {
> +	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4200_ps_duty_ratio_enum),
> +	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4200_ps_duty_ratio_enum),
> +	{ },
> +};
> +
>  static const struct regmap_config vcnl4000_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -228,7 +312,11 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
>  		if (ret < 0)
>  			return ret;
>  
> -		duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
> +		ret = vcnl4200_get_ps_duty_ratio(iio_priv_to_dev(data), NULL);
> +		if (ret < 0)
> +			return ret;
> +		duty_ratio = data->vcnl4200_ps.ps_duty_ratio[ret];
> +
>  		/*
>  		 * Integration time multiplied by duty ratio.
>  		 * Add 20% of part to part tolerance.
> @@ -236,6 +324,7 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
>  		data->vcnl4200_ps.sampling_rate =
>  			ktime_set(((it_val * duty_ratio) * 6) / 5,
>  				  (((it_val2 * duty_ratio) * 6) / 5) * 1000);
> +
Please check patch series for stray whitespace changes like this one.
They add noise and slow down acceptance of patches.
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -284,7 +373,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
>  		data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
>  		data->vcnl4200_ps.int_time_size =
>  			ARRAY_SIZE(vcnl4200_ps_int_time);
> -		data->vcnl4200_ps.ps_duty_ratio = 160;
> +		data->vcnl4200_ps.ps_duty_ratio = vcnl4200_ps_duty_ratio;
>  		data->al_scale = 24000;
>  		break;
>  	case VCNL4040_PROD_ID:
> @@ -293,7 +382,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
>  		data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
>  		data->vcnl4200_ps.int_time_size =
>  			ARRAY_SIZE(vcnl4040_ps_int_time);
> -		data->vcnl4200_ps.ps_duty_ratio = 40;
> +		data->vcnl4200_ps.ps_duty_ratio = vcnl4040_ps_duty_ratio;
>  		data->al_scale = 120000;
>  		break;
>  	}
> @@ -512,6 +601,43 @@ static int vcnl4200_set_int_time(struct vcnl4000_data *data,
>  	return -EINVAL;
>  }
>  
> +static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> +	ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val &= VCNL4200_PS_DUTY_MASK;
> +	val >>= VCNL4200_PS_DUTY_SHIFT;
> +
> +	return val;
> +};
> +
> +static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      unsigned int mode)
> +{
> +	int ret;
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> +	ret = vcnl4200_check_enabled(data, IIO_PROXIMITY);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
> +				 VCNL4200_PS_DUTY_MASK,
> +				 mode << VCNL4200_PS_DUTY_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> +};
> +
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.prod = "VCNL4000",
> @@ -533,8 +659,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	},
>  	[VCNL4040] = {
>  		.prod = "VCNL4040",
> -		.channels = vcnl4200_channels,
> -		.num_channels = ARRAY_SIZE(vcnl4200_channels),
> +		.channels = vcnl4040_channels,
> +		.num_channels = ARRAY_SIZE(vcnl4040_channels),
>  		.regmap_config = &vcnl4200_regmap_config,
>  		.init = vcnl4200_init,
>  		.measure_light = vcnl4200_measure_light,

Re: [PATCH 4/5] iio: light: vcnl4000: add control of duty ratio

[Tomas Novotny]

Hi Jonathan,

On Sat, 8 Feb 2020 15:11:18 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed,  5 Feb 2020 11:16:54 +0100
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > Duty ratio controls the proximity sensor response time. More information
> > is available in the added documentation.  
> 
> As always there is significant burden to defining custom ABI for
> a device.  Generic userspace will have no idea what to do with it.
> In this particular case I can't really think of a straight forward

Would be usage of sampling frequency too confusing/dirty? Led on time is
controlled solely by integration time.

> way of mapping this to existing ABI so I guess we will have to
> go with custom.  May be better to make it explicit what it is a
> duty ratio of.  I initially thought it was of sampling for the
> proximity sensor.
>
> Perhaps something in_proximity_led_duty_cycle
> 
> A few comments inline.
> 
> 
> 
> > 
> > Duty ratio is specific only for proximity sensor. Only the vcnl4040 and
> > vcnl4200 hardware supports this settings.
> > 
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 |  18 +++
> >  drivers/iio/light/vcnl4000.c                     | 138 ++++++++++++++++++++++-
> >  2 files changed, 150 insertions(+), 6 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > new file mode 100644
> > index 000000000000..4860f409dbf0
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > @@ -0,0 +1,18 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio
> > +Date:		February 2020
> > +KernelVersion:	5.7
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Duty ratio controls the proximity sensor response time. It is a
> > +		control of on/off led current ratio. The final period depends
> > +		also on integration time. The formula is simple: integration
> > +		time * duty ratio off part. The settings cannot be changed when
> > +		the proximity channel is enabled.  Valid values are in the
> > +		respective '_available' attribute.  
> 
> Fix the cannot be changed, by a disable / modify / enable cycle unless there
> is a very strong reason not to do that.

ok, I will do.
 
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio_available
> > +Date:		February 2020
> > +KernelVersion:	5.7
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The on/off available duty ratios.
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 0bad082d762d..a8c2ce1056a6 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -62,6 +62,8 @@
> >  #define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
> >  #define VCNL4200_PS_IT_MASK	GENMASK(3, 1) /* Proximity integration time */
> >  #define VCNL4200_PS_IT_SHIFT	1
> > +#define VCNL4200_PS_DUTY_MASK	GENMASK(7, 6) /* Proximity duty ratio */
> > +#define VCNL4200_PS_DUTY_SHIFT	6
> >  #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> >  
> >  enum vcnl4000_device_ids {
> > @@ -78,7 +80,7 @@ struct vcnl4200_channel {
> >  	struct mutex lock;
> >  	const int *int_time;
> >  	size_t int_time_size;
> > -	int ps_duty_ratio;	/* Proximity specific */
> > +	const int *ps_duty_ratio;	/* Proximity specific */
> >  };
> >  
> >  struct vcnl4000_data {
> > @@ -132,6 +134,25 @@ static const struct iio_chan_spec vcnl4000_channels[] = {
> >  	}
> >  };
> >  
> > +static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[];
> > +
> > +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) |
> > +			BIT(IIO_CHAN_INFO_INT_TIME),
> > +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> > +		.ext_info = vcnl4040_ps_ext_info,
> > +	}
> > +};
> > +static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[];
> > +
> >  static const struct iio_chan_spec vcnl4200_channels[] = {
> >  	{
> >  		.type = IIO_LIGHT,
> > @@ -144,6 +165,7 @@ static const struct iio_chan_spec vcnl4200_channels[] = {
> >  			BIT(IIO_CHAN_INFO_ENABLE) |
> >  			BIT(IIO_CHAN_INFO_INT_TIME),
> >  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> > +		.ext_info = vcnl4200_ps_ext_info,
> >  	}
> >  };
> >  
> > @@ -171,6 +193,68 @@ static const int vcnl4200_ps_int_time[] = {
> >  	0, 270
> >  };
> >  
> > +/* Values are directly mapped to register values. */
> > +static const int vcnl4040_ps_duty_ratio[] = {
> > +	40,
> > +	80,
> > +	160,
> > +	320
> > +};
> > +
> > +static const char * const vcnl4040_ps_duty_ratio_items[] = {
> > +	"1/40",
> > +	"1/80",
> > +	"1/160",
> > +	"1/320"
> > +};
> > +
> > +/* Values are directly mapped to register values. */
> > +static const int vcnl4200_ps_duty_ratio[] = {
> > +	160,
> > +	320,
> > +	640,
> > +	1280
> > +};
> > +
> > +static const char * const vcnl4200_ps_duty_ratio_items[] = {
> > +	"1/160",  
> 
> We don't have any interfaces expressed as a fraction.
> Please use decimal, even if it is less compact.

ok.

> > +	"1/320",
> > +	"1/640",
> > +	"1/1280"
> > +};
> > +
> > +static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan);
> > +static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan,
> > +				      unsigned int mode);
> > +
> > +static const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
> > +	.items = vcnl4040_ps_duty_ratio_items,
> > +	.num_items = ARRAY_SIZE(vcnl4040_ps_duty_ratio_items),
> > +	.get = vcnl4200_get_ps_duty_ratio,
> > +	.set = vcnl4200_set_ps_duty_ratio,
> > +};
> > +
> > +static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
> > +	.items = vcnl4200_ps_duty_ratio_items,
> > +	.num_items = ARRAY_SIZE(vcnl4200_ps_duty_ratio_items),
> > +	.get = vcnl4200_get_ps_duty_ratio,
> > +	.set = vcnl4200_set_ps_duty_ratio,
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[] = {
> > +	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4040_ps_duty_ratio_enum),
> > +	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4040_ps_duty_ratio_enum),
> > +	{ },
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[] = {
> > +	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4200_ps_duty_ratio_enum),
> > +	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4200_ps_duty_ratio_enum),
> > +	{ },
> > +};
> > +
> >  static const struct regmap_config vcnl4000_regmap_config = {
> >  	.reg_bits = 8,
> >  	.val_bits = 8,
> > @@ -228,7 +312,11 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> >  		if (ret < 0)
> >  			return ret;
> >  
> > -		duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
> > +		ret = vcnl4200_get_ps_duty_ratio(iio_priv_to_dev(data), NULL);
> > +		if (ret < 0)
> > +			return ret;
> > +		duty_ratio = data->vcnl4200_ps.ps_duty_ratio[ret];
> > +
> >  		/*
> >  		 * Integration time multiplied by duty ratio.
> >  		 * Add 20% of part to part tolerance.
> > @@ -236,6 +324,7 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> >  		data->vcnl4200_ps.sampling_rate =
> >  			ktime_set(((it_val * duty_ratio) * 6) / 5,
> >  				  (((it_val2 * duty_ratio) * 6) / 5) * 1000);
> > +  
> Please check patch series for stray whitespace changes like this one.
> They add noise and slow down acceptance of patches.

:(. Sorry, it wasn't intentional.

Thanks,

Tomas

> >  		return 0;
> >  	default:
> >  		return -EINVAL;
> > @@ -284,7 +373,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> >  		data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
> >  		data->vcnl4200_ps.int_time_size =
> >  			ARRAY_SIZE(vcnl4200_ps_int_time);
> > -		data->vcnl4200_ps.ps_duty_ratio = 160;
> > +		data->vcnl4200_ps.ps_duty_ratio = vcnl4200_ps_duty_ratio;
> >  		data->al_scale = 24000;
> >  		break;
> >  	case VCNL4040_PROD_ID:
> > @@ -293,7 +382,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> >  		data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
> >  		data->vcnl4200_ps.int_time_size =
> >  			ARRAY_SIZE(vcnl4040_ps_int_time);
> > -		data->vcnl4200_ps.ps_duty_ratio = 40;
> > +		data->vcnl4200_ps.ps_duty_ratio = vcnl4040_ps_duty_ratio;
> >  		data->al_scale = 120000;
> >  		break;
> >  	}
> > @@ -512,6 +601,43 @@ static int vcnl4200_set_int_time(struct vcnl4000_data *data,
> >  	return -EINVAL;
> >  }
> >  
> > +static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan)
> > +{
> > +	int ret;
> > +	unsigned int val;
> > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > +	ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	val &= VCNL4200_PS_DUTY_MASK;
> > +	val >>= VCNL4200_PS_DUTY_SHIFT;
> > +
> > +	return val;
> > +};
> > +
> > +static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan,
> > +				      unsigned int mode)
> > +{
> > +	int ret;
> > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > +	ret = vcnl4200_check_enabled(data, IIO_PROXIMITY);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
> > +				 VCNL4200_PS_DUTY_MASK,
> > +				 mode << VCNL4200_PS_DUTY_SHIFT);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> > +};
> > +
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.prod = "VCNL4000",
> > @@ -533,8 +659,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	},
> >  	[VCNL4040] = {
> >  		.prod = "VCNL4040",
> > -		.channels = vcnl4200_channels,
> > -		.num_channels = ARRAY_SIZE(vcnl4200_channels),
> > +		.channels = vcnl4040_channels,
> > +		.num_channels = ARRAY_SIZE(vcnl4040_channels),
> >  		.regmap_config = &vcnl4200_regmap_config,
> >  		.init = vcnl4200_init,
> >  		.measure_light = vcnl4200_measure_light,  
> 

Re: [PATCH 4/5] iio: light: vcnl4000: add control of duty ratio

[Jonathan Cameron]

On Tue, 11 Feb 2020 17:50:02 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Hi Jonathan,
> 
> On Sat, 8 Feb 2020 15:11:18 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Wed,  5 Feb 2020 11:16:54 +0100
> > Tomas Novotny <tomas@novotny.cz> wrote:
> >   
> > > Duty ratio controls the proximity sensor response time. More information
> > > is available in the added documentation.    
> > 
> > As always there is significant burden to defining custom ABI for
> > a device.  Generic userspace will have no idea what to do with it.
> > In this particular case I can't really think of a straight forward  
> 
> Would be usage of sampling frequency too confusing/dirty? Led on time is
> controlled solely by integration time.

I think that would be even more confusing.  In this case custom ABI
is probably necessary.  It's the first time I've seen this particular
hardware trick, so it may never turn up on anything else :)

> 
> > way of mapping this to existing ABI so I guess we will have to
> > go with custom.  May be better to make it explicit what it is a
> > duty ratio of.  I initially thought it was of sampling for the
> > proximity sensor.
> >
> > Perhaps something in_proximity_led_duty_cycle
> > 
> > A few comments inline.
> > 
> > 
> >   
> > > 
> > > Duty ratio is specific only for proximity sensor. Only the vcnl4040 and
> > > vcnl4200 hardware supports this settings.
> > > 
> > > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 |  18 +++
> > >  drivers/iio/light/vcnl4000.c                     | 138 ++++++++++++++++++++++-
> > >  2 files changed, 150 insertions(+), 6 deletions(-)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > > new file mode 100644
> > > index 000000000000..4860f409dbf0
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > > @@ -0,0 +1,18 @@
> > > +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio
> > > +Date:		February 2020
> > > +KernelVersion:	5.7
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Duty ratio controls the proximity sensor response time. It is a
> > > +		control of on/off led current ratio. The final period depends
> > > +		also on integration time. The formula is simple: integration
> > > +		time * duty ratio off part. The settings cannot be changed when
> > > +		the proximity channel is enabled.  Valid values are in the
> > > +		respective '_available' attribute.    
> > 
> > Fix the cannot be changed, by a disable / modify / enable cycle unless there
> > is a very strong reason not to do that.  
> 
> ok, I will do.
>  
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_duty_ratio_available
> > > +Date:		February 2020
> > > +KernelVersion:	5.7
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		The on/off available duty ratios.
> > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > > index 0bad082d762d..a8c2ce1056a6 100644
> > > --- a/drivers/iio/light/vcnl4000.c
> > > +++ b/drivers/iio/light/vcnl4000.c
> > > @@ -62,6 +62,8 @@
> > >  #define VCNL4200_AL_SD		BIT(0) /* Ambient light shutdown */
> > >  #define VCNL4200_PS_IT_MASK	GENMASK(3, 1) /* Proximity integration time */
> > >  #define VCNL4200_PS_IT_SHIFT	1
> > > +#define VCNL4200_PS_DUTY_MASK	GENMASK(7, 6) /* Proximity duty ratio */
> > > +#define VCNL4200_PS_DUTY_SHIFT	6
> > >  #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> > >  
> > >  enum vcnl4000_device_ids {
> > > @@ -78,7 +80,7 @@ struct vcnl4200_channel {
> > >  	struct mutex lock;
> > >  	const int *int_time;
> > >  	size_t int_time_size;
> > > -	int ps_duty_ratio;	/* Proximity specific */
> > > +	const int *ps_duty_ratio;	/* Proximity specific */
> > >  };
> > >  
> > >  struct vcnl4000_data {
> > > @@ -132,6 +134,25 @@ static const struct iio_chan_spec vcnl4000_channels[] = {
> > >  	}
> > >  };
> > >  
> > > +static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[];
> > > +
> > > +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) |
> > > +			BIT(IIO_CHAN_INFO_INT_TIME),
> > > +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> > > +		.ext_info = vcnl4040_ps_ext_info,
> > > +	}
> > > +};
> > > +static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[];
> > > +
> > >  static const struct iio_chan_spec vcnl4200_channels[] = {
> > >  	{
> > >  		.type = IIO_LIGHT,
> > > @@ -144,6 +165,7 @@ static const struct iio_chan_spec vcnl4200_channels[] = {
> > >  			BIT(IIO_CHAN_INFO_ENABLE) |
> > >  			BIT(IIO_CHAN_INFO_INT_TIME),
> > >  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
> > > +		.ext_info = vcnl4200_ps_ext_info,
> > >  	}
> > >  };
> > >  
> > > @@ -171,6 +193,68 @@ static const int vcnl4200_ps_int_time[] = {
> > >  	0, 270
> > >  };
> > >  
> > > +/* Values are directly mapped to register values. */
> > > +static const int vcnl4040_ps_duty_ratio[] = {
> > > +	40,
> > > +	80,
> > > +	160,
> > > +	320
> > > +};
> > > +
> > > +static const char * const vcnl4040_ps_duty_ratio_items[] = {
> > > +	"1/40",
> > > +	"1/80",
> > > +	"1/160",
> > > +	"1/320"
> > > +};
> > > +
> > > +/* Values are directly mapped to register values. */
> > > +static const int vcnl4200_ps_duty_ratio[] = {
> > > +	160,
> > > +	320,
> > > +	640,
> > > +	1280
> > > +};
> > > +
> > > +static const char * const vcnl4200_ps_duty_ratio_items[] = {
> > > +	"1/160",    
> > 
> > We don't have any interfaces expressed as a fraction.
> > Please use decimal, even if it is less compact.  
> 
> ok.
> 
> > > +	"1/320",
> > > +	"1/640",
> > > +	"1/1280"
> > > +};
> > > +
> > > +static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
> > > +				      const struct iio_chan_spec *chan);
> > > +static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> > > +				      const struct iio_chan_spec *chan,
> > > +				      unsigned int mode);
> > > +
> > > +static const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
> > > +	.items = vcnl4040_ps_duty_ratio_items,
> > > +	.num_items = ARRAY_SIZE(vcnl4040_ps_duty_ratio_items),
> > > +	.get = vcnl4200_get_ps_duty_ratio,
> > > +	.set = vcnl4200_set_ps_duty_ratio,
> > > +};
> > > +
> > > +static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
> > > +	.items = vcnl4200_ps_duty_ratio_items,
> > > +	.num_items = ARRAY_SIZE(vcnl4200_ps_duty_ratio_items),
> > > +	.get = vcnl4200_get_ps_duty_ratio,
> > > +	.set = vcnl4200_set_ps_duty_ratio,
> > > +};
> > > +
> > > +static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[] = {
> > > +	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4040_ps_duty_ratio_enum),
> > > +	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4040_ps_duty_ratio_enum),
> > > +	{ },
> > > +};
> > > +
> > > +static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[] = {
> > > +	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4200_ps_duty_ratio_enum),
> > > +	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4200_ps_duty_ratio_enum),
> > > +	{ },
> > > +};
> > > +
> > >  static const struct regmap_config vcnl4000_regmap_config = {
> > >  	.reg_bits = 8,
> > >  	.val_bits = 8,
> > > @@ -228,7 +312,11 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> > >  		if (ret < 0)
> > >  			return ret;
> > >  
> > > -		duty_ratio = data->vcnl4200_ps.ps_duty_ratio;
> > > +		ret = vcnl4200_get_ps_duty_ratio(iio_priv_to_dev(data), NULL);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +		duty_ratio = data->vcnl4200_ps.ps_duty_ratio[ret];
> > > +
> > >  		/*
> > >  		 * Integration time multiplied by duty ratio.
> > >  		 * Add 20% of part to part tolerance.
> > > @@ -236,6 +324,7 @@ static int vcnl4200_set_samp_rate(struct vcnl4000_data *data,
> > >  		data->vcnl4200_ps.sampling_rate =
> > >  			ktime_set(((it_val * duty_ratio) * 6) / 5,
> > >  				  (((it_val2 * duty_ratio) * 6) / 5) * 1000);
> > > +    
> > Please check patch series for stray whitespace changes like this one.
> > They add noise and slow down acceptance of patches.  
> 
> :(. Sorry, it wasn't intentional.
> 
> Thanks,
> 
> Tomas
> 
> > >  		return 0;
> > >  	default:
> > >  		return -EINVAL;
> > > @@ -284,7 +373,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> > >  		data->vcnl4200_ps.int_time = vcnl4200_ps_int_time;
> > >  		data->vcnl4200_ps.int_time_size =
> > >  			ARRAY_SIZE(vcnl4200_ps_int_time);
> > > -		data->vcnl4200_ps.ps_duty_ratio = 160;
> > > +		data->vcnl4200_ps.ps_duty_ratio = vcnl4200_ps_duty_ratio;
> > >  		data->al_scale = 24000;
> > >  		break;
> > >  	case VCNL4040_PROD_ID:
> > > @@ -293,7 +382,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> > >  		data->vcnl4200_ps.int_time = vcnl4040_ps_int_time;
> > >  		data->vcnl4200_ps.int_time_size =
> > >  			ARRAY_SIZE(vcnl4040_ps_int_time);
> > > -		data->vcnl4200_ps.ps_duty_ratio = 40;
> > > +		data->vcnl4200_ps.ps_duty_ratio = vcnl4040_ps_duty_ratio;
> > >  		data->al_scale = 120000;
> > >  		break;
> > >  	}
> > > @@ -512,6 +601,43 @@ static int vcnl4200_set_int_time(struct vcnl4000_data *data,
> > >  	return -EINVAL;
> > >  }
> > >  
> > > +static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
> > > +				      const struct iio_chan_spec *chan)
> > > +{
> > > +	int ret;
> > > +	unsigned int val;
> > > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > > +
> > > +	ret = regmap_read(data->regmap, VCNL4200_PS_CONF1, &val);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	val &= VCNL4200_PS_DUTY_MASK;
> > > +	val >>= VCNL4200_PS_DUTY_SHIFT;
> > > +
> > > +	return val;
> > > +};
> > > +
> > > +static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> > > +				      const struct iio_chan_spec *chan,
> > > +				      unsigned int mode)
> > > +{
> > > +	int ret;
> > > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > > +
> > > +	ret = vcnl4200_check_enabled(data, IIO_PROXIMITY);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = regmap_update_bits(data->regmap, VCNL4200_PS_CONF1,
> > > +				 VCNL4200_PS_DUTY_MASK,
> > > +				 mode << VCNL4200_PS_DUTY_SHIFT);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> > > +};
> > > +
> > >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  	[VCNL4000] = {
> > >  		.prod = "VCNL4000",
> > > @@ -533,8 +659,8 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  	},
> > >  	[VCNL4040] = {
> > >  		.prod = "VCNL4040",
> > > -		.channels = vcnl4200_channels,
> > > -		.num_channels = ARRAY_SIZE(vcnl4200_channels),
> > > +		.channels = vcnl4040_channels,
> > > +		.num_channels = ARRAY_SIZE(vcnl4040_channels),
> > >  		.regmap_config = &vcnl4200_regmap_config,
> > >  		.init = vcnl4200_init,
> > >  		.measure_light = vcnl4200_measure_light,    
> >   

[PATCH 5/5] iio: light: vcnl4000: add control of multi pulse

[Tomas Novotny]

Multi pulse settings allow to emit more pulses during one measurement
(up to 8 on vcnl4040 and vcnl4200). The raw reading is approximately
multiplied by the multi pulse settings. More information is available in
the added documentation.

Multi pulse is specific only for proximity sensor. Only the vcnl4040 and
vcnl4200 hardware supports this settings.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 | 21 +++++++++
 drivers/iio/light/vcnl4000.c                     | 60 ++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
index 4860f409dbf0..923a78dc9869 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
+++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
@@ -16,3 +16,24 @@ KernelVersion:	5.7
 Contact:	linux-iio@vger.kernel.org
 Description:
 		The on/off available duty ratios.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_multi_pulse
+Date:		February 2020
+KernelVersion:	5.7
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Instead of one single pulse per every measurement, more pulses
+		may be programmed. This leads to a longer led current on-time
+		for each proximity measurement, which also results in a higher
+		detection range. The raw reading is approximately multiplied by
+		the multi pulse settings. The duty ration is not changed. The
+		settings cannot be changed when the proximity channel is
+		enabled.  Valid values are in the respective '_available'
+		attribute.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_multi_pulse_available
+Date:		February 2020
+KernelVersion:	5.7
+Contact:	linux-iio@vger.kernel.org
+Description:
+		List of multi pulse values.
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index a8c2ce1056a6..cc75e5e7e634 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -46,6 +46,7 @@
 
 #define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
 #define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
+#define VCNL4200_PS_CONF3	0x04 /* Proximity conf., white channel, LED I */
 #define VCNL4200_PS_DATA	0x08 /* Proximity data */
 #define VCNL4200_AL_DATA	0x09 /* Ambient light data */
 #define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
@@ -65,6 +66,8 @@
 #define VCNL4200_PS_DUTY_MASK	GENMASK(7, 6) /* Proximity duty ratio */
 #define VCNL4200_PS_DUTY_SHIFT	6
 #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
+#define VCNL4200_PS_MPS_MASK	GENMASK(6, 5)
+#define VCNL4200_PS_MPS_SHIFT	5
 
 enum vcnl4000_device_ids {
 	VCNL4000,
@@ -223,11 +226,24 @@ static const char * const vcnl4200_ps_duty_ratio_items[] = {
 	"1/1280"
 };
 
+/* Values are directly mapped to register values. */
+static const char * const vcnl4200_ps_multi_pulse_items[] = {
+	"1",
+	"2",
+	"4",
+	"8"
+};
+
 static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
 				      const struct iio_chan_spec *chan);
 static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
 				      const struct iio_chan_spec *chan,
 				      unsigned int mode);
+static int vcnl4200_get_ps_multi_pulse(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan);
+static int vcnl4200_set_ps_multi_pulse(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan,
+				       unsigned int mode);
 
 static const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
 	.items = vcnl4040_ps_duty_ratio_items,
@@ -243,15 +259,26 @@ static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
 	.set = vcnl4200_set_ps_duty_ratio,
 };
 
+static const struct iio_enum vcnl4200_ps_multi_pulse_enum = {
+	.items = vcnl4200_ps_multi_pulse_items,
+	.num_items = ARRAY_SIZE(vcnl4200_ps_multi_pulse_items),
+	.get = vcnl4200_get_ps_multi_pulse,
+	.set = vcnl4200_set_ps_multi_pulse,
+};
+
 static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[] = {
 	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4040_ps_duty_ratio_enum),
 	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4040_ps_duty_ratio_enum),
+	IIO_ENUM("multi_pulse", IIO_SEPARATE, &vcnl4200_ps_multi_pulse_enum),
+	IIO_ENUM_AVAILABLE("multi_pulse", &vcnl4200_ps_multi_pulse_enum),
 	{ },
 };
 
 static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[] = {
 	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4200_ps_duty_ratio_enum),
 	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4200_ps_duty_ratio_enum),
+	IIO_ENUM("multi_pulse", IIO_SEPARATE, &vcnl4200_ps_multi_pulse_enum),
+	IIO_ENUM_AVAILABLE("multi_pulse", &vcnl4200_ps_multi_pulse_enum),
 	{ },
 };
 
@@ -638,6 +665,39 @@ static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
 	return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
 };
 
+static int vcnl4200_get_ps_multi_pulse(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan)
+{
+	int ret;
+	unsigned int val;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	ret = regmap_read(data->regmap, VCNL4200_PS_CONF3, &val);
+	if (ret < 0)
+		return ret;
+
+	val &= VCNL4200_PS_MPS_MASK;
+	val >>= VCNL4200_PS_MPS_SHIFT;
+
+	return val;
+};
+
+static int vcnl4200_set_ps_multi_pulse(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan,
+				       unsigned int mode)
+{
+	int ret;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	ret = vcnl4200_check_enabled(data, IIO_PROXIMITY);
+	if (ret < 0)
+		return ret;
+
+	return regmap_update_bits(data->regmap, VCNL4200_PS_CONF3,
+				  VCNL4200_PS_MPS_MASK,
+				  mode << VCNL4200_PS_MPS_SHIFT);
+};
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
-- 
2.16.4

Re: [PATCH 5/5] iio: light: vcnl4000: add control of multi pulse

[Jonathan Cameron]

On Wed,  5 Feb 2020 11:16:55 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Multi pulse settings allow to emit more pulses during one measurement
> (up to 8 on vcnl4040 and vcnl4200). The raw reading is approximately
> multiplied by the multi pulse settings. More information is available in
> the added documentation.
> 
> Multi pulse is specific only for proximity sensor. Only the vcnl4040 and
> vcnl4200 hardware supports this settings.

A few comments but this one is more or less good.

Thanks,

Jonathan

> 
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 | 21 +++++++++
>  drivers/iio/light/vcnl4000.c                     | 60 ++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> index 4860f409dbf0..923a78dc9869 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> @@ -16,3 +16,24 @@ KernelVersion:	5.7
>  Contact:	linux-iio@vger.kernel.org
>  Description:
>  		The on/off available duty ratios.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_multi_pulse

This is less ambiguous than duty_cycle, but perhaps something 
in_proximity_led_pulse_count is more specific?

> +Date:		February 2020
> +KernelVersion:	5.7
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Instead of one single pulse per every measurement, more pulses
> +		may be programmed. This leads to a longer led current on-time
> +		for each proximity measurement, which also results in a higher
> +		detection range. The raw reading is approximately multiplied by
> +		the multi pulse settings. The duty ration is not changed. The

Hmm. Normal meaning of duty ratio would be changed by this...  It's uneven but
multiple pulses == more on time hence lower duty ratio.

> +		settings cannot be changed when the proximity channel is
> +		enabled.  Valid values are in the respective '_available'
> +		attribute.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_multi_pulse_available
> +Date:		February 2020
> +KernelVersion:	5.7
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		List of multi pulse values.
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index a8c2ce1056a6..cc75e5e7e634 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -46,6 +46,7 @@
>  
>  #define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
>  #define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
> +#define VCNL4200_PS_CONF3	0x04 /* Proximity conf., white channel, LED I */
>  #define VCNL4200_PS_DATA	0x08 /* Proximity data */
>  #define VCNL4200_AL_DATA	0x09 /* Ambient light data */
>  #define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
> @@ -65,6 +66,8 @@
>  #define VCNL4200_PS_DUTY_MASK	GENMASK(7, 6) /* Proximity duty ratio */
>  #define VCNL4200_PS_DUTY_SHIFT	6
>  #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> +#define VCNL4200_PS_MPS_MASK	GENMASK(6, 5)
> +#define VCNL4200_PS_MPS_SHIFT	5

You could probably use the FIELD_PREP  macros etc to avoid having both mask and
shift defines.

>  
>  enum vcnl4000_device_ids {
>  	VCNL4000,
> @@ -223,11 +226,24 @@ static const char * const vcnl4200_ps_duty_ratio_items[] = {
>  	"1/1280"
>  };
>  
> +/* Values are directly mapped to register values. */
> +static const char * const vcnl4200_ps_multi_pulse_items[] = {
> +	"1",
> +	"2",
> +	"4",
> +	"8"
> +};
> +
>  static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
>  				      const struct iio_chan_spec *chan);
>  static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
>  				      const struct iio_chan_spec *chan,
>  				      unsigned int mode);
> +static int vcnl4200_get_ps_multi_pulse(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan);
> +static int vcnl4200_set_ps_multi_pulse(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan,
> +				       unsigned int mode);
>  
>  static const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
>  	.items = vcnl4040_ps_duty_ratio_items,
> @@ -243,15 +259,26 @@ static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
>  	.set = vcnl4200_set_ps_duty_ratio,
>  };
>  
> +static const struct iio_enum vcnl4200_ps_multi_pulse_enum = {
> +	.items = vcnl4200_ps_multi_pulse_items,
> +	.num_items = ARRAY_SIZE(vcnl4200_ps_multi_pulse_items),
> +	.get = vcnl4200_get_ps_multi_pulse,
> +	.set = vcnl4200_set_ps_multi_pulse,
> +};
> +
>  static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[] = {
>  	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4040_ps_duty_ratio_enum),
>  	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4040_ps_duty_ratio_enum),
> +	IIO_ENUM("multi_pulse", IIO_SEPARATE, &vcnl4200_ps_multi_pulse_enum),
> +	IIO_ENUM_AVAILABLE("multi_pulse", &vcnl4200_ps_multi_pulse_enum),
>  	{ },
>  };
>  
>  static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[] = {
>  	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4200_ps_duty_ratio_enum),
>  	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4200_ps_duty_ratio_enum),
> +	IIO_ENUM("multi_pulse", IIO_SEPARATE, &vcnl4200_ps_multi_pulse_enum),
> +	IIO_ENUM_AVAILABLE("multi_pulse", &vcnl4200_ps_multi_pulse_enum),
>  	{ },
>  };
>  
> @@ -638,6 +665,39 @@ static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
>  	return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
>  };
>  
> +static int vcnl4200_get_ps_multi_pulse(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> +	ret = regmap_read(data->regmap, VCNL4200_PS_CONF3, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val &= VCNL4200_PS_MPS_MASK;
> +	val >>= VCNL4200_PS_MPS_SHIFT;
> +
> +	return val;
> +};
> +
> +static int vcnl4200_set_ps_multi_pulse(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan,
> +				       unsigned int mode)
> +{
> +	int ret;
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> +	ret = vcnl4200_check_enabled(data, IIO_PROXIMITY);
> +	if (ret < 0)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, VCNL4200_PS_CONF3,
> +				  VCNL4200_PS_MPS_MASK,
> +				  mode << VCNL4200_PS_MPS_SHIFT);
> +};
> +
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.prod = "VCNL4000",

Re: [PATCH 5/5] iio: light: vcnl4000: add control of multi pulse

[Tomas Novotny]

Hi Jonathan,

On Sat, 8 Feb 2020 15:17:10 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed,  5 Feb 2020 11:16:55 +0100
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > Multi pulse settings allow to emit more pulses during one measurement
> > (up to 8 on vcnl4040 and vcnl4200). The raw reading is approximately
> > multiplied by the multi pulse settings. More information is available in
> > the added documentation.
> > 
> > Multi pulse is specific only for proximity sensor. Only the vcnl4040 and
> > vcnl4200 hardware supports this settings.  
> 
> A few comments but this one is more or less good.
> 
> Thanks,
> 
> Jonathan
> 
> > 
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 | 21 +++++++++
> >  drivers/iio/light/vcnl4000.c                     | 60 ++++++++++++++++++++++++
> >  2 files changed, 81 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000 b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > index 4860f409dbf0..923a78dc9869 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-vcnl4000
> > @@ -16,3 +16,24 @@ KernelVersion:	5.7
> >  Contact:	linux-iio@vger.kernel.org
> >  Description:
> >  		The on/off available duty ratios.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_multi_pulse  
> 
> This is less ambiguous than duty_cycle, but perhaps something 
> in_proximity_led_pulse_count is more specific?

ok.

> > +Date:		February 2020
> > +KernelVersion:	5.7
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Instead of one single pulse per every measurement, more pulses
> > +		may be programmed. This leads to a longer led current on-time
> > +		for each proximity measurement, which also results in a higher
> > +		detection range. The raw reading is approximately multiplied by
> > +		the multi pulse settings. The duty ration is not changed. The  

oh, typo.

> Hmm. Normal meaning of duty ratio would be changed by this...  It's uneven but
> multiple pulses == more on time hence lower duty ratio.
>
> > +		settings cannot be changed when the proximity channel is
> > +		enabled.  Valid values are in the respective '_available'
> > +		attribute.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_proximity_multi_pulse_available
> > +Date:		February 2020
> > +KernelVersion:	5.7
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		List of multi pulse values.
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index a8c2ce1056a6..cc75e5e7e634 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -46,6 +46,7 @@
> >  
> >  #define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
> >  #define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
> > +#define VCNL4200_PS_CONF3	0x04 /* Proximity conf., white channel, LED I */
> >  #define VCNL4200_PS_DATA	0x08 /* Proximity data */
> >  #define VCNL4200_AL_DATA	0x09 /* Ambient light data */
> >  #define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
> > @@ -65,6 +66,8 @@
> >  #define VCNL4200_PS_DUTY_MASK	GENMASK(7, 6) /* Proximity duty ratio */
> >  #define VCNL4200_PS_DUTY_SHIFT	6
> >  #define VCNL4200_PS_SD		BIT(0) /* Proximity shutdown */
> > +#define VCNL4200_PS_MPS_MASK	GENMASK(6, 5)
> > +#define VCNL4200_PS_MPS_SHIFT	5  
> 
> You could probably use the FIELD_PREP  macros etc to avoid having both mask and
> shift defines.

As Marco suggested, I will use reg_field.

Thanks a lot for the whole review.

Tomas

> >  enum vcnl4000_device_ids {
> >  	VCNL4000,
> > @@ -223,11 +226,24 @@ static const char * const vcnl4200_ps_duty_ratio_items[] = {
> >  	"1/1280"
> >  };
> >  
> > +/* Values are directly mapped to register values. */
> > +static const char * const vcnl4200_ps_multi_pulse_items[] = {
> > +	"1",
> > +	"2",
> > +	"4",
> > +	"8"
> > +};
> > +
> >  static int vcnl4200_get_ps_duty_ratio(struct iio_dev *indio_dev,
> >  				      const struct iio_chan_spec *chan);
> >  static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> >  				      const struct iio_chan_spec *chan,
> >  				      unsigned int mode);
> > +static int vcnl4200_get_ps_multi_pulse(struct iio_dev *indio_dev,
> > +				       const struct iio_chan_spec *chan);
> > +static int vcnl4200_set_ps_multi_pulse(struct iio_dev *indio_dev,
> > +				       const struct iio_chan_spec *chan,
> > +				       unsigned int mode);
> >  
> >  static const struct iio_enum vcnl4040_ps_duty_ratio_enum = {
> >  	.items = vcnl4040_ps_duty_ratio_items,
> > @@ -243,15 +259,26 @@ static const struct iio_enum vcnl4200_ps_duty_ratio_enum = {
> >  	.set = vcnl4200_set_ps_duty_ratio,
> >  };
> >  
> > +static const struct iio_enum vcnl4200_ps_multi_pulse_enum = {
> > +	.items = vcnl4200_ps_multi_pulse_items,
> > +	.num_items = ARRAY_SIZE(vcnl4200_ps_multi_pulse_items),
> > +	.get = vcnl4200_get_ps_multi_pulse,
> > +	.set = vcnl4200_set_ps_multi_pulse,
> > +};
> > +
> >  static const struct iio_chan_spec_ext_info vcnl4040_ps_ext_info[] = {
> >  	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4040_ps_duty_ratio_enum),
> >  	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4040_ps_duty_ratio_enum),
> > +	IIO_ENUM("multi_pulse", IIO_SEPARATE, &vcnl4200_ps_multi_pulse_enum),
> > +	IIO_ENUM_AVAILABLE("multi_pulse", &vcnl4200_ps_multi_pulse_enum),
> >  	{ },
> >  };
> >  
> >  static const struct iio_chan_spec_ext_info vcnl4200_ps_ext_info[] = {
> >  	IIO_ENUM("duty_ratio", IIO_SEPARATE, &vcnl4200_ps_duty_ratio_enum),
> >  	IIO_ENUM_AVAILABLE("duty_ratio", &vcnl4200_ps_duty_ratio_enum),
> > +	IIO_ENUM("multi_pulse", IIO_SEPARATE, &vcnl4200_ps_multi_pulse_enum),
> > +	IIO_ENUM_AVAILABLE("multi_pulse", &vcnl4200_ps_multi_pulse_enum),
> >  	{ },
> >  };
> >  
> > @@ -638,6 +665,39 @@ static int vcnl4200_set_ps_duty_ratio(struct iio_dev *indio_dev,
> >  	return vcnl4200_set_samp_rate(data, IIO_PROXIMITY);
> >  };
> >  
> > +static int vcnl4200_get_ps_multi_pulse(struct iio_dev *indio_dev,
> > +				       const struct iio_chan_spec *chan)
> > +{
> > +	int ret;
> > +	unsigned int val;
> > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > +	ret = regmap_read(data->regmap, VCNL4200_PS_CONF3, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	val &= VCNL4200_PS_MPS_MASK;
> > +	val >>= VCNL4200_PS_MPS_SHIFT;
> > +
> > +	return val;
> > +};
> > +
> > +static int vcnl4200_set_ps_multi_pulse(struct iio_dev *indio_dev,
> > +				       const struct iio_chan_spec *chan,
> > +				       unsigned int mode)
> > +{
> > +	int ret;
> > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > +	ret = vcnl4200_check_enabled(data, IIO_PROXIMITY);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return regmap_update_bits(data->regmap, VCNL4200_PS_CONF3,
> > +				  VCNL4200_PS_MPS_MASK,
> > +				  mode << VCNL4200_PS_MPS_SHIFT);
> > +};
> > +
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.prod = "VCNL4000",  
>