lkml.org 
[lkml]   [2009]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] atmel-mci: change use of dma slave interface
    Andrew Morton :
    > On Thu, 17 Sep 2009 18:29:26 +0200
    > Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
    >
    >> Allow the use of another DMA controller driver in atmel-mci sd/mmc driver. This
    >> adds a generic dma_slave pointer to the mci platform structure where we can
    >> store DMA controller information. In atmel-mci we use information provided by
    >> this structure to initialize the driver (with new helper functions that are
    >> architecture dependant).
    >> This also adds at32/avr32 chip modifications to cope with this new access
    >> method.
    >>
    >> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
    >> ---
    >> arch/avr32/mach-at32ap/at32ap700x.c | 6 ++-
    >> drivers/mmc/host/atmel-mci.c | 82 ++++++++++++++++++++++++++---------
    >> include/linux/atmel-mci.h | 3 +-
    >> 3 files changed, 68 insertions(+), 23 deletions(-)
    >>
    >> diff --git a/arch/avr32/mach-at32ap/at32ap700x.c b/arch/avr32/mach-at32ap/at32ap700x.c
    >> index eb9d4dc..d1fe145 100644
    >> --- a/arch/avr32/mach-at32ap/at32ap700x.c
    >> +++ b/arch/avr32/mach-at32ap/at32ap700x.c
    >> @@ -1320,7 +1320,7 @@ struct platform_device *__init
    >> at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
    >> {
    >> struct platform_device *pdev;
    >> - struct dw_dma_slave *dws = &data->dma_slave;
    >> + struct dw_dma_slave *dws;
    >> u32 pioa_mask;
    >> u32 piob_mask;
    >>
    >> @@ -1339,6 +1339,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
    >> ARRAY_SIZE(atmel_mci0_resource)))
    >> goto fail;
    >>
    >> + dws = kzalloc(sizeof(struct dw_dma_slave), GFP_KERNEL);
    >
    > I don't see anywhere where this gets freed again?

    Well, in fact those are platform initialization functions that have no
    "exit" equivalent. Is this the proper way of managing this ?

    Anyway, I have forgotten to free memory in case of a "fail" error case
    that is present in this function. I will correct this in my v2 patch.

    >
    >> dws->dma_dev = &dw_dmac0_device.dev;
    >> dws->reg_width = DW_DMA_SLAVE_WIDTH_32BIT;
    >> dws->cfg_hi = (DWC_CFGH_SRC_PER(0)
    >> @@ -1346,6 +1348,8 @@ at32_add_device_mci(unsigned int id, struct mci_platform_data *data)
    >> dws->cfg_lo &= ~(DWC_CFGL_HS_DST_POL
    >> | DWC_CFGL_HS_SRC_POL);
    >>
    >> + data->dma_slave = dws;
    >> +
    >> if (platform_device_add_data(pdev, data,
    >> sizeof(struct mci_platform_data)))
    >> goto fail;
    >> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
    >> index 065fa81..1689396 100644
    >> --- a/drivers/mmc/host/atmel-mci.c
    >> +++ b/drivers/mmc/host/atmel-mci.c
    >> @@ -1575,16 +1575,71 @@ static void __exit atmci_cleanup_slot(struct atmel_mci_slot *slot,
    >> }
    >>
    >> #ifdef CONFIG_MMC_ATMELMCI_DMA
    >> -static bool filter(struct dma_chan *chan, void *slave)
    >> +static struct device *find_slave_dev(void *slave)
    >> +{
    >> + if (!slave)
    >> + return NULL;
    >> +
    >> + if (cpu_is_at32ap7000())
    >> + return ((struct dw_dma_slave *)slave)->dma_dev;
    >> + else
    >> + return ((struct at_dma_slave *)slave)->dma_dev;
    >> +}
    >
    > Quite a few unsafeish typecasts here.

    I am afraid, yes.

    >> struct mci_platform_data {
    >> - struct dw_dma_slave dma_slave;
    >> + void *dma_slave;
    >> struct mci_slot_pdata slot[ATMEL_MCI_MAX_NR_SLOTS];
    >> };
    >
    > I think the code would come out better if this has type dw_dma_slave*.

    Do you mean that I would leave dw_dma_slave* in mci_platform_data and
    use this field for struct at_dma_slave content where I need it ? I
    thought it was more confusing...

    > You'll still need typecasts to support the dma_request_channel()
    > callback, but the code will be safer and cleaner, I expect.

    My concern are:
    1/ allow the use of either dmaengine driver
    2/ avoid too much modification to dw_dma_slave as it
    is also used for audio stuff on avr32 platforms...
    3/ not introduce heavy weigh solution like the use of an union

    Best regards,
    --
    Nicolas Ferre



    \
     
     \ /
      Last update: 2009-09-30 15:35    [W:4.216 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site