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

[Andy Shevchenko]

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, &reg);
> +       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, &reg);
> +       if (rc)
> +               goto err_unlock;
> +       *val |= reg;
> +       dev_dbg(data->dev, "result low byte 0x%x", rc);

-- 
With Best Regards,
Andy Shevchenko