linux-kernel - Re: [PATCH 2/2] regulator: Add MAX20086-MAX20089 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 4 Jan 2022 14:47:26 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-kernel@...r.kernel.org, Watson Chow <watson.chow@...et.com>,
        Liam Girdwood <lgirdwood@...il.com>
Subject: Re: [PATCH 2/2] regulator: Add MAX20086-MAX20089 driver

On Tue, Jan 04, 2022 at 04:33:52PM +0200, Laurent Pinchart wrote:
> On Tue, Jan 04, 2022 at 02:16:33PM +0000, Mark Brown wrote:

> > > +	chip->num_outputs = num;

> > The number of regulators the device supports should be known from the
> > compatible, I'd expect a data table for this.  It should be possible to
> > read the state of regulators not described in the DT.

> Does this mean that the driver should register all regulators, even the
> ones not described in DT ? Who would read the state ?

Yes, just register everything.  Someone doing system debugging or
bringup might want to read the state, and this also goes along with the
ability to have the core pull the constraints out of the DT based on
data supplied by the driver - the general goal is to have routine
drivers simply register data tables with the core and need minimal code.

> > > +	/* Get the chip out of low-power shutdown state. */
> > > +	chip->gpio_en = devm_gpiod_get(chip->dev, "enable", GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(chip->gpio_en)) {
> > > +		ret = PTR_ERR(chip->gpio_en);
> > > +		dev_err(chip->dev, "Failed to get enable GPIO: %d\n", ret);
> > > +		return ret;
> > > +	}

> > This one is more OK - it's changing the state of the outputs that's an
> > issue.  I guess this might cause the outputs to come on though if the
> > GPIO was left off by the bootloader which is awkward.  If there's
> > nothing other than the outputs going on with the chip I would be tempted
> > to map this onto the per regulator enable GPIO that the core supports,
> > the core will then be able to manage the low power state at runtime.
> > That's *probably* the least bad option we have with current interfaces.

> While fishing for code I can copy in the always unfashionable cargocult
> style, I came across max8973-regulator.c that handles the enable GPIO in
> the following way:

> 		if (ridata && (ridata->constraints.always_on ||
> 			       ridata->constraints.boot_on))
> 			gflags = GPIOD_OUT_HIGH;
> 		else
> 			gflags = GPIOD_OUT_LOW;
> 		gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
> 		gpiod = devm_gpiod_get_optional(&client->dev,
> 						"maxim,enable",
> 						gflags);

> Should I try to replicate that ? It gets more difficult with multiple
> regulators that share the same GPIO. That's why I left it as-is.

We should really factor that bit out to the core too, though at the
minute we pass in a gpio_desc so it's too late.  Doing the above is
probably best, though I wouldn't loose any sleep over it being missing.
you should definitely set the _NONEXCLUSIVE flag.  If someone specifies
an incompatible mix of settings in the machine constraints I wouldn't
worry about it too much, there's limits on what we can sort out.

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