[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Guennadi Liakhovetski]

On Thu, 8 May 2008, Haavard Skinnemoen wrote:

> Guennadi Liakhovetski <lg at denx.de> wrote:
> > On Thu, 8 May 2008, Haavard Skinnemoen wrote:
> > 
> > > But I have to say I like the idea about passing a spi_slave
> > > "handle" around...
> > > 
> > > How about something like this?
> > > 
> > > /*
> > >  * Set up communication parameters for a SPI slave. This doesn't
> > >  * necessarily enable the controller.
> > >  */
> > > struct spi_slave *spi_create_slave(int bus, int cs, unsigned int
> > > 		max_hz, unsigned int mode);
> > 
> > Don't quite like the "create" name. To me it sounds like "we just create a 
> > slave object for you, do what you want with it." Whereas, I was thinking 
> > about an "attach" method, where the host driver adds the slave pointer to 
> > its driver list, possibly initializes the hardware, and can always verify 
> > whether the slave pointer it is handed in by one of further method is 
> > indeed one of the drivers on the list.
> 
> Ok. spi_setup_slave()? spi_init_slave()? I'm not a huge fan of "attach"
> since it implies that some global state changes. It should be perfectly
> acceptable for a driver to simply initialize a spi_slave struct and
> return it, without updating any internal lists or hardware state.

Ok, let's go for spi_setup_slave then:-)

> > > /*
> > >  * Get exclusive access to the SPI bus for communicating with the given
> > >  * slave. Returns a negative value if the bus is busy (drivers may omit
> > >  * such checks if they don't want the extra code/data overhead.)
> > >  */
> > > int spi_claim_bus(struct spi_slave *slave);
> > 
> > Is this needed? Getting the spi_slave handle you gain exclusive access to 
> > the spi device, but claiming the whole bus? Drivers may be lazy releasing 
> > the chip-select between transfers, but are there cases where you _really_ 
> > must have exclusive access to the bus or cannot release the cs?
> 
> For some controllers, I believe claiming/releasing the bus explicitly
> makes a lot of sense. For example, the SPI controller on AT91 and AVR32
> allows you to select a given slave by writing to a register, and then
> start transferring data, toggling chip selects as needed.
> 
> If a driver is sloppy with the chip select control, it's buggy. Many
> SPI devices have very strict (and often strange) requirements about
> when the chip select should be asserted and when it should not.
> 
> I think a claim/release interface is the best way to ensure that a
> device driver can do multiple transfers more or less atomically (i.e.
> without releasing the chip select and without having any traffic on the
> bus in between) in a flexible way.

Is it compulsory then to claim / release a bus or not? Looks like it is 
not from your description above. But - even if you don't claim a bus for 
exclusive access, you still have to release it, right? Otherwise, if 
someone has setup a slave without claiming the bus, noone will ever be 
able to claim it afterwards? Because you shouldn't be able to claim it as 
long as there are even non-exclusive users on the bus? Actually, what's 
the mode variable in the setup method for? Maybe we could just use it to 
configure exclusive / non-exclusive access? Then you get a simple API like

slave = spi_setup_slave(host, dev, hz, 0 /*| SPI_ACCESS_EXCLUSIVE */);
if (!slave)
	return;
/* Now you MUST release the slave / host */
spi_xfer();
...
spi_release();

And you get access to the bus if there are no exclusive users, and you get 
exclusive access, only if there are no users currently at all.

> > > /*
> > >  * Release the SPI bus. This may disable the controller and/or put it
> > >  * in low-power mode
> > >  */
> > > void spi_release_bus(struct spi_slave *slave);
> > > 
> > > /*
> > >  * Transfer data on the SPI bus.
> > >  *
> > >  * flags can be a combination of any of the following bits:
> > >  *   SPI_XFER_BEGIN: Assert CS before starting the transfer
> > >  *   SPI_XFER_END: Deassert CS after the transfer
> > >  */
> > > int spi_xfer(struct spi_slave *slave, int bitlen, const void *dout,
> > > 		void *din, unsigned long flags);
> > 
> > See above - would anything break if we just deassert the cs between 
> > transfers?
> 
> Yes.
> 
> We've had so many chipselect-related problems in Linux that it's
> not even funny. Controllers, drivers, devices, everything seems to be
> making assumptions about how the chip select is supposed to behave -- I
> think we should at least try not to make any assumptions on the
> interface level.
> 
> Maybe we can do it in a different way, but simply saying that the chip
> select should be deasserted between transfers is broken.

What do we do, if there are several non-exclusive users on a bus, and one 
of them doesn't release its cs between transfers? Return an error to 
others if they try to communicate at this time?

Looks like most of this API implementation is hardware independent, and 
should go into an spi.c?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de