linux-kernel - Re: [PATCH v5 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VeLtPkb0e4uNP+1LGgtquBXkb5=bPi54O1U92uaO5jqvw@mail.gmail.com>
Date:   Mon, 30 Mar 2020 22:07:43 +0300
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 v5 2/2] iio: proximity: Add driver support for vcnl3020
 proximity sensor

On Mon, Mar 30, 2020 at 6:27 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.

Still my tag applies, but
I have few more comments below.

...

> +#define VCNL_DRV_NAME          "vcnl3020"
> +#define VCNL_REGMAP_NAME       "vcnl3020_regmap"

I'm wondering why you need the second one.

...

> +       rc = device_property_read_u32(data->dev, "vishay,led-current-milliamp",
> +                                     &led_current);

> +       if (rc == 0) {
> +               rc = regmap_write(data->regmap, VCNL_LED_CURRENT, led_current);
> +               if (rc)
> +                       dev_err(data->dev,
> +                               "Error (%d) setting LED current", rc);
> +       }
> +
> +       return rc;

Why not to use standard pattern, i.e.

  if (rc)
    return rc;
  ...
  rc = regmap_write(...);

?

...

> +       if (rc) {
> +               dev_err(data->dev,

> +                       "vcnl3020_measure() failed with error (%d)", rc);

Perhaps you keep same pattern for error messages as in previous function(s).

> +               goto err_unlock;
> +       }

> +       rc = regmap_bulk_read(data->regmap, VCNL_PS_RESULT_HI, &res, 2);

sizeof(res)

> +       if (rc)
> +               goto err_unlock;

-- 
With Best Regards,
Andy Shevchenko