On Mon, Mar 23, 2015 at 02:51:40PM -0700, Maxime Ripard wrote:
> > > + if (vd) {
> > > + lli = desc->v_lli;
> > > + while (true) {
> > > + bytes += sdma->ops->lli_size(lli);
> > > +
> > > + if (!sdma->ops->lli_has_next(lli))
> > > + break;
> > > +
> > > + lli = sdma->ops->lli_next(lli);
> >
> > wouldn't it be nicer to just use a link list here which is embedded
> > in a generic struct which in turn can be embedded in a driver
> > specific one, so we actually don't care what driver specific
> > implementation is...
>
> I'm not sure to get what you mean here. We will always have to care
> about the driver specific implementation.
by providing a callback driver can do whatever it wants whereas the framework
does generic stuff
>
> > that way we cna get rid of lli_size and lli_has_next and lli_next
> > ones. Further that will allow drivers/framework to break a txn into
> > multiple smaller txns internally.
>
> I chose this accessors/iterators pattern because it actually make this
> very easy to add new features on the LLI (like debugging, we just have
> to add a new lli_dump) and that works.
>
> We can also manage more easily the LLI, by allocating ourself the LLI
> items, and freeing it afterwards, which is something that also is
> often wrong, or at least addressed in different ways from one driver
> to another (for example, the Atmel drivers have their own descriptor
> allocation logic, while the others mostly rely on the dma_pools. Which
> one is right, I don't know, but the right one should be shared by
> everyone).
>
> That would also be much easier to add software-emulated LLI, since you
> just have to set the lli management functions to some common library
> functions, and we're done.
>
> I feel like it would be much more flexible, and would remove logic
> from the driver itself, which is the point of this whole
> "sub-framework".
Yes the intent is right, I was thinking that using a kernel link frees us
from managing the list, lesser chances of bugs that way
> > typically driver would do calculations for descriptor setting up the
> > registers values which cna be programmed, so a driver callback would be nice
> > here. That way lesser work in start function and which is good as I am
> > assuming start should be done from irq context
>
> I'm not really sure that would make sense. You don't have the
> guarantee that the dma_chan pointer you are passing to it is actually
> mapped to a physical channel at this time.
>
> And I thought that the prep functions could be called from irq context
> too?
Yes that is true but if you have descriptor prepared and only need to
program the hw that it is the fastest way. Plus the HW is idle so faster we
can submit a new transaction the better it is. Btw the recommendation is
that new txn should be submitted in ISR and not wait till tasklet, most
driver do latter with few doing former.
The computation part of registers to be programmed should be anyway
independent of channel, while actual programming and starting of channel we
need to ensure right channel and thereby offsets are used :)
>
> > Also this needs to take into account the dma_slave_config, which is allowed
> > to me modified for subsequent txns
>
> memcpy doesn't rely on dma_slave_config, does it?
Nope but slave does :) this comment is for slave txn :)
--
~Vinod
Attachment:
signature.asc
Description: Digital signature
