Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1298786

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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.

Since my intent here is to cover the lp8720 use case, I suggest that we
implement this using the core regulator GPIO framework (I have patches
ready for that) and leave out the case where CONFIG=1, which could be
dealt with later by providing that piece of information via platform
data (and devicetree) and then either use the regulator GPIO framework
(when CONFIG=0 and default) or register the GPIO within the driver and
keep it on at all times (when CONFIG=1).

I could most certainly implement that behaviour, but I'd rather leave it
to someone else (or at least the testing) since I don't have any lp8725
to play with.

What do you think?

-- 
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/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/6] LG Optimus Black (P970) codename sniper support and lp872x improvements Paul Kocialkowski <contact@paulk.fr> - 2015-12-23 12:10 +0100
  [PATCH 3/6] regulator: lp872x: Remove warning about invalid DVS GPIO Paul Kocialkowski <contact@paulk.fr> - 2015-12-23 12:10 +0100
    Re: [PATCH 3/6] regulator: lp872x: Remove warning about invalid DVS  GPIO Mark Brown <broonie@kernel.org> - 2015-12-23 12:50 +0100
      Re: [PATCH 3/6] regulator: lp872x: Remove warning about invalid DVS  GPIO Paul Kocialkowski <contact@paulk.fr> - 2015-12-23 13:00 +0100
        Re: [PATCH 3/6] regulator: lp872x: Remove warning about invalid DVS  GPIO Mark Brown <broonie@kernel.org> - 2015-12-23 13:00 +0100
  [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-23 12:10 +0100
    Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Mark Brown <broonie@kernel.org> - 2015-12-23 13:00 +0100
      Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-23 14:00 +0100
      Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-24 19:20 +0100
        Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Mark Brown <broonie@kernel.org> - 2015-12-24 20:40 +0100
          Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-24 21:10 +0100
      Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Milo Kim <milo.kim@ti.com> - 2015-12-28 03:50 +0100
        Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-29 00:00 +0100
          Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Milo Kim <milo.kim@ti.com> - 2015-12-29 01:50 +0100
            Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-29 12:20 +0100
              Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Milo Kim <milo.kim@ti.com> - 2015-12-30 01:30 +0100
                Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-30 09:40 +0100
                Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Mark Brown <broonie@kernel.org> - 2015-12-30 17:40 +0100
                Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-30 19:40 +0100
                Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-31 23:00 +0100
                Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Mark Brown <broonie@kernel.org> - 2015-12-31 23:20 +0100
                Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Mark Brown <broonie@kernel.org> - 2015-12-31 23:00 +0100
    Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Milo Kim <milo.kim@ti.com> - 2015-12-28 03:50 +0100
    Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Rob Herring <robh@kernel.org> - 2015-12-29 21:10 +0100
      Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Paul Kocialkowski <contact@paulk.fr> - 2015-12-29 22:30 +0100
        Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support Rob Herring <robh@kernel.org> - 2015-12-29 23:00 +0100
  [PATCH 6/6] ARM: multi_v7_defconfig: Enable LP872x regulator support Paul Kocialkowski <contact@paulk.fr> - 2015-12-23 12:10 +0100
  [PATCH 2/6] regulator: lp872x: Get rid of duplicate reference to DVS GPIO Paul Kocialkowski <contact@paulk.fr> - 2015-12-23 12:10 +0100
    Applied "regulator: lp872x: Get rid of duplicate reference to DVS GPIO" to the regulator tree Mark Brown <broonie@kernel.org> - 2015-12-23 13:20 +0100
    Re: [PATCH 2/6] regulator: lp872x: Get rid of duplicate reference to  DVS GPIO Milo Kim <milo.kim@ti.com> - 2015-12-28 03:50 +0100

csiph-web