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
