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
Mostly standard i2c driver with some additional led-current option
for vcnl3020.
Signed-off-by: Ivan Mikhaylov <[email protected]>
---
.../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 <[email protected]>
+
+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
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 <[email protected]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("Vishay VCNL3020 proximity sensor driver");
+MODULE_LICENSE("GPL");
--
2.21.1
On Mon, Mar 23, 2020 at 12:41 PM Ivan Mikhaylov <[email protected]> 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 <[email protected]>
...
> 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
On Mon, 2020-03-23 at 14:10 +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 12:41 PM Ivan Mikhaylov <[email protected]> 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 <[email protected]>
>
> ...
>
> > 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.
On Tue, Mar 24, 2020 at 1:52 PM Ivan Mikhaylov <[email protected]> wrote:
> On Mon, 2020-03-23 at 14:10 +0200, Andy Shevchenko wrote:
> > On Mon, Mar 23, 2020 at 12:41 PM Ivan Mikhaylov <[email protected]> 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