Re: [PATCH 1/1] pinctrl: imx1: Allow parsing DT without function nodes

["Sébastien Szymanski" <sebastien.szymanski@armadeus.com>]

Hello,

On 5/4/26 7:08 PM, Frank Li wrote:
> The old format to define pinctrl settings for imx in DT has two hierarchy
> levels. The first level are function device nodes. The second level are
> pingroups which contain a property fsl,pins. The original ntention was to
> define all pin functions in a single dtsi file and just reference the
> correct ones in the board files.
> 
> The commit ("5fcdf6a7ed95e pinctrl: imx: Allow parsing DT without function
> nodes") already make moden i.MX chip support flatten layout.
> 
> Make legacy chipes (more than 15 years) support this flatten layout also.
> 
> Fixes: e948cbdc41d6f ("ARM: dts: imx: remove redundant intermediate node in pinmux hierarchy")
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> build test only
> 
> Sébastien Szymanski:
> 	Can you help test it? I am happy i.MX27 still alive.

Sure! Thanks for the patch! With this patch applied on Linux 7.1-rc2 the 
board boots again. I see the following messages, though:

[    0.085139] imx27-pinctrl 10015000.pinmux: Not a valid fsl,pins 
property (gpio)
[    0.085226] imx27-pinctrl 10015000.pinmux: Not a valid fsl,pins 
property (gpio)
[    0.085281] imx27-pinctrl 10015000.pinmux: Not a valid fsl,pins 
property (gpio)
[    0.085327] imx27-pinctrl 10015000.pinmux: Not a valid fsl,pins 
property (gpio)
[    0.085372] imx27-pinctrl 10015000.pinmux: Not a valid fsl,pins 
property (gpio)
[    0.085416] imx27-pinctrl 10015000.pinmux: Not a valid fsl,pins 
property (gpio)
[    0.092693] imx27-pinctrl 10015000.pinmux: initialized IMX pinctrl driver

That's because there is no fsl,pins property in the 6 gpio subnodes.

Tested-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>

I one have comment bellow.

> 
> Frank
> ---
>   drivers/pinctrl/freescale/pinctrl-imx1-core.c | 48 ++++++++++++++++---
>   1 file changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c b/drivers/pinctrl/freescale/pinctrl-imx1-core.c
> index b36c8a1461b7c..bf07e0c64a098 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c
> @@ -540,10 +540,34 @@ static int imx1_pinctrl_parse_functions(struct device_node *np,
>   	return 0;
>   }
>   
> +/*
> + * Check if the DT contains pins in the direct child nodes. This indicates the
> + * newer DT format to store pins. This function returns true if the first found
> + * fsl,pins property is in a child of np. Otherwise false is returned.
> + */
> +static bool imx1_pinctrl_dt_is_flat_functions(struct device_node *np)
> +{
> +	struct device_node *function_np;
> +	struct device_node *pinctrl_np;
> +
> +	for_each_child_of_node(np, function_np) {
> +		if (of_property_read_bool(function_np, "fsl,pins"))

Isn't of_property_present better here...

> +			return true;
> +
> +		for_each_child_of_node(function_np, pinctrl_np) {
> +			if (of_property_read_bool(pinctrl_np, "fsl,pins"))

...and here ?

Regards,

> +				return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>   static int imx1_pinctrl_parse_dt(struct platform_device *pdev,
>   		struct imx1_pinctrl *pctl, struct imx1_pinctrl_soc_info *info)
>   {
>   	struct device_node *np = pdev->dev.of_node;
> +	bool flat_funcs;
>   	int ret;
>   	u32 nfuncs = 0;
>   	u32 ngroups = 0;
> @@ -552,9 +576,15 @@ static int imx1_pinctrl_parse_dt(struct platform_device *pdev,
>   	if (!np)
>   		return -ENODEV;
>   
> -	for_each_child_of_node_scoped(np, child) {
> -		++nfuncs;
> -		ngroups += of_get_child_count(child);
> +	flat_funcs = imx1_pinctrl_dt_is_flat_functions(np);
> +	if (flat_funcs) {
> +		nfuncs = 1;
> +		ngroups = of_get_child_count(np);
> +	} else {
> +		for_each_child_of_node_scoped(np, child) {
> +			++nfuncs;
> +			ngroups += of_get_child_count(child);
> +		}
>   	}
>   
>   	if (!nfuncs) {
> @@ -574,10 +604,14 @@ static int imx1_pinctrl_parse_dt(struct platform_device *pdev,
>   	if (!info->functions || !info->groups)
>   		return -ENOMEM;
>   
> -	for_each_child_of_node_scoped(np, child) {
> -		ret = imx1_pinctrl_parse_functions(child, info, ifunc++);
> -		if (ret == -ENOMEM)
> -			return -ENOMEM;
> +	if (flat_funcs) {
> +		imx1_pinctrl_parse_functions(np, info, 0);
> +	} else {
> +		for_each_child_of_node_scoped(np, child) {
> +			ret = imx1_pinctrl_parse_functions(child, info, ifunc++);
> +			if (ret == -ENOMEM)
> +				return -ENOMEM;
> +		}
>   	}
>   
>   	return 0;


-- 
Sébastien Szymanski, Armadeus Systems
Software engineer