On Fri, Feb 11, 2022 at 09:07:32PM +1300, Paulo Miguel Almeida wrote: > dev_<level> functions don't support printing hex dumps and the > alternative available (print_hex_dump_debug) doesn't print the device > information such as device's driver name and device name. That type of > information which comes in handy for situations in which you can more > than 1 device attached at the same type. > > this patch adds a utility function that can obtain the same result as > print_hex_dump_debug while being able to honour all possible flags that > one may be interested in when dynamic debug is used. > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@xxxxxxxxx> > --- I feel like this patch is over engineering your debug code. Is this really worthwhile? If you really prefer the new format that's fine but it seems like not necessarily a good use of energy. > Meta-comments: > > the initial discussion to use print_hex_dump_debug started in this patch > but the original idea got merged into the brach. > > https://lore.kernel.org/lkml/a630d8381cee0f543e0d77614052e1d04ab162a5.camel@xxxxxxxxxxx/#t > > --- > drivers/staging/pi433/rf69.c | 39 ++++++++++++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c > index 901f8db3e3ce..82d4ba24c35f 100644 > --- a/drivers/staging/pi433/rf69.c > +++ b/drivers/staging/pi433/rf69.c > @@ -822,9 +822,37 @@ int rf69_set_dagc(struct spi_device *spi, enum dagc dagc) > > /*-------------------------------------------------------------------------*/ > > +static void rf69_dbg_hex(struct spi_device *spi, u8 *buf, unsigned int size, > + const char *fmt, ...) > +{ > + va_list args; > + char textbuf[512] = {}; No need to initialize this. > + char *text = textbuf; > + int text_pos; > + Don't put a blank line in the middle of the declaration block. > + int rowsize = 16; > + int i, linelen, remaining = size; > + > + va_start(args, fmt); > + text_pos = vscnprintf(text, sizeof(textbuf), fmt, args); > + text += text_pos; When you're printing to a buffer like this then I find it's easier to leave the start of the buffer constant. So get rid of the text pointer and just use "textbuf + prefix_len" instead of having a "text" pointer. > + va_end(args); > + > + for (i = 0; i < size; i += rowsize) { > + linelen = min(remaining, rowsize); > + remaining -= rowsize; > + > + hex_dump_to_buffer(buf + i, linelen, rowsize, 1, > + text, sizeof(textbuf) - text_pos, false); > + > + dev_dbg(&spi->dev, "%s\n", textbuf); > + > + memset(text, 0, sizeof(textbuf) - text_pos); No need for this. > + } Instead of printing lineline at a time in a loop, what happens if you just print size bytes? (I honestly don't know because I have never used this function before). regards, dan carpenter
