Re: [PATCH 1/2] atmel-mci: change use of dma slave interface

[Nicolas Ferre <nicolas.ferre@atmel.com>]

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