[permalink] [raw]

Subject: [PATCH 11/16] arm64: dts: imx8mq-librem5-devkit: Align pin configuration group names with schema

Device tree schema expects pin configuration groups to end with 'grp'
suffix, otherwise dtbs_check complain with a warning like:

... do not match any of the regexes: 'grp$', 'pinctrl-[0-9]+'

Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
.../boot/dts/freescale/imx8mq-librem5-devkit.dts | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
index 6900ac274f5b..377591a0e6e9 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
@@ -734,7 +734,7 @@
>;
};

- pinctrl_usdhc1_100mhz: usdhc1grp100mhz {
+ pinctrl_usdhc1_100mhz: usdhc1-100mhzgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x8d
MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xcd
@@ -751,7 +751,7 @@
>;
};

- pinctrl_usdhc1_200mhz: usdhc1grp200mhz {
+ pinctrl_usdhc1_200mhz: usdhc1-200mhzgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x9f
MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xdf
@@ -768,13 +768,13 @@
>;
};

- pinctrl_usdhc2_pwr: usdhc2grppwr {
+ pinctrl_usdhc2_pwr: usdhc2pwrgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD2_RESET_B_GPIO2_IO19 0x41
>;
};

- pinctrl_usdhc2_gpio: usdhc2grpgpio {
+ pinctrl_usdhc2_gpio: usdhc2gpiogrp {
fsl,pins = <
MX8MQ_IOMUXC_SD2_WP_GPIO2_IO20 0x80 /* WIFI_WAKE */
>;
@@ -791,7 +791,7 @@
>;
};

- pinctrl_usdhc2_100mhz: usdhc2grp100mhz {
+ pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x8d
MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcd
@@ -802,7 +802,7 @@
>;
};

- pinctrl_usdhc2_200mhz: usdhc2grp200mhz {
+ pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x9f
MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcf
--
2.17.1

2020-08-24 19:10:01

by Krzysztof Kozlowski

[permalink] [raw]

Subject: [PATCH 13/16] arm64: dts: imx8mq-pico-pi: Align pin configuration group names with schema

Device tree schema expects pin configuration groups to end with 'grp'
suffix, otherwise dtbs_check complain with a warning like:

... do not match any of the regexes: 'grp$', 'pinctrl-[0-9]+'

Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
arch/arm64/boot/dts/freescale/imx8mq-pico-pi.dts | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq-pico-pi.dts b/arch/arm64/boot/dts/freescale/imx8mq-pico-pi.dts
index 59da96b7143f..f4d5748a7bd6 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-pico-pi.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-pico-pi.dts
@@ -297,7 +297,7 @@
>;
};

- pinctrl_pmic: pmicirq {
+ pinctrl_pmic: pmicirqgrp {
fsl,pins = <
MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x41
>;
@@ -335,7 +335,7 @@
>;
};

- pinctrl_usdhc1_100mhz: usdhc1grp100mhz {
+ pinctrl_usdhc1_100mhz: usdhc1-100mhzgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x85
MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc5
@@ -351,7 +351,7 @@
>;
};

- pinctrl_usdhc1_200mhz: usdhc1grp200mhz {
+ pinctrl_usdhc1_200mhz: usdhc1-200mhzgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x87
MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc7
@@ -367,7 +367,7 @@
>;
};

- pinctrl_usdhc2_gpio: usdhc2grpgpio {
+ pinctrl_usdhc2_gpio: usdhc2gpiogrp {
fsl,pins = <
MX8MQ_IOMUXC_SD2_CD_B_GPIO2_IO12 0x41
>;
@@ -385,7 +385,7 @@
>;
};

- pinctrl_usdhc2_100mhz: usdhc2grp100mhz {
+ pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x85
MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc5
@@ -397,7 +397,7 @@
>;
};

- pinctrl_usdhc2_200mhz: usdhc2grp200mhz {
+ pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
fsl,pins = <
MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x87
MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc7
--
2.17.1

2020-08-24 19:10:08

by Krzysztof Kozlowski

[permalink] [raw]

On Tue, 2020-08-25 at 10:27 +0200, [email protected] wrote:
> On Tue, Aug 25, 2020 at 08:22:18AM +0000, Vaittinen, Matti wrote:
> > Hello Krzysztof,
> >
> > On Tue, 2020-08-25 at 09:50 +0200, [email protected] wrote:
> > > On Tue, Aug 25, 2020 at 09:45:00AM +0200, [email protected] wrote:
> > > > On Tue, Aug 25, 2020 at 09:25:37AM +0200, [email protected]
> > > > wrote:
> > > > > On Tue, Aug 25, 2020 at 06:51:33AM +0000, Vaittinen, Matti
> > > > > wrote:
> > > > > > Hello Krzysztof,
> > > > > >
> > > > > > Just some questions - please ignore if I misunderstood the
> > > > > > impact of
> > > > > > the change.
> > > > > >
> > > > > > On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski
> > > > > > wrote:
> > > > > > > Device tree schema expects regulator names to be
> > > > > > > lowercase. This
> > > > > > > fixes
> > > > > > > dtbs_check warnings like:
> > > > > > >
> > > > > > > arch/arm64/boot/dts/freescale/imx8mn-ddr4-
> > > > > > > evk.dt.yaml:
> > > > > > > pmic@4b:
> > > > > > > regulators:LDO1:regulator-name:0: 'LDO1' does not match
> > > > > > > '^ldo[1-6]$'
> > > > > > >
> > > > > > > Signed-off-by: Krzysztof Kozlowski <[email protected]>
> > > > > > > ---
> > > > > > > .../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22
> > > > > > > +++++++++------
> > > > > > > ----
> > > > > > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-
> > > > > > > evk.dts
> > > > > > > b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > > > > index a1e5483dbbbe..299caed5d46e 100644
> > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > > > > > > @@ -60,7 +60,7 @@
> > > > > > >
> > > > > > > regulators {
> > > > > > > buck1_reg: BUCK1 {
> > > > > > > - regulator-name = "BUCK1";
> > > > > > > + regulator-name = "buck1";
> > > > > >
> > > > > > I am not against this change but I would expect seeing some
> > > > > > other
> > > > > > patches too? I guess this will change the regulator name in
> > > > > > regulator
> > > > > > core, right? So maybe I am mistaken but it looks to me this
> > > > > > change is
> > > > > > visible in suppliers, sysfs and debugfs too? Thus changing
> > > > > > this
> > > > > > sounds
> > > > > > a bit like asking for a nose bleed :) Am I right that the
> > > > > > impact of
> > > > > > this change has been thoroughly tested? Are there any other
> > > > > > patches
> > > > > > (that I have not seen) related to this change?
> > > > >
> > > > > Oh, crap, the names of regulators in the driver are
> > > > > lowercase,
> > > > > but they
> > > > > use of_match_ptr for upper case. Seriously, why making a
> > > > > binding
> > > > > which
> > > > > is contradictory to the driver implementation on the first
> > > > > day?
> > > > >
> > > > > The driver goes with binding, right? One expects uppercase,
> > > > > other
> > > > > lowercase...
> > > > >
> > > > > And tell me, what is now the ABI? The binding or the
> > > > > incorrect
> > > > > implementation?
> > > >
> > > > Wait, my mistake. I got confused by my own change. The node
> > > > name
> > > > stays
> > > > the same, so of_match will be correct.
> >
> > Yes. I think so too. Match will still work as earler.
> >
> > > > The driver internally already uses lowercase names.
> >
> > Yep. I was simply thinking that if anyone has been specifying the
> > regulators as suppliers by name - then this change will change
> > things
> > (as is seen for LDO5). Additionally, if any user-space SW has been
> > reading the regulator states from sysfs - then these names will
> > also
> > change the sysfs. Debugfs change is hopefully not such a big deal.
>
> About user-space, I think the embedded DT is not part of kernel ABI,
> so
> there is no such requirement about keeping it stable. I agree though
> it
> might be annoying surprise.

Just to ensure we are talking about same thing:
I see you are talking about embedded DT not being an ABI. I agree with
you - DT itself is not ABI. But in case you missed this we have:

static ssize_t name_show(struct device *dev, struct device_attribute
*attr,
char *buf)
{
struct regulator_dev *rdev = dev_get_drvdata(dev);

return sprintf(buf, "%s\n", rdev_get_name(rdev));
}
static DEVICE_ATTR_RO(name);

in regulator core. I believe the rdev_get_name(rdev) shall change
according to regulator-name. (But as I said, I have no idea if this is
used by user-space on your board - I'll leave this for you & others to
judge).

>
> > Whether this really breaks anything is beyond my knowledge as I
> > don't
> > even have this board. Anyways, I think that by minimum the commit
> > message should point out that this change will be visible outside
> > DTS
> > and the BD718x7 driver - up to the user-space.
>
> Good point, I will extend the commit msg about possible impact and
> fixing supplies.
>

Thanks :)

--Matti

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)