linux-kernel - Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support

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: <1451464521.2531.4.camel@collins>
Date:	Wed, 30 Dec 2015 09:35:21 +0100
From:	Paul Kocialkowski <contact@...lk.fr>
To:	Milo Kim <milo.kim@...com>
Cc:	Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Russell King <linux@....linux.org.uk>,
	Benoît Cousson <bcousson@...libre.com>,
	Tony Lindgren <tony@...mide.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-omap@...r.kernel.org
Subject: Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support

Hi Milo,

Le mercredi 30 décembre 2015 à 09:22 +0900, Milo Kim a écrit :
> Hi Paul,
> 
> On 29/12/15 20:13, Paul Kocialkowski wrote:
> > Hi Milo,
> >
> > Le mardi 29 décembre 2015 à 09:45 +0900, Milo Kim a écrit :
> >> Hi Paul,
> >>
> >> On 29/12/15 07:49, Paul Kocialkowski wrote:
> >>> Hi Milo, thanks for the review,
> >>>
> >>> Le lundi 28 décembre 2015 à 09:56 +0900, Milo Kim a écrit :
> >>>> Hi Paul,
> >>>>
> >>>> On 23/12/15 20:56, Mark Brown wrote:
> >>>>> On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote:
> >>>>>
> >>>>>> +	gpio = lp->pdata->enable_gpio;
> >>>>>> +	if (!gpio_is_valid(gpio))
> >>>>>> +		return 0;
> >>>>>> +
> >>>>>> +	/* Always set enable GPIO high. */
> >>>>>> +	ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
> >>>>>> +	if (ret) {
> >>>>>> +		dev_err(lp->dev, "gpio request err: %d\n", ret);
> >>>>>> +		return ret;
> >>>>>> +	}
> >>>>>
> >>>>> This isn't really adding support for the enable GPIO as the changelog
> >>>>> suggests, it's requesting but not managing the GPIO.  Since there is
> >>>>> core support for manging enable GPIOs this seems especially silly,
> >>>>> please tell the core about the GPIO and then it will work at runtime
> >>>>> too.
> >>>>>
> >>>>
> >>>> With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in
> >>>> LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only
> >>>> can be controlled by external pin when CONFIG pin is grounded.
> >>>>
> >>>> Please see the description at page 5 of the datasheet.
> >>>>
> >>>> 	http://www.ti.com/lit/ds/symlink/lp8725.pdf
> >>>
> >>> After reading the datasheets thoroughly, it seems to me that for the
> >>> lp8720, the EN pin is used to enable the regulators output, which is a
> >>> good fit for the core regulator GPIO framework, as there is no reason to
> >>> keep it on when no regulator is in use. The serial interface is already
> >>> available when EN=0 and regulators can be configured in that state. The
> >>> lp8725 seems seems to behave the same when CONFIG=0 (the datasheet
> >>> clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if
> >>> EN=0"). On the other hand, it is indeed used as a power-on pin when
> >>> CONFIG=1.
> >>
> >> I think it's different use case. LP8720/5 are designed for system PMU,
> >> so some regulators are enabled by default after the device is on. EN pin
> >> is used for turning on/off the chip. This pin does not control regulator
> >> outputs directly. It's separate functional block in the silicon.
> >
> > Well, I really don't understand why the EN pin would turn on/off the
> > chip. All it does it enable the regulators outputs (by entering IDLE
> > mode), the serial block is already available in STANDBY state.
> >
> > If we want some regulators enabled at boot, the best thing to do seems
> > to be to request the GPIO with the GPIOF_INIT_HIGH flag, as done in e.g.
> > the max8952 regulator driver:
> >
> > 	if (pdata->reg_data->constraints.boot_on)
> > 		config.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
> 
> According to MAX8952 datasheet, output regulator is enabled/disabled 
> using EN pin, so ena_gpio is used correctly.
> However, LP8720/5 regulators are enabled/disabled through I2C command. 
> Only few regulators of LP8725 can be on/off by separate external pins. 
> (B2_EN and LDO3_EN)
> Please note that EN pin in LP8720/5 is not the control pin for 
> enabling/disabling regulators.

Oh, I should have mentionned that I also suggest making it possible to
use *both* an enable GPIO and registers to enable a regulator. You are
of course right in saying that the GPIO alone is not enough, and the
core regulator framework will currently only do GPIO or registers to
enable not both.

In my opinion, it would be more elegant to adapt the core regulator
framework to first enable the GPIO and then call the regulator enable
ops callback instead of handling the GPIO in the driver.

However, if that change is not welcome to the core regulator framework,
we'll have to deal with the GPIO in the driver (and probably keep it
enabled at all times there).

Cheers,

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/


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