On Tue, 2020-04-21 at 23:23 +0300, Andy Shevchenko wrote:
> On Tue, Apr 21, 2020 at 10:39 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.
>
> ...
>
> > +static int vcnl3020_get_and_apply_property(struct vcnl3020_data *data,
> > + const char *prop, u32 reg)
> > +{
> > + int rc;
> > + u32 val;
> > +
> > + rc = device_property_read_u32(data->dev, prop, &val);
> > + if (rc)
> > + return 0;
> > +
> > + /* An example of conversion from uA to reg val:
> > + * 200000 uA == 200 mA == 20
> > + */
> > + if (!strcmp(prop, "vishay,led-current-microamp"))
> > + val /= 10000;
>
> I probably missed the point why this function is needed at all, since
> we always call only for a single property.
Andy, I'm planning to add more properties but not for now, this driver the
placeholder for upcoming work what I've. And I wanted something like:
rc = vcnl3020_get_and_apply_property(option1);
if (rc)
return rc;
rc = vcnl3020_get_and_apply_property(option2);
if (rc)
return rc;
etc.
And all other work about conversion will be hidden in get_and_apply_property.
> On top of that, why do we have this nasty strcmp()? Can't we simple do
> something like
> static int vcnl3020_get_and_apply_property(struct vcnl3020_data *data,
> const char *prop, u32 reg, u32 div)
> {
> ...
> val /= div;
> ...
> }
>
> static int vcnl3020_get_and_apply_led_current_property(struct
> vcnl3020_data *data)
> {
> /*
> * An example of conversion from uA to reg val:
> * 200000 uA == 200 mA == 20
> */
> // Note by the way comments style
> return vcnl3020_get_and_apply_property(data, "vishay,led-current-microamp",
> VCNL_LED_CURRENT, 10000);
> }
>
> ?
I think your approach with wrapper is better, I'll think how I can do that
without additional 'div' param.
Thanks.
