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