Re: [PATCH v2 1/8] spi: core: add spi_message_dump and spi_transfer_dump

[Martin Sperl <kernel@xxxxxxxxxxxxxxxx>]

> On 21.12.2015, at 13:42, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>> +               /* format the next 16 bytes */
>> +               snprintf(hexdata, sizeof(hexdata), "%16ph", ptr + i);
>> +
>> +               /* truncate the unnecessarily printed bytes */
> 
> Again, you don't need the intermediate buffer along with this all
> stuff. Just use %*ph properly (hint: star is put here and elsewhere on
> purpose).
> 
> drivers/hid/i2c-hid/i2c-hid.c:187:      i2c_hid_dbg(ihid, "%s:
> cmd=%*ph\n", __func__, length, cmd->data);
> 
That is what was missing - did not find any sensible example that showed
that this is possible and also Documentation/printk-formats.txt does
not mention this explicitly either.


>> +               l = min_t(size_t, len - i, spi_dump_bytes_per_line) * 3 - 1;
>> +               hexdata[l] = 0;
>> +
>> +               /* and print data including address and offset */
>> +               dev_info(&spi->dev, "%soff=0x%.5zx ptr=0x%pK: %s\n",
>> +                        prefix, i, ptr + i, hexdata);
>> +       }
>> +}
>> +
>> +static void spi_transfer_dump_buffer(struct spi_device *spi, char *prefix,
>> +                                    const void *ptr, size_t len,
>> +                                    size_t head_len, size_t tail_len)
>> +{
> 
> The function body looks too complicated. Can you describe what
> function does in different cases?
> 
Can add kernel do here

>> +               if (head_len | tail_len)
> 
> I think || looks a bit more logical here and below.
true...

> 
>> +                       spi_transfer_dump_buffer(spi, "\t",
>> +                                                xfer->tx_buf, xfer->len,
>> +                                                head_len, tail_len);
>> +       }
>> +       if (xfer->rx_buf) {
>> +               dev_info(dev, "    rx_buf:      %pK\n", xfer->rx_buf);
>> +               if (xfer->rx_dma)
>> +                       dev_info(dev, "    rx_dma:      %pad\n",
>> +                                &xfer->rx_dma);
>> +               if (head_len | tail_len)
true...
> 
>> +                       spi_transfer_dump_buffer(spi, "\t",
>> +                                                xfer->rx_buf, xfer->len,
>> +                                                head_len, tail_len);
> 
> Is the DMA buffer is in coherent memory? Otherwise you have to call
> dma_sync for it before reading on CPU side.

It has to be because it is not necessary that DMA is used at all.
Also the “feature” is depreciated anyway and Mark wants to see it going away.

Thanks again for the review…

Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html