* Re: [RESEND. PATCH v1] pinctrl: mediatek: paris: Revert "Rework support for PIN_CONFIG_{INPUT,OUTPUT}_ENABLE"
2024-10-17 9:14 ` [RESEND. PATCH v1] pinctrl: mediatek: paris: Revert "Rework support for PIN_CONFIG_{INPUT,OUTPUT}_ENABLE" Bo Ye
@ 2024-10-17 13:07 ` Macpaul Lin
2024-10-18 5:17 ` Chen-Yu Tsai
1 sibling, 0 replies; 3+ messages in thread
From: Macpaul Lin @ 2024-10-17 13:07 UTC (permalink / raw)
To: Bo Ye, Sean Wang, Linus Walleij, Matthias Brugger,
AngeloGioacchino Del Regno, Chen-Yu Tsai, Jades.shih, ivan.tseng
Cc: Yongdong Zhang, Xiujuan Tan, Browse Zhang, Light Hsieh, Evan Cao,
linux-mediatek, linux-gpio, linux-kernel, linux-arm-kernel,
Bear Wang, Pablo Sun
On 10/17/24 17:14, Bo Ye wrote:
> [This reverts commit c5d3b64c568a344e998830e0e94a7c04e372f89b.]
First, this should be "PATCH v2" since you've already modified commit
message.
> 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
> be implemented according to view of its purpose - set GPIO direction and output value (for
> output only) according to specific HW design.
>
> 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.
>
> Fixes: c5d3b64c568 ("pinctrl: mediatek: paris: Rework support for PIN_CONFIG_{INPUT,OUTPUT}_ENABLE")
> Signed-off-by: Light Hsieh <light.hsieh@mediatek.com>
> Signed-off-by: Evan Cao <ot_evan.cao@mediatek.com>
> Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 38 +++++++++++++++++-------
> 1 file changed, 27 insertions(+), 11 deletions(-)
Please add change logs in this section.
For example:
Changes for v2:
- Update "Fixes: " tag in commit message.
> 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,
[snip]
Please reply all the questions address by the reviewers.
For example:
[1]
https://lore.kernel.org/lkml/433295fe-8d34-af8b-f6bf-be1953b6e479@mediatek.com/T/#m168c6138c374009990025b0407f617ba10dede8d
Otherwise this patch will hard to be reviewed.
Any open-minded discussion contribute to the improvement of Linux code
and the community.
Thanks
Macpaul Lin
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [RESEND. PATCH v1] pinctrl: mediatek: paris: Revert "Rework support for PIN_CONFIG_{INPUT,OUTPUT}_ENABLE"
2024-10-17 9:14 ` [RESEND. PATCH v1] pinctrl: mediatek: paris: Revert "Rework support for PIN_CONFIG_{INPUT,OUTPUT}_ENABLE" Bo Ye
2024-10-17 13:07 ` Macpaul Lin
@ 2024-10-18 5:17 ` Chen-Yu Tsai
1 sibling, 0 replies; 3+ messages in thread
From: Chen-Yu Tsai @ 2024-10-18 5:17 UTC (permalink / raw)
To: Bo Ye
Cc: Sean Wang, Linus Walleij, Matthias Brugger,
AngeloGioacchino Del Regno, Yongdong Zhang, Xiujuan Tan,
Browse Zhang, Light Hsieh, Evan Cao, linux-mediatek, linux-gpio,
linux-kernel, linux-arm-kernel
On Thu, Oct 17, 2024 at 5:14 PM Bo Ye <bo.ye@mediatek.com> wrote:
Please avoid sending more than one version per day. Most people in
the community are not in the Asian time zones. Give people time
to respond.
> [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
> be implemented according to view of its purpose - set GPIO direction and output value (for
> output only) according to specific HW design.
>
> 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.
>
> Fixes: c5d3b64c568 ("pinctrl: mediatek: paris: Rework support for PIN_CONFIG_{INPUT,OUTPUT}_ENABLE")
As Macpaul mentioned, you changed the commit message by adding a tag, and
thus this patch should be marked "v2". Please also provide a changelog
on what changed in this version in the footer, i.e. under the "---" line
but before the actual patch context.
ChenYu
> Signed-off-by: Light Hsieh <light.hsieh@mediatek.com>
> Signed-off-by: Evan Cao <ot_evan.cao@mediatek.com>
> Signed-off-by: Bo Ye <bo.ye@mediatek.com>
> ---
> 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;
> 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);
> break;
> case PIN_CONFIG_SLEW_RATE:
> /* regard all non-zero value as enable */
> --
> 2.17.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread