[PATCH 0/3] pinctrl: imx-scmi: Introdue nxp,iomuxc-daisy-off

[PATCH 0/3] pinctrl: imx-scmi: Introdue nxp,iomuxc-daisy-off

[Peng Fan (OSS)]

It might be a bit late, since it is 6.15-RC6 now. No rush, the target is 6.17.

i.MX9 SoC family IOMUXC features Daisy chain(multi pads driving same module
input pin), each SoC has its own daisy register offset. When add a new SoC
support, need to hardcode the register offset in pinctrl-imx-scmi.c just as
"
if (of_machine_is_compatible("fsl,imx95"))
   daisy_off = IMX95_DAISY_OFF;
else if
   ...
else
   ...
"

This is no good to long term maintainence.

With nxp,iomuxc-daisy-off introduced, things will be simplified to set
daisy register offset:
"device_property_read_u32(dev, "nxp,iomuxc-daisy-off", &pmx->daisy_off);"

The new property is set as required, so there might be dtbs_check error
without patch 3 applied. This is expected.

Patchset based on next-20250508

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (3):
      dt-bindings: firmware: nxp,imx95-scmi-pinctrl: Introduce nxp,iomuxc-daisy-off
      pinctrl: imx-scmi: Get daisy register offset from DT
      arm64: dts: imx95: Add property nxp,iomuxc-daisy-off

 .../bindings/firmware/nxp,imx95-scmi-pinctrl.yaml  |  8 +++++++
 arch/arm64/boot/dts/freescale/imx95.dtsi           |  1 +
 drivers/pinctrl/freescale/pinctrl-imx-scmi.c       | 26 +++++++++++++---------
 3 files changed, 24 insertions(+), 11 deletions(-)
---
base-commit: f48887a98b78880b7711aca311fbbbcaad6c4e3b
change-id: 20250509-pin-e5b563bbd55d

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH 1/3] dt-bindings: firmware: nxp,imx95-scmi-pinctrl: Introduce nxp,iomuxc-daisy-off

[Peng Fan (OSS)]

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:
+    $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

Re: [PATCH 1/3] dt-bindings: firmware: nxp,imx95-scmi-pinctrl: Introduce nxp,iomuxc-daisy-off

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

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.

> +    $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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: firmware: nxp,imx95-scmi-pinctrl: Introduce nxp,iomuxc-daisy-off

[Peng Fan]

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
>> 

Re: [PATCH 1/3] dt-bindings: firmware: nxp,imx95-scmi-pinctrl: Introduce nxp,iomuxc-daisy-off

[Linus Walleij]

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

Re: [PATCH 1/3] dt-bindings: firmware: nxp,imx95-scmi-pinctrl: Introduce nxp,iomuxc-daisy-off

[Peng Fan]

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

[PATCH 2/3] pinctrl: imx-scmi: Get daisy register offset from DT

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Parsing the "nxp,iomuxc-daisy-off" to get the daisy register offset,
then no need to hardcode the register offset in driver for new SoCs.

To keep backwards comatibility, still keep the register offset for i.MX95.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
index 8f15c4c4dc4412dddb40505699fc3f459fdc0adc..7c15244371c20e0d99b1802e480c39aaeeec495b 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
@@ -35,6 +35,7 @@ struct scmi_pinctrl_imx {
 	struct pinctrl_dev *pctldev;
 	struct pinctrl_desc pctl_desc;
 	const struct scmi_pinctrl_proto_ops *ops;
+	uint32_t daisy_off;
 };
 
 /* SCMI pin control types, aligned with SCMI firmware */
