Messages in this thread | | | Subject | Re: [PATCH v5 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor | From | Ivan Mikhaylov <> | Date | Mon, 30 Mar 2020 23:41:15 +0300 |
| |
On Mon, 2020-03-30 at 22:07 +0300, Andy Shevchenko wrote: > On Mon, Mar 30, 2020 at 6:27 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. > > Still my tag applies, but > I have few more comments below. > > ... > > > +#define VCNL_DRV_NAME "vcnl3020" > > +#define VCNL_REGMAP_NAME "vcnl3020_regmap" > > I'm wondering why you need the second one.
For regmap initialize as name in static const struct regmap_config vcnl3020_regmap_config = { .name = VCNL_REGMAP_NAME,
I can get rid of it from struct with name definition.
> > + rc = device_property_read_u32(data->dev, "vishay,led-current- > > milliamp", > > + &led_current); > > + if (rc == 0) { > > + rc = regmap_write(data->regmap, VCNL_LED_CURRENT, > > led_current); > > + if (rc) > > + dev_err(data->dev, > > + "Error (%d) setting LED current", rc); > > + } > > + > > + return rc; > > Why not to use standard pattern, i.e. > > if (rc) > return rc; > ... > rc = regmap_write(...); > > ?
Optional parameter. There exists a lot of ways to do it:
rc = device_property_read_u32(dev, "milliamp", &led_current); rc = regmap_write(regmap, VCNL_LED_CURRENT, (!rc) : led_current ? 0); or
rc = regmap_write(regmap, VCNL_LED_CURRENT, (!rc) : led_current ? VCNL_LED_CURRENT_DEFAULT); or even maybe make a function like get_led_current:
rc = regmap_write(regmap, VCNL_LED_CURRENT, get_led_curent(dev)); where static u32 get_led_current(dev) { u32 led_current; rc = device_property_read_u32(dev, "milliamp", &led_current); return (!rc) : led_current ? VCNL_LED_CURRENT_DEFAULT; } Which one would be more preferable?
> ... > > > + if (rc) { > > + dev_err(data->dev, > > + "vcnl3020_measure() failed with error (%d)", rc); > > Perhaps you keep same pattern for error messages as in previous function(s).
Sure.
> > > + goto err_unlock; > > + } > > + rc = regmap_bulk_read(data->regmap, VCNL_PS_RESULT_HI, &res, 2); > > sizeof(res)
Oops.
| |