[PATCH 0/3] spi: bitbang: use core message queue

[PATCH 0/3] spi: bitbang: use core message queue

[Guennadi Liakhovetski]

Hi

This is a re-spin of the "spi: bitbang: convert to using core message 
queue" patch, sent earlier to the list and 2 cosmetic improvement patches, 
that it depends on.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

[PATCH 1/3] spi: bitbang: avoid needless loop flow manipulations

[Guennadi Liakhovetski]

This patch makes a loop look cleaner by replacing a "break" and a "continue"
in its body by a single "if".

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi-bitbang.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index aef59b1..f5ae6e9 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -346,17 +346,14 @@ static void bitbang_work(struct work_struct *work)
 			if (t->delay_usecs)
 				udelay(t->delay_usecs);
 
-			if (!cs_change)
-				continue;
-			if (t->transfer_list.next == &m->transfers)
-				break;
-
-			/* sometimes a short mid-message deselect of the chip
-			 * may be needed to terminate a mode or command
-			 */
-			ndelay(nsecs);
-			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
-			ndelay(nsecs);
+			if (cs_change && !list_is_last(&t->transfer_list, &m->transfers)) {
+				/* sometimes a short mid-message deselect of the chip
+				 * may be needed to terminate a mode or command
+				 */
+				ndelay(nsecs);
+				bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
+				ndelay(nsecs);
+			}
 		}
 
 		m->status = status;
-- 
1.7.2.5


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH 1/3] spi: bitbang: avoid needless loop flow manipulations

[Grant Likely]

On Mon, 21 May 2012 13:25:13 +0200 (CEST), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> This patch makes a loop look cleaner by replacing a "break" and a "continue"
> in its body by a single "if".
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>

Queued for v3.6.

g.

> ---
>  drivers/spi/spi-bitbang.c |   19 ++++++++-----------
>  1 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> index aef59b1..f5ae6e9 100644
> --- a/drivers/spi/spi-bitbang.c
> +++ b/drivers/spi/spi-bitbang.c
> @@ -346,17 +346,14 @@ static void bitbang_work(struct work_struct *work)
>  			if (t->delay_usecs)
>  				udelay(t->delay_usecs);
>  
> -			if (!cs_change)
> -				continue;
> -			if (t->transfer_list.next == &m->transfers)
> -				break;
> -
> -			/* sometimes a short mid-message deselect of the chip
> -			 * may be needed to terminate a mode or command
> -			 */
> -			ndelay(nsecs);
> -			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> -			ndelay(nsecs);
> +			if (cs_change && !list_is_last(&t->transfer_list, &m->transfers)) {
> +				/* sometimes a short mid-message deselect of the chip
> +				 * may be needed to terminate a mode or command
> +				 */
> +				ndelay(nsecs);
> +				bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> +				ndelay(nsecs);
> +			}
>  		}
>  
>  		m->status = status;
> -- 
> 1.7.2.5
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

[PATCH 2/3] spi: bitbang: (cosmetic) simplify list manipulation

[Guennadi Liakhovetski]

Use a proper list iterator instead of an open-coded loop and remove a
superfluous list head initialisation.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi-bitbang.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index f5ae6e9..8b3d8ef 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -260,11 +260,11 @@ static void bitbang_work(struct work_struct *work)
 	struct spi_bitbang	*bitbang =
 		container_of(work, struct spi_bitbang, work);
 	unsigned long		flags;
+	struct spi_message	*m, *_m;
 
 	spin_lock_irqsave(&bitbang->lock, flags);
 	bitbang->busy = 1;
