[PATCH 0/4] spi: loopback-test: improvements and move dump method

[PATCH 0/4] spi: loopback-test: improvements and move dump method

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Move spi_message dump method into SPI-core and make use of
this extended version of the dumping code in loopback-test.

Also fix the module parameter dump_messages so that it is now a
bitmask that allows for better granularity when to dump what.

Finally add support for SPI_LOOP when spi_master supports it -
this has been sugested by Geert Uytterhoeven.

Martin Sperl (4):
  spi: core: add spi_message_dump and spi_transfer_dump
  spi: loopback-test: move to use spi_message_dump_data
  spi: loopback-test: make dump_messages module parameter a bitmask
  spi: loopback-test: added support for HW-loopback mode

 drivers/spi/spi-loopback-test.c |  123 ++++++++++++---------------------
 drivers/spi/spi.c               |  146 +++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h         |   30 ++++++++
 3 files changed, 220 insertions(+), 79 deletions(-)

--
1.7.10.4

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

[PATCH 1/4] spi: core: add spi_message_dump and spi_transfer_dump

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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;
+
+	/* 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);
+}

 /*---------------------------------------------------------------------------*/

--
1.7.10.4

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

[PATCH 2/4] spi: loopback-test: move to use spi_message_dump_data

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Move loopback-test to use the core spi_message_dump_data method.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-loopback-test.c |   75 +++------------------------------------
 1 file changed, 4 insertions(+), 71 deletions(-)

diff --git a/drivers/spi/spi-loopback-test.c b/drivers/spi/spi-loopback-test.c
index 7f497ac..8dac6a4 100644
--- a/drivers/spi/spi-loopback-test.c
+++ b/drivers/spi/spi-loopback-test.c
@@ -331,74 +331,6 @@ MODULE_LICENSE("GPL");
 /* we allocate one page more, to allow for offsets */
 #define SPI_TEST_MAX_SIZE_PLUS (SPI_TEST_MAX_SIZE + PAGE_SIZE)
 
