On Thu, Mar 26, 2020 at 5:51 PM Ivan Mikhaylov <i.mikhaylov@xxxxxxxxx> 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@xxxxxxxxx>
(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