-	while (!list_empty(&bitbang->queue)) {
-		struct spi_message	*m;
+	list_for_each_entry_safe(m, _m, &bitbang->queue, queue) {
 		struct spi_device	*spi;
 		unsigned		nsecs;
 		struct spi_transfer	*t = NULL;
@@ -273,9 +273,7 @@ static void bitbang_work(struct work_struct *work)
 		int			status;
 		int			do_setup = -1;
 
-		m = container_of(bitbang->queue.next, struct spi_message,
-				queue);
-		list_del_init(&m->queue);
+		list_del(&m->queue);
 		spin_unlock_irqrestore(&bitbang->lock, flags);
 
 		/* FIXME this is made-up ... the correct value is known to
-- 
1.7.2.5


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH 2/3] spi: bitbang: (cosmetic) simplify list manipulation

[Grant Likely]

On Mon, 21 May 2012 13:25:17 +0200 (CEST), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> Use a proper list iterator instead of an open-coded loop and remove a
> superfluous list head initialisation.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> ---
>  drivers/spi/spi-bitbang.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)

Queued for v3.6, Thanks.

g.

> 
> diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> index f5ae6e9..8b3d8ef 100644
> --- a/drivers/spi/spi-bitbang.c
> +++ b/drivers/spi/spi-bitbang.c
> @@ -260,11 +260,11 @@ static void bitbang_work(struct work_struct *work)
>  	struct spi_bitbang	*bitbang =
>  		container_of(work, struct spi_bitbang, work);
>  	unsigned long		flags;
> +	struct spi_message	*m, *_m;
>  
>  	spin_lock_irqsave(&bitbang->lock, flags);
>  	bitbang->busy = 1;
> -	while (!list_empty(&bitbang->queue)) {
> -		struct spi_message	*m;
> +	list_for_each_entry_safe(m, _m, &bitbang->queue, queue) {
>  		struct spi_device	*spi;
>  		unsigned		nsecs;
>  		struct spi_transfer	*t = NULL;
> @@ -273,9 +273,7 @@ static void bitbang_work(struct work_struct *work)
>  		int			status;
>  		int			do_setup = -1;
>  
> -		m = container_of(bitbang->queue.next, struct spi_message,
> -				queue);
> -		list_del_init(&m->queue);
> +		list_del(&m->queue);
>  		spin_unlock_irqrestore(&bitbang->lock, flags);
>  
>  		/* FIXME this is made-up ... the correct value is known to
> -- 
> 1.7.2.5
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

[PATCH 3/3] spi: bitbang: convert to using core message queue

[Guennadi Liakhovetski]

The SPI subsystem core now manages message queues internally. Remove the
local message queue implementation from the spi-bitbang driver and
migrate to the common one.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi-bitbang.c       |  278 +++++++++++++++------------------------
 include/linux/spi/spi_bitbang.h |    7 -
 2 files changed, 107 insertions(+), 178 deletions(-)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index 8b3d8ef..e0f8a14 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -244,161 +244,124 @@ static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t)
 
 /*----------------------------------------------------------------------*/
 
-/*
- * SECOND PART ... simple transfer queue runner.
- *
- * This costs a task context per controller, running the queue by
- * performing each transfer in sequence.  Smarter hardware can queue
- * several DMA transfers at once, and process several controller queues
- * in parallel; this driver doesn't match such hardware very well.
- *
- * Drivers can provide word-at-a-time i/o primitives, or provide
- * transfer-at-a-time ones to leverage dma or fifo hardware.
- */
-static void bitbang_work(struct work_struct *work)
+static int spi_bitbang_transfer_one_message(struct spi_master *master,
+					    struct spi_message *msg)
 {
-	struct spi_bitbang	*bitbang =
-		container_of(work, struct spi_bitbang, work);
+	struct spi_bitbang	*bitbang = spi_master_get_devdata(master);
+	struct spi_device	*spi = msg->spi;
+	unsigned		nsecs;
+	struct spi_transfer	*t = NULL;
 	unsigned long		flags;
-	struct spi_message	*m, *_m;
+	unsigned		cs_change;
+	int			status;
+	int			do_setup = -1;
 
+	/* Protect against chip-select release in .setup() */
 	spin_lock_irqsave(&bitbang->lock, flags);
 	bitbang->busy = 1;
-	list_for_each_entry_safe(m, _m, &bitbang->queue, queue) {
-		struct spi_device	*spi;
-		unsigned		nsecs;
-		struct spi_transfer	*t = NULL;
-		unsigned		tmp;
-		unsigned		cs_change;
-		int			status;
-		int			do_setup = -1;
-
-		list_del(&m->queue);
-		spin_unlock_irqrestore(&bitbang->lock, flags);
-
-		/* FIXME this is made-up ... the correct value is known to
-		 * word-at-a-time bitbang code, and presumably chipselect()
-		 * should enforce these requirements too?
-		 */
-		nsecs = 100;
+	spin_unlock_irqrestore(&bitbang->lock, flags);
 
-		spi = m->spi;
-		tmp = 0;
-		cs_change = 1;
-		status = 0;
+	/* FIXME this is made-up ... the correct value is known to
+	 * word-at-a-time bitbang code, and presumably chipselect()
+	 * should enforce these requirements too?
+	 */
+	nsecs = 100;
 
-		list_for_each_entry (t, &m->transfers, transfer_list) {
-
-			/* override speed or wordsize? */
-			if (t->speed_hz || t->bits_per_word)
-				do_setup = 1;
-
-			/* init (-1) or override (1) transfer params */
-			if (do_setup != 0) {
-				status = bitbang->setup_transfer(spi, t);
-				if (status < 0)
-					break;
-				if (do_setup == -1)
-					do_setup = 0;
-			}
-
-			/* set up default clock polarity, and activate chip;
-			 * this implicitly updates clock and spi modes as
-			 * previously recorded for this device via setup().
-			 * (and also deselects any other chip that might be
-			 * selected ...)
-			 */
-			if (cs_change) {
-				bitbang->chipselect(spi, BITBANG_CS_ACTIVE);
-				ndelay(nsecs);
-			}
-			cs_change = t->cs_change;
-			if (!t->tx_buf && !t->rx_buf && t->len) {
-				status = -EINVAL;
-				break;
-			}
+	cs_change = 1;
+	status = 0;
 
-			/* transfer data.  the lower level code handles any
-			 * new dma mappings it needs. our caller always gave
-			 * us dma-safe buffers.
-			 */
-			if (t->len) {
-				/* REVISIT dma API still needs a designated
-				 * DMA_ADDR_INVALID; ~0 might be better.
-				 */
-				if (!m->is_dma_mapped)
-					t->rx_dma = t->tx_dma = 0;
-				status = bitbang->txrx_bufs(spi, t);
-			}
-			if (status > 0)
-				m->actual_length += status;
-			if (status != t->len) {
-				/* always report some kind of error */
-				if (status >= 0)
-					status = -EREMOTEIO;
+	list_for_each_entry (t, &msg->transfers, transfer_list) {
+
+		/* override speed or wordsize? */
+		if (t->speed_hz || t->bits_per_word)
+			do_setup = 1;
+
+		/* init (-1) or override (1) transfer params */
+		if (do_setup != 0) {
+			status = bitbang->setup_transfer(spi, t);
+			if (status < 0)
 				break;
-			}
-			status = 0;
-
-			/* protocol tweaks before next transfer */
-			if (t->delay_usecs)
-				udelay(t->delay_usecs);
-
-			if (cs_change && !list_is_last(&t->transfer_list, &m->transfers)) {
-				/* sometimes a short mid-message deselect of the chip
-				 * may be needed to terminate a mode or command
-				 */
-				ndelay(nsecs);
-				bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
-				ndelay(nsecs);
-			}
+			if (do_setup == -1)
+				do_setup = 0;
 		}
 
-		m->status = status;
-		m->complete(m->context);
+		/* set up default clock polarity, and activate chip;
+		 * this implicitly updates clock and spi modes as
+		 * previously recorded for this device via setup().
+		 * (and also deselects any other chip that might be
+		 * selected ...)
+		 */
+		if (cs_change) {
+			bitbang->chipselect(spi, BITBANG_CS_ACTIVE);
+			ndelay(nsecs);
+		}
+		cs_change = t->cs_change;
+		if (!t->tx_buf && !t->rx_buf && t->len) {
+			status = -EINVAL;
+			break;
+		}
 
-		/* normally deactivate chipselect ... unless no error and
-		 * cs_change has hinted that the next message will probably
-		 * be for this chip too.
+		/* transfer data.  the lower level code handles any
+		 * new dma mappings it needs. our caller always gave
+		 * us dma-safe buffers.
 		 */
-		if (!(status == 0 && cs_change)) {
+		if (t->len) {
+			/* REVISIT dma API still needs a designated
+			 * DMA_ADDR_INVALID; ~0 might be better.
+			 */
+			if (!msg->is_dma_mapped)
+				t->rx_dma = t->tx_dma = 0;
+			status = bitbang->txrx_bufs(spi, t);
+		}
+		if (status > 0)
+			msg->actual_length += status;
+		if (status != t->len) {
+			/* always report some kind of error */
+			if (status >= 0)
+				status = -EREMOTEIO;
+			break;
+		}
+		status = 0;
+
+		/* protocol tweaks before next transfer */
+		if (t->delay_usecs)
+			udelay(t->delay_usecs);
+
+		if (cs_change && !list_is_last(&t->transfer_list, &msg->transfers)) {
+			/* sometimes a short mid-message deselect of the chip
+			 * may be needed to terminate a mode or command
+			 */
 			ndelay(nsecs);
 			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
 			ndelay(nsecs);
 		}
-
-		spin_lock_irqsave(&bitbang->lock, flags);
 	}
-	bitbang->busy = 0;
-	spin_unlock_irqrestore(&bitbang->lock, flags);
-}
 
-/**
- * spi_bitbang_transfer - default submit to transfer queue
- */
-int spi_bitbang_transfer(struct spi_device *spi, struct spi_message *m)
-{
-	struct spi_bitbang	*bitbang;
-	unsigned long		flags;
-	int			status = 0;
-
-	m->actual_length = 0;
-	m->status = -EINPROGRESS;
+	msg->status = status;
 
-	bitbang = spi_master_get_devdata(spi->master);
+	/* normally deactivate chipselect ... unless no error and
+	 * cs_change has hinted that the next message will probably
+	 * be for this chip too.
+	 */
+	if (!(status == 0 && cs_change)) {
+		ndelay(nsecs);
+		bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
+		ndelay(nsecs);
+	}
 
 	spin_lock_irqsave(&bitbang->lock, flags);
-	if (!spi->max_speed_hz)
-		status = -ENETDOWN;
-	else {
-		list_add_tail(&m->queue, &bitbang->queue);
-		queue_work(bitbang->workqueue, &bitbang->work);
-	}
+	bitbang->busy = 0;
 	spin_unlock_irqrestore(&bitbang->lock, flags);
 
-	return status;
+	spi_finalize_current_message(master);
+
+	return 0;
+}
+
+static int spi_bitbang_dummy_prepare(struct spi_master *master)
+{
+	return 0;
 }
-EXPORT_SYMBOL_GPL(spi_bitbang_transfer);
 
 /*----------------------------------------------------------------------*/
 
@@ -407,9 +370,7 @@ EXPORT_SYMBOL_GPL(spi_bitbang_transfer);
  * @bitbang: driver handle
  *
  * Caller should have zero-initialized all parts of the structure, and then
- * provided callbacks for chip selection and I/O loops.  If the master has
- * a transfer method, its final step should call spi_bitbang_transfer; or,
- * that's the default if the transfer routine is not initialized.  It should
+ * provided callbacks for chip selection and I/O loops.  It should
  * also set up the bus number and number of chipselects.
  *
  * For i/o loops, provide callbacks either per-word (for bitbanging, or for
@@ -427,58 +388,37 @@ EXPORT_SYMBOL_GPL(spi_bitbang_transfer);
  */
 int spi_bitbang_start(struct spi_bitbang *bitbang)
 {
-	int	status;
+	struct spi_master *master = bitbang->master;
 
-	if (!bitbang->master || !bitbang->chipselect)
+	if (!master || !bitbang->chipselect)
 		return -EINVAL;
 
-	INIT_WORK(&bitbang->work, bitbang_work);
 	spin_lock_init(&bitbang->lock);
-	INIT_LIST_HEAD(&bitbang->queue);
 
-	if (!bitbang->master->mode_bits)
-		bitbang->master->mode_bits = SPI_CPOL | SPI_CPHA | bitbang->flags;
+	if (!master->mode_bits)
+		master->mode_bits = SPI_CPOL | SPI_CPHA | bitbang->flags;
+
+	master->transfer_one_message = spi_bitbang_transfer_one_message;
+	master->prepare_transfer_hardware = spi_bitbang_dummy_prepare;
+	master->unprepare_transfer_hardware = spi_bitbang_dummy_prepare;
 
-	if (!bitbang->master->transfer)
-		bitbang->master->transfer = spi_bitbang_transfer;
 	if (!bitbang->txrx_bufs) {
 		bitbang->use_dma = 0;
 		bitbang->txrx_bufs = spi_bitbang_bufs;
-		if (!bitbang->master->setup) {
+		if (!master->setup) {
 			if (!bitbang->setup_transfer)
 				bitbang->setup_transfer =
 					 spi_bitbang_setup_transfer;
-			bitbang->master->setup = spi_bitbang_setup;
-			bitbang->master->cleanup = spi_bitbang_cleanup;
+			master->setup = spi_bitbang_setup;
+			master->cleanup = spi_bitbang_cleanup;
 		}
-	} else if (!bitbang->master->setup)
-		return -EINVAL;
-	if (bitbang->master->transfer == spi_bitbang_transfer &&
-			!bitbang->setup_transfer)
+	} else if (!master->setup)
 		return -EINVAL;
 
-	/* this task is the only thing to touch the SPI bits */
-	bitbang->busy = 0;
-	bitbang->workqueue = create_singlethread_workqueue(
-			dev_name(bitbang->master->dev.parent));
-	if (bitbang->workqueue == NULL) {
-		status = -EBUSY;
-		goto err1;
-	}
-
 	/* driver may get busy before register() returns, especially
 	 * if someone registered boardinfo for devices
 	 */
-	status = spi_register_master(bitbang->master);
-	if (status < 0)
-		goto err2;
-
-	return status;
-
-err2:
-	destroy_workqueue(bitbang->workqueue);
-err1:
-	return status;
+	return spi_register_master(master);
 }
 EXPORT_SYMBOL_GPL(spi_bitbang_start);
 
@@ -489,10 +429,6 @@ int spi_bitbang_stop(struct spi_bitbang *bitbang)
 {
 	spi_unregister_master(bitbang->master);
 
-	WARN_ON(!list_empty(&bitbang->queue));
-
-	destroy_workqueue(bitbang->workqueue);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(spi_bitbang_stop);
diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
index f987a2b..f85a7b8 100644
--- a/include/linux/spi/spi_bitbang.h
+++ b/include/linux/spi/spi_bitbang.h
@@ -1,14 +1,8 @@
 #ifndef	__SPI_BITBANG_H
 #define	__SPI_BITBANG_H
 
-#include <linux/workqueue.h>
-
 struct spi_bitbang {
-	struct workqueue_struct	*workqueue;
-	struct work_struct	work;
-
 	spinlock_t		lock;
-	struct list_head	queue;
 	u8			busy;
 	u8			use_dma;
 	u8			flags;		/* extra spi->mode support */
@@ -41,7 +35,6 @@ struct spi_bitbang {
  */
 extern int spi_bitbang_setup(struct spi_device *spi);
 extern void spi_bitbang_cleanup(struct spi_device *spi);
-extern int spi_bitbang_transfer(struct spi_device *spi, struct spi_message *m);
 extern int spi_bitbang_setup_transfer(struct spi_device *spi,
 				      struct spi_transfer *t);
 
-- 
1.7.2.5


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH 3/3] spi: bitbang: convert to using core message queue

[Linus Walleij]

On Mon, May 21, 2012 at 1:25 PM, Guennadi Liakhovetski
<g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:

> The SPI subsystem core now manages message queues internally. Remove the
> local message queue implementation from the spi-bitbang driver and
> migrate to the common one.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>

Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Thanks,
Linus Walleij

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH 3/3] spi: bitbang: convert to using core message queue

[Grant Likely]

On Mon, 21 May 2012 13:25:23 +0200 (CEST), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> The SPI subsystem core now manages message queues internally. Remove the
> local message queue implementation from the spi-bitbang driver and
> migrate to the common one.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>

Okay, I'm going to be really nitpicky here.  The change is fine, but the
diff is too big; particularly for anyone trying to figure out what has
changed if it causes breakage.  I wouldn't worry about this as much
for an individual driver, but spi_bitbang is common infrastructure.

For example, "struct spi_master *master = bitbang->master;" is added
to spi_bitbang start, and then all the references are changed from
bitbang->master to master; bloating the diffstat (with the -w flag to
ignore whitespace changes).  I have no problem with that change, but
it must be in a seperate patch so readers have a fighting chance to
understand what actually changed functionally.

Ditto in spi_bitbang_transfer_one_message which changes 'm' to 'msg'.
Ditto in spi_bitbang_transfer_one_message moving the assignment of
spi = m->spi.

There are other things that change unnecessarily.  Take a look at the
output of git show -w and try to cut down the diff.

Here is the diff of things that I've undone to make the diffstat a lot
smaller:

g.

> ---
>  drivers/spi/spi-bitbang.c       |  278 +++++++++++++++------------------------
>  include/linux/spi/spi_bitbang.h |    7 -
>  2 files changed, 107 insertions(+), 178 deletions(-)
> 
> diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> index 8b3d8ef..e0f8a14 100644
> --- a/drivers/spi/spi-bitbang.c
> +++ b/drivers/spi/spi-bitbang.c
> @@ -244,161 +244,124 @@ static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t)
>  
>  /*----------------------------------------------------------------------*/
>  
> -/*
> - * SECOND PART ... simple transfer queue runner.
> - *
> - * This costs a task context per controller, running the queue by
> - * performing each transfer in sequence.  Smarter hardware can queue
> - * several DMA transfers at once, and process several controller queues
> - * in parallel; this driver doesn't match such hardware very well.
> - *
> - * Drivers can provide word-at-a-time i/o primitives, or provide
> - * transfer-at-a-time ones to leverage dma or fifo hardware.
> - */
> -static void bitbang_work(struct work_struct *work)
> +static int spi_bitbang_transfer_one_message(struct spi_master *master,
> +					    struct spi_message *msg)
>  {
> -	struct spi_bitbang	*bitbang =
> -		container_of(work, struct spi_bitbang, work);
> +	struct spi_bitbang	*bitbang = spi_master_get_devdata(master);
> +	struct spi_device	*spi = msg->spi;
> +	unsigned		nsecs;
> +	struct spi_transfer	*t = NULL;
>  	unsigned long		flags;
> -	struct spi_message	*m, *_m;
> +	unsigned		cs_change;
> +	int			status;
> +	int			do_setup = -1;
>  
> +	/* Protect against chip-select release in .setup() */
>  	spin_lock_irqsave(&bitbang->lock, flags);
>  	bitbang->busy = 1;
> -	list_for_each_entry_safe(m, _m, &bitbang->queue, queue) {
> -		struct spi_device	*spi;
> -		unsigned		nsecs;
> -		struct spi_transfer	*t = NULL;
> -		unsigned		tmp;
> -		unsigned		cs_change;
> -		int			status;
> -		int			do_setup = -1;
> -
> -		list_del(&m->queue);
> -		spin_unlock_irqrestore(&bitbang->lock, flags);
> -
> -		/* FIXME this is made-up ... the correct value is known to
> -		 * word-at-a-time bitbang code, and presumably chipselect()
> -		 * should enforce these requirements too?
> -		 */
> -		nsecs = 100;
> +	spin_unlock_irqrestore(&bitbang->lock, flags);
>  
> -		spi = m->spi;
> -		tmp = 0;
> -		cs_change = 1;
> -		status = 0;
> +	/* FIXME this is made-up ... the correct value is known to
> +	 * word-at-a-time bitbang code, and presumably chipselect()
> +	 * should enforce these requirements too?
> +	 */
> +	nsecs = 100;
>  
> -		list_for_each_entry (t, &m->transfers, transfer_list) {
> -
> -			/* override speed or wordsize? */
> -			if (t->speed_hz || t->bits_per_word)
> -				do_setup = 1;
> -
> -			/* init (-1) or override (1) transfer params */
> -			if (do_setup != 0) {
> -				status = bitbang->setup_transfer(spi, t);
> -				if (status < 0)
> -					break;
> -				if (do_setup == -1)
> -					do_setup = 0;
> -			}
> -
> -			/* set up default clock polarity, and activate chip;
> -			 * this implicitly updates clock and spi modes as
> -			 * previously recorded for this device via setup().
> -			 * (and also deselects any other chip that might be
> -			 * selected ...)
> -			 */
> -			if (cs_change) {
> -				bitbang->chipselect(spi, BITBANG_CS_ACTIVE);
> -				ndelay(nsecs);
> -			}
> -			cs_change = t->cs_change;
> -			if (!t->tx_buf && !t->rx_buf && t->len) {
> -				status = -EINVAL;
> -				break;
> -			}
> +	cs_change = 1;
> +	status = 0;
>  
> -			/* transfer data.  the lower level code handles any
> -			 * new dma mappings it needs. our caller always gave
> -			 * us dma-safe buffers.
> -			 */
> -			if (t->len) {
> -				/* REVISIT dma API still needs a designated
> -				 * DMA_ADDR_INVALID; ~0 might be better.
> -				 */
> -				if (!m->is_dma_mapped)
> -					t->rx_dma = t->tx_dma = 0;
> -				status = bitbang->txrx_bufs(spi, t);
> -			}
> -			if (status > 0)
> -				m->actual_length += status;
> -			if (status != t->len) {
> -				/* always report some kind of error */
> -				if (status >= 0)
> -					status = -EREMOTEIO;
> +	list_for_each_entry (t, &msg->transfers, transfer_list) {
> +
> +		/* override speed or wordsize? */
> +		if (t->speed_hz || t->bits_per_word)
> +			do_setup = 1;
> +
> +		/* init (-1) or override (1) transfer params */
> +		if (do_setup != 0) {
> +			status = bitbang->setup_transfer(spi, t);
> +			if (status < 0)
>  				break;
> -			}
> -			status = 0;
> -
> -			/* protocol tweaks before next transfer */
> -			if (t->delay_usecs)
> -				udelay(t->delay_usecs);
> -
> -			if (cs_change && !list_is_last(&t->transfer_list, &m->transfers)) {
> -				/* sometimes a short mid-message deselect of the chip
> -				 * may be needed to terminate a mode or command
> -				 */
> -				ndelay(nsecs);
> -				bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> -				ndelay(nsecs);
> -			}
> +			if (do_setup == -1)
> +				do_setup = 0;
>  		}
>  
> -		m->status = status;
> -		m->complete(m->context);
> +		/* set up default clock polarity, and activate chip;
> +		 * this implicitly updates clock and spi modes as
> +		 * previously recorded for this device via setup().
> +		 * (and also deselects any other chip that might be
> +		 * selected ...)
> +		 */
> +		if (cs_change) {
> +			bitbang->chipselect(spi, BITBANG_CS_ACTIVE);
> +			ndelay(nsecs);
> +		}
> +		cs_change = t->cs_change;
> +		if (!t->tx_buf && !t->rx_buf && t->len) {
> +			status = -EINVAL;
> +			break;
> +		}
>  
> -		/* normally deactivate chipselect ... unless no error and
> -		 * cs_change has hinted that the next message will probably
> -		 * be for this chip too.
> +		/* transfer data.  the lower level code handles any
> +		 * new dma mappings it needs. our caller always gave
> +		 * us dma-safe buffers.
>  		 */
> -		if (!(status == 0 && cs_change)) {
> +		if (t->len) {
> +			/* REVISIT dma API still needs a designated
> +			 * DMA_ADDR_INVALID; ~0 might be better.
> +			 */
> +			if (!msg->is_dma_mapped)
> +				t->rx_dma = t->tx_dma = 0;
> +			status = bitbang->txrx_bufs(spi, t);
> +		}
> +		if (status > 0)
> +			msg->actual_length += status;
> +		if (status != t->len) {
> +			/* always report some kind of error */
> +			if (status >= 0)
> +				status = -EREMOTEIO;
> +			break;
> +		}
> +		status = 0;
> +
> +		/* protocol tweaks before next transfer */
> +		if (t->delay_usecs)
> +			udelay(t->delay_usecs);
> +
> +		if (cs_change && !list_is_last(&t->transfer_list, &msg->transfers)) {
> +			/* sometimes a short mid-message deselect of the chip
> +			 * may be needed to terminate a mode or command
> +			 */
>  			ndelay(nsecs);
>  			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
>  			ndelay(nsecs);
>  		}
> -
> -		spin_lock_irqsave(&bitbang->lock, flags);
>  	}
> -	bitbang->busy = 0;
> -	spin_unlock_irqrestore(&bitbang->lock, flags);
> -}
>  
> -/**
> - * spi_bitbang_transfer - default submit to transfer queue
> - */
> -int spi_bitbang_transfer(struct spi_device *spi, struct spi_message *m)
> -{
> -	struct spi_bitbang	*bitbang;
> -	unsigned long		flags;
> -	int			status = 0;
> -
> -	m->actual_length = 0;
> -	m->status = -EINPROGRESS;
> +	msg->status = status;
>  
> -	bitbang = spi_master_get_devdata(spi->master);
> +	/* normally deactivate chipselect ... unless no error and
> +	 * cs_change has hinted that the next message will probably
> +	 * be for this chip too.
> +	 */
> +	if (!(status == 0 && cs_change)) {
> +		ndelay(nsecs);
> +		bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
> +		ndelay(nsecs);
> +	}
>  
>  	spin_lock_irqsave(&bitbang->lock, flags);
> -	if (!spi->max_speed_hz)
> -		status = -ENETDOWN;
> -	else {
> -		list_add_tail(&m->queue, &bitbang->queue);
> -		queue_work(bitbang->workqueue, &bitbang->work);
> -	}
> +	bitbang->busy = 0;
>  	spin_unlock_irqrestore(&bitbang->lock, flags);
>  
> -	return status;
> +	spi_finalize_current_message(master);
> +
> +	return 0;
> +}
> +
> +static int spi_bitbang_dummy_prepare(struct spi_master *master)
> +{
> +	return 0;
>  }
> -EXPORT_SYMBOL_GPL(spi_bitbang_transfer);
>  
>  /*----------------------------------------------------------------------*/
>  
> @@ -407,9 +370,7 @@ EXPORT_SYMBOL_GPL(spi_bitbang_transfer);
>   * @bitbang: driver handle
>   *
>   * Caller should have zero-initialized all parts of the structure, and then
> - * provided callbacks for chip selection and I/O loops.  If the master has
> - * a transfer method, its final step should call spi_bitbang_transfer; or,
> - * that's the default if the transfer routine is not initialized.  It should
> + * provided callbacks for chip selection and I/O loops.  It should
>   * also set up the bus number and number of chipselects.
>   *
>   * For i/o loops, provide callbacks either per-word (for bitbanging, or for
> @@ -427,58 +388,37 @@ EXPORT_SYMBOL_GPL(spi_bitbang_transfer);
>   */
>  int spi_bitbang_start(struct spi_bitbang *bitbang)
>  {
> -	int	status;
> +	struct spi_master *master = bitbang->master;
>  
> -	if (!bitbang->master || !bitbang->chipselect)
> +	if (!master || !bitbang->chipselect)
>  		return -EINVAL;
>  
> -	INIT_WORK(&bitbang->work, bitbang_work);
>  	spin_lock_init(&bitbang->lock);
> -	INIT_LIST_HEAD(&bitbang->queue);
>  
> -	if (!bitbang->master->mode_bits)
> -		bitbang->master->mode_bits = SPI_CPOL | SPI_CPHA | bitbang->flags;
> +	if (!master->mode_bits)
> +		master->mode_bits = SPI_CPOL | SPI_CPHA | bitbang->flags;
> +
> +	master->transfer_one_message = spi_bitbang_transfer_one_message;
> +	master->prepare_transfer_hardware = spi_bitbang_dummy_prepare;
> +	master->unprepare_transfer_hardware = spi_bitbang_dummy_prepare;
>  
> -	if (!bitbang->master->transfer)
> -		bitbang->master->transfer = spi_bitbang_transfer;
>  	if (!bitbang->txrx_bufs) {
>  		bitbang->use_dma = 0;
>  		bitbang->txrx_bufs = spi_bitbang_bufs;
> -		if (!bitbang->master->setup) {
> +		if (!master->setup) {
>  			if (!bitbang->setup_transfer)
>  				bitbang->setup_transfer =
>  					 spi_bitbang_setup_transfer;
> -			bitbang->master->setup = spi_bitbang_setup;
> -			bitbang->master->cleanup = spi_bitbang_cleanup;
> +			master->setup = spi_bitbang_setup;
> +			master->cleanup = spi_bitbang_cleanup;
>  		}
> -	} else if (!bitbang->master->setup)
> -		return -EINVAL;
> -	if (bitbang->master->transfer == spi_bitbang_transfer &&
> -			!bitbang->setup_transfer)
> +	} else if (!master->setup)
>  		return -EINVAL;
>  
> -	/* this task is the only thing to touch the SPI bits */
> -	bitbang->busy = 0;
> -	bitbang->workqueue = create_singlethread_workqueue(
> -			dev_name(bitbang->master->dev.parent));
> -	if (bitbang->workqueue == NULL) {
> -		status = -EBUSY;
> -		goto err1;
> -	}
> -
>  	/* driver may get busy before register() returns, especially
>  	 * if someone registered boardinfo for devices
>  	 */
> -	status = spi_register_master(bitbang->master);
> -	if (status < 0)
> -		goto err2;
> -
> -	return status;
> -
> -err2:
> -	destroy_workqueue(bitbang->workqueue);
> -err1:
> -	return status;
> +	return spi_register_master(master);
>  }
>  EXPORT_SYMBOL_GPL(spi_bitbang_start);
>  
> @@ -489,10 +429,6 @@ int spi_bitbang_stop(struct spi_bitbang *bitbang)
>  {
>  	spi_unregister_master(bitbang->master);
>  
> -	WARN_ON(!list_empty(&bitbang->queue));
> -
> -	destroy_workqueue(bitbang->workqueue);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(spi_bitbang_stop);
> diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
> index f987a2b..f85a7b8 100644
> --- a/include/linux/spi/spi_bitbang.h
> +++ b/include/linux/spi/spi_bitbang.h
> @@ -1,14 +1,8 @@
>  #ifndef	__SPI_BITBANG_H
>  #define	__SPI_BITBANG_H
>  
> -#include <linux/workqueue.h>
> -
>  struct spi_bitbang {
> -	struct workqueue_struct	*workqueue;
> -	struct work_struct	work;
> -
>  	spinlock_t		lock;
> -	struct list_head	queue;
>  	u8			busy;
>  	u8			use_dma;
>  	u8			flags;		/* extra spi->mode support */
> @@ -41,7 +35,6 @@ struct spi_bitbang {
>   */
>  extern int spi_bitbang_setup(struct spi_device *spi);
>  extern void spi_bitbang_cleanup(struct spi_device *spi);
> -extern int spi_bitbang_transfer(struct spi_device *spi, struct spi_message *m);
>  extern int spi_bitbang_setup_transfer(struct spi_device *spi,
>  				      struct spi_transfer *t);
>  
> -- 
> 1.7.2.5
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH 3/3] spi: bitbang: convert to using core message queue

[Linus Walleij]

On Sat, May 26, 2012 at 1:29 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Mon, 21 May 2012 13:25:23 +0200 (CEST), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
>> The SPI subsystem core now manages message queues internally. Remove the
>> local message queue implementation from the spi-bitbang driver and
>> migrate to the common one.
>>
>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
>
> Okay, I'm going to be really nitpicky here.  The change is fine, but the
> diff is too big; particularly for anyone trying to figure out what has
> changed if it causes breakage.  I wouldn't worry about this as much
> for an individual driver, but spi_bitbang is common infrastructure.

Did this ever get around to get fixed? I guess not....

Guennadi, are you going to fix the patch according to Grant's
requests or should I try to clean it and resubmit? The problem
is that I have no real way of testing this. But if I break something
I guess people will notice :-D

Yours,
Linus Walleij

------------------------------------------------------------------------------
Master SQL Server Development, Administration, T-SQL, SSAS, SSIS, SSRS
and more. Get SQL Server skills now (including 2012) with LearnDevNow -
200+ hours of step-by-step video tutorials by Microsoft MVPs and experts.
SALE $99.99 this month only - learn more at:
http://p.sf.net/sfu/learnmore_122512

Re: [PATCH 3/3] spi: bitbang: convert to using core message queue

[Guennadi Liakhovetski]

Hi Linus

On Tue, 8 Jan 2013, Linus Walleij wrote:

> On Sat, May 26, 2012 at 1:29 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> > On Mon, 21 May 2012 13:25:23 +0200 (CEST), Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> >> The SPI subsystem core now manages message queues internally. Remove the
> >> local message queue implementation from the spi-bitbang driver and
> >> migrate to the common one.
> >>
> >> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> >
> > Okay, I'm going to be really nitpicky here.  The change is fine, but the
> > diff is too big; particularly for anyone trying to figure out what has
> > changed if it causes breakage.  I wouldn't worry about this as much
> > for an individual driver, but spi_bitbang is common infrastructure.
> 
> Did this ever get around to get fixed? I guess not....
> 
> Guennadi, are you going to fix the patch according to Grant's
> requests or should I try to clean it and resubmit? The problem
> is that I have no real way of testing this. But if I break something
> I guess people will notice :-D

Thanks for your suggestion and your interest in this patch. As you 
probably have seen by now, it turns out, that just mechanical splitting of 
this patch didn't result in a complete success, so, committing it without 
testing probably wouldn't be a very good idea...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

------------------------------------------------------------------------------
Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery
and much more. Keep your Java skills current with LearnJavaNow -
200+ hours of step-by-step video tutorials by Java experts.
SALE $49.99 this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122612