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

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg at denx.de> wrote:
> Ok, I see what you mean now. But then we need another function - an 
> opposite to spi_setup, to free any allocated RAM, etc. I thought this was 
> going to happen in release_bus, but after this explanation this doesn't 
> seem to be the case.
> 
> So, just add an spi_free(), and, as a counterpart to it, an spi_init() 
> might sound better than spi_setup:-)

Yes, perhaps we need that. But I was sort of thinking that once you
initialize a slave, it will just stick around until you boot an OS or
reset. Most drivers don't seem to have any cleanup hooks anyway, and
some don't even have init hooks. In the latter case, you can simply
keep a static struct spi_slave * around and if it's NULL, you
initialize it.

Normally, having an allocation function without a corresponding free
would seem like a bad design, but in U-Boot's case, I'm not so sure...

> > > 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.
> > 
> > I don't see how having multiple active users on the bus at the same
> > time can possibly make any sense...once you start a transfer, you
> > better make sure nobody else is doing a transfer at the same time since
> > you're using the same MOSI/MISO/SCK lines...
> 
> No, not "active." Multiple users having set up different slaves. But not 
> communicating simultaneously:-)

Ah, but they would only claim the bus when they're actually going to
communicate :-)

> > > Looks like most of this API implementation is hardware independent, and 
> > > should go into an spi.c?
> > 
> > Not really...what "claiming" the bus really means is highly hardware
> > dependent. And SPI slave setup is mostly about decoding
> > hardware-independent parameters like SCK rate and mode bits into
> > hardware register values. But any convenience wrappers like spi_w8r8()
> > probably belongs somewhere hardware-independent.
> 
> I thought, like (pseudocode)
> 
> static struct spi_host busses[SPI_BUSSES];
> 
> struct spi_slave *spi_init()
> {
> 	list_for_each_entry(slave, &busses[bus].slaves, list) {
> 		if (slave->device == device)
> 			return (struct spi_slave *)-EBUSY;
> 	}
> 	slave = malloc();
> 	slave->bus = bus;
> 	slave->device = device;
> 	ret = busses[bus].init(slave);
> 	if (ret) {
> 		free(slave);
> 		return (struct spi_slave *)ret;
> 	}
> 	return slave;
> }
>
> int spi_xfer()
> {
> 	list_for_each_entry(ix, &busses[bus].slaves, list) {
> 		if (ix == slave)
> 			break;
> 	}
> 	if (ix != slave)
> 		return -EINVAL;
> 
> 	if (slave->bus->busy)
> 		return -EBUSY;
> 
> 	return slave->bus->xfer();
> }

I was thinking about something much simpler:

struct spi_slave *spi_init_slave(bus, cs, max_hz, mode)
{
	slave = malloc();
	slave->regs = get_spi_controller_base_address(bus);
	slave->mode_reg = get_suitable_settings_for(cs, max_hz, mode);
	return slave;
}

int spi_xfer(slave, ...)
{
	__raw_writel(slave->mode_reg, slave->regs + SPI_MR);
	if (flags & SPI_XFER_BEGIN)
		assert_chip_select();

	do_the_actual_spi_transfer();

	if (flags & SPI_XFER_END)
		deassert_chip_select();

	return whatever;
}

Of course, your solution will work with multiple, different SPI
controllers while mine won't, but is that really necessary?

Your solution comes with more error checking as well, which might be a
good thing, but since it comes with a cost of additional memory and
flash footprint, I think it should be optional. Maybe we could provide
some library functions to simplify the drivers that want this?

Haavard