Hi Pratyush,
Il 25/01/2021 18:09 Pratyush Yadav p.yadav@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@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;@@ -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);}
2.17.1
-- Regards, Pratyush Yadav Texas Instruments India
Regards, Dario