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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
> ---
> .../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.
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 <[email protected]>
> > ---
> > .../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
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
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
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.