Hi Pratyush, > Il 25/01/2021 18:09 Pratyush Yadav <p.ya...@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 <dario...@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", ®); > > + 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 "®" 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 > "®". 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", ®, val); > > + dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); > > In a similar fashion as above, shouldn't "®" 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 ® 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", ®, 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", ®); > > + 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", ®, val); > > + dev_dbg(dev, " reg/val %pa/0x%08x\n", ®, val); > > } > > return 0; > > } > > -- > > 2.17.1 > > > > -- > Regards, > Pratyush Yadav > Texas Instruments India Regards, Dario