Re: [PATCH 1/3] DMAENGINE: generic slave channel control

[Dan Williams]

On Mon, Jul 19, 2010 at 3:44 PM, Linus Walleij
<linus.ml.walleij@xxxxxxxxx> wrote:
> 2010/7/19 Dan Williams <dan.j.williams@xxxxxxxxx>:
>
>> The recently submitted intel_mid driver [1] seems to have a similar structure:
>
> This is very interesting, let's examine this closely as a comparison!
> I'm looking at it from the point of a generic slave control mechanism,
> though I suspect it is designed to be Intel-specific so keep that in mind.
>
>> +/**
>> + * struct intel_mid_dma_slave - DMA slave structure
>> + *
>> + * @dma_dev: DMA master client
>> + * @tx_reg: physical address of data register used for
>> + *     memory-to-peripheral transfers
>> + * @rx_reg: physical address of data register used for
>> + *     peripheral-to-memory transfers
>> + * @tx_width: tx register width
>> + * @rx_width: rx register width
>> + * @dirn: DMA trf direction
>> + * @cfg_hi: Platform-specific initializer for the CFG_HI register
>> + * @cfg_lo: Platform-specific initializer for the CFG_LO register
>> + * @ tx_width: width of src and dstn
>> + * @ hs_mode: SW or HW handskaking mode
>> + * @ cfg_mode: Mode configuration, DMA mem to mem to dev & mem
>> + */
>> +struct intel_mid_dma_slave {
>> +       enum dma_data_direction         dirn;
>> +       enum intel_mid_dma_width        src_width; /*width of DMA src txn*/
>> +       enum intel_mid_dma_width        dst_width; /*width of DMA dst txn*/
>> +       enum intel_mid_dma_hs_mode      hs_mode;  /*handshaking*/
>> +       enum intel_mid_dma_mode         cfg_mode; /*mode configuration*/
>> +       enum intel_mid_dma_msize        src_msize; /*size if src burst*/
>> +       enum intel_mid_dma_msize        dst_msize; /*size of dst burst*/
>> +       dma_async_tx_callback           callback; /*callback function*/
>> +       void                            *callback_param; /*param for callback*/
>> +       unsigned int            device_instance; /*0, 1 for periphral instance*/
>> +};
>> +
>
> kerneldoc and struct members seem to be out-of-sync but I
> see the general idea.
>
> Of these members handshaking, cfg_mode, hs_mode
> and I suspect also device_instance are candidates for
> the private config  since they cannot be assumed to
> exist on all DMA engines. (Also goes for cfg_[lo|hi] from
> the kerneldoc)
>
> The callback and callback param are configured
> per-transfer in the current API, so I don't see what they
> are doing here at all.

Yeah, I already pointed that out...

>
> Remains: direction, then register address, burstsize and
> word width of the source and destination.
>
> (Register address present in kerneldoc but not in struct,
> burstsize present in struct but not in kerneldoc, whatever)
>
> I don't have src/dst pairs in my API, because since the
> only data provision mechanisms to slaves are sglists,
> and these provide either source or destination address
> depending on transfer direction.
>
> Also I assume that since sglists will be mem->peripheral
> or peripheral->mem, you know what is required on the
> memory side of things for word width and burstsize, and
> these only affect the device side of things.
>
> So that is why my API is more minimalistic.
>
> If you feel src/dst versions of wordwidth, register address
> and burstsize are a must, I can add them, no big thing,
> just makes for some nonused parameters, mostly.

Unused parameters is what I want to avoid, and getting drivers that
can wrap dma_slave_config to actually do so is the final kicker.

Thoughts Vinod?

Thanks,
Dan
--
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/