Re: [PATCH RESEND] media: omap3isp: Use dma_request_chan() to requesting DMA channel

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Peter,

On Thursday 03 Nov 2016 15:14:19 Peter Ujfalusi wrote:
> On 11/03/2016 11:23 AM, Peter Ujfalusi wrote:
> > On 11/02/2016 11:19 PM, Laurent Pinchart wrote:
> >>> On Wednesday 02 Nov 2016 14:39:59 Peter Ujfalusi wrote:
> >>>>> With the new dma_request_chan() the client driver does not need to
> >>>>> look for the DMA resource and it does not need to pass filter_fn
> >>>>> anymore. By switching to the new API the driver can now support
> >>>>> deferred probing against DMA.
> >>> 
> >>> I believe this breaks the OMAP3 ISP driver.
> >>> dma_request_slave_channel_compat() is a superset of dma_request_chan()
> >>> that will, when called with
> >>> omap_dma_filter_fn, return as a fallback any free channel handled by
> >>> the OMAP DMA engine driver. This feature is actively used by this
> >>> driver and must be preserved.
> > 
> > The fallback to use the filter_fn is used only when booted in legacy
> > mode or when requesting a channel for non slave DMA operation.
> > Based on the code in the driver it is handling slave transfers, so it
> > must have DMA request line coming from somewhere. If that is missing the
> > driver should not be able to work as it will not start the transfer.
> > 
> > dma_request_chan() is to be used when you want to have slave channel
> > with DMA request.
> > 
> > If legacy mode needs to be supported then adding the hist DMA request
> > number to the omap3xxx_sdma_map in arch/arm/mach-omap2/dma.c should be
> > done. The reason the omap3isp is not in the table is that I could not
> > find any place where the DMA resource was set (nor the DMA is specified
> > in DT).
> > 
> > I'm unsure how this driver could work w/o DMA request line over a random
> > (and SW triggered) DMA channel with slave operation. For the slave DMA
> > you need to have DMA request line.
> 
> I see now.
> The omap3isp never supposed to get valid DMA request line via
> platform_get_resource_byname(), it should never find the "hist" DMA in
> the DT data. If it does, the driver would stop working as there is no
> DMA request associated with the histogram data reading.

That's correct.

> It is a bit misleading that it used dma_request_slave_channel_compat()
> for getting the channel.
> 
> I think what would be correct is:
> dma_cap_mask_t mask;
> 
> dma_cap_zero(mask);
> dma_cap_set(DMA_SLAVE, mask);
> hist->dma_ch = dma_request_chan_by_mask(&mask);
> 
> We will get any DMA channel capable of slave configuration, but we will
> configure no DMA request number for the channel.

I believe that should work. It could in theory result in a different behaviour 
as it could return a DMA channel not handled by the OMAP SDMA engine, but I 
don't think that would be an issue.

> and document this in the driver...

-- 
Regards,

Laurent Pinchart