[PATCH 0/2] iio: proximity: driver for vcnl3020

[PATCH 0/2] iio: proximity: driver for vcnl3020

[Ivan Mikhaylov]

Add proximity sensor driver for Vishay vcnl3020. Only on-demand
measurement is supported for now.

Ivan Mikhaylov (2):
  iio: proximity: provide device tree binding document
  iio: proximity: Add driver support for vcnl3020 proximity sensor

 .../bindings/iio/proximity/vcnl3020.yaml      |  47 ++++
 drivers/iio/proximity/Kconfig                 |  10 +
 drivers/iio/proximity/Makefile                |   1 +
 drivers/iio/proximity/vcnl3020.c              | 233 ++++++++++++++++++
 4 files changed, 291 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/vcnl3020.yaml
 create mode 100644 drivers/iio/proximity/vcnl3020.c

-- 
2.21.1

[PATCH 1/2] iio: proximity: provide device tree binding document

[Ivan Mikhaylov]

Mostly standard i2c driver with some additional led-current option
for vcnl3020.

Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
---
 .../bindings/iio/proximity/vcnl3020.yaml      | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/vcnl3020.yaml

diff --git a/Documentation/devicetree/bindings/iio/proximity/vcnl3020.yaml b/Documentation/devicetree/bindings/iio/proximity/vcnl3020.yaml
new file mode 100644
index 000000000000..76c29514c663
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/vcnl3020.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/proximity/vcnl3020.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Integrated Proximity Sensor With Infrared Emitter
+
+maintainers:
+  - Ivan Mikhaylov <i.mikhaylov@yadro.com>
+
+description: |
+  The VCNL3020 is a fully integrated proximity sensor. Fully integrated means
+  that the infrared emitter is included in the package. It has 16-bit
+  resolution. It includes a signal processing IC and features standard I2C
+  communication interface. It features an interrupt function.
+
+  Specifications about the devices can be found at:
+  https://www.vishay.com/docs/84150/vcnl3020.pdf
+
+properties:
+  compatible:
+    enum:
+      - vishay,vcnl3020
+
+  reg:
+    maxItems: 1
+
+  led-current:
+    description:
+        IR LED current value with valid Range = 0 to 20d. e.g. 0 = 0 mA,
+        1 = 10 mA, 20 = 200 mA (2 = 20 mA = DEFAULT). LED Current is
+        limited to 200 mA for values higher than decimal 20.
+
+required:
+  - compatible
+  - reg
+  - led-current
+
+examples:
+  - |
+    iio-proximity@13 {
+       compatible = "vishay,vcnl3020";
+       reg = <0x13>;
+
+       led-current = <0x14>;
+    };
-- 
2.21.1

