Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Williams, Dan J]

On Mon, Jul 25, 2011 at 12:17 PM, Jaswinder Singh
<jaswinder.singh@xxxxxxxxxx> wrote:
> On 26 July 2011 00:25, Williams, Dan J <dan.j.williams@xxxxxxxxx> wrote:
>>>> Unless you guaranteed that every id is globally unique I don't see how
>>>> they are generically usable by common clients?
>>>>
>>> Yes. And the first step is to allow DMAC drivers to freely set chan_id value.
>>> Platform could pass the list and mapping of supported 'global channels'
>>> via platform_data(?) which the DMAC drivers could set in chan_id
>>> And I am not sure of defining a new variable for that, because chan_id
>>> is actually used only by some dmac drivers for internal purpose only -
>>> which they could do by private variables.
>>
>> Yes, it does appear to have grown a lot of dubious usage in
>> drivers/dma/ for something that is described as just a "channel ID for
>> sysfs".  The intent was for drivers that need to maintain their own
>> hardware specific identification to define a local id scheme (like
>> iop-adma does in the iop3xx case).
>
> You mean chan_id is meant to be used for _internal_ purposes by DMAC drivers ?
>
> Shouldn't there instead be just something like 'void *dmac_data' for
> DMAC drivers to hang their private info on?
>
> Sorry, I couldn't find chan_id in drivers/dma/iop-adma.c
> Do I misunderstand ?

/**
 * struct iop_adma_device - internal representation of an ADMA device
 * @pdev: Platform device
 * @id: HW ADMA Device selector
 * @dma_desc_pool: base of DMA descriptor region (DMA address)
 * @dma_desc_pool_virt: base of DMA descriptor region (CPU address)
 * @common: embedded struct dma_device
 */
struct iop_adma_device {
        struct platform_device *pdev;
        int id;
        dma_addr_t dma_desc_pool;
        void *dma_desc_pool_virt;
        struct dma_device common;
};

Where that 'id' is the internal identifier and chan_id from struct
dma_chan is ignored.

>> I can sort of see why it is attractive for the goal of having clients
>> being able to talk to multiple DMACs since it is a field that all
>> channels already implement.  But, I don't think we should be mixing
>> sysfs presentation details with a capability that indicates a certain
>> compatibility level in a DMAC driver implementation.  If the goal is
>> to be able to use a single client with multiple drivers that, to me,
>> is asking for a new capability bit (dma_transaction_type) rather than
>> an id.  Do you have an example of the client and the DMACs that would
>> first take advantage of such a cross DMAC compatibility?
>>
> Apparently I fail to explain my well. let me ask you this....
>
> Some DMAC drivers initialize chan_id before calling dma_async_device_register().
> And dma_async_device_register() overwrites the chan_id _always_.
>
> Clearly only _one_ of them should be setting chan_id. Which was meant to be?

Correct, it is meant that chan_id is only a sysfs property.  Any
driver usage that is assuming chan_id is anything more than a
guaranteed unique number within a given dma_device's list of channels
is probably inferring too much.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/