| Message ID | 20200401162416.24474-3-i.mikhaylov@yadro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: proximity: driver for vcnl3020 | expand |
On Wed, 2020-04-01 at 19:35 +0300, Andy Shevchenko wrote: > On Wed, Apr 1, 2020 at 7:24 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 an update, my comments below. > > ... > > > +static int get_and_apply_property(struct vcnl3020_data *data, const char > > *prop, > > + u32 reg) > > +{ > > + int rc; > > + u32 val; > > + > > + rc = device_property_read_u32(data->dev, prop, &val); > > + if (rc) > > + return 0; > > + > > + rc = regmap_write(data->regmap, reg, val); > > + if (rc) > > + dev_err(data->dev, "Error (%d) setting property (%s)", > > + rc, prop); > > This requires {} according to the coding style checkpatch.pl doesn't say anything bad on this spot. Do you mean to make something like this? rc = regmap_write(data->regmap, reg, val); if (rc) { dev_err(data->dev, "Error (%d) setting property (%s)", rc, prop); } In previous notes you said to remove them. > . > > > + return rc; > > +} > > ... > > > +static int vcnl3020_probe(struct i2c_client *client) > > +{ > > + indio_dev->name = VCNL_DRV_NAME; > > It's definitely not a driver name. You have to put part number here. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/tsl4531.c?h=v5.6#n199 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/max44009.c?h=v5.6#n507 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/vl6180.c?h=v5.6#n515
On Thu, Apr 2, 2020 at 11:24 AM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > > On Wed, 2020-04-01 at 19:35 +0300, Andy Shevchenko wrote: > > On Wed, Apr 1, 2020 at 7:24 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 an update, my comments below. > > > > ... > > > > > +static int get_and_apply_property(struct vcnl3020_data *data, const char > > > *prop, > > > + u32 reg) > > > +{ > > > + int rc; > > > + u32 val; > > > + > > > + rc = device_property_read_u32(data->dev, prop, &val); > > > + if (rc) > > > + return 0; > > > + > > > + rc = regmap_write(data->regmap, reg, val); > > > + if (rc) > > > + dev_err(data->dev, "Error (%d) setting property (%s)", > > > + rc, prop); > > > > This requires {} according to the coding style > > checkpatch.pl doesn't say anything bad on this spot. Do you mean to make > something like this? > > rc = regmap_write(data->regmap, reg, val); > if (rc) { > dev_err(data->dev, "Error (%d) setting property (%s)", > rc, prop); > } Yes. Checkpatch is neither strict nor fully comprehensive tool. > In previous notes you said to remove them. When it's one line, it fine, otherwise you need {} surround. https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces > > > + return rc; > > > +} > > > > ... > > > > > +static int vcnl3020_probe(struct i2c_client *client) > > > +{ > > > + indio_dev->name = VCNL_DRV_NAME; > > > > It's definitely not a driver name. You have to put part number here. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/tsl4531.c?h=v5.6#n199 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/max44009.c?h=v5.6#n507 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/vl6180.c?h=v5.6#n515 Let's Jonathan speak up.
On Thu, 2 Apr 2020 15:42:02 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Apr 2, 2020 at 11:24 AM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > > > > On Wed, 2020-04-01 at 19:35 +0300, Andy Shevchenko wrote: > > > On Wed, Apr 1, 2020 at 7:24 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 an update, my comments below. > > > > > > ... > > > > > > > +static int get_and_apply_property(struct vcnl3020_data *data, const char > > > > *prop, > > > > + u32 reg) > > > > +{ > > > > + int rc; > > > > + u32 val; > > > > + > > > > + rc = device_property_read_u32(data->dev, prop, &val); > > > > + if (rc) > > > > + return 0; > > > > + > > > > + rc = regmap_write(data->regmap, reg, val); > > > > + if (rc) > > > > + dev_err(data->dev, "Error (%d) setting property (%s)", > > > > + rc, prop); > > > > > > This requires {} according to the coding style > > > > checkpatch.pl doesn't say anything bad on this spot. Do you mean to make > > something like this? > > > > rc = regmap_write(data->regmap, reg, val); > > if (rc) { > > dev_err(data->dev, "Error (%d) setting property (%s)", > > rc, prop); > > } > > Yes. Checkpatch is neither strict nor fully comprehensive tool. > > > In previous notes you said to remove them. > > When it's one line, it fine, otherwise you need {} surround. > > https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces > > > > > + return rc; > > > > +} > > > > > > ... > > > > > > > +static int vcnl3020_probe(struct i2c_client *client) > > > > +{ > > > > + indio_dev->name = VCNL_DRV_NAME; > > > > > > It's definitely not a driver name. You have to put part number here. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/tsl4531.c?h=v5.6#n199 That one is actually fine (if not very pretty) because the driver only supports one part and it happens to also be the name of the driver. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/max44009.c?h=v5.6#n507 Also only one part supported, so fine if liable to accidentally get broken if we support more parts. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/vl6180.c?h=v5.6#n515 Again, one part supported and the driver has the same name. > > Let's Jonathan speak up. So, the real point here is not the value being assigned but the fact it's explicitly linked to the name of the driver. I'd argue that you could use VCNL_NAME as the define and that link is clearly broken. Or just put the string inline in both places and don't worry about the tiny bit of replication! Jonathan >
On Sun, Apr 5, 2020 at 1:13 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Thu, 2 Apr 2020 15:42:02 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Apr 2, 2020 at 11:24 AM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > > > On Wed, 2020-04-01 at 19:35 +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 1, 2020 at 7:24 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > > > > > + indio_dev->name = VCNL_DRV_NAME; > > > > > > > > It's definitely not a driver name. You have to put part number here. > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/tsl4531.c?h=v5.6#n199 > That one is actually fine (if not very pretty) because the driver only supports one part and it happens > to also be the name of the driver. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/max44009.c?h=v5.6#n507 > Also only one part supported, so fine if liable to accidentally get broken if we support more parts. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/light/vl6180.c?h=v5.6#n515 > > Again, one part supported and the driver has the same name. > > > > > Let's Jonathan speak up. > > So, the real point here is not the value being assigned but the fact it's > explicitly linked to the name of the driver. I'd argue that you could use > VCNL_NAME as the define and that link is clearly broken. Or just put the string > inline in both places and don't worry about the tiny bit of replication! My comments, except this one, to this version are quite minor, so, after addressing this in a way Jonathan likes, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> P.S. Don't forget to include given tags in the commit message of next (fixed) version.
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig index d53601447da4..b8d2b17e60ac 100644 --- a/drivers/iio/proximity/Kconfig +++ b/drivers/iio/proximity/Kconfig @@ -112,6 +112,17 @@ config SRF08 To compile this driver as a module, choose M here: the module will be called srf08. +config VCNL3020 + tristate "VCNL3020 proximity sensor" + select REGMAP_I2C + 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. + config VL53L0X_I2C tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)" depends on I2C diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile index 0bb5f9de13d6..8245978ced30 100644 --- a/drivers/iio/proximity/Makefile +++ b/drivers/iio/proximity/Makefile @@ -12,5 +12,6 @@ obj-$(CONFIG_RFD77402) += rfd77402.o obj-$(CONFIG_SRF04) += srf04.o obj-$(CONFIG_SRF08) += srf08.o obj-$(CONFIG_SX9500) += sx9500.o +obj-$(CONFIG_VCNL3020) += vcnl3020.o obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c new file mode 100644 index 000000000000..86f4a926268b --- /dev/null +++ b/drivers/iio/proximity/vcnl3020.c @@ -0,0 +1,234 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Support for Vishay VCNL3020 proximity sensor on i2c bus. + * Based on Vishay VCNL4000 driver code. + * + * TODO: interrupts. + */ + +#include <linux/module.h> +#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> + +#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 + */ + +#define VCNL_ON_DEMAND_TIMEOUT_US 100000 +#define VCNL_POLL_US 20000 + +/** + * struct vcnl3020_data - vcnl3020 specific data. + * @regmap: device register map. + * @dev: vcnl3020 device. + * @rev: revision id. + * @lock: lock for protecting access to device hardware registers. + */ +struct vcnl3020_data { + struct regmap *regmap; + struct device *dev; + u8 rev; + struct mutex lock; +}; + +static int get_and_apply_property(struct vcnl3020_data *data, const char *prop, + u32 reg) +{ + int rc; + u32 val; + + rc = device_property_read_u32(data->dev, prop, &val); + if (rc) + return 0; + + rc = regmap_write(data->regmap, reg, val); + if (rc) + dev_err(data->dev, "Error (%d) setting property (%s)", + rc, prop); + + return rc; +} + +static int vcnl3020_init(struct vcnl3020_data *data) +{ + int rc; + unsigned int reg; + + rc = regmap_read(data->regmap, VCNL_PROD_REV, ®); + if (rc) { + dev_err(data->dev, + "Error (%d) reading product revision", rc); + return rc; + } + + if (reg != VCNL3020_PROD_ID) { + dev_err(data->dev, + "Product id (%x) did not match vcnl3020 (%x)", reg, + VCNL3020_PROD_ID); + return -ENODEV; + } + + data->rev = reg; + mutex_init(&data->lock); + + return get_and_apply_property(data, "vishay,led-current-milliamp", + VCNL_LED_CURRENT); +}; + +static int vcnl3020_measure_proximity(struct vcnl3020_data *data, int *val) +{ + int rc; + unsigned int reg; + __be16 res; + + mutex_lock(&data->lock); + + rc = regmap_write(data->regmap, VCNL_COMMAND, VCNL_PS_OD); + if (rc) + goto err_unlock; + + /* wait for data to become ready */ + rc = regmap_read_poll_timeout(data->regmap, VCNL_COMMAND, reg, + reg & VCNL_PS_RDY, VCNL_POLL_US, + VCNL_ON_DEMAND_TIMEOUT_US); + if (rc) { + dev_err(data->dev, + "Error (%d) reading vcnl3020 command register", rc); + goto err_unlock; + } + + /* high & low result bytes read */ + rc = regmap_bulk_read(data->regmap, VCNL_PS_RESULT_HI, &res, + sizeof(res)); + if (rc) + goto err_unlock; + + *val = be16_to_cpu(res); + +err_unlock: + mutex_unlock(&data->lock); + + return rc; +} + +static const struct iio_chan_spec vcnl3020_channels[] = { + { + .type = IIO_PROXIMITY, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + }, +}; + +static int vcnl3020_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + int 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) + 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 const struct regmap_config vcnl3020_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = VCNL_PS_MOD_ADJ, +}; + +static int vcnl3020_probe(struct i2c_client *client) +{ + struct vcnl3020_data *data; + struct iio_dev *indio_dev; + struct regmap *regmap; + int rc; + + regmap = devm_regmap_init_i2c(client, &vcnl3020_regmap_config); + if (IS_ERR(regmap)) { + dev_err(&client->dev, "regmap_init failed!"); + return PTR_ERR(regmap); + } + + 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->regmap = regmap; + data->dev = &client->dev; + + rc = vcnl3020_init(data); + if (rc) + return rc; + + 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; + + return devm_iio_device_register(&client->dev, indio_dev); +} + +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_new = vcnl3020_probe, +}; +module_i2c_driver(vcnl3020_driver); + +MODULE_AUTHOR("Ivan Mikhaylov <i.mikhaylov@yadro.com>"); +MODULE_DESCRIPTION("Vishay VCNL3020 proximity sensor driver"); +MODULE_LICENSE("GPL");
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: http://www.vishay.com/docs/84150/vcnl3020.pdf Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> --- drivers/iio/proximity/Kconfig | 11 ++ drivers/iio/proximity/Makefile | 1 + drivers/iio/proximity/vcnl3020.c | 234 +++++++++++++++++++++++++++++++ 3 files changed, 246 insertions(+) create mode 100644 drivers/iio/proximity/vcnl3020.c