| Message ID | 20210123182711.7177-4-dariobin@libero.it |
|---|---|
| State | Superseded |
| Delegated to: | Lokesh Vutla |
| Headers | show |
| Series | Add support for pinmux status command on beaglebone | expand |
Hi Dario, On Sat, 23 Jan 2021 at 11:27, Dario Binacchi <dariobin@libero.it> 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'. Please start your commit message with the problem and then describe what your patch does. It is unclear from this commit message why we need this patch :-) > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > --- > > drivers/pinctrl/pinctrl-single.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) Regards, Simon
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; > 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. > } > 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 >
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; > > 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
On 26/01/21 12:20PM, Dario Binacchi wrote: > 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; > > > 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. 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", ®); > > > + 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
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); 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); } 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; }
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'. Signed-off-by: Dario Binacchi <dariobin@libero.it> --- drivers/pinctrl/pinctrl-single.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)