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.
Attachment:
signature.asc
Description: PGP signature