-static void spi_test_print_hex_dump(char *pre, const void *ptr, size_t len)
-{
-	/* limit the hex_dump */
-	if (len < 1024) {
-		print_hex_dump(KERN_INFO, pre,
-			       DUMP_PREFIX_OFFSET, 16, 1,
-			       ptr, len, 0);
-		return;
-	}
-	/* print head */
-	print_hex_dump(KERN_INFO, pre,
-		       DUMP_PREFIX_OFFSET, 16, 1,
-		       ptr, 512, 0);
-	/* print tail */
-	pr_info("%s truncated - continuing at offset %04zx\n",
-		pre, len - 512);
-	print_hex_dump(KERN_INFO, pre,
-		       DUMP_PREFIX_OFFSET, 16, 1,
-		       ptr + (len - 512), 512, 0);
-}
-
-static void spi_test_dump_message(struct spi_device *spi,
-				  struct spi_message *msg,
-				  bool dump_data)
-{
-	struct spi_transfer *xfer;
-	int i;
-	u8 b;
-
-	dev_info(&spi->dev, "  spi_msg@%pK\n", msg);
-	if (msg->status)
-		dev_info(&spi->dev, "    status:        %i\n",
-			 msg->status);
-	dev_info(&spi->dev, "    frame_length:  %i\n",
-		 msg->frame_length);
-	dev_info(&spi->dev, "    actual_length: %i\n",
-		 msg->actual_length);
-
-	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
-		dev_info(&spi->dev, "    spi_transfer@%pK\n", xfer);
-		dev_info(&spi->dev, "      len:    %i\n", xfer->len);
-		dev_info(&spi->dev, "      tx_buf: %pK\n", xfer->tx_buf);
-		if (dump_data && xfer->tx_buf)
-			spi_test_print_hex_dump("          TX: ",
-						xfer->tx_buf,
-						xfer->len);
-
-		dev_info(&spi->dev, "      rx_buf: %pK\n", xfer->rx_buf);
-		if (dump_data && xfer->rx_buf)
-			spi_test_print_hex_dump("          RX: ",
-						xfer->rx_buf,
-						xfer->len);
-		/* check for unwritten test pattern on rx_buf */
-		if (xfer->rx_buf) {
-			for (i = 0 ; i < xfer->len ; i++) {
-				b = ((u8 *)xfer->rx_buf)[xfer->len - 1 - i];
-				if (b != SPI_TEST_PATTERN_UNWRITTEN)
-					break;
-			}
-			if (i)
-				dev_info(&spi->dev,
-					 "      rx_buf filled with %02x starts at offset: %i\n",
-					 SPI_TEST_PATTERN_UNWRITTEN,
-					 xfer->len - i);
-		}
-	}
-}
-
 struct rx_ranges {
 	struct list_head list;
 	u8 *start;
@@ -819,7 +751,7 @@ int spi_test_execute_msg(struct spi_device *spi, struct spi_test *test,
 	if (!simulate_only) {
 		/* dump the complete message before and after the transfer */
 		if (dump_messages == 3)
-			spi_test_dump_message(spi, msg, true);
+			spi_message_dump_data(spi, msg, 1024);
 
 		/* run spi message */
 		ret = spi_sync(spi, msg);
@@ -855,8 +787,9 @@ int spi_test_execute_msg(struct spi_device *spi, struct spi_test *test,
 	/* if requested or on error dump message (including data) */
 exit:
 	if (dump_messages || ret)
-		spi_test_dump_message(spi, msg,
-				      (dump_messages >= 2) || (ret));
+		spi_message_dump_data(spi, msg,
+				      ((dump_messages >= 2) || (ret)) ?
+				      1024 : 0);
 
 	return ret;
 }
-- 
1.7.10.4

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

[PATCH 3/4] spi: loopback-test: make dump_messages module parameter a bitmask

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Make dump_messages module parameter a bitmask to allow for better
granularity when dumping messages.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-loopback-test.c |   40 ++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-loopback-test.c b/drivers/spi/spi-loopback-test.c
index 8dac6a4..6e76c3e 100644
--- a/drivers/spi/spi-loopback-test.c
+++ b/drivers/spi/spi-loopback-test.c
@@ -36,11 +36,17 @@ MODULE_PARM_DESC(simulate_only, "if not 0 do not execute the spi message");
 
 /* dump spi messages */
 int dump_messages;
+#define DUMP_MESSAGE_AFTER		BIT(0)
+#define DUMP_MESSAGE_BEFORE		BIT(1)
+#define DUMP_MESSAGE_DATA_AFTER		BIT(2)
+#define DUMP_MESSAGE_DATA_BEFORE	BIT(3)
 module_param(dump_messages, int, 0);
-MODULE_PARM_DESC(dump_message,
-		 "=1 dump the basic spi_message_structure, " \
-		 "=2 dump the spi_message_structure including data, " \
-		 "=3 dump the spi_message structure before and after execution");
+MODULE_PARM_DESC(dump_messages,
+		 "BIT(0) - dump the basic spi_message_structure after processing, "
+		 "BIT(1) - dump the basic spi_message_structure before processing, "
+		 "BIT(2) - also dump the spi_message data after processing, "
+		 "BIT(3) - also dump the spi_message data before processing");
+
 /* the device is jumpered for loopback - enabling some rx_buf tests */
 int loopback;
 module_param(loopback, int, 0);
@@ -749,9 +755,15 @@ int spi_test_execute_msg(struct spi_device *spi, struct spi_test *test,
 
 	/* only if we do not simulate */
 	if (!simulate_only) {
-		/* dump the complete message before and after the transfer */
-		if (dump_messages == 3)
-			spi_message_dump_data(spi, msg, 1024);
+		/* dump the complete message before the transfer */
+		if (dump_messages & DUMP_MESSAGE_BEFORE) {
+			/* calc bytes to dump */
+			if (dump_messages & DUMP_MESSAGE_DATA_BEFORE)
+				i = 1024;
+			else
+				i = 0;
+			spi_message_dump_data(spi, msg, i);
+		}
 
 		/* run spi message */
 		ret = spi_sync(spi, msg);
@@ -786,10 +798,16 @@ int spi_test_execute_msg(struct spi_device *spi, struct spi_test *test,
 
 	/* if requested or on error dump message (including data) */
 exit:
-	if (dump_messages || ret)
-		spi_message_dump_data(spi, msg,
-				      ((dump_messages >= 2) || (ret)) ?
-				      1024 : 0);
+	if ((dump_messages & DUMP_MESSAGE_AFTER) || ret) {
+		/* calc bytes to dump */
+		i = 0;
+		if (dump_messages & DUMP_MESSAGE_DATA_AFTER)
+			i = 1024;
+		if (ret)
+			i = 1024;
+		/* dump the message */
+		spi_message_dump_data(spi, msg, i);
+	}
 
 	return ret;
 }
-- 
1.7.10.4

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

[PATCH 4/4] spi: loopback-test: added support for HW-loopback mode

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

If the module parameter "loopback" for testing RX to match TX
is not set and the spi_master supports SPI_LOOP then
SPI_LOOP as well as RX-data testing will get enabled.

When the "loopback" module parameter is set
then the SPI_LOOP support in spi_master will not get enabled,
which means that MOSI needs to get connected to MISO by some
other means (possibly via a jumper connection or a SN74AHCT125)

Suggested-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-loopback-test.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Note: this has been tested by faking SPI_LOOP support
inside spi-bcm2835 (master->mode_bits |= SPI_LOOP;)
and retaining the external link between MOSI and MISO.

diff --git a/drivers/spi/spi-loopback-test.c b/drivers/spi/spi-loopback-test.c
index 6e76c3e..15207a7 100644
--- a/drivers/spi/spi-loopback-test.c
+++ b/drivers/spi/spi-loopback-test.c
@@ -289,7 +289,21 @@ static int spi_loopback_test_probe(struct spi_device *spi)
 {
 	int ret;

-	dev_info(&spi->dev, "Executing spi-loopback-tests\n");
+	/* enable HW-loopback automatically if the master supports it
+	 * and we have the loopback module parameter unset
+	 */
+	if ((!loopback) && (spi->master->mode_bits & SPI_LOOP)) {
+		spi->mode |= SPI_LOOP;
+		ret = spi_setup(spi);
+		if (ret)
+			dev_err(&spi->dev,
+				"spi_setup failed to enable loopback: %i\n",
+				ret);
+		loopback = 1;
+	}
+
+	dev_info(&spi->dev, "Executing spi-loopback-tests%s\n",
+		 loopback ? " with loopback tests enabled" : "");

 	ret = spi_test_run_tests(spi, spi_tests);

--
1.7.10.4

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

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

[Andy Shevchenko]

On Fri, Dec 18, 2015 at 2:44 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Martin Sperl]

> On 18.12.2015, at 14:32, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Fri, Dec 18, 2015 at 2:44 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Shevchenko]

On Fri, Dec 18, 2015 at 6:53 PM, Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
>> On 18.12.2015, at 14:32, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Fri, Dec 18, 2015 at 2:44 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] spi: loopback-test: make dump_messages module parameter a bitmask

[Andy Shevchenko]

On Fri, Dec 18, 2015 at 2:44 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>
> Make dump_messages module parameter a bitmask to allow for better
> granularity when dumping messages.
>
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> ---
>  drivers/spi/spi-loopback-test.c |   40 ++++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi-loopback-test.c b/drivers/spi/spi-loopback-test.c
> index 8dac6a4..6e76c3e 100644
> --- a/drivers/spi/spi-loopback-test.c
> +++ b/drivers/spi/spi-loopback-test.c
> @@ -36,11 +36,17 @@ MODULE_PARM_DESC(simulate_only, "if not 0 do not execute the spi message");
>
>  /* dump spi messages */
>  int dump_messages;
> +#define DUMP_MESSAGE_AFTER             BIT(0)
> +#define DUMP_MESSAGE_BEFORE            BIT(1)
> +#define DUMP_MESSAGE_DATA_AFTER                BIT(2)
> +#define DUMP_MESSAGE_DATA_BEFORE       BIT(3)

DUMP is used in lib/hexdump.c, I would suggest
SPI_DUMP_MSG_

>  module_param(dump_messages, int, 0);
> -MODULE_PARM_DESC(dump_message,
> -                "=1 dump the basic spi_message_structure, " \
> -                "=2 dump the spi_message_structure including data, " \
> -                "=3 dump the spi_message structure before and after execution");
> +MODULE_PARM_DESC(dump_messages,
> +                "BIT(0) - dump the basic spi_message_structure after processing, "
> +                "BIT(1) - dump the basic spi_message_structure before processing, "
> +                "BIT(2) - also dump the spi_message data after processing, "
> +                "BIT(3) - also dump the spi_message data before processing");

Logically it is exactly two bits, not four.

> +
>  /* the device is jumpered for loopback - enabling some rx_buf tests */
>  int loopback;
>  module_param(loopback, int, 0);
> @@ -749,9 +755,15 @@ int spi_test_execute_msg(struct spi_device *spi, struct spi_test *test,
>
>         /* only if we do not simulate */
>         if (!simulate_only) {
> -               /* dump the complete message before and after the transfer */
> -               if (dump_messages == 3)
> -                       spi_message_dump_data(spi, msg, 1024);
> +               /* dump the complete message before the transfer */
> +               if (dump_messages & DUMP_MESSAGE_BEFORE) {
> +                       /* calc bytes to dump */

> +                       if (dump_messages & DUMP_MESSAGE_DATA_BEFORE)
> +                               i = 1024;
> +                       else
> +                               i = 0;

Ternary operator ?

> +                       spi_message_dump_data(spi, msg, i);
> +               }
>
>                 /* run spi message */
>                 ret = spi_sync(spi, msg);
> @@ -786,10 +798,16 @@ int spi_test_execute_msg(struct spi_device *spi, struct spi_test *test,
>
>         /* if requested or on error dump message (including data) */
>  exit:
> -       if (dump_messages || ret)
> -               spi_message_dump_data(spi, msg,
> -                                     ((dump_messages >= 2) || (ret)) ?
> -                                     1024 : 0);
> +       if ((dump_messages & DUMP_MESSAGE_AFTER) || ret) {
> +               /* calc bytes to dump */

> +               i = 0;
> +               if (dump_messages & DUMP_MESSAGE_DATA_AFTER)
> +                       i = 1024;
> +               if (ret)
> +                       i = 1024;

Ditto.

i = (dump_messages & DUMP_MESSAGE_DATA_AFTER) || ret ? 1024 : 0;

> +               /* dump the message */
> +               spi_message_dump_data(spi, msg, i);
> +       }
>
>         return ret;
>  }
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: loopback-test: added support for HW-loopback mode

[Andy Shevchenko]

On Fri, Dec 18, 2015 at 2:44 PM,  <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>
> If the module parameter "loopback" for testing RX to match TX
> is not set and the spi_master supports SPI_LOOP then
> SPI_LOOP as well as RX-data testing will get enabled.
>
> When the "loopback" module parameter is set

I would suggest to distinguish somehow internal and external loopback
somehow, e.g. name it ext_loopback.

> then the SPI_LOOP support in spi_master will not get enabled,
> which means that MOSI needs to get connected to MISO by some
> other means (possibly via a jumper connection or a SN74AHCT125)
>
> Suggested-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> ---
>  drivers/spi/spi-loopback-test.c |   16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> Note: this has been tested by faking SPI_LOOP support
> inside spi-bcm2835 (master->mode_bits |= SPI_LOOP;)
> and retaining the external link between MOSI and MISO.
>
> diff --git a/drivers/spi/spi-loopback-test.c b/drivers/spi/spi-loopback-test.c
> index 6e76c3e..15207a7 100644
> --- a/drivers/spi/spi-loopback-test.c
> +++ b/drivers/spi/spi-loopback-test.c
> @@ -289,7 +289,21 @@ static int spi_loopback_test_probe(struct spi_device *spi)
>  {
>         int ret;
>
> -       dev_info(&spi->dev, "Executing spi-loopback-tests\n");
> +       /* enable HW-loopback automatically if the master supports it
> +        * and we have the loopback module parameter unset
> +        */

Capital letter and dot. check all your patches for that.
Moreover, block comment style in most cases looks like
/*
 * Text.
 */

> +       if ((!loopback) && (spi->master->mode_bits & SPI_LOOP)) {

Redundant parens for !.

> +               spi->mode |= SPI_LOOP;
> +               ret = spi_setup(spi);
> +               if (ret)
> +                       dev_err(&spi->dev,
> +                               "spi_setup failed to enable loopback: %i\n",
> +                               ret);
> +               loopback = 1;
> +       }
> +

> +       dev_info(&spi->dev, "Executing spi-loopback-tests%s\n",
> +                loopback ? " with loopback tests enabled" : "");

Perhaps "… with %s loopback enabled", ext_loopback ? "external" : "internal"

>
>         ret = spi_test_run_tests(spi, spi_tests);
>
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html