Re: [PATCH v2 3/4] arm64: dts: nuvoton: Add pinctrl support for ma35d1

From: Jacky Huang
Date: Thu Nov 30 2023 - 03:24:04 EST




On 2023/11/30 下午 04:04, Krzysztof Kozlowski wrote:
On 30/11/2023 02:10, Jacky Huang wrote:
Dear Krzysztof,


On 2023/11/29 下午 06:54, Krzysztof Kozlowski wrote:
On 29/11/2023 11:14, Jacky Huang wrote:
Dear Krzysztof,


On 2023/11/29 下午 06:02, Krzysztof Kozlowski wrote:
On 29/11/2023 10:41, Jacky Huang wrote:
Dear Krzysztof,


On 2023/11/29 下午 04:11, Krzysztof Kozlowski wrote:
On 29/11/2023 04:35, Jacky Huang wrote:
Best regards,
Krzysztof

Yes, it did pass the 'dtbs_check'. I guess the tool does not detect such
issues.
Anyway, I will fix it in the next version.
Hm, I see your bindings indeed allow pin-.* and unit addresses, so it is
the binding issue.

The examples you used as reference - xlnx,zynqmp-pinctrl.yaml and
realtek,rtd1315e-pinctrl.yaml - do not mix these as you do.

I don't understand why do you need them yet. I don't see any populate of
children. There are no compatibles, either.

Which part of your driver uses them exactly?

Best regards,
Krzysztof

I will move the 'pcfg_default: pin-default' from dtsi to dts, like this:

&pinctrl {
    pcfg_default: pin-default {
        slew-rate = <0>;
        input-schmitt-disable;
        bias-disable;
        power-source = <1>;
        drive-strength = <17100>;
    };
This solves nothing. It's the same placement.


Best regards,
Krzysztof

OK, it stil be the binding issues.
For "^pin-[a-z0-9]+$", I reference to the "pcfg-[a-z0-9-]+$" of
rockchip,pinctrl.yaml.

My intention is to describe a generic pin configuration, aiming to make
the pin
description more concise. In actual testing, it proves to be effective.
Can you instead respond to my actual questions?

Best regards,
Krzysztof

The the last one item of nuvoton,pins is a phandle, which can refer to
'&pin-default'. The following code of driver pinctrl-ma35.c parse
"nuvoton,pins", including the node reference by phandle. list =
of_get_property(np, "nuvoton,pins", &size); size /= sizeof(*list); if
(!size || size % 4) { dev_err(npctl->dev, "wrong setting!\n"); return
-EINVAL; } grp->npins = size / 4; grp->pins = devm_kzalloc(npctl->dev,
grp->npins * sizeof(*grp->pins), GFP_KERNEL); if (!grp->pins) return
-ENOMEM; pin = grp->settings = devm_kzalloc(npctl->dev, grp->npins *
sizeof(*grp->settings), GFP_KERNEL); if (!grp->settings) return -ENOMEM;
for (i = 0, j = 0; i < size; i += 4, j++) { struct device_node
*np_config; const __be32 *phandle; pin->offset = be32_to_cpu(*list++) *
MA35_MFP_REG_SZ_PER_BANK + MA35_MFP_REG_BASE; pin->shift =
(be32_to_cpu(*list++) * MA35_MFP_BITS_PER_PORT) % 32; pin->muxval =
be32_to_cpu(*list++); phandle = list++; if (!phandle) return -EINVAL;
np_config = of_find_node_by_phandle(be32_to_cpup(phandle)); ret =
pinconf_generic_parse_dt_config(np_config, NULL, &pin->configs,
&pin->nconfigs); if (ret) return ret; grp->pins[j] =
npctl->info->get_pin_num(pin->offset, pin->shift); pin++; } Best
Regards, Jacky Huang
Sorry, I cannot parse it.

I was referring to the children with unit addresses. I don't see any
populate of the children, so why do you need them?

There are no compatibles, either.

Which part of your driver uses them exactly?

Best regards,
Krzysztof

So, I should update the binding from "^pin-[a-z0-9]+$" to something like
"-pincfg$".
Just remove the unit address part, and it will become:

    default-pincfg {
        slew-rate = <0>;
        input-schmitt-disable;
        bias-disable;
        power-source = <1>;
        drive-strength = <17100>;
    };

No, it solves nothing. Instead of pasting more code, can you answer my
questions?

Best regards,
Krzysztof


Yes, this node is invalid.
I will drop this node and update the yaml.


Best Regards,
Jacky Huang