Re: [PATCH 11/31] dma: add channel request API that supports deferred probe

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 11/25/2013 11:42 AM, Dan Williams wrote:
> On Mon, Nov 25, 2013 at 10:00 AM, Russell King - ARM Linux
> <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
>> On Mon, Nov 25, 2013 at 09:45:18AM -0800, Dan Williams wrote:
>>> Regardless of whether the driver was dma_request_channel as a
>>> fallback, it will currently use NULL to indicate an allocation
>>> failure.
>>
>> At the moment, dma_request_slave_channel()'s return values are valid
>> pointer or NULL.  I'd suggest as that's how it's been established,
>> that function is left alone - changing the return values in this kind
>> of invisible way is Really Bad(tm) because you can't check that it's
>> right in any future users - unless you're prepared to review every
>> single new user of this function for the next few years or more.
>>
>> Instead, leaving it as is and introducing a new function name with
>> the different return error method is the right way to do this.
> 
> I was attempting to tap the brakes on the number of request_channel
> apis in circulation.
> dma_request_channel
> dma_request_slave_channel
> dma_request_slave_channel_compat
> ... and now dma_request_slave_channel_reason
> 
> ...but given we already have things like samsung_dmadev_request() in
> the wild, tracking correct usage going forward will be an uphill
> battle.  We can do this the other way round.  Introduce the new api
> and delete the old one (after some time).
> 
> Stephen,
> 
> Can we make the new api not pass the 'defer' flag.  You mention it is
> for compat reasons, but since there are no users I'm not seeing how
> there can be compatibility concerns.  Can you elaborate?

dma_request_slave_channel_or_err() doesn't have a defer flag. Rather,
__dma_request_slave_channel_or_err() did, which is the shared
implementation between the existing existing dma_request_slave_channel()
and the new dma_request_slave_channel_or_err().

That flag exists so it can be passed to of_dma_request_slave_channel().
That function needs it because I didn't want to change the existing
behaviour of dma_request_slave_channel() at all, yet I wanted slightly
different behaviour from dma_request_slave_channel_or_err().

Specifically, if of_dma_request_slave_channel() finds an entry in the
dmas DT property for which there is no registered DMA controller, it
simply ignores that entry and carries on, hoping to find an alternative
entry that can satisfy the request (since some controllers can be
supported by multiple DMA controllers). However, to support deferred
probe, we really need to bail out from the loop early with -EPROBE_DEFER
if any DMA controller isn't found, in order to wait for it to be registered.

I suppose an alternative would be to remove that flag, and have the loop
in of_dma_request_slave_channel() initially ignore any unregistered DMA
controllers, and still continue to look through the property for any
alternative controller, and return a channel from one if it is found.
Then, at the very end of the function, we could always return
-EPROBE_DEFER if any unregistered DMA controllers were found, otherwise
return -ENODEV. That would keep compatible behaviour, but it would mean
that device probe order would/could influence which dmas entry provided
the channel, since some entries might be ignored based simply on
timing/ordering of DMA controller registration. Is that acceptable?

Either way, just let me know and I'll adjust the code accordingly; the
code changes for either option are simple.