Message-ID: <1253520516.3925.143.camel@adam-desktop>
Date:	Mon, 21 Sep 2009 16:08:36 +0800
From:	Li Yi <yi.li@...log.com>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
CC:	Mike Frysinger <vapier.adi@...il.com>,
	spi-devel-general@...ts.sourceforge.net,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: RE: [spi-devel-general] [PATCH 1/2] spi: new SPI bus
 lock/unlockfunctions

On Fri, 2009-09-18 at 19:00 -0400, 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.
> 
> 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.
> 
Thanks for the feedback.

The original thought of returning '0' was trying not to break mmc_spi
driver on system without spi_lock_bus() implementation.
What do you think of a check in mmc_spi_probe() like this?

if (spi->master->num_chipselect > 1 && !spi->master->lock_bus) {
	struct count_children cc;

	cc.n = 0;
	cc.bus = spi->dev.bus;
	status = device_for_each_child(spi->dev.parent, &cc,
		maybe_count_child);
	if (status < 0) {
		dev_err(&spi->dev, "can't share SPI bus\n");
		return status;
	}

	dev_warn(&spi->dev, "ASSUMING SPI bus stays unshared!\n");
}

And spi_lock_bus() still returns 0 if not implemented.

> Also, as Andrew Morton pointed out, calling spi_unlock_bus() without having
> a valid lock by calling spi_lock_bus() is a bug.
> 
Will be fixed.

> 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.
> 
Will add code to check the return code. 
> >> 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.
> 
In spi_bf5xx.c, if a spi device (e.g, whose chip select number is "3")
locks the bus, the global status "locked" is set to "3" - indicates the
spi bus is locked.

> What happens to next_msg if it is for other devices on the bus?
> 
This spi message will be left in the queue until the spi bus is
unlocked. The messages for the spi device holding the lock will be
selected and transferred firstly. 

Regards,
-Yi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists