Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 20 Jan 2023 16:52:51 +0100 | Subject | Re: [PATCH v2 2/2] leds: lp55xx: configure internal charge pump | From | Maarten Zanders <> |
| |
>> >> + 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
| ![\](/images/icornerr.gif) |