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

[Pratyush Yadav]

On 26/01/21 12:20PM, Dario Binacchi wrote:
> 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.

Makes sense. Thanks for the explanation.

My comments about commit message and splitting out changes still hold. 
If you think these changes should be in the same commit then please 
explain why in the commit message.
 
> > >  	}
> > >  	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

-- 
Regards,
Pratyush Yadav
Texas Instruments India