Hi Grant On Sat, 17 Mar 2012, Grant Likely wrote:
> On Thu, 15 Mar 2012 22:32:14 +0100 (CET), Guennadi Liakhovetski > <[email protected]> wrote: > > By supporting the new SPI_BOARD_FLAG_RELEASE_CS flag we let underlying > > hardware drivers implement more efficient runtime power-management. > > > > Signed-off-by: Guennadi Liakhovetski <[email protected]> > > --- > > drivers/spi/spi-bitbang.c | 22 ++++++++++------------ > > 1 files changed, 10 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c > > index fe06e1b..c895d85 100644 > > --- a/drivers/spi/spi-bitbang.c > > +++ b/drivers/spi/spi-bitbang.c > > @@ -347,17 +347,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; > > @@ -367,7 +364,8 @@ static void bitbang_work(struct work_struct *work) > > * cs_change has hinted that the next message will probably > > * be for this chip too. > > */ > > - if (!(status == 0 && cs_change)) { > > + if (!(status == 0 && cs_change) || > > + spi->flags & SPI_BOARD_FLAG_RELEASE_CS) { > > That's rather heavy handed, and will probably break drivers that lock the spi > bus to guarantee concurrent messages (like the MMC layer). I don't think you > can do it this way. > > It might be okay to force CS disable if the spi bus hasn't been exclusively > locked. I know the comment talks about hinting (possibly for performance > reasonse), > but I'm more interested in correctness at this point. I understand your concerns about breaking existing configurations, but let us not forget, that this behaviour would only be activated by a new platform flag. This means, no existing set up should be affected and any new users should know what they are doing... But let's think about the bus-lock too. A comment in spi.c says: * This call should be used by drivers that require exclusive access to the * SPI bus. "Exclusive access" also means, that the chip-select stays active, which also means, the bus shall behave sanely, right? In other words, it shall not be suspended. It is easy to add a check in the above patch for master->bus_lock_flag, but we also probably want to retry releasing the CS, when the bus is unlocked. So, we need to inform the bus-driver, that the bus has just been released. We could add a callback for that, but wouldn't it be better to use runtime PM? Calls to spi_bus_lock() / unlock() are balanced, so, we could just add pm_runtime_get_sync(&master->dev) and pm_runtime_put(&master->dev) to them respectively? Would this be acceptable? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ------------------------------------------------------------------------------ For Developers, A Lot Can Happen In A Second. Boundary is the first to Know...and Tell You. Monitor Your Applications in Ultra-Fine Resolution. Try it FREE! http://p.sf.net/sfu/Boundary-d2dvs2 _______________________________________________ spi-devel-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/spi-devel-general
