Re: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

[Mike Frysinger <vapier.adi@gmail.com>]

On Fri, Sep 18, 2009 at 19:00, H Hartley Sweeten wrote:
> On Thursday, September 17, 2009 3:54 PM, Mike Frysinger wrote:
>>> I assume the spi master driver must supply the {lock/unlock}_bus methods
>>> to properly support the locking.
>>
>> currently, yes.  a common solution would be nice.  ideas/patches welcome ;).
>>
>>> But, by returning 0 when the methods
>>> are not supplied you are basically saying all the current master drivers
>>> in mainline support bus locking.  I think this is really only "true" if
>>> spi->master->num_chipselect == 1.
>>
>> right, but that is no different from what we have today.  there is no
>> way for a client to guarantee exclusive access, so you cant write code
>> assuming it in the first place.  the only consumer thus far (mmc_spi)
>> actually errors out if such conditions exist.
>>
>> in other words, we arent breaking anything.
>
> Actually you are breaking the mmc_spi driver.

no we arent.  the current code does some sloppy checking for possible
concurrent access and then aborts.  the new code drops that sloppy
checking in place of the bus master doing it right.  any setup that is
working today will continue to work fine.  and setup *that is already
broken* will continue to be broken.

> By returning 0 when the methods are not supplied you are saying that the
> master driver supports and locked the bus.  At a minimum, I think spi_lock_bus()
> should return an error code if locking is not supported.
>
> Also, as Andrew Morton pointed out, calling spi_unlock_bus() without having
> a valid lock by calling spi_lock_bus() is a bug.
>
> In addition your patch to mmc_spi should check the return code from
> spi_lock_bus().  If the driver "requires" that the bus be locked it should
> trigger an error path if it cannot be locked.
>
>>> Also, do you have a master driver that does have the {lock/unlock}_bus
>>> methods?  I would like to see how you handled it.
>>
>> http://git.kernel.org/?p=linux/kernel/git/vapier/blackfin.git;a=commitdiff;h=cc54fa8ed63e11a000031bc650d41d57b441803d
>
> Oiy... The lock/unlock functions are simple enough but the change needed
> to bfin_spi_pump_messages() is a bit complicated.
>
> What happens to next_msg if it is for other devices on the bus?

Yi can take a stab at addressing these issues since he is the guy who
has been putting together in the first place.
-mike