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

[Ivan Mikhaylov <i.mikhaylov@yadro.com>]

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@yadro.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@yadro.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.