diff mbox series

Avoid error message on rk3328 use

Message ID ZhqO-DEmh-6TeHrt@Z926fQmE5jqhFMgp6 (mailing list archive)
State New, archived
Headers show
Series Avoid error message on rk3328 use | expand

Commit Message

Etienne Buira April 13, 2024, 1:56 p.m. UTC
rockchip,rk3328-grf-gpio is handled as syscon, but syscon mandates
presence of gpio,syscon-dev node (or it will call dev_err() when probed).
Correct rk3328.dtsi and related documentation to follow syscon's
expectations.

Signed-off-by: Etienne Buira <etienne.buira@free.fr>
---
 .../devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml      | 2 ++
 arch/arm64/boot/dts/rockchip/rk3328.dtsi                        | 1 +
 2 files changed, 3 insertions(+)


base-commit: 20cb38a7af88dc40095da7c2c9094da3873fea23

Comments

Krzysztof Kozlowski April 13, 2024, 3:09 p.m. UTC | #1
On 13/04/2024 15:56, Etienne Buira wrote:
> rockchip,rk3328-grf-gpio is handled as syscon, but syscon mandates

syscon does not need such property. I see it in gpio-syscon, but not in
syscon.

> presence of gpio,syscon-dev node (or it will call dev_err() when probed).
> Correct rk3328.dtsi and related documentation to follow syscon's
> expectations.

No, look at gpio-syscon driver. Parent is used.


> 
> Signed-off-by: Etienne Buira <etienne.buira@free.fr>
> ---
>  .../devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml      | 2 ++

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

>  arch/arm64/boot/dts/rockchip/rk3328.dtsi                        | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
> index d8cce73ea0ae..2c878e7db900 100644
> --- a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
> +++ b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
> @@ -38,6 +38,7 @@ required:
>    - compatible
>    - gpio-controller
>    - "#gpio-cells"
> +  - gpio,syscon-dev

No, not needed. And also incomplete - where is the property defined?

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.



Best regards,
Krzysztof
Rob Herring (Arm) April 13, 2024, 3:20 p.m. UTC | #2
On Sat, 13 Apr 2024 15:56:08 +0200, Etienne Buira wrote:
> rockchip,rk3328-grf-gpio is handled as syscon, but syscon mandates
> presence of gpio,syscon-dev node (or it will call dev_err() when probed).
> Correct rk3328.dtsi and related documentation to follow syscon's
> expectations.
> 
> Signed-off-by: Etienne Buira <etienne.buira@free.fr>
> ---
>  .../devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml      | 2 ++
>  arch/arm64/boot/dts/rockchip/rk3328.dtsi                        | 1 +
>  2 files changed, 3 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.example.dtb: gpio: 'gpio,syscon-dev' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/gpio/rockchip,rk3328-grf-gpio.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/ZhqO-DEmh-6TeHrt@Z926fQmE5jqhFMgp6

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Etienne Buira April 13, 2024, 3:23 p.m. UTC | #3
Hi Krzysztof,

On Sat, Apr 13, 2024 at 05:09:33PM +0200, Krzysztof Kozlowski wrote:
> On 13/04/2024 15:56, Etienne Buira wrote:
> > rockchip,rk3328-grf-gpio is handled as syscon, but syscon mandates
> 
> syscon does not need such property. I see it in gpio-syscon, but not in
> syscon.

gpio-syscon, indeed.

> > presence of gpio,syscon-dev node (or it will call dev_err() when probed).
> > Correct rk3328.dtsi and related documentation to follow syscon's
> > expectations.
> 
> No, look at gpio-syscon driver. Parent is used.

Parent is used, but the next lines are:
ret = of_property_read_u32_index(np, "gpio,syscon-dev", 1, &priv->dreg_offset);
if (ret)
	dev_err(...)

So if gpio,syscon-dev does not have at least 2 items (or is missing),
dev_err will be called, 3 items for dev_dbg.
Current tree displays a spurious "can't read the data register offset"
message.

> > 
> > Signed-off-by: Etienne Buira <etienne.buira@free.fr>
> > ---
> >  .../devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml      | 2 ++
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Then please
> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code
> here looks like it needs a fix. Feel free to get in touch if the warning
> is not clear.

