From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:07:21
The input clock and number of clock provider cells are not required for
the PMIC to operate. They are needed only for the optional bd718x7
clock driver.
Add also clock-output-names as driver takes use of it.
This fixes dtbs_check warnings like:
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b: 'clocks' is a required property
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b: '#clock-cells' is a required property
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../devicetree/bindings/mfd/rohm,bd71847-pmic.yaml | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
@@ -38,6 +38,9 @@ properties:"#clock-cells":const:0+clock-output-names:+maxItems:1+# The BD71847 abd BD71850 support two different HW states as reset target# states. States are called as SNVS and READY. At READY state all the PMIC# power outputs go down and OTP is reload. At the SNVS state all other logic
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:07:28
Driver requires different amount of clocks for different SoCs. Describe
these requirements properly to fix dtbs_check warnings like:
arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../devicetree/bindings/mtd/gpmi-nand.yaml | 76 +++++++++++++++----
1 file changed, 61 insertions(+), 15 deletions(-)
@@ -9,9 +9,6 @@ title: Freescale General-Purpose Media Interface (GPMI) bindingmaintainers:-Han Xu <han.xu@nxp.com>-allOf:--$ref:"nand-controller.yaml"-description:|The GPMI nand controller provides an interface to control the NANDflash chips. The device tree may optionally contain sub-nodes
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:07:32
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22 +++++++++----------
1 file changed, 11 insertions(+), 11 deletions(-)
@@ -79,14 +79,14 @@buck3_reg:BUCK3{// BUCK5 in datasheet-regulator-name="BUCK3";+regulator-name="buck3";regulator-min-microvolt=<700000>;regulator-max-microvolt=<1350000>;};buck4_reg:BUCK4{// BUCK6 in datasheet-regulator-name="BUCK4";+regulator-name="buck4";regulator-min-microvolt=<3000000>;regulator-max-microvolt=<3300000>;regulator-boot-on;
@@ -95,7 +95,7 @@buck5_reg:BUCK5{// BUCK7 in datasheet-regulator-name="BUCK5";+regulator-name="buck5";regulator-min-microvolt=<1605000>;regulator-max-microvolt=<1995000>;regulator-boot-on;
@@ -104,7 +104,7 @@buck6_reg:BUCK6{// BUCK8 in datasheet-regulator-name="BUCK6";+regulator-name="buck6";regulator-min-microvolt=<800000>;regulator-max-microvolt=<1400000>;regulator-boot-on;
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:07:36
Since the "spi-cs-high" property is not present, the SPI chip select pin
polarity is active low.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:07:45
Device tree schema expects pin configuration groups to end with 'grp'
suffix. This fixes dtbs_check warnings like:
pinctrl@30330000: 'pcal6414-gpio', 'pmicirq', 'usdhc1grp100mhz', 'usdhc1grp200mhz', 'usdhc1grpgpio',
'usdhc2grp100mhz', 'usdhc2grp200mhz', 'usdhc2grpgpio', 'usdhc3grp100mhz', 'usdhc3grp200mhz'
do not match any of the regexes: 'grp$', 'pinctrl-[0-9]+'
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mm-beacon-baseboard.dtsi | 8 ++++----
arch/arm64/boot/dts/freescale/imx8mm-beacon-som.dtsi | 12 ++++++------
2 files changed, 10 insertions(+), 10 deletions(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:07:51
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 <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mm-evk.dts | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:07:54
The ROHM BD71847 PMIC has a 32.768 kHz clock. Adding necessary parent
allows to probe the bd718x7 clock driver fixing boot errors:
bd718xx-clk bd71847-clk.1.auto: No parent clk found
bd718xx-clk: probe of bd71847-clk.1.auto failed with error -22
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mm-evk.dts | 4 ++++
1 file changed, 4 insertions(+)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:07:56
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 <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:08:02
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 <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mn-evk.dtsi | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:08:07
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 <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mq-evk.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:08:14
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mq-librem5-devkit.dts | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:08:18
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 <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mq-phanbell.dts | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:08:24
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 <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mq-pico-pi.dts | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:08:28
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 <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8mq-sr-som.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:08:32
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mq-hummingboard-pulse.dts | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-24 19:08:37
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 <krzk@kernel.org>
---
arch/arm64/boot/dts/freescale/imx8qxp-colibri.dtsi | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Vaittinen, Matti <hidden> Date: 2020-08-25 06:23:50
Hello Krzysztof,
On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote:
The input clock and number of clock provider cells are not required
for
the PMIC to operate. They are needed only for the optional bd718x7
clock driver.
I have always found the DT bindings hard to do. I quite often end up
having a different view with Rob so I probably could just shut-up and
watch how this evolves :)
But as keeping my mouth is so difficult...
...All of the drivers are optional. The PMIC can power-on without any
drivers. Drivers are mostly used just for disabling the voltage from
graphics accelerator block when it is not needed (optional). Or some
DVS (optional). But yes, maybe the clk driver is "more optional" than
the rest. XD So, I am not against this.
quoted hunk
Add also clock-output-names as driver takes use of it.
This fixes dtbs_check warnings like:
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
'clocks' is a required property
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
'#clock-cells' is a required property
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../devicetree/bindings/mfd/rohm,bd71847-pmic.yaml | 9
+++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
I had this in original binding (text) document patch series. For some
reason it was later dropped. Unfortunately I didn't easily find a
reason as to why. Adding it back now is absolutely fine for me though.
quoted hunk
+
# The BD71847 abd BD71850 support two different HW states as reset
target
# states. States are called as SNVS and READY. At READY state all
the PMIC
# power outputs go down and OTP is reload. At the SNVS state all
other logic
This is new to me. Please educate me - does this simply mean that if
'#clock-cells' is given, then also the 'clocks' must be given - and the
other way around?
If so, then:
Acked-By: Matti Vaittinen <redacted>
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
K
iviharjunlenkki 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)
On Mon, Aug 24, 2020 at 09:06:47PM +0200, Krzysztof Kozlowski wrote:
quoted hunk
Driver requires different amount of clocks for different SoCs. Describe
these requirements properly to fix dtbs_check warnings like:
arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../devicetree/bindings/mtd/gpmi-nand.yaml | 76 +++++++++++++++----
1 file changed, 61 insertions(+), 15 deletions(-)
This enforces this specific order of the clocks given in the dts. The
clock binding itself doesn't require any specific order, that's what we
have the names array for.
Is this really what we want?
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2020-08-25 06:49:17
On Tue, Aug 25, 2020 at 08:40:20AM +0200, Sascha Hauer wrote:
On Mon, Aug 24, 2020 at 09:06:47PM +0200, Krzysztof Kozlowski wrote:
quoted
Driver requires different amount of clocks for different SoCs. Describe
these requirements properly to fix dtbs_check warnings like:
arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../devicetree/bindings/mtd/gpmi-nand.yaml | 76 +++++++++++++++----
1 file changed, 61 insertions(+), 15 deletions(-)
This enforces this specific order of the clocks given in the dts. The
clock binding itself doesn't require any specific order, that's what we
have the names array for.
Is this really what we want?
Indeed but have in mind that the specific order was there already. This
patch does not address that part, only number of clocks.
Fixing this for any order could be done with patterns. I can work on
that.
Best regards,
Krzysztof
From: Vaittinen, Matti <hidden> Date: 2020-08-25 06:51:41
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:
quoted hunk
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22 +++++++++------
----
1 file changed, 11 insertions(+), 11 deletions(-)
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?
What I see in bd718x7-regulator.c for LDO6 desc is:
/* LDO6 is supplied by buck5 */
.supply_name = "buck5",
So, is this change going to change the supply-chain for the board? Is
this intended? (Or am I mistaken on what is the impact of regulator-
name property?)
Best Regards
Matti Vaittinen
On Tue, Aug 25, 2020 at 06:23:36AM +0000, Vaittinen, Matti wrote:
Hello Krzysztof,
On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote:
quoted
The input clock and number of clock provider cells are not required
for
the PMIC to operate. They are needed only for the optional bd718x7
clock driver.
I have always found the DT bindings hard to do. I quite often end up
having a different view with Rob so I probably could just shut-up and
watch how this evolves :)
But as keeping my mouth is so difficult...
...All of the drivers are optional. The PMIC can power-on without any
drivers. Drivers are mostly used just for disabling the voltage from
graphics accelerator block when it is not needed (optional). Or some
DVS (optional). But yes, maybe the clk driver is "more optional" than
the rest. XD So, I am not against this.
Each regulator node is optional, it can be skipped. And device will
work and regulator driver will bind. The difference here is that without
clocks the clock driver won't even bind... but if we keep clocks as
required, then multiple DTSes do not pass the bindings check.
I don't have strong feelings about dropping requirement for clocks, just
this looks easier to implement and logical to me (this is a PMIC so
clock is a secondary feature).
quoted
Add also clock-output-names as driver takes use of it.
This fixes dtbs_check warnings like:
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
'clocks' is a required property
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
'#clock-cells' is a required property
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../devicetree/bindings/mfd/rohm,bd71847-pmic.yaml | 9
+++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
I had this in original binding (text) document patch series. For some
reason it was later dropped. Unfortunately I didn't easily find a
reason as to why. Adding it back now is absolutely fine for me though.
quoted
+
# The BD71847 abd BD71850 support two different HW states as reset
target
# states. States are called as SNVS and READY. At READY state all
the PMIC
# power outputs go down and OTP is reload. At the SNVS state all
other logic
This is new to me. Please educate me - does this simply mean that if
'#clock-cells' is given, then also the 'clocks' must be given - and the
other way around?
Yes, because the clocks do not have sense without clock-cells and vice versa.
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:
quoted
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22 +++++++++------
----
1 file changed, 11 insertions(+), 11 deletions(-)
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?
What I see in bd718x7-regulator.c for LDO6 desc is:
/* LDO6 is supplied by buck5 */
.supply_name = "buck5",
So, is this change going to change the supply-chain for the board? Is
this intended? (Or am I mistaken on what is the impact of regulator-
name property?)
The names will take regulator names from the driver. The problem is with
matching the of_node.
Dear Rob,
Maybe you have an idea how to fix this driver-binding ABI
incompatibility? Or better just leave it?
Best regards,
Krzysztof
On Tue, Aug 25, 2020 at 09:25:37AM +0200, krzk@kernel.org wrote:
On Tue, Aug 25, 2020 at 06:51:33AM +0000, Vaittinen, Matti wrote:
quoted
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:
quoted
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22 +++++++++------
----
1 file changed, 11 insertions(+), 11 deletions(-)
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.
The driver internally already uses lowercase names.
Everything looks good. I will just double check whether the constraints
did not change on the board after boot.
What I see in bd718x7-regulator.c for LDO6 desc is:
/* LDO6 is supplied by buck5 */
.supply_name = "buck5",
So, is this change going to change the supply-chain for the board? Is
this intended? (Or am I mistaken on what is the impact of regulator-
name property?)
Good point, let me check the supplies.
The names will take regulator names from the driver. The problem is with
matching the of_node.
Dear Rob,
Maybe you have an idea how to fix this driver-binding ABI
incompatibility? Or better just leave it?
Not valid anymore, I just got confused...
Best regards,
Krzysztof
On Tue, Aug 25, 2020 at 09:45:00AM +0200, krzk@kernel.org wrote:
On Tue, Aug 25, 2020 at 09:25:37AM +0200, krzk@kernel.org wrote:
quoted
On Tue, Aug 25, 2020 at 06:51:33AM +0000, Vaittinen, Matti wrote:
quoted
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:
quoted
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22 +++++++++------
----
1 file changed, 11 insertions(+), 11 deletions(-)
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.
The driver internally already uses lowercase names.
Everything looks good. I will just double check whether the constraints
did not change on the board after boot.
What I see in bd718x7-regulator.c for LDO6 desc is:
/* LDO6 is supplied by buck5 */
.supply_name = "buck5",
So, is this change going to change the supply-chain for the board? Is
this intended? (Or am I mistaken on what is the impact of regulator-
name property?)
From: Vaittinen, Matti <hidden> Date: 2020-08-25 08:22:28
Hello Krzysztof,
On Tue, 2020-08-25 at 09:50 +0200, krzk@kernel.org wrote:
On Tue, Aug 25, 2020 at 09:45:00AM +0200, krzk@kernel.org wrote:
quoted
On Tue, Aug 25, 2020 at 09:25:37AM +0200, krzk@kernel.org wrote:
quoted
On Tue, Aug 25, 2020 at 06:51:33AM +0000, Vaittinen, Matti wrote:
quoted
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:
quoted
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22
+++++++++------
----
1 file changed, 11 insertions(+), 11 deletions(-)
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.
quoted
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.
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.
quoted
Everything looks good. I will just double check whether the
constraints
did not change on the board after boot.
What I see in bd718x7-regulator.c for LDO6 desc is:
/* LDO6 is supplied by buck5 */
.supply_name = "buck5",
So, is this change going to change the supply-chain for the
board? Is
this intended? (Or am I mistaken on what is the impact of
regulator-
name property?)
Good point, let me check the supplies.
This patch actually fixes the supplies which before were not working
because of case mismatch.
Before:
regulator use open bypass opmode voltage
current min max
-------------------------------------------------------------------
--------------------
regulator-dummy 4 5 0
unknown 0mV 0mA 0mV 0mV
LDO6 1 0 0
unknown 1200mV 0mA 900mV 1800mV
BUCK1 1 0 0
unknown 850mV 0mA 700mV 1300mV
BUCK2 2 1 0
unknown 1000mV 0mA 700mV 1300mV
cpu0-
cpu 1 0mA 1000m
V 1000mV
BUCK3 1 0 0
unknown 975mV 0mA 700mV 1350mV
BUCK4 1 0 0
unknown 3300mV 0mA 3000mV 3300mV
BUCK5 1 0 0
unknown 1800mV 0mA 1605mV 1995mV
BUCK6 1 0 0
unknown 1200mV 0mA 800mV 1400mV
LDO1 1 0 0
unknown 1800mV 0mA 1600mV 1900mV
LDO2 1 0 0
unknown 800mV 0mA 800mV 900mV
LDO3 1 0 0
unknown 1800mV 0mA 1800mV 3300mV
LDO4 1 0 0
unknown 900mV 0mA 900mV 1800mV
ldo5 1 4 0
unknown 1800mV 0mA 1800mV 1800mV
After:
regulator use open bypass opmode voltage
current min max
-------------------------------------------------------------------
--------------------
buck1 1 0 0
unknown 850mV 0mA 700mV 1300mV
buck2 2 1 0
unknown 850mV 0mA 700mV 1300mV
cpu0-
cpu 1 0mA 850m
V 850mV
buck3 1 0 0
unknown 975mV 0mA 700mV 1350mV
buck4 1 0 0
unknown 3300mV 0mA 3000mV 3300mV
buck5 2 1 0
unknown 1800mV 0mA 1605mV 1995mV
ldo6 1 0 0
That was my point :) Before this commit the system has acted
differently - either by accident or by purpose. In any case, the DTS
change will change supply logic and this should probably be mentioned
in commit log to help bisecting possible issues :)
But as I said, I am not opposed to this change - I am merely somewhat
cautious with changes like this.
Best regards
Matti Vaittinen
On Tue, Aug 25, 2020 at 08:22:18AM +0000, Vaittinen, Matti wrote:
Hello Krzysztof,
On Tue, 2020-08-25 at 09:50 +0200, krzk@kernel.org wrote:
quoted
On Tue, Aug 25, 2020 at 09:45:00AM +0200, krzk@kernel.org wrote:
quoted
On Tue, Aug 25, 2020 at 09:25:37AM +0200, krzk@kernel.org wrote:
quoted
On Tue, Aug 25, 2020 at 06:51:33AM +0000, Vaittinen, Matti wrote:
quoted
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:
quoted
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22
+++++++++------
----
1 file changed, 11 insertions(+), 11 deletions(-)
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.
quoted
quoted
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.
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.
quoted
quoted
Everything looks good. I will just double check whether the
constraints
did not change on the board after boot.
What I see in bd718x7-regulator.c for LDO6 desc is:
/* LDO6 is supplied by buck5 */
.supply_name = "buck5",
So, is this change going to change the supply-chain for the
board? Is
this intended? (Or am I mistaken on what is the impact of
regulator-
name property?)
Good point, let me check the supplies.
This patch actually fixes the supplies which before were not working
because of case mismatch.
Before:
regulator use open bypass opmode voltage
current min max
-------------------------------------------------------------------
--------------------
regulator-dummy 4 5 0
unknown 0mV 0mA 0mV 0mV
LDO6 1 0 0
unknown 1200mV 0mA 900mV 1800mV
BUCK1 1 0 0
unknown 850mV 0mA 700mV 1300mV
BUCK2 2 1 0
unknown 1000mV 0mA 700mV 1300mV
cpu0-
cpu 1 0mA 1000m
V 1000mV
BUCK3 1 0 0
unknown 975mV 0mA 700mV 1350mV
BUCK4 1 0 0
unknown 3300mV 0mA 3000mV 3300mV
BUCK5 1 0 0
unknown 1800mV 0mA 1605mV 1995mV
BUCK6 1 0 0
unknown 1200mV 0mA 800mV 1400mV
LDO1 1 0 0
unknown 1800mV 0mA 1600mV 1900mV
LDO2 1 0 0
unknown 800mV 0mA 800mV 900mV
LDO3 1 0 0
unknown 1800mV 0mA 1800mV 3300mV
LDO4 1 0 0
unknown 900mV 0mA 900mV 1800mV
ldo5 1 4 0
unknown 1800mV 0mA 1800mV 1800mV
After:
regulator use open bypass opmode voltage
current min max
-------------------------------------------------------------------
--------------------
buck1 1 0 0
unknown 850mV 0mA 700mV 1300mV
buck2 2 1 0
unknown 850mV 0mA 700mV 1300mV
cpu0-
cpu 1 0mA 850m
V 850mV
buck3 1 0 0
unknown 975mV 0mA 700mV 1350mV
buck4 1 0 0
unknown 3300mV 0mA 3000mV 3300mV
buck5 2 1 0
unknown 1800mV 0mA 1605mV 1995mV
ldo6 1 0 0
That was my point :) Before this commit the system has acted
differently - either by accident or by purpose. In any case, the DTS
change will change supply logic and this should probably be mentioned
in commit log to help bisecting possible issues :)
But as I said, I am not opposed to this change - I am merely somewhat
cautious with changes like this.
From: Vaittinen, Matti <hidden> Date: 2020-08-25 09:35:40
On Tue, 2020-08-25 at 10:27 +0200, krzk@kernel.org wrote:
On Tue, Aug 25, 2020 at 08:22:18AM +0000, Vaittinen, Matti wrote:
quoted
Hello Krzysztof,
On Tue, 2020-08-25 at 09:50 +0200, krzk@kernel.org wrote:
quoted
On Tue, Aug 25, 2020 at 09:45:00AM +0200, krzk@kernel.org wrote:
quoted
On Tue, Aug 25, 2020 at 09:25:37AM +0200, krzk@kernel.org
wrote:
quoted
On Tue, Aug 25, 2020 at 06:51:33AM +0000, Vaittinen, Matti
wrote:
quoted
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:
quoted
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 <krzk@kernel.org>
---
.../boot/dts/freescale/imx8mn-ddr4-evk.dts | 22
+++++++++------
----
1 file changed, 11 insertions(+), 11 deletions(-)
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.
quoted
quoted
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).
quoted
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)