Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 28 Nov 2020 11:39:58 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     Michael Klein <michael@fossekall.de>
Cc:     Rob Herring <robh+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] ARM: dts: sun8i-h2-plus-bananapi-m2-zero: add
 poweroff node to DT
Message-ID: <20201128103958.q6glewhhch7vtczr@gilmour>
References: <20201124131933.2l2go6z6ltar5fpa@gilmour>
 <20201124133633.672259-1-michael@fossekall.de>
 <20201124142656.wz6ipwa2xayn433x@gilmour>
 <20201124223159.GA2865084@a98shuttle.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="stejl7vgq72gqvys"
Content-Disposition: inline
In-Reply-To: <20201124223159.GA2865084@a98shuttle.de>
Precedence: bulk


--stejl7vgq72gqvys
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Nov 24, 2020 at 11:31:59PM +0100, Michael Klein wrote:
> On Tue, Nov 24, 2020 at 03:26:56PM +0100, Maxime Ripard wrote:
> > On Tue, Nov 24, 2020 at 02:36:33PM +0100, Michael Klein wrote:
> > > Add poweroff node to allow the board to power itself off after shutdo=
wn
> > > by disabling the SYSTEM and CPUX regulators (U5 resp. U6).  The RST
> > > button can be used to restart the board.
> > >=20
> > > Signed-off-by: Michael Klein <michael@fossekall.de>
> > > ---
> > >  arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 5 +++++
> > >  1 file changed, 5 insertions(+)
> >=20
> > You should have a summary of the changes between versions here
> >=20
> > > diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/a=
rch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > > index 4c6704e4c57e..ea2fa48a1647 100644
> > > --- a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > > +++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
> > > @@ -46,6 +46,11 @@ sw4 {
> > >  		};
> > >  	};
> > >=20
> > > +	poweroff {
> > > +		compatible =3D "gpio-poweroff";
> > > +		gpios =3D <&r_pio 0 8 GPIO_ACTIVE_LOW>; /* PL8 */
> > > +	};
> > > +
> >=20
> > Like I said in the previous version, this should really be modelled as a
> > regulator instead of just a GPIO
>=20
> Please excuse my ignorance, do you mean something like this?
>=20
>        reg_vdd_sys: vdd-sys {
>                compatible =3D "regulator-fixed";
>                regulator-name =3D "vdd-sys";
>                regulator-min-microvolt =3D <1200000>;
>                regulator-max-microvolt =3D <1200000>;
>                regulator-always-on;
>                regulator-boot-on;
>                enable-active-high;
>                gpio =3D <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
>                vin-supply =3D <&reg_vcc5v0>;
>         };
>=20
> With this, the board still draws 60mA (cheap USB ampere meter) after
> shutdown, presumably because of "regulator-always-on".  Without this
> property the board powers off shortly after booting up.

Yes, because you're only describing the regulator itself here, but
you're not telling linux that it needs to shut it down to power-down the
board.

You'd need a driver similar to gpio-poweroff, using a regulator instead,
and calling regulator_force_disable to shut it down

Maxime

--stejl7vgq72gqvys
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCX8Io/QAKCRDj7w1vZxhR
xXyaAP9dhQ6ZoKWFbs6dcJc5Sqvox9mY6u0YvG1VjDCv//w7bgEAnAvV7LTYzXcP
gvqQIqZAI1Y6fd0YSrdnwif+0yfjhgI=
=lHji
-----END PGP SIGNATURE-----

--stejl7vgq72gqvys--