linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Li Yi <yi.li@analog.com>
To: H Hartley Sweeten <hartleys@visionengravers.com>
Cc: Mike Frysinger <vapier.adi@gmail.com>,
	spi-devel-general@lists.sourceforge.net,
	David Brownell <dbrownell@users.sourceforge.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org
Subject: RE: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions
Date: Mon, 21 Sep 2009 16:08:36 +0800	[thread overview]
Message-ID: <1253520516.3925.143.camel@adam-desktop> (raw)
In-Reply-To: <BD79186B4FD85F4B8E60E381CAEE190901D41FB1@mi8nycmail19.Mi8.com>

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

  parent reply	other threads:[~2009-09-21  8:08 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-09-17 22:03 [PATCH 1/2] spi: new SPI bus lock/unlock functions Mike Frysinger
2009-09-17 22:03 ` [PATCH 2/2] mmc_spi: lock the SPI bus when accessing the card Mike Frysinger
2009-09-18  0:29   ` [spi-devel-general] [PATCH 2/2] mmc_spi: lock the SPI bus whenaccessing " H Hartley Sweeten
2009-09-18  0:52     ` [PATCH 2/2 v2] mmc_spi: lock the SPI bus when accessing " Mike Frysinger
2009-09-18 21:30       ` Andrew Morton
2009-09-17 22:45 ` [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions H Hartley Sweeten
2009-09-17 22:53   ` Mike Frysinger
2009-09-18 23:00     ` H Hartley Sweeten
2009-09-19 19:11       ` Mike Frysinger
2009-09-21  8:08       ` Li Yi [this message]
2009-09-18 21:29 ` [PATCH 1/2] spi: new SPI bus lock/unlock functions Andrew Morton
2009-09-21  6:33   ` Li Yi
2009-09-21 13:36     ` Mike Frysinger
2009-09-22 14:15 ` Mike Frysinger
2009-12-16 22:49 ` [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions H Hartley Sweeten
2009-12-16 23:00   ` Mike Frysinger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1253520516.3925.143.camel@adam-desktop \
    --to=yi.li@analog.com \
    --cc=akpm@linux-foundation.org \
    --cc=dbrownell@users.sourceforge.net \
    --cc=hartleys@visionengravers.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=spi-devel-general@lists.sourceforge.net \
    --cc=vapier.adi@gmail.com \
    --subject='RE: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).