Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 26 Mar 2020 22:39:13 +0200 | Subject | Re: [PATCH v3 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor |
| |
On Thu, Mar 26, 2020 at 5:51 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 (only minors) comments below. After addressing them,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
(Don't forget to fix DTS schema as Rob's bot pointed out)
...
> +#define VCNL_ON_DEMAND_TIMEOUT 100000 /* on-demand measurement timeout in us */
This also should have _US suffix, and thus comment is not necessary.
> +#define VCNL_POLL_US 20000
...
> + if (reg == VCNL3020_PROD_ID) { > + data->rev = reg; > + mutex_init(&data->lock); > + } else { > + dev_err(data->dev, > + "Product id (%x) did not match vcnl3020 (%x)", reg, > + VCNL3020_PROD_ID); > + return -ENODEV; > + }
I would recommend to use standard pattern, i.e. if (reg != ...) { ...error handling... }
in this case else branch becomes a mainstream code in the function.
...
> + rc = regmap_write(data->regmap, VCNL_LED_CURRENT, led_current); > + if (rc) { > + dev_err(data->dev, "Error (%d) setting LED current", rc); > + }
Now curly braces are redundant.
> + return rc;
...
> + rc = regmap_read(data->regmap, VCNL_PS_RESULT_HI, ®); > + if (rc) > + goto err_unlock;
> + *val = reg << 8;
This actually breaks the principle of not touching output buffers / variables in case of error.
Perhaps
unsigned int hi, lo;
instead of / along with reg?
> + dev_dbg(data->dev, "result high byte 0x%x", rc); > + > + rc = regmap_read(data->regmap, VCNL_PS_RESULT_LO, ®); > + if (rc) > + goto err_unlock; > + *val |= reg; > + dev_dbg(data->dev, "result low byte 0x%x", rc);
-- With Best Regards, Andy Shevchenko
| |