Will do, if we agree on the interest of patch.

> >  arch/arm64/boot/dts/rockchip/rk3328.dtsi                        | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
> > index d8cce73ea0ae..2c878e7db900 100644
> > --- a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
> > +++ b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
> > @@ -38,6 +38,7 @@ required:
> >    - compatible
> >    - gpio-controller
> >    - "#gpio-cells"
> > +  - gpio,syscon-dev
> 
> No, not needed. And also incomplete - where is the property defined?
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.

ditto

Regards
Krzysztof Kozlowski April 13, 2024, 3:49 p.m. UTC | #4
On 13/04/2024 17:23, Etienne Buira wrote:
>>> presence of gpio,syscon-dev node (or it will call dev_err() when probed).
>>> Correct rk3328.dtsi and related documentation to follow syscon's
>>> expectations.
>>
>> No, look at gpio-syscon driver. Parent is used.
> 
> Parent is used, but the next lines are:
> ret = of_property_read_u32_index(np, "gpio,syscon-dev", 1, &priv->dreg_offset);
> if (ret)
> 	dev_err(...)
> 
> So if gpio,syscon-dev does not have at least 2 items (or is missing),
> dev_err will be called, 3 items for dev_dbg.
> Current tree displays a spurious "can't read the data register offset"
> message.

Hm, indeed, then I think driver, so
aa1fdda8f7ebf83f678e8d3c2ab4f1638c94195f, should be fixed. Otherwise
please say why binding is not correct and driver is good.


Best regards,
Krzysztof
Etienne Buira April 13, 2024, 4:11 p.m. UTC | #5
On Sat, Apr 13, 2024 at 05:49:13PM +0200, Krzysztof Kozlowski wrote:
> On 13/04/2024 17:23, Etienne Buira wrote:
> >>> presence of gpio,syscon-dev node (or it will call dev_err() when probed).
> >>> Correct rk3328.dtsi and related documentation to follow syscon's
> >>> expectations.
> >>
> >> No, look at gpio-syscon driver. Parent is used.
> > 
> > Parent is used, but the next lines are:
> > ret = of_property_read_u32_index(np, "gpio,syscon-dev", 1, &priv->dreg_offset);
> > if (ret)
> > 	dev_err(...)
> > 
> > So if gpio,syscon-dev does not have at least 2 items (or is missing),
> > dev_err will be called, 3 items for dev_dbg.
> > Current tree displays a spurious "can't read the data register offset"
> > message.
> 
> Hm, indeed, then I think driver, so
> aa1fdda8f7ebf83f678e8d3c2ab4f1638c94195f, should be fixed. Otherwise
> please say why binding is not correct and driver is good.

I tried fixing the driver first, this was discussed here:
https://lore.kernel.org/linux-gpio/ZhptEWb7tD5pummq@Z926fQmE5jqhFMgp6/T/#t
you're welcome to comment.

I have no strong opinion over what should be fixed, i just wished to
shut a spurious error message, that i expected to be straightforward at
first (hence good candidate for first kernel patch).

Regards
Diederik de Haas April 13, 2024, 4:25 p.m. UTC | #6
On Saturday, 13 April 2024 17:23:50 CEST Etienne Buira wrote:
> Current tree displays a spurious "can't read the data register offset"
> message.

Sounds useful to mention that (specific) error message as that did rang a bell.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
index d8cce73ea0ae..2c878e7db900 100644
--- a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml
@@ -38,6 +38,7 @@  required:
   - compatible
   - gpio-controller
   - "#gpio-cells"
+  - gpio,syscon-dev
 
 additionalProperties: false
 
@@ -47,4 +48,5 @@  examples:
       compatible = "rockchip,rk3328-grf-gpio";
       gpio-controller;
       #gpio-cells = <2>;
+      gpio,syscon-dev = <&grf 0 0>;
     };
diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index b6f045069ee2..fd25d5bee19f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -296,6 +296,7 @@  grf_gpio: gpio {
 			compatible = "rockchip,rk3328-grf-gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			gpio,syscon-dev = <&grf 0 0>;
 		};
 
 		power: power-controller {