Guennadi Liakhovetski lg@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