lkml.org 
[lkml]   [2021]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 08/11] dmaengine: xilinx_dpdma: stop using slave_id field
On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote:
> > On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote:
> > > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote:
> > > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan,
> > > > spin_lock_irqsave(&chan->lock, flags);
> > > >
> > > > /*
> > > > - * Abuse the slave_id to indicate that the channel is part of a video
> > > > - * group.
> > > > + * Abuse the peripheral_config to indicate that the channel is part
> > >
> > > Is it still an abuse, or is this now the right way to pass custom data
> > > to the DMA engine driver ?
> >
> > It doesn't make the driver any more portable, but it's now being
> > more explicit about it. As far as I can tell, this is the best way
> > to pass data that cannot be expressed through the regular interfaces
> > in DT and the dmaengine API.
> >
> > Ideally there would be a generic way to pass this flag, but I couldn't
> > figure out what this is actually doing, or whether there is a better
> > way. Maybe Vinod has an idea.
>
> I don't think we need a generic API in this case. The DMA engine is
> specific to the display device, I don't foresee a need to mix-n-match.


Right. I wonder if there is even a point in using the dmaengine API
in that case, I think for other single-purpose drivers we tend to just
integrate the functionality in the client driver. No point changing this
now of course, but it does feel odd.

From my earlier reading of the driver, my impression was that this
is just a memory-to-memory device, so it could be used that way
as well, but does need a flag when working on the video memory.
I couldn't quite make sense of that though.

> > /*
> > * Use the peripheral_config to indicate that the channel is part
> > * of a video group. This requires matching use of the custom
> > * structure in each driver.
> > */
> > pconfig = config->peripheral_config;
> > if (WARN_ON(config->peripheral_size != 0 &&
> > config->peripheral_size != sizeof(*pconfig)))
> > return -EINVAL;
>
> How about
>
> if (WARN_ON(config->peripheral_config &&
> config->peripheral_size != sizeof(*pconfig)))
>
> >
> > spin_lock_irqsave(&chan->lock, flags);
> > if (chan->id <= ZYNQMP_DPDMA_VIDEO2 &&
> > config->peripheral_size == sizeof(*pconfig))
>
> And here you can test pconfig != NULL.

Good idea. Changed now, using 'if (pconfig)' without the '!= NULL'
in both expressions.

Arnd

\
 
 \ /
  Last update: 2021-11-15 13:39    [W:0.131 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site