Avoid error message on rk3328 use

[PATCH] Avoid error message on rk3328 use

Posted by Etienne Buira 1 year, 10 months ago

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(+)

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 {

base-commit: 20cb38a7af88dc40095da7c2c9094da3873fea23
-- 
2.43.0

Re: [PATCH] Avoid error message on rk3328 use

Posted by Rob Herring 1 year, 10 months ago

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.

Re: [PATCH] Avoid error message on rk3328 use

Posted by Krzysztof Kozlowski 1 year, 10 months ago

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

Re: [PATCH] Avoid error message on rk3328 use

Posted by Etienne Buira 1 year, 10 months ago

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

Re: [PATCH] Avoid error message on rk3328 use

Posted by Diederik de Haas 1 year, 10 months ago

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.

Re: [PATCH] Avoid error message on rk3328 use

Posted by Krzysztof Kozlowski 1 year, 10 months ago

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

Re: [PATCH] Avoid error message on rk3328 use

Posted by Etienne Buira 1 year, 10 months ago

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