[PATCH 03/11] pinctrl: single: fix debug messages formatting

[Pratyush Yadav]

Hi Dario,

On 23/01/21 07:27PM, Dario Binacchi wrote:
> The printf '%pa' format specifier appends the '0x' prefix to the
> displayed address. Furthermore, the offset variable is displayed with
> the '%x' format specifier instead of '%pa'.

I agree with Simon that the commit message does not explain why this 
change is needed.
 
> Signed-off-by: Dario Binacchi <dariobin at libero.it>
> ---
> 
>  drivers/pinctrl/pinctrl-single.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 49ed15211d..cec00e289c 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -77,15 +77,17 @@ static int single_configure_pins(struct udevice *dev,
>  	struct single_pdata *pdata = dev_get_plat(dev);
>  	int n, count = size / sizeof(struct single_fdt_pin_cfg);
>  	phys_addr_t reg;
> -	u32 val;
> +	u32 offset, val;
>  
>  	for (n = 0; n < count; n++, pins++) {
> -		reg = fdt32_to_cpu(pins->reg);
> -		if ((reg < 0) || (reg > pdata->offset)) {
> -			dev_dbg(dev, "  invalid register offset 0x%pa\n", &reg);
> +		offset = fdt32_to_cpu(pins->reg);
> +		if (offset < 0 || offset > pdata->offset) {
> +			dev_dbg(dev, "  invalid register offset 0x%x\n",
> +				offset);

You are not just fixing "debug messages formatting" here. You have made 
other changes to the structure of the code here. While these changes 
might all be correct, they don't belong in a commit that claims to fix 
message formatting.

For example, this dev_dbg() statement in the pre-image prints "&reg" as 
the offset. This would be the address of the variable reg on the stack. 
This looks like it is a bug. The offset is stored in "reg", not in 
"&reg". The post-image fixes this bug. A person reading this diff might 
not look so closely at this because you only claim to change formatting. 
And some important discussion/context on this bug might be skipped over.

Please re-word the commit subject and message to clearly explain why you 
are making these structural changes and separate them from the 
formatting changes (which also warrant an explanation on their own).

>  			continue;
>  		}
> -		reg += pdata->base;
> +
> +		reg = pdata->base + offset;
>  		val = fdt32_to_cpu(pins->val) & pdata->mask;
>  		switch (pdata->width) {
>  		case 16:
> @@ -99,7 +101,7 @@ static int single_configure_pins(struct udevice *dev,
>  				 pdata->width);
>  			continue;
>  		}
> -		dev_dbg(dev, "  reg/val 0x%pa/0x%08x\n", &reg, val);
> +		dev_dbg(dev, "  reg/val %pa/0x%08x\n", &reg, val);

In a similar fashion as above, shouldn't "&reg" be replaced with "reg"? 
I am not too familiar with the code to say for sure. Same for the 
changes below.

>  	}
>  	return 0;
>  }
> @@ -111,15 +113,17 @@ static int single_configure_bits(struct udevice *dev,
>  	struct single_pdata *pdata = dev_get_plat(dev);
>  	int n, count = size / sizeof(struct single_fdt_bits_cfg);
>  	phys_addr_t reg;
> -	u32 val, mask;
> +	u32 offset, val, mask;
>  
>  	for (n = 0; n < count; n++, pins++) {
> -		reg = fdt32_to_cpu(pins->reg);
> -		if ((reg < 0) || (reg > pdata->offset)) {
> -			dev_dbg(dev, "  invalid register offset 0x%pa\n", &reg);
> +		offset = fdt32_to_cpu(pins->reg);
> +		if (offset < 0 || offset > pdata->offset) {
> +			dev_dbg(dev, "  invalid register offset 0x%x\n",
> +				offset);
>  			continue;
>  		}
> -		reg += pdata->base;
> +
> +		reg = pdata->base + offset;
>  
>  		mask = fdt32_to_cpu(pins->mask);
>  		val = fdt32_to_cpu(pins->val) & mask;
> @@ -136,7 +140,7 @@ static int single_configure_bits(struct udevice *dev,
>  				 pdata->width);
>  			continue;
>  		}
> -		dev_dbg(dev, "  reg/val 0x%pa/0x%08x\n", &reg, val);
> +		dev_dbg(dev, "  reg/val %pa/0x%08x\n", &reg, val);
>  	}
>  	return 0;
>  }
> -- 
> 2.17.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India