linux-kernel - Re: [PATCH 5/6] arm64: dts: rockchip: correct rk3328-roc regulator map

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]

Message-Id: <D682VQLD81Y8.1K8OD4ECAOSW3@cknow.org>
Date: Tue, 10 Dec 2024 15:08:29 +0100
From: "Diederik de Haas" <didi.debian@...ow.org>
To: "Peter Geis" <pgwipeout@...il.com>
Cc: "Heiko Stuebner" <heiko@...ech.de>, "Conor Dooley"
 <conor+dt@...nel.org>, "Dragan Simic" <dsimic@...jaro.org>, "Johan Jonker"
 <jbx6244@...il.com>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Levin Du"
 <djw@...hip.com.cn>, "Rob Herring" <robh@...nel.org>,
 <devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
 <linux-kernel@...r.kernel.org>, <linux-rockchip@...ts.infradead.org>
Subject: Re: [PATCH 5/6] arm64: dts: rockchip: correct rk3328-roc regulator
 map

On Tue Dec 10, 2024 at 2:04 PM CET, Peter Geis wrote:
> On Tue, Dec 10, 2024 at 6:31 AM Diederik de Haas <didi.debian@...ow.org> wrote:
> > On Tue Dec 10, 2024 at 2:30 AM CET, Peter Geis wrote:
> > > voltages to vcc_host1_5v, and standardize the order of regulator
> > > properties among the fixed regulators.
> >
> > Big fan of standardization :-) ...
> >
> > >
> > > Fixes: 2171f4fdac06 ("arm64: dts: rockchip: add roc-rk3328-cc board")
> > > Signed-off-by: Peter Geis <pgwipeout@...il.com>
> > > ---
> > >
> > >  arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 23 +++++++++++++-------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > > index f782c8220dd3..6984387ff8b3 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
> > > @@ -24,22 +24,23 @@ gmac_clkin: external-gmac-clock {
> > >               #clock-cells = <0>;
> > >       };
> > >
> > > -     dc_12v: regulator-dc-12v {
> > > +     /* fed from passive usb input connector */
> > > +     dc_5v: regulator-dc-5v {
> > >               compatible = "regulator-fixed";
> > > -             regulator-name = "dc_12v";
> > > +             regulator-name = "dc_5v";
> > >               regulator-always-on;
> > >               regulator-boot-on;
> > > -             regulator-min-microvolt = <12000000>;
> > > -             regulator-max-microvolt = <12000000>;
> > > +             regulator-min-microvolt = <5000000>;
> > > +             regulator-max-microvolt = <5000000>;
> > >       };
> > >
> > >       vcc_sd: regulator-sdmmc {
> > >               compatible = "regulator-fixed";
> > > +             regulator-name = "vcc_sd";
> > >               gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_LOW>;
> > >               pinctrl-names = "default";
> > >               pinctrl-0 = <&sdmmc0m1_pin>;
> > >               regulator-boot-on;
> > > -             regulator-name = "vcc_sd";
> > >               regulator-min-microvolt = <3300000>;
> > >               regulator-max-microvolt = <3300000>;
> > >               vin-supply = <&vcc_io>;
> >
> > ... but why not put regulator-name as the first of the regulator
> > properties as is done in the rk3328-rock64.dts ...
> >
> > > @@ -50,22 +51,25 @@ vcc_sdio: regulator-sdmmcio {
> > >               states = <1800000 0x1>, <3300000 0x0>;
> > >               regulator-name = "vcc_sdio";
> > >               regulator-type = "voltage";
> > > +             regulator-always-on;
> > >               regulator-min-microvolt = <1800000>;
> > >               regulator-max-microvolt = <3300000>;
> > > -             regulator-always-on;
> > >               vin-supply = <&vcc_sys>;
> > >       };
> > >
> > >       vcc_host1_5v: vcc_otg_5v: regulator-vcc-host1-5v {
> > >               compatible = "regulator-fixed";
> > > +             regulator-name = "vcc_host1_5v";
> > >               enable-active-high;
> > >               pinctrl-names = "default";
> > >               pinctrl-0 = <&usb20_host_drv>;
> > > -             regulator-name = "vcc_host1_5v";
> > >               regulator-always-on;
> > > +             regulator-min-microvolt = <5000000>;
> > > +             regulator-max-microvolt = <5000000>;
> > >               vin-supply = <&vcc_sys>;
> > >       };
> >
> > ... and was the case here?
>
> That's fair, thank you. I like the alphabetical approach, I'll go that
> way when I split this out.

FWIW, I'm fine when regulator-name and regulator-type would be put on
the top of the regulator-* properties, also because that's currently
the case for the rk3328-rock64.dts.
And a strict alphabetical order would look weird anyway as then
-max-microvolt should be ordered above -min-microvolt.

Cheers,
  Diederik

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