[PATCH 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor

[Ivan Mikhaylov]

Proximity sensor driver based on light/vcnl4000.c code.
For now supports only the single on-demand measurement.

The VCNL3020 is a fully integrated proximity sensor. Fully
integrated means that the infrared emitter is included in the
package. It has 16-bit resolution. It includes a signal
processing IC and features standard I2C communication
interface. It features an interrupt function.

Datasheet available at:
http://www.vishay.com/docs/84150/vcnl3020.pdf

Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
---
 drivers/iio/proximity/Kconfig    |  10 ++
 drivers/iio/proximity/Makefile   |   1 +
 drivers/iio/proximity/vcnl3020.c | 233 +++++++++++++++++++++++++++++++
 3 files changed, 244 insertions(+)
 create mode 100644 drivers/iio/proximity/vcnl3020.c

diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index d53601447da4..936cd181916a 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -123,4 +123,14 @@ config VL53L0X_I2C
 	  To compile this driver as a module, choose M here: the
 	  module will be called vl53l0x-i2c.
 
+config VCNL3020
+	tristate "VCNL3020 proximity sensor"
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Vishay VCNL3020
+	  proximity sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called vcnl3020.
+
 endmenu
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index 0bb5f9de13d6..96b6330e90ff 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -13,4 +13,5 @@ obj-$(CONFIG_SRF04)		+= srf04.o
 obj-$(CONFIG_SRF08)		+= srf08.o
 obj-$(CONFIG_SX9500)		+= sx9500.o
 obj-$(CONFIG_VL53L0X_I2C)	+= vl53l0x-i2c.o
+obj-$(CONFIG_VCNL3020)		+= vcnl3020.o
 
diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c
new file mode 100644
index 000000000000..f6d9a21eed0b
--- /dev/null
+++ b/drivers/iio/proximity/vcnl3020.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * vcnl3020.c - Support for Vishay VCNL3020 proximity sensor
+ *
+ * based on vcnl4000.c
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define VCNL_DRV_NAME		"vcnl3020"
+#define VCNL3020_PROD_ID	0x21
+
+#define VCNL_COMMAND		0x80 /* Command register */
+#define VCNL_PROD_REV		0x81 /* Product ID and Revision ID */
+#define VCNL_PROXIMITY_RATE	0x82 /* Rate of Proximity Measurement */
+#define VCNL_LED_CURRENT	0x83 /* IR LED current for proximity mode */
+#define VCNL_PS_RESULT_HI	0x87 /* Proximity result register, MSB */
+#define VCNL_PS_RESULT_LO	0x88 /* Proximity result register, LSB */
+#define VCNL_PS_ICR		0x89 /* Interrupt Control Register  */
+
+#define VCNL_PS_LO_THR_HI	0x8a /* High byte of low threshold value */
+#define VCNL_PS_LO_THR_LO	0x8b /* Low byte of low threshold value */
+#define VCNL_PS_HI_THR_HI	0x8c /* High byte of high threshold value */
+#define VCNL_PS_HI_THR_LO	0x8d /* Low byte of high threshold value */
+#define VCNL_ISR		0x8e /* Interrupt Status Register */
+#define VCNL_PS_MOD_ADJ		0x8f /* Proximity Modulator Timing Adjustment */
+
+/* Bit masks for COMMAND register */
+#define VCNL_PS_RDY		BIT(5) /* proximity data ready? */
+#define VCNL_PS_OD		BIT(3) /* start on-demand proximity
+					* measurement
+					*/
+
+struct vcnl3020_data {
+	struct i2c_client *client;
+	u32 rev;
+	struct mutex vcnl3020_lock; /* for i2c operations */
+};
+
+static const struct i2c_device_id vcnl3020_id[] = {
+	{ "vcnl3020", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, vcnl3020_id);
+
+static int32_t vcnl3020_init(struct vcnl3020_data *data)
+{
+	s32 rc;
+	u32 led_current;
+	struct device *dev = &data->client->dev;
+
+	rc = device_property_read_u32(dev, "led-current", &led_current);
+	if (rc) {
+		dev_err(dev, "Couldn't get led current (%d)", rc);
+		goto exit;
+	}
+
+	rc = i2c_smbus_read_byte_data(data->client, VCNL_PROD_REV);
+	if (rc < 0) {
+		dev_err(dev, "Error (%d) reading product revision", rc);
+		goto exit;
+	}
+
+	if (rc == VCNL3020_PROD_ID) {
+		data->rev = rc & 0xff;
+		mutex_init(&data->vcnl3020_lock);
+	} else {
+		dev_err(dev, "Product id (%x) did not match vcnl3020 (%x)", rc,
+			VCNL3020_PROD_ID);
+		rc = -ENODEV;
+		goto exit;
+	}
+
+	/* set led current */
+	rc = i2c_smbus_write_byte_data(data->client, VCNL_LED_CURRENT,
+				       led_current);
+	if (rc < 0) {
+		dev_err(dev, "Error (%d) setting LED current", rc);
+		goto exit;
+	}
+
+	return 0;
+exit:
+	return rc;
+};
+
+static int32_t vcnl3020_measure_proximity(struct vcnl3020_data *data,
+					  int32_t *val)
+{
+	s32 tries = 20;
+	s32 rc;
+
+	mutex_lock(&data->vcnl3020_lock);
+
+	rc = i2c_smbus_write_byte_data(data->client, VCNL_COMMAND,
+				       VCNL_PS_OD);
+	if (rc < 0)
+		goto fail;
+
+	/* wait for data to become ready */
+	while (tries--) {
+		rc = i2c_smbus_read_byte_data(data->client, VCNL_COMMAND);
+		if (rc < 0)
+			goto fail;
+		if (rc & VCNL_PS_RDY)
+			break;
+		msleep(20); /* measurement takes up to 100 ms */
+	}
+
+	if (tries < 0) {
+		dev_err(&data->client->dev,
+			"vcnl3020_measure() failed, data is not ready");
+		rc = -EIO;
+		goto fail;
+	}
+
+	rc = i2c_smbus_read_byte_data(data->client, VCNL_PS_RESULT_HI);
+	if (rc < 0)
+		goto fail;
+	*val = (rc & 0xff) << 8;
+	dev_dbg(&data->client->dev, "result high byte 0x%x", rc);
+
+	rc = i2c_smbus_read_byte_data(data->client, VCNL_PS_RESULT_LO);
+	if (rc < 0)
+		goto fail;
+	*val |= rc & 0xff;
+	dev_dbg(&data->client->dev, "result low byte 0x%x", rc);
+
+fail:
+	mutex_unlock(&data->vcnl3020_lock);
+
+	return rc;
+}
+
+static const struct iio_chan_spec vcnl3020_channels[] = {
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}
+};
+
+static int32_t vcnl3020_read_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 s32 *val, s32 *val2, long mask)
+{
+	s32 rc;
+	struct vcnl3020_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			rc = vcnl3020_measure_proximity(data, val);
+			if (rc < 0)
+				return rc;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info vcnl3020_info = {
+	.read_raw = vcnl3020_read_raw,
+};
+
+static int32_t vcnl3020_probe(struct i2c_client *client,
+			      const struct i2c_device_id *id)
+{
+	struct vcnl3020_data *data;
+	struct iio_dev *indio_dev;
+	s32 rc;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	rc = vcnl3020_init(data);
+	if (rc < 0)
+		goto out;
+
+	dev_info(&client->dev, "Proximity sensor, Rev: %02x\n",
+		 data->rev);
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &vcnl3020_info;
+	indio_dev->channels = vcnl3020_channels;
+	indio_dev->num_channels = ARRAY_SIZE(vcnl3020_channels);
+	indio_dev->name = VCNL_DRV_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	rc = devm_iio_device_register(&client->dev, indio_dev);
+	if (rc != 0)
+		goto out;
+
+	return rc;
+out:
+	devm_iio_device_free(&client->dev, indio_dev);
+	return rc;
+}
+
+static const struct of_device_id vcnl3020_of_match[] = {
+	{
+		.compatible = "vishay,vcnl3020",
+	},
+};
+MODULE_DEVICE_TABLE(of, vcnl3020_of_match);
+
+static struct i2c_driver vcnl3020_driver = {
+	.driver = {
+		.name   = VCNL_DRV_NAME,
+		.of_match_table = vcnl3020_of_match,
+	},
+	.probe  = vcnl3020_probe,
+	.id_table = vcnl3020_id,
+};
+module_i2c_driver(vcnl3020_driver);
+
+MODULE_AUTHOR("Ivan Mikhaylov <i.mikhaylov@yadro.com>");
+MODULE_DESCRIPTION("Vishay VCNL3020 proximity sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.21.1

Re: [PATCH 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor

[Andy Shevchenko]

On Mon, Mar 23, 2020 at 12:41 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
>
> Proximity sensor driver based on light/vcnl4000.c code.
> For now supports only the single on-demand measurement.
>
> The VCNL3020 is a fully integrated proximity sensor. Fully
> integrated means that the infrared emitter is included in the
> package. It has 16-bit resolution. It includes a signal
> processing IC and features standard I2C communication
> interface. It features an interrupt function.

Thank you for a patch, my comments below.

> Datasheet available at:
> http://www.vishay.com/docs/84150/vcnl3020.pdf

I'm thinking that we may simple introduce new tag, called Datesheet:
to put such links.

> Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>

...

>  obj-$(CONFIG_SRF08)            += srf08.o
>  obj-$(CONFIG_SX9500)           += sx9500.o
>  obj-$(CONFIG_VL53L0X_I2C)      += vl53l0x-i2c.o
> +obj-$(CONFIG_VCNL3020)         += vcnl3020.o

Perhaps keep ordered?

...

> +/*
> + * vcnl3020.c - Support for Vishay VCNL3020 proximity sensor

Using file names in themselves is a bad idea. Whenever you would
rename file (for instance, to support new sensors from the same family
in the future) you will forget (often, I see this in practice!) to
update this line.
Just drop it from here and try to avoid in the future.

> + *
> + * based on vcnl4000.c

This sounds like a continuation of previous sentence. Drop line in
between and use proper English grammar and punctuation.

> + */

...

> +struct vcnl3020_data {
> +       struct i2c_client *client;
> +       u32 rev;

> +       struct mutex vcnl3020_lock; /* for i2c operations */

Simple 'lock' is enough, the rest is dup noise.
Also, consider kernel doc format instead of odd comments.

> +};

...

> +static const struct i2c_device_id vcnl3020_id[] = {
> +       { "vcnl3020", 0 },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(i2c, vcnl3020_id);

Can you group this with OF table below?

...

> +static int32_t vcnl3020_init(struct vcnl3020_data *data)

int32_t...

> +{

> +       s32 rc;

...s32?!

Applies to entire code.

> +       u32 led_current;
> +       struct device *dev = &data->client->dev;

Reversed xmas tree order looks better.

> +       rc = i2c_smbus_read_byte_data(data->client, VCNL_PROD_REV);

Can you use regmap I²C API?

> +       if (rc < 0) {
> +               dev_err(dev, "Error (%d) reading product revision", rc);
> +               goto exit;
> +       }
> +

> +       if (rc == VCNL3020_PROD_ID) {
> +               data->rev = rc & 0xff;

This conjunction looks strange. Also, why type of rev is u32 instead of u8?

> +               mutex_init(&data->vcnl3020_lock);
> +       } else {
> +               dev_err(dev, "Product id (%x) did not match vcnl3020 (%x)", rc,
> +                       VCNL3020_PROD_ID);
> +               rc = -ENODEV;
> +               goto exit;
> +       }
> +
> +       /* set led current */
> +       rc = i2c_smbus_write_byte_data(data->client, VCNL_LED_CURRENT,
> +                                      led_current);
> +       if (rc < 0) {
> +               dev_err(dev, "Error (%d) setting LED current", rc);
> +               goto exit;
> +       }
> +
> +       return 0;

> +exit:
> +       return rc;

Useless. Return directly.

> +};

...

> +       /* wait for data to become ready */
> +       while (tries--) {
> +               rc = i2c_smbus_read_byte_data(data->client, VCNL_COMMAND);
> +               if (rc < 0)
> +                       goto fail;
> +               if (rc & VCNL_PS_RDY)
> +                       break;
> +               msleep(20); /* measurement takes up to 100 ms */
> +       }

Timeout loops look more naturally in do {} while format.

  unsigned int tries = 5;
  ...

  do {
  ...
  } while (--tries);

...

> +       *val = (rc & 0xff) << 8;

> +       *val |= rc & 0xff;

All these conjunctions looks fishy. Why do you need them? Cant you
rely on the returned value by I²C API?

...

> +fail:

Better name is 'err_unlock' or 'out_unlock'. The rule of thumb to
describe in the label what you *about to do* there.

> +       mutex_unlock(&data->vcnl3020_lock);
> +
> +       return rc;
> +}

...

> +                       rc = vcnl3020_measure_proximity(data, val);
> +                       if (rc < 0)

Can rc be positive? Drop all these ' < 0' in cases where it is
guaranteed not to be the case.

> +                               return rc;

...

> +static int32_t vcnl3020_probe(struct i2c_client *client,
> +                             const struct i2c_device_id *id)

Can you switch to ->probe_new() ?

...

> +       dev_info(&client->dev, "Proximity sensor, Rev: %02x\n",
> +                data->rev);

Noise.

...

> +       rc = devm_iio_device_register(&client->dev, indio_dev);
> +       if (rc != 0)

Redundant ' != 0' part.

> +               goto out;
> +
> +       return rc;

> +out:
> +       devm_iio_device_free(&client->dev, indio_dev);
> +       return rc;

Managed resources are exactly for this not to be appeared in the code.

> +}

...

> +static const struct of_device_id vcnl3020_of_match[] = {
> +       {
> +               .compatible = "vishay,vcnl3020",
> +       },

Missed terminator. How did you test this?

> +};

--
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor

[Ivan Mikhaylov]

On Mon, 2020-03-23 at 14:10 +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 12:41 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
> > Proximity sensor driver based on light/vcnl4000.c code.
> > For now supports only the single on-demand measurement.
> > 
> > The VCNL3020 is a fully integrated proximity sensor. Fully
> > integrated means that the infrared emitter is included in the
> > package. It has 16-bit resolution. It includes a signal
> > processing IC and features standard I2C communication
> > interface. It features an interrupt function.
> 
> Thank you for a patch, my comments below.
> 
> > Datasheet available at:
> > http://www.vishay.com/docs/84150/vcnl3020.pdf
> 
> I'm thinking that we may simple introduce new tag, called Datesheet:
> to put such links.
> 
> > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
> 
> ...
> 
> >  obj-$(CONFIG_SRF08)            += srf08.o
> >  obj-$(CONFIG_SX9500)           += sx9500.o
> >  obj-$(CONFIG_VL53L0X_I2C)      += vl53l0x-i2c.o
> > +obj-$(CONFIG_VCNL3020)         += vcnl3020.o
> 
> Perhaps keep ordered?

Oops.

> 
> ...
> 
> > +static int32_t vcnl3020_init(struct vcnl3020_data *data)
> 
> int32_t...
> 
> > +{
> > +       s32 rc;
> 
> ...s32?!
> 
> Applies to entire code.

checkpatch.pl --strict doesn't show anything bad in it but I can change from
int32_t/s32 into int easily, it's not a problem for me.

> 
> > +       u32 led_current;
> > +       struct device *dev = &data->client->dev;
> 
> Reversed xmas tree order looks better.
> 
> > +       rc = i2c_smbus_read_byte_data(data->client, VCNL_PROD_REV);
> 
> Can you use regmap I²C API?
> 

That's a nice idea.

> ...
> 
> > +       dev_info(&client->dev, "Proximity sensor, Rev: %02x\n",
> > +                data->rev);
> 
> Noise.

Doesn't it help to determine the presence of driver to a common user?

> 
> > +               goto out;
> > +
> > +       return rc;
> > +out:
> > +       devm_iio_device_free(&client->dev, indio_dev);
> > +       return rc;
> 
> Managed resources are exactly for this not to be appeared in the code.

I can do something like this:
return devm_iio_device_register(&client->dev, indio_dev);

Would it suffice?

> 
> > +}
> 
> ...
> 
> > +static const struct of_device_id vcnl3020_of_match[] = {
> > +       {
> > +               .compatible = "vishay,vcnl3020",
> > +       },
> 
> Missed terminator. How did you test this?

All works fine with real hw, I'll add terminator.
Agree with everything else.

Thanks. 

Re: [PATCH 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor

[Andy Shevchenko]

On Tue, Mar 24, 2020 at 1:52 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
> On Mon, 2020-03-23 at 14:10 +0200, Andy Shevchenko wrote:
> > On Mon, Mar 23, 2020 at 12:41 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:

...

> > > +static int32_t vcnl3020_init(struct vcnl3020_data *data)
> >
> > int32_t...
> >
> > > +{
> > > +       s32 rc;
> >
> > ...s32?!
> >
> > Applies to entire code.
>
> checkpatch.pl --strict doesn't show anything bad in it but I can change from
> int32_t/s32 into int easily, it's not a problem for me.

The problem is consistency. You code should be self-consistent.
And yes, s32 is quite unusual for (generic) returning variables.

...

> > > +       dev_info(&client->dev, "Proximity sensor, Rev: %02x\n",
> > > +                data->rev);
> >
> > Noise.
>
> Doesn't it help to determine the presence of driver to a common user?

You have a lot of knobs to detect this. Starting from sysfs to 'initcall_debug'.

...

> > > +               goto out;
> > > +
> > > +       return rc;
> > > +out:
> > > +       devm_iio_device_free(&client->dev, indio_dev);
> > > +       return rc;
> >
> > Managed resources are exactly for this not to be appeared in the code.
>
> I can do something like this:
> return devm_iio_device_register(&client->dev, indio_dev);
>
> Would it suffice?

I think so, but you may double check with documentation.

-- 
With Best Regards,
Andy Shevchenko