linux-kernel - Re: [PATCH 2/2] regulator: max20339: add Maxim MAX20339 regulator driver

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZuqUDJThMvReRskQ@finisterre.sirena.org.uk>
Date: Wed, 18 Sep 2024 10:49:16 +0200
From: Mark Brown <broonie@...nel.org>
To: André Draszik <andre.draszik@...aro.org>
Cc: Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Michael Walle <mwalle@...nel.org>,
	Peter Griffin <peter.griffin@...aro.org>,
	Tudor Ambarus <tudor.ambarus@...aro.org>,
	Will McVicker <willmcvicker@...gle.com>, kernel-team@...roid.com,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 2/2] regulator: max20339: add Maxim MAX20339 regulator
 driver

On Tue, Sep 17, 2024 at 12:41:16PM +0100, André Draszik wrote:
> On Tue, 2024-09-17 at 11:19 +0200, Mark Brown wrote:

> > This is an error on the input, not an error from this regulator, so the
> > notification isn't appropriate here.

> The input is usually a USB plug / cable. Is there a better option to report
> this? I guess I could register a power supply.

Yes, that's a power supply.

> > > +	return FIELD_GET(MAX20339_INSWCLOSED, val) == 1;
> > > +}

> > This does not appear to be an enable control, it's reading back a status
> > register rather than turning on or off a regulator.

> This is the regulator_ops::is_enabled() callback, shouldn't it return the
> status in effect? It's required to return effective status for one of the
> code paths in _regulator_do_enable(), when .poll_enabled_time is != 0.

No, if there are separate enable and status bits it should return the
value written to the enable bit.  Some devices overload the
functionality, this one splits them.

> > > +static int max20339_set_voltage_sel(struct regulator_dev *rdev,
> > > +				    unsigned int sel)
> > > +{
> > > +	return max20339_set_ovlo_helper(rdev,
> > > +					FIELD_PREP(MAX20339_OVLOSEL_INOVLOSEL,
> > > +						   sel));
> > > +}

> > This device does not appear to be a voltage regualtor, it is a
> > protection device.  A set_voltage() operation is therfore inappropriate
> > for it, any voltage configuration would need to be done on the parent
> > regulator.

> This is handling one of the switches, and the input usually is
> a USB plug / cable.

> Based on the use-case (peripheral / OTG / wireless charging), the
> overvoltage voltage needs to be modified at runtime for full
> protection.

Sure, but that's not setting the voltage of a regulator that's
configuring the protection.

> The set-voltage APIs seemed like a good fit for that, given the
> regulator APIs allow setting those thresholds already (during init).

Don't shoehorn vaugely related things into a somewhat similar looking
API, that'll just blow up whenever something actually assumes that using
the API does the thing it's supposed to.

> I'll see if I could maybe add a power supply as the parent and leave out
> all the voltage and current related settings here altogether and make it
> just control the switches, like some other regulator drivers do.

An API for dynamically configuring limits for regulators at runtime
would be OK too.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)