Re: [PATCH] pinctrl: mediatek: paris: Revert "Rework support for PIN_CONFIG_{INPUT,OUTPUT}_ENABLE"

Chen-Yu Tsai <wenst@xxxxxxxxxxxx> · Thu, 17 Oct 2024 16:58:39 +0800

On Thu, Oct 17, 2024 at 3:57 PM Bo Ye <bo.ye@xxxxxxxxxxxx> wrote:
>
> [This reverts commit c5d3b64c568a344e998830e0e94a7c04e372f89b.]
>
> For MTK HW,
> 1. to enable GPIO input direction: set DIR=0, IES=1
> 2. to enable GPIO output direction: set DIR=1, and set DO=1 to output high, set DO=0 to out low
>
> The PIN_CONFIG_INPUT/PIN_CONFIG_OUTPUT/PIN_CONFIG_INPUT_ENABLE/PIN_CONFIG_OUTPUT_ENABLE shall

Of note, there is no "PIN_CONFIG_INPUT" option.

> be implemented according to view of its purpose - set GPIO direction and output value (for
> output only) according to specific HW design.

I disagree. The PIN_CONFIG_* options have proper meanings defined in

    include/linux/pinctrl/pinconf-generic.h

that every driver implementing the generic pin config API should follow.

For PIN_CONFIG_INPUT_ENABLE:  enable the pin's input.  Note that this
does not affect the pin's ability to drive output.

Note the latter sentence. When you say set the direction of the GPIO
function, it clearly affects the pin's ability to drive output.
Similar argument for PIN_CONFIG_OUTPUT_ENABLE.

My understanding is that PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT_ENABLE
are _not_ for setting GPIO function directions. Another angle to think
about it from: if the pin _was not_ muxed to the GPIO function, this
kind of does nothing in terms of the function of the pin. It still
continues passing signals for whatever function it is currently muxed
to.

> However, the reverted patch implement according to author's own explanation of IES without
> understanding of MTK's HW. Such patch does not correctly set DIR/IES bit to control GPIO
> direction on MTK's HW.

Can you provide a correct and detailed explanation then?

MediaTek's datasheet says _nothing_ about the hardware implementation
details or how it's supposed to be configured. There's just a diagram
showing the hardware, which seems to be a closer match to type A as
described in "GPIO mode pitfalls" from Documentation/driver-api/pinctl.rst

Neither does the upstream binding say anything about using the generic
pin config properties with MediaTek pinctrl devices. The original commit
adding this driver mentions "add pinctrl-paris that implements the vendor
dt-bindings", vendor bindings that don't seem to say anything, at least
not the upstream ones.

The generic pin config binding however *does* clearly describe what
each of these properties mean. See

    Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml

For example:

    input-enable:
      type: boolean
      description: enable input on pin (no effect on output, such as
        enabling an input buffer)

And

    output-low:
      type: boolean
      description: set the pin to output mode with low level

If the hardware has some quirks or limitations, such as the SMT bit
can't be set when the GPIO direction is set to output, then that
should absolutely be considered, fixed and *properly documented*.

So what exactly is the breakage you are seeing? What options were
used in what scenario? What is it intending to do?

> Signed-off-by: Light Hsieh <light.hsieh@xxxxxxxxxxxx>
> Signed-off-by: Evan Cao <ot_evan.cao@xxxxxxxxxxxx>
> Signed-off-by: Bo Ye <bo.ye@xxxxxxxxxxxx>
> ---
>  drivers/pinctrl/mediatek/pinctrl-paris.c | 38 +++++++++++++++++-------
>  1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 87e958d827bf..a8af62e6f8ca 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -165,21 +165,20 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
>                 err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_SR, &ret);
>                 break;
>         case PIN_CONFIG_INPUT_ENABLE:
> -               err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_IES, &ret);
> -               if (!ret)
> -                       err = -EINVAL;
> -               break;
> -       case PIN_CONFIG_OUTPUT:
> +       case PIN_CONFIG_OUTPUT_ENABLE:
>                 err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &ret);
>                 if (err)
>                         break;
> +               /*     CONFIG     Current direction return value
> +                * -------------  ----------------- ----------------------
> +                * OUTPUT_ENABLE       output       1 (= HW value)
> +                *                     input        0 (= HW value)
> +                * INPUT_ENABLE        output       0 (= reverse HW value)
> +                *                     input        1 (= reverse HW value)
> +                */
> +               if (param == PIN_CONFIG_INPUT_ENABLE)
> +                       ret = !ret;
>
> -               if (!ret) {
> -                       err = -EINVAL;
> -                       break;
> -               }
> -
> -               err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DO, &ret);
>                 break;
>         case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
>                 err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &ret);
> @@ -284,9 +283,26 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>                         break;
>                 err = hw->soc->bias_set_combo(hw, desc, 0, arg);
>                 break;
> +       case PIN_CONFIG_OUTPUT_ENABLE:
> +               err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_SMT,
> +                                      MTK_DISABLE);
> +               /* Keep set direction to consider the case that a GPIO pin
> +                *  does not have SMT control
> +                */
> +               if (err != -ENOTSUPP)
> +                       break;
> +
> +               err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DIR,
> +                                      MTK_OUTPUT);
> +               break;

Looking now, the device tree bindings don't even allow "output-enable"
or "output-disable" so the PIN_CONFIG_OUTPUT_ENABLE condition won't even
ever get exercised.

So I don't know why this section even exists. At least this part of my
patch was correct.

>         case PIN_CONFIG_INPUT_ENABLE:
>                 /* regard all non-zero value as enable */
>                 err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_IES, !!arg);
> +               if (err)
> +                       break;
> +
> +               err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DIR,
> +                                      MTK_INPUT);

FWIW the implementation is also slightly wrong in that it is ignoring
the argument. If the DT specifies "input-disable", then what happens
is you get PIN_CONFIG_INPUT_ENABLE with "arg = 0", but you still set
it to the input direction.

Thanks
ChenYu

>                 break;
>         case PIN_CONFIG_SLEW_RATE:
>                 /* regard all non-zero value as enable */
> --
> 2.17.0
>
>