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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHp75VfL726QwyR4eJL1HCVLvyQ5W0tJfbEJ+mhmsRWHkB8=6w@mail.gmail.com>
Date:   Thu, 26 Mar 2020 22:39:13 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Ivan Mikhaylov <i.mikhaylov@...ro.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>
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@...ro.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@...il.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, &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