lkml.org 
[lkml]   [2011]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 1/2] dmaengine: at_hdmac: platform data move to use .id_table
    On 10/24/2011 11:34 AM, Grant Likely :
    > On Mon, Oct 17, 2011 at 02:56:40PM +0200, Nicolas Ferre wrote:
    >> We remove the use of platform data from DMA controller driver.
    >> We now use of .id_table to distinguish between compatible
    >> types. The two implementations allow to determine the
    >> number of channels and the capabilities of the controller.
    >>
    >> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
    >> Acked-by: Grant Likely <grant.likely@secretlab.ca>
    >> ---
    >> drivers/dma/at_hdmac.c | 48 ++++++++++++++++++++++++++++++++++---------
    >> drivers/dma/at_hdmac_regs.h | 8 +++++++
    >> 2 files changed, 46 insertions(+), 10 deletions(-)
    >>
    >> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
    >> index fcfa0a8..d1869c5 100644
    >> --- a/drivers/dma/at_hdmac.c
    >> +++ b/drivers/dma/at_hdmac.c
    >> @@ -1175,6 +1175,18 @@ static void atc_free_chan_resources(struct dma_chan *chan)
    >>
    >> /*-- Module Management -----------------------------------------------*/
    >>
    >> +static struct platform_device_id atdma_devtypes[] = {
    >> + {
    >> + .name = "at91sam9rl_dma",
    >> + .driver_data = ATDMA_DEVTYPE_SAM9RL,
    >> + }, {
    >> + .name = "at91sam9g45_dma",
    >> + .driver_data = ATDMA_DEVTYPE_SAM9G45,
    >> + }, {
    >> + /* sentinel */
    >> + }
    >> +};
    >> +
    >> /**
    >> * at_dma_off - disable DMA controller
    >> * @atdma: the Atmel HDAMC device
    >> @@ -1193,18 +1205,32 @@ static void at_dma_off(struct at_dma *atdma)
    >>
    >> static int __init at_dma_probe(struct platform_device *pdev)
    >> {
    >> - struct at_dma_platform_data *pdata;
    >> struct resource *io;
    >> struct at_dma *atdma;
    >> size_t size;
    >> int irq;
    >> int err;
    >> int i;
    >> + u32 nr_channels;
    >> + dma_cap_mask_t cap_mask = {};
    >> + enum atdma_devtype atdmatype;
    >> +
    >> + dma_cap_set(DMA_MEMCPY, cap_mask);
    >> +
    >> + /* get DMA parameters from controller type */
    >> + atdmatype = platform_get_device_id(pdev)->driver_data;
    >>
    >> - /* get DMA Controller parameters from platform */
    >> - pdata = pdev->dev.platform_data;
    >> - if (!pdata || pdata->nr_channels > AT_DMA_MAX_NR_CHANNELS)
    >> + switch (atdmatype) {
    >> + case ATDMA_DEVTYPE_SAM9RL:
    >> + nr_channels = 2;
    >> + break;
    >> + case ATDMA_DEVTYPE_SAM9G45:
    >> + nr_channels = 8;
    >> + dma_cap_set(DMA_SLAVE, cap_mask);
    >> + break;
    >> + default:
    >> return -EINVAL;
    >
    > Instead of this song and dance, why not make a configuration structure
    > and embed that into the platform_device_id table? Like this:
    >
    > struct at_dma_platform_data at91sam9rl_config {
    > .nr_channels = 2;
    > .cap_mask = 0;
    > };
    > struct at_dma_platform_data at91samg45_config {
    > .nr_channels = 8;
    > .cap_mask = DMA_SLAVE;
    > };
    > static struct platform_device_id atdma_devtypes[] = {
    > {
    > .name = "at91sam9rl_dma",
    > .driver_data = (unsigned long) at91sam9rl_config,
    > /*
    > * Yes, I know, ugly cast; but one case will be needed
    > * regardless when the of_device_id table is added.
    > * It's due to the platform_device_id not using a
    > * void*
    > */
    > }, {
    > .name = "at91sam9g45_dma",
    > .driver_data = (unsigned long) at91sam9g45_config,
    > }, {
    > /* sentinel */
    > }
    > };
    >
    > And then the enum can be eliminated entirely.

    That looks nice!

    I send a patch that goes on top of this series to simplify things like
    you indicate.

    Thanks for your review.

    >> + }
    >>
    >> io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    >> if (!io)
    >> @@ -1215,14 +1241,15 @@ static int __init at_dma_probe(struct platform_device *pdev)
    >> return irq;
    >>
    >> size = sizeof(struct at_dma);
    >> - size += pdata->nr_channels * sizeof(struct at_dma_chan);
    >> + size += nr_channels * sizeof(struct at_dma_chan);
    >> atdma = kzalloc(size, GFP_KERNEL);
    >> if (!atdma)
    >> return -ENOMEM;
    >>
    >> - /* discover transaction capabilites from the platform data */
    >> - atdma->dma_common.cap_mask = pdata->cap_mask;
    >> - atdma->all_chan_mask = (1 << pdata->nr_channels) - 1;
    >> + /* discover transaction capabilities */
    >> + atdma->dma_common.cap_mask = cap_mask;
    >> + atdma->all_chan_mask = (1 << nr_channels) - 1;
    >> + atdma->devtype = atdmatype;
    >>
    >> size = resource_size(io);
    >> if (!request_mem_region(io->start, size, pdev->dev.driver->name)) {
    >> @@ -1268,7 +1295,7 @@ static int __init at_dma_probe(struct platform_device *pdev)
    >>
    >> /* initialize channels related values */
    >> INIT_LIST_HEAD(&atdma->dma_common.channels);
    >> - for (i = 0; i < pdata->nr_channels; i++) {
    >> + for (i = 0; i < nr_channels; i++) {
    >> struct at_dma_chan *atchan = &atdma->chan[i];
    >>
    >> atchan->chan_common.device = &atdma->dma_common;
    >> @@ -1313,7 +1340,7 @@ static int __init at_dma_probe(struct platform_device *pdev)
    >> dev_info(&pdev->dev, "Atmel AHB DMA Controller ( %s%s), %d channels\n",
    >> dma_has_cap(DMA_MEMCPY, atdma->dma_common.cap_mask) ? "cpy " : "",
    >> dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask) ? "slave " : "",
    >> - pdata->nr_channels);
    >> + nr_channels);
    >>
    >> dma_async_device_register(&atdma->dma_common);
    >>
    >> @@ -1495,6 +1522,7 @@ static const struct dev_pm_ops at_dma_dev_pm_ops = {
    >> static struct platform_driver at_dma_driver = {
    >> .remove = __exit_p(at_dma_remove),
    >> .shutdown = at_dma_shutdown,
    >> + .id_table = atdma_devtypes,
    >> .driver = {
    >> .name = "at_hdmac",
    >> .pm = &at_dma_dev_pm_ops,
    >> diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h
    >> index aa4c9ae..d7d6737 100644
    >> --- a/drivers/dma/at_hdmac_regs.h
    >> +++ b/drivers/dma/at_hdmac_regs.h
    >> @@ -248,9 +248,16 @@ static inline struct at_dma_chan *to_at_dma_chan(struct dma_chan *dchan)
    >>
    >> /*-- Controller ------------------------------------------------------*/
    >>
    >> +enum atdma_devtype {
    >> + ATDMA_DEVTYPE_UNDEFINED = 0,
    >> + ATDMA_DEVTYPE_SAM9RL, /* compatible with SAM9RL DMA controller */
    >> + ATDMA_DEVTYPE_SAM9G45, /* compatible with SAM9G45 DMA controller */
    >> +};
    >> +
    >> /**
    >> * struct at_dma - internal representation of an Atmel HDMA Controller
    >> * @chan_common: common dmaengine dma_device object members
    >> + * @atdma_devtype: identifier of DMA controller compatibility
    >> * @ch_regs: memory mapped register base
    >> * @clk: dma controller clock
    >> * @save_imr: interrupt mask register that is saved on suspend/resume cycle
    >> @@ -260,6 +267,7 @@ static inline struct at_dma_chan *to_at_dma_chan(struct dma_chan *dchan)
    >> */
    >> struct at_dma {
    >> struct dma_device dma_common;
    >> + enum atdma_devtype devtype;
    >> void __iomem *regs;
    >> struct clk *clk;
    >> u32 save_imr;
    >> --
    >> 1.7.5.4
    >>
    >
    > _______________________________________________
    > linux-arm-kernel mailing list
    > linux-arm-kernel@lists.infradead.org
    > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
    >


    --
    Nicolas Ferre


    \
     
     \ /
      Last update: 2011-11-22 11:57    [W:0.037 / U:148.384 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site