Re: [PATCH v8 03/11] pinctrl: k1: fix pull-up/pull-down configuration

Hi Eric,

On 00:40 Sun 02 Aug     , Eric Chung wrote:
> Fix pull-up/pull-down configuration to honor the argument parameter:
> when argument is non-zero, enable the requested pull direction;
> when argument is zero, clear all pull bits to disable.

No, This will be problem .. see my comment below

> 
> Signed-off-by: Eric Chung <[email protected]>
> 
> ---
> v7:
> - Simplify pull-up/pull-down configuration by using a single
>   clrsetbits_le32() call.
> v6:
> - Fix pull-up/pull-down configuration.
> ---
>  drivers/pinctrl/spacemit/pinctrl-k1.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
> index 6ed511879daf..4e65fad1e8c5 100644
> --- a/drivers/pinctrl/spacemit/pinctrl-k1.c
> +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
> @@ -409,7 +409,6 @@ static int spacemit_pinconf_set(struct udevice *dev, unsigned int pin_selector,
>  	struct spacemit_pinctrl_data *data;
>  	struct spacemit_pinctrl_priv *priv = dev_get_priv(dev);
>  	void __iomem *addr;
> -	u32 mask;
>  	unsigned int io_type;
>  	u8 ds;
>  	bool found;
> @@ -424,12 +423,12 @@ static int spacemit_pinconf_set(struct udevice *dev, unsigned int pin_selector,
>  		clrbits_le32(addr, PAD_PULLUP | PAD_PULLDOWN | PAD_PULL_EN);
>  		break;
>  	case PIN_CONFIG_BIAS_PULL_DOWN:
> -		mask = PAD_PULLDOWN | PAD_PULLUP | PAD_PULL_EN;
> -		clrsetbits_le32(addr, mask, PAD_PULLDOWN | PAD_PULL_EN);
> +		clrsetbits_le32(addr, PAD_PULLDOWN | PAD_PULLUP | PAD_PULL_EN,
> +				argument ? PAD_PULLDOWN | PAD_PULL_EN : 0);
>  		break;
..
>  	case PIN_CONFIG_BIAS_PULL_UP:
> -		mask = PAD_PULLDOWN | PAD_PULLUP | PAD_PULL_EN;
> -		clrsetbits_le32(addr, mask, PAD_PULLUP | PAD_PULL_EN);
> +		clrsetbits_le32(addr, PAD_PULLDOWN | PAD_PULLUP | PAD_PULL_EN,
> +				argument ? PAD_PULLUP | PAD_PULL_EN : 0);
In kernel side, the argument for pull up settings has documentated as
 0 - normal pull up
 1 - strong pull up
well, you can still use bias-disable property to disable bias settings if needed

I'd suggest to have this aligned with kernel DT files, which would more easily
for us to reuse upstream dts files

>  		break;
>  	case PIN_CONFIG_DRIVE_STRENGTH:
>  		io_type = IO_TYPE_1V8;
> 
> -- 
> 2.51.0
> 

-- 
Yixun Lan (dlan)