| Message ID | 20250512-pin-v1-1-d9f1555a55ad@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: imx-scmi: Introdue nxp,iomuxc-daisy-off | expand |
Hi Conor, On Mon, May 12, 2025 at 05:20:17PM +0100, Conor Dooley wrote: >On Mon, May 12, 2025 at 10:14:14AM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> The IOMUX Controller in i.MX9 family has Daisy chain that multi pads drive >> same module input pin. Each SoC has its own register offset, so >> introduce "nxp,iomuxc-daisy-off" property to specify the daisy register >> offset. With this property being parsed by driver, there is no need >> to hardcode the offset in pinctrl driver for each new SoC. >> >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> .../devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml b/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml >> index a96fc6cce502c10ab415e0b26bff1be8c3bc82f5..b5b2a9c8688a7f6525cdb6a32db22681f4f1a0b9 100644 >> --- a/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml >> +++ b/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml >> @@ -13,6 +13,11 @@ maintainers: >> allOf: >> - $ref: /schemas/pinctrl/pinctrl.yaml >> >> +properties: >> + nxp,iomuxc-daisy-off: > >Same comment here as was left on the driver. >I also don't get why there's a property being introduced from something >you can determine based on the soc. we are targeting a common pinctrl driver for i.MX SCMI based SoC. So that means pinctrl-imx-scmi.c needs support i.MX95, i.MX94 and i.MX9[X]. Each time we support a new SoC, we need to hardcode the register offset in the driver. But if using DT here, no need to update the pinctrl driver anymore when supporting a new i.MX SoC. Thanks, Peng > >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Specify the IOMUX Controller first Daisy register's offset >> + >> patternProperties: >> 'grp$': >> type: object >> @@ -51,3 +56,6 @@ patternProperties: >> - fsl,pins >> >> additionalProperties: true >> + >> +required: >> + - nxp,iomuxc-daisy-off >> >> -- >> 2.37.1 >>
On Tue, May 13, 2025 at 8:46 AM Peng Fan <peng.fan@oss.nxp.com> wrote: > >Same comment here as was left on the driver. > >I also don't get why there's a property being introduced from something > >you can determine based on the soc. I agree with Conor's observation. > we are targeting a common pinctrl driver for i.MX SCMI based SoC. > So that means pinctrl-imx-scmi.c needs support i.MX95, i.MX94 and i.MX9[X]. > > Each time we support a new SoC, we need to hardcode the register offset in > the driver. But if using DT here, no need to update the pinctrl driver anymore > when supporting a new i.MX SoC. I understand that it is convenient, but that doesn't mean it is the right thing to do. I would advice you to keep this in the driver and use the SoC compatible to determine the offset, just as is done today. If information can be deduced from what is already present in the device tree it is redundant to add stuff like this, and it inevitably will create copy-paste errors where the wrong offset is used with the wrong SoC. Yours, Linus Walleij
On Tue, May 13, 2025 at 03:20:44PM +0200, Linus Walleij wrote: >On Tue, May 13, 2025 at 8:46 AM Peng Fan <peng.fan@oss.nxp.com> wrote: > >> >Same comment here as was left on the driver. >> >I also don't get why there's a property being introduced from something >> >you can determine based on the soc. > >I agree with Conor's observation. > >> we are targeting a common pinctrl driver for i.MX SCMI based SoC. >> So that means pinctrl-imx-scmi.c needs support i.MX95, i.MX94 and i.MX9[X]. >> >> Each time we support a new SoC, we need to hardcode the register offset in >> the driver. But if using DT here, no need to update the pinctrl driver anymore >> when supporting a new i.MX SoC. > >I understand that it is convenient, but that doesn't mean it is the right >thing to do. > >I would advice you to keep this in the driver and use the SoC compatible >to determine the offset, just as is done today. > >If information can be deduced from what is already present in the >device tree it is redundant to add stuff like this, and it inevitably >will create copy-paste errors where the wrong offset is used >with the wrong SoC. Got it. Drop this patchset. Thanks, Peng > >Yours, >Linus Walleij
diff --git a/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml b/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml index a96fc6cce502c10ab415e0b26bff1be8c3bc82f5..b5b2a9c8688a7f6525cdb6a32db22681f4f1a0b9 100644 --- a/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml +++ b/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml @@ -13,6 +13,11 @@ maintainers: allOf: - $ref: /schemas/pinctrl/pinctrl.yaml +properties: + nxp,iomuxc-daisy-off: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Specify the IOMUX Controller first Daisy register's offset + patternProperties: 'grp$': type: object @@ -51,3 +56,6 @@ patternProperties: - fsl,pins additionalProperties: true + +required: + - nxp,iomuxc-daisy-off