| Message ID | 1450442668-2391-2-git-send-email-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 18, 2015 at 2:44 PM, <kernel@martin.sperl.org> wrote: > From: Martin Sperl <kernel@martin.sperl.org> > > This implements a means to dump a spi_message or spi_transfer. > > spi_loop_back_test requires a means to report on failed transfers > (including payload data), so it makes use of this. > > Such a functionality can also be helpful during development of > other drivers, so it has been exposed as a general facility. > > Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > --- > drivers/spi/spi.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/spi/spi.h | 30 ++++++++++ > 2 files changed, 176 insertions(+) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 9964835..6e9157f 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -31,6 +31,7 @@ > #include <linux/of_gpio.h> > #include <linux/pm_runtime.h> > #include <linux/pm_domain.h> > +#include <linux/printk.h> > #include <linux/export.h> > #include <linux/sched/rt.h> > #include <linux/delay.h> > @@ -718,6 +719,151 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n) > return 0; > } > > +static void __spi_transfer_dump_chunk(struct spi_device *spi, char *pre, > + const void *ptr, size_t start, > + size_t len) > +{ > + unsigned char linebuf[32 * 3 + 2 + 32 + 1]; > + int i; Unsigned. > + > + /* because we want to use dev_info as well as print > + * offset value as well as pointer > + * we can not use print_hex_dump directly Maybe provide a dev_hex_dump() function instead? > + */ > + for (i = start; i < len; i += 16) { To many magic numbers, like 16. Moreover, why you have buffer almost twice longer than needed? > + hex_dump_to_buffer(ptr + i, min_t(int, len - i, 16), > + 16, 1, > + linebuf, sizeof(linebuf), 0); Since you print without ASCII part the whole idea with hex_dump_to_buffer is a total overkill. > + dev_info(&spi->dev, "%soff=%.8x ptr=%p: %s\n", > + pre, i, ptr + i, linebuf); Documentation/printk-formats.txt is your friend. In particular a chapter about %*ph. > + } > +} > + > +static void spi_transfer_dump_buffer(struct spi_device *spi, char *pre, > + const void *ptr, size_t len, > + size_t dump_size) > +{ > + int start; > + > + /* align dump_size on 32 bytes */ > + if (dump_size & 31) > + dump_size += 32 - (dump_size & 31); roundup() / rounddown(). > + > + /* dump the whole chunk in one go if needed */ > + if (len <= dump_size) { > + __spi_transfer_dump_chunk(spi, pre, ptr, 0, len); > + return; > + } > + > + /* otherwise we need to dump chunks - head first */ > + __spi_transfer_dump_chunk(spi, pre, ptr, 0, dump_size / 2); > + > + /* calculate where we need to continue */ > + start = len - dump_size / 2; > + start &= ~15; /* align on the last multiple of 16 */ Magic. > + > + /* message about truncating */ > + dev_info(&spi->dev, "%s truncated - continuing at offset %04x\n", > + pre, start); > + > + /* now the tail */ > + __spi_transfer_dump_chunk(spi, pre, ptr, start, len); > +} > + > +/** > + * spi_transfer_dump - dump all the essential information > + * of a @spi_transfer, when dump_size is set, > + * then hex-dump that many bytes of data > + * @spi: @spi_device for which to dump this (dev_info) > + * @msg: @spi_message to which xfer belongs > + * @xfer: @spi_transfer to dump > + * @dump_size: total number of bytes to dump of each buffer > + * (multiple of 32 if not rounded up) > + */ > +void spi_transfer_dump(struct spi_device *spi, > + struct spi_message *msg, > + struct spi_transfer *xfer, > + size_t dump_size) > +{ > + struct device *dev = &spi->dev; > + > + dev_info(dev, " spi_transfer@%pK\n", xfer); > + dev_info(dev, " speed_hz: %u\n", xfer->speed_hz); > + dev_info(dev, " len: %u\n", xfer->len); > + dev_info(dev, " tx_nbits: %u\n", xfer->tx_nbits); > + dev_info(dev, " rx_nbits: %u\n", xfer->rx_nbits); > + dev_info(dev, " bits/word: %u\n", xfer->bits_per_word); > + if (xfer->delay_usecs) > + dev_info(dev, " delay_usecs: %u\n", > + xfer->delay_usecs); > + if (xfer->cs_change) > + dev_info(dev, " cs_change\n"); > + if (xfer->tx_buf) { > + dev_info(dev, " tx_buf: %pK\n", xfer->tx_buf); > + if (xfer->tx_dma) > + dev_info(dev, " tx_dma: %pad\n", > + &xfer->tx_dma); > + if (dump_size) > + spi_transfer_dump_buffer(spi, " ", > + xfer->tx_buf, xfer->len, > + dump_size); > + } > + 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 (dump_size) > + spi_transfer_dump_buffer(spi, " ", > + xfer->rx_buf, xfer->len, > + dump_size); > + } > +} > +EXPORT_SYMBOL_GPL(spi_transfer_dump); > + > +/** > + * spi_message_dump_custom - dump a spi message with ability to have > + * a custom dump method per transfer > + * @spi: @spi_device for which to dump this (dev_info) > + * @msg: @spi_message to dump > + * @dump_size: total number of bytes to dump of each buffer > + * @custom: custom dump code to execute per transfer > + * @context: context to pass to the custom dump code > + * > + * uses dev_info() to dump the lines Usually sentences are started with a capital letter and ended with a dot. > + */ > +void spi_message_dump_custom(struct spi_device *spi, > + struct spi_message *msg, > + size_t dump_size, > + spi_transfer_dump_custom_t custom, > + void *context) > +{ > + struct device *dev = &spi->dev; > + struct spi_transfer *xfer; > + > + /* dump the message */ > + dev_info(dev, "spi_msg@%pK\n", msg); > + if (msg->status) > + dev_info(dev, " status: %d\n", msg->status); > + dev_info(dev, " frame_length: %zu\n", msg->frame_length); > + dev_info(dev, " actual_length: %zu\n", msg->actual_length); > + if (msg->complete) > + dev_info(dev, " complete: %pF\n", msg->complete); > + if (msg->context) > + dev_info(dev, " context: %pF\n", msg->context); > + if (msg->is_dma_mapped) > + dev_info(dev, " is_dma_mapped\n"); > + dev_info(dev, " transfers-head: %pK\n", &msg->transfers); > + > + /* dump transfers themselves */ > + list_for_each_entry(xfer, &msg->transfers, transfer_list) { > + spi_transfer_dump(spi, msg, xfer, dump_size); > + if (custom) > + custom(spi, msg, xfer, context); > + } > +} > +EXPORT_SYMBOL_GPL(spi_message_dump_custom); > + > /*-------------------------------------------------------------------------*/ > > static void spi_set_cs(struct spi_device *spi, bool enable) > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index f055a47..a17be97 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -897,6 +897,36 @@ extern int spi_setup(struct spi_device *spi); > extern int spi_async(struct spi_device *spi, struct spi_message *message); > extern int spi_async_locked(struct spi_device *spi, > struct spi_message *message); > +/*---------------------------------------------------------------------------*/ > + > +extern void spi_transfer_dump(struct spi_device *spi, > + struct spi_message *msg, > + struct spi_transfer *xfer, > + size_t dump_size); > + > +typedef void (*spi_transfer_dump_custom_t)(struct spi_device *spi, > + struct spi_message *msg, > + struct spi_transfer *xfer, > + void *context); > + > +extern void spi_message_dump_custom(struct spi_device *spi, > + struct spi_message *msg, > + size_t dump_size, > + spi_transfer_dump_custom_t custom, > + void *context); > + > +static inline void spi_message_dump_data(struct spi_device *spi, > + struct spi_message *msg, > + size_t dump_size) > +{ > + spi_message_dump_custom(spi, msg, dump_size, NULL, NULL); > +} > + > +static inline void spi_message_dump(struct spi_device *spi, > + struct spi_message *msg) > +{ > + spi_message_dump_custom(spi, msg, 0, NULL, NULL); > +} How many users do you have for the helpers? I suggest to add a helper whenever there are at least 2+ users present. > > /*---------------------------------------------------------------------------*/ > > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-spi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 18.12.2015, at 14:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Dec 18, 2015 at 2:44 PM, <kernel@martin.sperl.org> wrote: >> +static void __spi_transfer_dump_chunk(struct spi_device *spi, char *pre, >> + const void *ptr, size_t start, >> + size_t len) ... all comments fixed in next patch (mostly by rewriting code to use %*ph) > Documentation/printk-formats.txt is your friend. In particular a > chapter about %*ph. had to do a sprintf for the format itself as %*ph does not allow for variable length - it still reduced the codesize... > > roundup() / rounddown(). > > Magic. > > Usually sentences are started with a capital letter and ended with a dot. > all comments fixed in next patch > > How many users do you have for the helpers? I suggest to add a helper > whenever there are at least 2+ users present. > As these are mostly convenience methods for use when debugging things there are not typically that many users that would be needing those. So I would prefer to keep them. Thanks for the comments, Martin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 18, 2015 at 6:53 PM, Martin Sperl <kernel@martin.sperl.org> wrote: >> On 18.12.2015, at 14:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Fri, Dec 18, 2015 at 2:44 PM, <kernel@martin.sperl.org> wrote: >>> +static void __spi_transfer_dump_chunk(struct spi_device *spi, char *pre, >>> + const void *ptr, size_t start, >>> + size_t len) > ... > all comments fixed in next patch (mostly by rewriting code to use %*ph) > >> Documentation/printk-formats.txt is your friend. In particular a >> chapter about %*ph. > had to do a sprintf for the format itself as %*ph does not allow for > variable length - it still reduced the codesize... I didn't get this. What variable length you are talking about? You may supply any amount of bytes you would like to print (will be printed up to 64 bytes in a line). >> How many users do you have for the helpers? I suggest to add a helper >> whenever there are at least 2+ users present. >> > As these are mostly convenience methods for use when debugging things > there are not typically that many users that would be needing those. > So I would prefer to keep them. Up to Mark, though in my practice most of the maintainers do not like orphaned helpers.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 9964835..6e9157f 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -31,6 +31,7 @@ #include <linux/of_gpio.h> #include <linux/pm_runtime.h> #include <linux/pm_domain.h> +#include <linux/printk.h> #include <linux/export.h> #include <linux/sched/rt.h> #include <linux/delay.h> @@ -718,6 +719,151 @@ int spi_register_board_info(struct spi_board_info const *info, unsigned n) return 0; } +static void __spi_transfer_dump_chunk(struct spi_device *spi, char *pre, + const void *ptr, size_t start, + size_t len) +{ + unsigned char linebuf[32 * 3 + 2 + 32 + 1]; + int i; + + /* because we want to use dev_info as well as print + * offset value as well as pointer + * we can not use print_hex_dump directly + */ + for (i = start; i < len; i += 16) { + hex_dump_to_buffer(ptr + i, min_t(int, len - i, 16), + 16, 1, + linebuf, sizeof(linebuf), 0); + dev_info(&spi->dev, "%soff=%.8x ptr=%p: %s\n", + pre, i, ptr + i, linebuf); + } +} + +static void spi_transfer_dump_buffer(struct spi_device *spi, char *pre, + const void *ptr, size_t len, + size_t dump_size) +{ + int start; + + /* align dump_size on 32 bytes */ + if (dump_size & 31) + dump_size += 32 - (dump_size & 31); + + /* dump the whole chunk in one go if needed */ + if (len <= dump_size) { + __spi_transfer_dump_chunk(spi, pre, ptr, 0, len); + return; + } + + /* otherwise we need to dump chunks - head first */ + __spi_transfer_dump_chunk(spi, pre, ptr, 0, dump_size / 2); + + /* calculate where we need to continue */ + start = len - dump_size / 2; + start &= ~15; /* align on the last multiple of 16 */ + + /* message about truncating */ + dev_info(&spi->dev, "%s truncated - continuing at offset %04x\n", + pre, start); + + /* now the tail */ + __spi_transfer_dump_chunk(spi, pre, ptr, start, len); +} + +/** + * spi_transfer_dump - dump all the essential information + * of a @spi_transfer, when dump_size is set, + * then hex-dump that many bytes of data + * @spi: @spi_device for which to dump this (dev_info) + * @msg: @spi_message to which xfer belongs + * @xfer: @spi_transfer to dump + * @dump_size: total number of bytes to dump of each buffer + * (multiple of 32 if not rounded up) + */ +void spi_transfer_dump(struct spi_device *spi, + struct spi_message *msg, + struct spi_transfer *xfer, + size_t dump_size) +{ + struct device *dev = &spi->dev; + + dev_info(dev, " spi_transfer@%pK\n", xfer); + dev_info(dev, " speed_hz: %u\n", xfer->speed_hz); + dev_info(dev, " len: %u\n", xfer->len); + dev_info(dev, " tx_nbits: %u\n", xfer->tx_nbits); + dev_info(dev, " rx_nbits: %u\n", xfer->rx_nbits); + dev_info(dev, " bits/word: %u\n", xfer->bits_per_word); + if (xfer->delay_usecs) + dev_info(dev, " delay_usecs: %u\n", + xfer->delay_usecs); + if (xfer->cs_change) + dev_info(dev, " cs_change\n"); + if (xfer->tx_buf) { + dev_info(dev, " tx_buf: %pK\n", xfer->tx_buf); + if (xfer->tx_dma) + dev_info(dev, " tx_dma: %pad\n", + &xfer->tx_dma); + if (dump_size) + spi_transfer_dump_buffer(spi, " ", + xfer->tx_buf, xfer->len, + dump_size); + } + 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 (dump_size) + spi_transfer_dump_buffer(spi, " ", + xfer->rx_buf, xfer->len, + dump_size); + } +} +EXPORT_SYMBOL_GPL(spi_transfer_dump); + +/** + * spi_message_dump_custom - dump a spi message with ability to have + * a custom dump method per transfer + * @spi: @spi_device for which to dump this (dev_info) + * @msg: @spi_message to dump + * @dump_size: total number of bytes to dump of each buffer + * @custom: custom dump code to execute per transfer + * @context: context to pass to the custom dump code + * + * uses dev_info() to dump the lines + */ +void spi_message_dump_custom(struct spi_device *spi, + struct spi_message *msg, + size_t dump_size, + spi_transfer_dump_custom_t custom, + void *context) +{ + struct device *dev = &spi->dev; + struct spi_transfer *xfer; + + /* dump the message */ + dev_info(dev, "spi_msg@%pK\n", msg); + if (msg->status) + dev_info(dev, " status: %d\n", msg->status); + dev_info(dev, " frame_length: %zu\n", msg->frame_length); + dev_info(dev, " actual_length: %zu\n", msg->actual_length); + if (msg->complete) + dev_info(dev, " complete: %pF\n", msg->complete); + if (msg->context) + dev_info(dev, " context: %pF\n", msg->context); + if (msg->is_dma_mapped) + dev_info(dev, " is_dma_mapped\n"); + dev_info(dev, " transfers-head: %pK\n", &msg->transfers); + + /* dump transfers themselves */ + list_for_each_entry(xfer, &msg->transfers, transfer_list) { + spi_transfer_dump(spi, msg, xfer, dump_size); + if (custom) + custom(spi, msg, xfer, context); + } +} +EXPORT_SYMBOL_GPL(spi_message_dump_custom); + /*-------------------------------------------------------------------------*/ static void spi_set_cs(struct spi_device *spi, bool enable) diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index f055a47..a17be97 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -897,6 +897,36 @@ extern int spi_setup(struct spi_device *spi); extern int spi_async(struct spi_device *spi, struct spi_message *message); extern int spi_async_locked(struct spi_device *spi, struct spi_message *message); +/*---------------------------------------------------------------------------*/ + +extern void spi_transfer_dump(struct spi_device *spi, + struct spi_message *msg, + struct spi_transfer *xfer, + size_t dump_size); + +typedef void (*spi_transfer_dump_custom_t)(struct spi_device *spi, + struct spi_message *msg, + struct spi_transfer *xfer, + void *context); + +extern void spi_message_dump_custom(struct spi_device *spi, + struct spi_message *msg, + size_t dump_size, + spi_transfer_dump_custom_t custom, + void *context); + +static inline void spi_message_dump_data(struct spi_device *spi, + struct spi_message *msg, + size_t dump_size) +{ + spi_message_dump_custom(spi, msg, dump_size, NULL, NULL); +} + +static inline void spi_message_dump(struct spi_device *spi, + struct spi_message *msg) +{ + spi_message_dump_custom(spi, msg, 0, NULL, NULL); +} /*---------------------------------------------------------------------------*/