Re: [PATCH] staging: pi433: add rf69_dbg_hex function

[Dan Carpenter <dan.carpenter@oracle.com>]

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@gmail.com>
> ---

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@perches.com/#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