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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y8q7F0cDNGFeOv99@google.com>
Date:   Fri, 20 Jan 2023 16:02:31 +0000
From:   Lee Jones <lee@...nel.org>
To:     Maarten Zanders <maarten.zanders@...d.be>
Cc:     Pavel Machek <pavel@....cz>, krzysztof.kozlowski@...aro.org,
        linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] leds: lp55xx: configure internal charge pump

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 [李琼斯]