Date:	Tue, 20 Jul 2010 00:44:40 +0200
From:	Linus Walleij <linus.ml.walleij@...il.com>
To:	Dan Williams <dan.j.williams@...el.com>
Cc:	linux-kernel@...r.kernel.org, vinod.koul@...el.com
Subject: Re: [PATCH 1/3] DMAENGINE: generic slave channel control

2010/7/19 Dan Williams <dan.j.williams@...el.com>:

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

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.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists