Re: [PATCH v2 2/2] leds: lp55xx: configure internal charge pump

[Maarten Zanders <maarten.zanders@xxxxxxx>]

> >   
> > +	pdata->charge_pump_mode = LP55XX_CP_AUTO;
> > +	ret = of_property_read_string(np, "ti,charge-pump-mode", &pm);
> > +	if (!ret) {
> > +		for (cp_mode = LP55XX_CP_OFF;
> > +		     cp_mode < ARRAY_SIZE(charge_pump_modes);
> > +		     cp_mode++) {
> > +			if (!strcasecmp(pm, charge_pump_modes[cp_mode])) {
> > +				pdata->charge_pump_mode = cp_mode;
> > +				break;
> > +			}
> > +		}
> > +	}
> A little over-engineered, no?
>
> Why not make the property a numerical value, then simply:
>
>    ret = of_property_read_u32(np, "ti,charge-pump-mode", &pdata->charge_pump_mode);
>    if (ret)
>            data->charge_pump_mode = LP55XX_CP_AUTO;
>
> Elevates the requirement for the crumby indexed array of strings above.
>
> Remainder looks sane enough.

Thanks for your feedback.

I won't argue that your implementation isn't far more simple. The idea 
was to have an elaborate and clear and obvious devicetree, but that can 
also be achieved by moving constants into 
/includes/dt-bindings/leds/leds-lp55xx.h. Would that be more acceptable?

Maarten