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? ... > +/* > + * vcnl3020.c - Support for Vishay VCNL3020 proximity sensor Using file names in themselves is a bad idea. Whenever you would rename file (for instance, to support new sensors from the same family in the future) you will forget (often, I see this in practice!) to update this line. Just drop it from here and try to avoid in the future. > + * > + * based on vcnl4000.c This sounds like a continuation of previous sentence. Drop line in between and use proper English grammar and punctuation. > + */ ... > +struct vcnl3020_data { > + struct i2c_client *client; > + u32 rev; > + struct mutex vcnl3020_lock; /* for i2c operations */ Simple 'lock' is enough, the rest is dup noise. Also, consider kernel doc format instead of odd comments. > +}; ... > +static const struct i2c_device_id vcnl3020_id[] = { > + { "vcnl3020", 0 }, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, vcnl3020_id); Can you group this with OF table below? ... > +static int32_t vcnl3020_init(struct vcnl3020_data *data) int32_t... > +{ > + s32 rc; ...s32?! Applies to entire code. > + 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? > + if (rc < 0) { > + dev_err(dev, "Error (%d) reading product revision", rc); > + goto exit; > + } > + > + if (rc == VCNL3020_PROD_ID) { > + data->rev = rc & 0xff; This conjunction looks strange. Also, why type of rev is u32 instead of u8? > + mutex_init(&data->vcnl3020_lock); > + } else { > + dev_err(dev, "Product id (%x) did not match vcnl3020 (%x)", rc, > + VCNL3020_PROD_ID); > + rc = -ENODEV; > + goto exit; > + } > + > + /* set led current */ > + rc = i2c_smbus_write_byte_data(data->client, VCNL_LED_CURRENT, > + led_current); > + if (rc < 0) { > + dev_err(dev, "Error (%d) setting LED current", rc); > + goto exit; > + } > + > + return 0; > +exit: > + return rc; Useless. Return directly. > +}; ... > + /* wait for data to become ready */ > + while (tries--) { > + rc = i2c_smbus_read_byte_data(data->client, VCNL_COMMAND); > + if (rc < 0) > + goto fail; > + if (rc & VCNL_PS_RDY) > + break; > + msleep(20); /* measurement takes up to 100 ms */ > + } Timeout loops look more naturally in do {} while format. unsigned int tries = 5; ... do { ... } while (--tries); ... > + *val = (rc & 0xff) << 8; > + *val |= rc & 0xff; All these conjunctions looks fishy. Why do you need them? Cant you rely on the returned value by I²C API? ... > +fail: Better name is 'err_unlock' or 'out_unlock'. The rule of thumb to describe in the label what you *about to do* there. > + mutex_unlock(&data->vcnl3020_lock); > + > + return rc; > +} ... > + rc = vcnl3020_measure_proximity(data, val); > + if (rc < 0) Can rc be positive? Drop all these ' < 0' in cases where it is guaranteed not to be the case. > + return rc; ... > +static int32_t vcnl3020_probe(struct i2c_client *client, > + const struct i2c_device_id *id) Can you switch to ->probe_new() ? ... > + dev_info(&client->dev, "Proximity sensor, Rev: %02x\n", > + data->rev); Noise. ... > + rc = devm_iio_device_register(&client->dev, indio_dev); > + if (rc != 0) Redundant ' != 0' part. > + 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. > +} ... > +static const struct of_device_id vcnl3020_of_match[] = { > + { > + .compatible = "vishay,vcnl3020", > + }, Missed terminator. How did you test this? > +}; -- With Best Regards, Andy Shevchenko
