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

[Dario Binacchi]

Hi Pratyush,

> Il 25/01/2021 18:09 Pratyush Yadav <p.yadav at ti.com> ha scritto:
> 
>  
> 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.
> 

Also I would have expected reg instead of &reg but at the link
https://www.kernel.org/doc/html/latest/core-api/printk-formats.html is
explained: 
...
"Phys_addr_t Physical Address Types"

%pa [p] 0x01234567 or 0x0123456789abcdef
For printing a phys_addr_t type (and its derivatives, such as resource_size_t) 
which can vary based on build options, regardless of the width of the CPU data path.

Passed by reference.
...

I also did some tests on the board:
dev_dbg(dev, "  reg/val %pa/0x%08x\n", &reg, val); --> reg/val 0x44e10940/0x00000030
dev_dbg(dev, "  reg/val %p/0x%08x\n", reg, val);   --> reg/val 44e10940/0x00000030

I'll continue to use the first one, which is documented.

> >  	}
> >  	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

Regards,
Dario