linux-kernel - Re: [PATCH 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]

Date:   Tue, 24 Mar 2020 14:52:30 +0300
From:   Ivan Mikhaylov <i.mikhaylov@...ro.com>
To:     Andy Shevchenko <andy.shevchenko@...il.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 2/2] iio: proximity: Add driver support for vcnl3020
 proximity sensor

On Mon, 2020-03-23 at 14:10 +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 12:41 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 a patch, my comments below.
> 
> > Datasheet available at:
> > http://www.vishay.com/docs/84150/vcnl3020.pdf
> 
> I'm thinking that we may simple introduce new tag, called Datesheet:
> to put such links.
> 
> > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@...ro.com>
> 
> ...
> 
> >  obj-$(CONFIG_SRF08)            += srf08.o
> >  obj-$(CONFIG_SX9500)           += sx9500.o
> >  obj-$(CONFIG_VL53L0X_I2C)      += vl53l0x-i2c.o
> > +obj-$(CONFIG_VCNL3020)         += vcnl3020.o
> 
> Perhaps keep ordered?

Oops.

> 
> ...
> 
> > +static int32_t vcnl3020_init(struct vcnl3020_data *data)
> 
> int32_t...
> 
> > +{
> > +       s32 rc;
> 
> ...s32?!
> 
> Applies to entire code.

checkpatch.pl --strict doesn't show anything bad in it but I can change from
int32_t/s32 into int easily, it's not a problem for me.

> 
> > +       u32 led_current;
> > +       struct device *dev = &data->client->dev;
> 
> Reversed xmas tree order looks better.
> 
> > +       rc = i2c_smbus_read_byte_data(data->client, VCNL_PROD_REV);
> 
> Can you use regmap I²C API?
> 

That's a nice idea.

> ...
> 
> > +       dev_info(&client->dev, "Proximity sensor, Rev: %02x\n",
> > +                data->rev);
> 
> Noise.

Doesn't it help to determine the presence of driver to a common user?

> 
> > +               goto out;
> > +
> > +       return rc;
> > +out:
> > +       devm_iio_device_free(&client->dev, indio_dev);
> > +       return rc;
> 
> Managed resources are exactly for this not to be appeared in the code.

I can do something like this:
return devm_iio_device_register(&client->dev, indio_dev);

Would it suffice?

> 
> > +}
> 
> ...
> 
> > +static const struct of_device_id vcnl3020_of_match[] = {
> > +       {
> > +               .compatible = "vishay,vcnl3020",
> > +       },
> 
> Missed terminator. How did you test this?

All works fine with real hw, I'll add terminator.
Agree with everything else.

Thanks.