| Message ID | 20240618165102.2380159-1-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings: pinctrl: pinctrl-single: Define a max count for "pinctrl-single,gpio-range" | expand |
On Tue, 18 Jun 2024 11:51:02 -0500, Nishanth Menon wrote: > "pinctrl-single,gpio-range" allows us to define a dis-contiguous > range of pinctrl registers that can have different mux settings for > GPIO mode of operation. However, the maxItems seem to be set to 1 in > processed schema for some reason. This is incorrect. For example: > arch/arm64/boot/dts/hisilicon/hi6220.dtsi and others have more than > one dis-contiguous range. > > Arbitrarily define a max 100 count to override the defaults. > > Signed-off-by: Nishanth Menon <nm@ti.com> > --- > I am not sure if I should call this RFC or not.. and if this is even the > right solution.. I am on 2024.05 dt-schema for this check. > > I noticed this when adding gpio-ranges for am62p platform: > https://gist.github.com/nmenon/7019cd2f24be47997640df5db60a7544 > > It is possible that this is a bug in dt-schema, but I have'nt been able > to track it down either. > > behavior seen is the following: > pinctrl-single,gpio-range = <&mcu_pmx_range 0 21 7>; > generates no warning > However, > pinctrl-single,gpio-range = <&mcu_pmx_range 0 21 7>, <&mcu_pmx_range 32 2 7>; > > generates "is too long" warning. > > > Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml | 1 + > 1 file changed, 1 insertion(+) > 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/pinctrl/pinctrl-single.yaml: properties:pinctrl-single,gpio-range: {'description': 'Optional list of pin base, nr pins & gpio function', '$ref': '/schemas/types.yaml#/definitions/phandle-array', 'maxItems': 100, 'items': [{'items': [{'description': 'phandle of a gpio-range node'}, {'description': 'pin base'}, {'description': 'number of pins'}, {'description': 'gpio function'}]}]} should not be valid under {'required': ['maxItems']} hint: "maxItems" is not needed with an "items" list from schema $id: http://devicetree.org/meta-schemas/items.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240618165102.2380159-1-nm@ti.com 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.
On 12:34-20240618, Rob Herring wrote: > > On Tue, 18 Jun 2024 11:51:02 -0500, Nishanth Menon wrote: > > "pinctrl-single,gpio-range" allows us to define a dis-contiguous > > range of pinctrl registers that can have different mux settings for > > GPIO mode of operation. However, the maxItems seem to be set to 1 in > > processed schema for some reason. This is incorrect. For example: > > arch/arm64/boot/dts/hisilicon/hi6220.dtsi and others have more than > > one dis-contiguous range. > > > > Arbitrarily define a max 100 count to override the defaults. > > > > Signed-off-by: Nishanth Menon <nm@ti.com> > > --- > > I am not sure if I should call this RFC or not.. and if this is even the > > right solution.. I am on 2024.05 dt-schema for this check. > > > > I noticed this when adding gpio-ranges for am62p platform: > > https://gist.github.com/nmenon/7019cd2f24be47997640df5db60a7544 > > > > It is possible that this is a bug in dt-schema, but I have'nt been able > > to track it down either. > > > > behavior seen is the following: > > pinctrl-single,gpio-range = <&mcu_pmx_range 0 21 7>; > > generates no warning > > However, > > pinctrl-single,gpio-range = <&mcu_pmx_range 0 21 7>, <&mcu_pmx_range 32 2 7>; > > > > generates "is too long" warning. > > > > > > Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml | 1 + > > 1 file changed, 1 insertion(+) > > > > 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/pinctrl/pinctrl-single.yaml: properties:pinctrl-single,gpio-range: {'description': 'Optional list of pin base, nr pins & gpio function', '$ref': '/schemas/types.yaml#/definitions/phandle-array', 'maxItems': 100, 'items': [{'items': [{'description': 'phandle of a gpio-range node'}, {'description': 'pin base'}, {'description': 'number of pins'}, {'description': 'gpio function'}]}]} should not be valid under {'required': ['maxItems']} > hint: "maxItems" is not needed with an "items" list > from schema $id: http://devicetree.org/meta-schemas/items.yaml# yes, I had expected the same, but processed schema indicates a maxItems of 1 for reasons I am unable to make sense of.. will be great to have some additional eyes: https://gist.github.com/nmenon/7019cd2f24be47997640df5db60a7544#file-processed-schema-pinctrl next-20240617 baseline
On 6/18/24 1:57 PM, Nishanth Menon wrote: > On 12:34-20240618, Rob Herring wrote: >> >> On Tue, 18 Jun 2024 11:51:02 -0500, Nishanth Menon wrote: >>> "pinctrl-single,gpio-range" allows us to define a dis-contiguous >>> range of pinctrl registers that can have different mux settings for >>> GPIO mode of operation. However, the maxItems seem to be set to 1 in >>> processed schema for some reason. This is incorrect. For example: >>> arch/arm64/boot/dts/hisilicon/hi6220.dtsi and others have more than >>> one dis-contiguous range. >>> >>> Arbitrarily define a max 100 count to override the defaults. >>> >>> Signed-off-by: Nishanth Menon <nm@ti.com> >>> --- >>> I am not sure if I should call this RFC or not.. and if this is even the >>> right solution.. I am on 2024.05 dt-schema for this check. >>> >>> I noticed this when adding gpio-ranges for am62p platform: >>> https://gist.github.com/nmenon/7019cd2f24be47997640df5db60a7544 >>> >>> It is possible that this is a bug in dt-schema, but I have'nt been able >>> to track it down either. >>> >>> behavior seen is the following: >>> pinctrl-single,gpio-range = <&mcu_pmx_range 0 21 7>; >>> generates no warning >>> However, >>> pinctrl-single,gpio-range = <&mcu_pmx_range 0 21 7>, <&mcu_pmx_range 32 2 7>; >>> >>> generates "is too long" warning. >>> >>> >>> Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml | 1 + >>> 1 file changed, 1 insertion(+) >>> >> >> 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/pinctrl/pinctrl-single.yaml: properties:pinctrl-single,gpio-range: {'description': 'Optional list of pin base, nr pins & gpio function', '$ref': '/schemas/types.yaml#/definitions/phandle-array', 'maxItems': 100, 'items': [{'items': [{'description': 'phandle of a gpio-range node'}, {'description': 'pin base'}, {'description': 'number of pins'}, {'description': 'gpio function'}]}]} should not be valid under {'required': ['maxItems']} >> hint: "maxItems" is not needed with an "items" list >> from schema $id: http://devicetree.org/meta-schemas/items.yaml# > > yes, I had expected the same, but processed schema indicates a maxItems > of 1 for reasons I am unable to make sense of.. will be great to have > some additional eyes: > As the warning says, setting the item count is not needed when the item list content (and therefore item count) is explicitly defined. The binding is saying there is a single item of 4 elements. "pinctrl-single,gpio-range"'s type is a single phandle-array[0]. What you want is an array of phandle-arrays. The length of each is usually set by the target of the phandle with its #*-cells property. This binding is a bit of a mess, the phandle is always a pointer to a node with the cells length hard-coded to 3. This looks to have been done to allow the driver to use the function "of_parse_phandle_with_args" which needs a property name for to find the cell count. But that makes no sense as the count is always 3, the driver cannot accept any other value. The driver should have just looped of_get_property() 3 times but wanted to use the helper. So a silly driver mistake has turned into a binding issue. We should drop the "pinctrl-single,gpio-range" from the binding and fix the driver. Andrew [0] https://patchwork.kernel.org/project/netdevbpf/patch/20220119015038.2433585-1-robh@kernel.org/ > https://gist.github.com/nmenon/7019cd2f24be47997640df5db60a7544#file-processed-schema-pinctrl > > next-20240617 baseline
On 11:19-20240619, Andrew Davis wrote: [...] > > This binding is a bit of a mess, the phandle is always a pointer to > a node with the cells length hard-coded to 3. This looks to have been done > to allow the driver to use the function "of_parse_phandle_with_args" which > needs a property name for to find the cell count. But that makes no sense > as the count is always 3, the driver cannot accept any other value. The > driver should have just looped of_get_property() 3 times but wanted to > use the helper. So a silly driver mistake has turned into a binding issue. > > We should drop the "pinctrl-single,gpio-range" from the binding and > fix the driver. Linus W: pinctrl-single,gpio-range -> any thoughts here? I think it is a valid (if a bit too flexible design looking at the existing users who just use a single mux value mapping for all modes)
On 6/21/24 8:36 AM, Nishanth Menon wrote: > On 11:19-20240619, Andrew Davis wrote: > [...] > >> >> This binding is a bit of a mess, the phandle is always a pointer to >> a node with the cells length hard-coded to 3. This looks to have been done >> to allow the driver to use the function "of_parse_phandle_with_args" which >> needs a property name for to find the cell count. But that makes no sense >> as the count is always 3, the driver cannot accept any other value. The >> driver should have just looped of_get_property() 3 times but wanted to >> use the helper. So a silly driver mistake has turned into a binding issue. >> >> We should drop the "pinctrl-single,gpio-range" from the binding and >> fix the driver. My bad, I meant to say drop "#pinctrl-single,gpio-range-cells" and drop the phandle from "pinctrl-single,gpio-range". The #-cells is meant for a producer to define args for a consumer phandle. This is so common in DT there is a helper function for it. The problem is the author of the driver here wanted to use that helper, even though it would be backwards in this case (pinmux is the consumer). So we have a node inside the pinmux node that exists only to expose a fixed number (3) of cells so that a different node inside the same pinmux node could point to itself to make a helper function happy (that didn't need to be used in the first place).. Andrew > > Linus W: pinctrl-single,gpio-range -> any thoughts here? I think it is a > valid (if a bit too flexible design looking at the existing users who > just use a single mux value mapping for all modes) >
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml index c11495524dd2..416a70db14af 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml @@ -74,6 +74,7 @@ properties: pinctrl-single,gpio-range: description: Optional list of pin base, nr pins & gpio function $ref: /schemas/types.yaml#/definitions/phandle-array + maxItems: 100 items: - items: - description: phandle of a gpio-range node
"pinctrl-single,gpio-range" allows us to define a dis-contiguous range of pinctrl registers that can have different mux settings for GPIO mode of operation. However, the maxItems seem to be set to 1 in processed schema for some reason. This is incorrect. For example: arch/arm64/boot/dts/hisilicon/hi6220.dtsi and others have more than one dis-contiguous range. Arbitrarily define a max 100 count to override the defaults. Signed-off-by: Nishanth Menon <nm@ti.com> --- I am not sure if I should call this RFC or not.. and if this is even the right solution.. I am on 2024.05 dt-schema for this check. I noticed this when adding gpio-ranges for am62p platform: https://gist.github.com/nmenon/7019cd2f24be47997640df5db60a7544 It is possible that this is a bug in dt-schema, but I have'nt been able to track it down either. behavior seen is the following: pinctrl-single,gpio-range = <&mcu_pmx_range 0 21 7>; generates no warning However, pinctrl-single,gpio-range = <&mcu_pmx_range 0 21 7>, <&mcu_pmx_range 32 2 7>; generates "is too long" warning. Documentation/devicetree/bindings/pinctrl/pinctrl-single.yaml | 1 + 1 file changed, 1 insertion(+) base-commit: 76db4c64526c5e8ba0f56ad3d890dce8f9b00bbc