Hi Grant On Tue, 20 Mar 2012, Grant Likely wrote:
> On Mon, 19 Mar 2012 00:33:14 +0100, Linus Walleij <[email protected]> > wrote: > > On Sat, Mar 17, 2012 at 12:39 AM, Guennadi Liakhovetski > > <[email protected]> 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 <[email protected]> > > > > This is great stuff! > > Acked-by: Linus Walleij <[email protected]> > > > > (Since I've never really used the bitbang driver I wouldn't trust me to > > do any deeper review.) > > In hindsite; I should have asked you to make the pl driver use bitbang > queueing since that was already used for generic queuing by a number > of drivers; and then refactored that to perform better. Doing that > would have been refactoring better tested code, but oh well. > > I'm going to ignore this series for the moment; please remind me to > look at it after the merge window has closed. Seems to be time:-) Thanks Guennadi > > g. > > > > > Quick thought: > > > > > +static int spi_bitbang_dummy_prepare(struct spi_master *master) > > > +{ > > > + Â Â Â return 0; > > (...) > > > + Â Â Â master->prepare_transfer_hardware = spi_bitbang_dummy_prepare; > > > + Â Â Â master->unprepare_transfer_hardware = > > > spi_bitbang_dummy_prepare; > > > > Maybe we should think about making these two hooks optional if this > > pattern turns up a lot. I'll try to fix some refactoring further down the > > road if nobody beats me to it. > > > > Yours, > > Linus Walleij > From: Grant Likely <[email protected]> > Subject: Re: [PATCH] spi: bitbang: convert to using core message queue > To: Guennadi Liakhovetski <[email protected]>, Linus Walleij > <[email protected]> > Cc: [email protected], Magnus Damm > <[email protected]> > In-Reply-To: <[email protected]> > References: <[email protected]> > <20120315092923.951203E04FD@localhost> > <cacrpkdznqzyx-hzjzymctpuqjtnymwq5ksde6rtfwkchuz_...@mail.gmail.com> > <[email protected]> > > On Sat, 17 Mar 2012 00:39:44 +0100 (CET), Guennadi Liakhovetski > <[email protected]> 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 <[email protected]> > > --- > > > > On Fri, 16 Mar 2012, Linus Walleij wrote: > > > > > On Thu, Mar 15, 2012 at 10:29 AM, Grant Likely > > > <[email protected]> wrote: > > > > On Wed, 14 Mar 2012 22:04:25 +0100 (CET), Guennadi Liakhovetski > > > > <[email protected]> wrote: > > > >> This patch adds a PM QoS requirement to the spi-bitbang driver, > > > >> preventing > > > >> the underlying SPI hardware driver to suspend for too long a time, as > > > >> long > > > >> as there are transfers on the queue. > > > >> > > > >> Signed-off-by: Guennadi Liakhovetski <[email protected]> > > > > > > > > Shouldn't this be part of the core spi infrastructure? Â Particularly > > > > since queuing > > > > is moving into the core. > > > > > > Hm, the spi-bitbang driver needs to be converted to use the central > > > message queue, Guennadi can you look into that since you're using it? > > > > > > The spi-pl022.c and also Samsung spi-s3c64xx.c is converted showing > > > examples on how go about with that. > > > > Something like this? Tested with spi-sh-msiof on SuperH and sh-mobile ARM > > and with spi-gpio. > > > > drivers/spi/spi-bitbang.c | 280 > > +++++++++++++++------------------------ > > include/linux/spi/spi_bitbang.h | 7 - > > 2 files changed, 108 insertions(+), 179 deletions(-) > > > > diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c > > index 6c0ec81..5f163d9 100644 > > --- a/drivers/spi/spi-bitbang.c > > +++ b/drivers/spi/spi-bitbang.c > > @@ -244,162 +244,125 @@ 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) || > > - spi->flags & SPI_BOARD_FLAG_RELEASE_CS) { > > + 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) || > > + spi->flags & SPI_BOARD_FLAG_RELEASE_CS) { > > + 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); > > > > /*----------------------------------------------------------------------*/ > > > > @@ -408,9 +371,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 > > @@ -428,58 +389,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); > > > > @@ -490,10 +430,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. > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ------------------------------------------------------------------------------ Better than sec? Nothing is better than sec when it comes to monitoring Big Data applications. Try Boundary one-second resolution app monitoring today. Free. http://p.sf.net/sfu/Boundary-dev2dev _______________________________________________ spi-devel-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/spi-devel-general
