On Fri, 20 Jan 2023, Maarten Zanders wrote:
>
> > > + 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?
Yes please.
--
Lee Jones [李琼斯]