@@ -57,6 +58,7 @@ static int pinctrl_scmi_imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 					   struct pinctrl_map **map,
 					   unsigned int *num_maps)
 {
+	struct scmi_pinctrl_imx *pmx = pinctrl_dev_get_drvdata(pctldev);
 	struct pinctrl_map *new_map;
 	const __be32 *list;
 	unsigned long *configs = NULL;
@@ -65,16 +67,6 @@ static int pinctrl_scmi_imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 	int mux_reg, conf_reg, input_reg, mux_val, conf_val, input_val;
 	int i, j;
 	uint32_t ncfg;
-	static uint32_t daisy_off;
-
-	if (!daisy_off) {
-		if (of_machine_is_compatible("fsl,imx95")) {
-			daisy_off = IMX95_DAISY_OFF;
-		} else {
-			dev_err(pctldev->dev, "platform not support scmi pinctrl\n");
-			return -EINVAL;
-		}
-	}
 
 	list = of_get_property(np, "fsl,pins", &size);
 	if (!list) {
@@ -126,7 +118,7 @@ static int pinctrl_scmi_imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 			ncfg -= 2;
 		} else {
 			cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_DAISY_ID,
-							    (input_reg - daisy_off) / 4);
+							    (input_reg - pmx->daisy_off) / 4);
 			cfg[j++] = pinconf_to_config_packed(IMX_SCMI_PIN_DAISY_CFG, input_val);
 		}
 
@@ -315,6 +307,18 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
 	if (!pmx)
 		return -ENOMEM;
 
+	ret = device_property_read_u32(dev, "nxp,iomuxc-daisy-off", &pmx->daisy_off);
+	if (ret) {
+		/*
+		 * To keep backwards compatible, new chips should specify
+		 * nxp,iomuxc-daisy_off
+		 */
+		if (of_machine_is_compatible("fsl,imx95"))
+			pmx->daisy_off = IMX95_DAISY_OFF;
+		else
+			return dev_err_probe(dev, ret, "Failed to get daisy off value\n");
+	}
+
 	pmx->ph = ph;
 	pmx->ops = pinctrl_ops;
 

-- 
2.37.1

Re: [PATCH 2/3] pinctrl: imx-scmi: Get daisy register offset from DT

[Sascha Hauer]

On Mon, May 12, 2025 at 10:14:15AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Parsing the "nxp,iomuxc-daisy-off" to get the daisy register offset,
> then no need to hardcode the register offset in driver for new SoCs.
> 
> To keep backwards comatibility, still keep the register offset for i.MX95.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> @@ -315,6 +307,18 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
>  	if (!pmx)
>  		return -ENOMEM;
>  
> +	ret = device_property_read_u32(dev, "nxp,iomuxc-daisy-off", &pmx->daisy_off);

"off" is really a poor acronym for "offset" as it unnecessarily confuses
the reader by suggesting this is about turning some feature off. Please
add the three bytes and just call it offset.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] pinctrl: imx-scmi: Get daisy register offset from DT

[Peng Fan]

On Mon, May 12, 2025 at 08:17:42AM +0200, Sascha Hauer wrote:
>On Mon, May 12, 2025 at 10:14:15AM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
>> Parsing the "nxp,iomuxc-daisy-off" to get the daisy register offset,
>> then no need to hardcode the register offset in driver for new SoCs.
>> 
>> To keep backwards comatibility, still keep the register offset for i.MX95.
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 26 +++++++++++++++-----------
>>  1 file changed, 15 insertions(+), 11 deletions(-)
>> 
>> @@ -315,6 +307,18 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
>>  	if (!pmx)
>>  		return -ENOMEM;
>>  
>> +	ret = device_property_read_u32(dev, "nxp,iomuxc-daisy-off", &pmx->daisy_off);
>
>"off" is really a poor acronym for "offset" as it unnecessarily confuses
>the reader by suggesting this is about turning some feature off. Please
>add the three bytes and just call it offset.

I will use "nxp,iomuxc-daisy-offset" in V2.

Thanks,
Peng

>
>Sascha
>
>-- 
>Pengutronix e.K.                           |                             |
>Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/3] arm64: dts: imx95: Add property nxp,iomuxc-daisy-off

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

i.MX95 IOMUXC Daisy register offset is at IOMUXC offset 0x408, set it.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx95.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
index 632631a2911224cadc16a943cdb467e091e43384..9b60bbe3b5c679331d38c86e2aca4b0b9d4340c8 100644
--- a/arch/arm64/boot/dts/freescale/imx95.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
@@ -348,6 +348,7 @@ scmi_sensor: protocol@15 {
 
 			scmi_iomuxc: protocol@19 {
 				reg = <0x19>;
+				nxp,iomuxc-daisy-off = <0x408>;
 			};
 
 			scmi_bbm: protocol@81 {

-- 
2.37.1