Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 23 Mar 2020 14:10:27 +0200 | Subject | Re: [PATCH 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor |
| |
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?
...
> +/* > + * 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
| |