[U-Boot] [PATCH 06/25] dm: spi: Add a uclass for SPI

[Pavel Herrmann]

Hi

On Wednesday 16 of July 2014 23:39:44 Simon Glass wrote:
> Hi Pavel,
> 
> On 15 July 2014 02:26, Pavel Herrmann <morpheus.ibis at gmail.com> wrote:
> > Hi
> > 
> > On Monday 14 of July 2014 18:56:13 Simon Glass wrote:
> > > Add a uclass which provides access to SPI buses and includes operations
> > > required by SPI.
> > > 
> > > For a time driver model will need to co-exist with the legacy SPI
> > > interface
> > > so some parts of the header file are changed depending on which is in
> > > use.
> > > The exports are adjusted also since some functions are not available
> > > with
> > > driver model.
> > > 
> > > Boards must define CONFIG_DM_SPI to use driver model for SPI.
> > > 
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > ---
> > > 
> > > ...
> > > +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> > > +          const void *dout, void *din, unsigned long flags)
> > > +{
> > > +     struct udevice *dev = slave->dev;
> > > +     struct udevice *bus = dev->parent;
> > 
> > is this the best interface here?
> > I think it would be cleaner if bus drivers had interfaces which follow a
> > certain template, such as
> > bus_ops(struct udevice *bus, struct udevice *child, ...)
> 
> Thanks for your comments.
> 
> Well I thought about that long and hard. Clearly in a pure
> driver-model work we would pass a udevice, not a spi_slave.
> 
> > struct spi_slave would be a prime candidate to have in child->parentdata
> > (which should only be accessed by the parent IIUC)
> 
> Yes, it is. In other words, 'struct spi_slave' is the child's parent
> data. The only reason that spi_xfer() is passed a spi_slave rather
> than a udevice is to maintain compatibility with the existing SPI
> system. I tried various other approachs, such as '#define spi_slave
> udevice' and the like. At some point we can change it, but it is
> really painful to have two completely different APIs co-existing in
> U-Boot.
>
> This way, driver model becomes a fairly soft transition, the amount of
> rewriting needed is reduced, and I think it is much more likely that
> people will use it. As an example, one of the reasons that the generic
> board change has been relatively painless is that people can just
> define CONFIG_SYS_GENERIC_BOARD in their board header file, and in
> most cases it just works. If we require people to rewrite things it
> will take longer, etc. There is already enough rewriting required in
> individual drivers to keep people busy. It will be a relatively easy
> change to do in the future if we want to.

OK, that reason I understand.

However, what you are doing now is limiting what parentdata a SPI bus 
controller can use.
This means that each bus driver has its parentdata defined by what uclass it 
belongs to. Are you sure this won't be a limitation? I mean that you could end 
up with uclasses that have the same calls but a slightly different parentdata.

Yes, you could have a shared parentdata from the uclass (that makes sense for 
all controllers, because of the way the bus works), and a controller-specific 
parentdata as an extension (read "void *private" at the end of the parentdata 
structure)

> Re passing both the bus and the device, really, what's the point? The
> only valid bus to pass to the function is dev->parent. If you pass
> anything else it is an error. It is redundant and therefore just
> introduces the possibility of error.

Well, yes, sort of.

Consider a bus with transparent bridges, like USB or PCI.

If you were to represent these bridges as udevices, you would end up with 
something in between the actual devices and the bus controller, that forwards 
requests with no or minimal change.
in case of USB specifically (IIRC), hubs are visible during initialization, but 
when the device is up it has its own descriptor that is used to 
in case of PCI, the device address actually contains the bus number, but the 
device itself doesn't really care about it, and is only used to select which 
bus the command goes to.

consider the following scenario:
------     ----------     ---------     ---------     ------------
|root| --- |ehci_hcd| --- |usb_hub| --- |usb_hub| --- |usb_device|
------     ----------     ---------     ---------     ------------

If you were to flatten the bus, the udevice tree would not really correspond to 
how the bus looks like in reality, and it might give you some hurdles with 
initialization.

note that in these cases you cannot pass a child udevice to the bus controller 
as a part of the upcall, since it is not necessarily its child. this is in 
contradiction to what I wrote previously, simply because I didn't think hard 
enough to find these counter examples.

regards
Pavel Herrmann