Re: [PATCH v8 02/11] pinctrl: k1: fix drive strength configuration

On 00:40 Sun 02 Aug     , Eric Chung wrote:
> Fix drive strength configuration by:
> - Using FIELD_PREP() to properly set the PAD_DRIVE mask bits
> - Not overriding io_type with IO_TYPE_EXTERNAL for external pins
> - Returning the pin number instead of 0 from pinmux_property_set
> - Removing redundant zero-initialization of the mask variable
> 
> Signed-off-by: Eric Chung <[email protected]>
> 
Checked this patch, the logic looks ok to me (only few minor comments),
And you can still add my R-B, thanks

Reviewed-by: Yixun Lan <[email protected]>

> ---
> v6:
> - Fix drive strength configuration.
> ---
>  drivers/pinctrl/spacemit/pinctrl-k1.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
> index a6a22eacac7b..6ed511879daf 100644
> --- a/drivers/pinctrl/spacemit/pinctrl-k1.c
> +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
> @@ -9,6 +9,7 @@
>  #include <dm/device_compat.h>
>  #include <dm/pinctrl.h>
>  #include <dm/read.h>
> +#include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/errno.h>
>  #include <linux/io.h>
> @@ -386,10 +387,12 @@ static int spacemit_pinmux_set(struct udevice *dev, unsigned int pin,
>  static int spacemit_pinmux_property_set(struct udevice *dev, u32 pinmux_group)
>  {
>  	u32 pin, mux;
> +	int ret;
>  
>  	pin = spacemit_dt_get_pin(pinmux_group);
>  	mux = spacemit_dt_get_pin_mux(pinmux_group);
> -	return spacemit_pinmux_set(dev, pin, mux);
> +	ret = spacemit_pinmux_set(dev, pin, mux);
..
> +	return ret ? ret : pin;
I'd prefer doing as below, which would more explicitly tell that's an error handler
	if (ret < 0)
		return ret;
	
	return pin;

>  }
>  
>  static const struct pinconf_param spacemit_pinconf_params[] = {
> @@ -406,7 +409,7 @@ 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 = 0;
> +	u32 mask;
This isn't a necessary change, which doesn't alter any logic or fix anything,
and more worse, the follow-up patch drop line of this variable

I mean bringing in unrelated change only increase the review burden..

>  	unsigned int io_type;
>  	u8 ds;
>  	bool found;
> @@ -433,7 +436,8 @@ static int spacemit_pinconf_set(struct udevice *dev, unsigned int pin_selector,
>  		for (i = 0; i < priv->nr_io_pins; i++) {
>  			if (priv->io_pins[i].pin != pin_selector)
>  				continue;
> -			io_type = priv->io_pins[i].io_type;
> +			if (priv->io_pins[i].io_type != IO_TYPE_EXTERNAL)
> +				io_type = priv->io_pins[i].io_type;
>  			break;
>  		}
>  		if (io_type != IO_TYPE_3V3 && io_type != IO_TYPE_1V8) {
> @@ -441,7 +445,7 @@ static int spacemit_pinconf_set(struct udevice *dev, unsigned int pin_selector,
>  			return -EINVAL;
>  		}
>  		ds = spacemit_get_drive_strength(io_type, argument);
> -		clrsetbits_le32(addr, PAD_DRIVE, ds);
> +		clrsetbits_le32(addr, PAD_DRIVE, FIELD_PREP(PAD_DRIVE, ds));
>  		break;
>  	case PIN_CONFIG_POWER_SOURCE:
>  		for (i = 0, found = false; i < priv->nr_io_pins; i++) {
> 
> -- 
> 2.51.0
> 

-- 
Yixun Lan (dlan)