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

[Haavard Skinnemoen]

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.

> > /*
> >  * 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.

> > /*
> >  * 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.

Haavard