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 partIs 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