RE: [PATCH][RESEND][mmc/host]:Blackfin SD Host Controller Driver

[Cai, Cliff]

>-----Original Message-----
>From: Pierre Ossman [mailto:pierre@xxxxxxxxx] 
>Sent: Monday, April 27, 2009 3:51 AM
>To: cliffcai.sh@xxxxxxxxx
>Cc: linux-kernel@xxxxxxxxxxxxxxx; Cai, Cliff; Cliff Cai
>Subject: Re: [PATCH][RESEND][mmc/host]:Blackfin SD Host 
>Controller Driver
>
>On Fri, 17 Apr 2009 01:40:41 +0800
>cliffcai.sh@xxxxxxxxx wrote:
>
>> From: Cliff Cai <cliffcai.sh@xxxxxxxxx>
>> 
>> Signed-off-by: Cliff Cai <cliffcai.sh@xxxxxxxxx>
>> ---
>>  drivers/mmc/host/Kconfig    |   19 ++
>>  drivers/mmc/host/Makefile   |    1 +
>>  drivers/mmc/host/bfin_sdh.c |  648 
>> +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 668 insertions(+), 0 deletions(-)  create mode 
>> 100644 drivers/mmc/host/bfin_sdh.c
>> 
>
>I'd also like a MAINTAINERS entry for the driver.
>
>> +config SDH_BFIN
>> +	tristate "Blackfin Secure Digital Host support"
>> +	depends on MMC && ((BF54x && !BF544) || (BF51x && !BF512))
>
>You don't need "depends on MMC" as the entire block is 
>conditioned on MMC already.
>
>> +/* In term of 
>> +ADSP_BF5xx_Blackfin_Processor_Peripheral_Hardware_Reference,
>> + * the SDH allows software to detect a card when it is 
>inserted into its slot.
>> + * The SD_DATA3 pin powers up low due to a special pull-down 
>> +resistor. When an
>> + * SD Card is inserted in its slot, the resistance increases and a 
>> +rising edge
>> + * is detected by the SDH module.
>> + * But this doesn't work sometimes. When a MMC/SD card is inserted, 
>> +the voltage
>> + * doesn't rise on SD_DATA3. In term of The MultiMediaCard System 
>> +Specification,
>> + * SD_DATA3 is used as CS pin in SPI mode. The MultiMediaCard wakes 
>> +up in the
>> + * MultiMediaCard mode. During the scan procedure, host will send 
>> +CMD0 to reset
>> + * MMC card, if CS pin is low, MMC card will enter SPI mode. Of 
>> +course Secure
>> + * Digital Host controller is not a SPI controller. So the Card 
>> +detect function
>> + * has to be disabled. After card is inserted run "echo 0 > 
>/proc/driver/sdh"
>> + * to trigger card scanning */
>
>If the controller can only do DAT3-detection, then I think 
>it's best if we put it into polling mode.

What did you mean the polling mode,using a timer?


>> +	BUG_ON(data->blksz & (data->blksz -1));
>
>Not a bug so you need to deal with this. Most likely fail the 
>request with EINVAL.
>
>> +#if defined(CONFIG_BF54x)
>> +	dma_cfg |= DMAFLOW_ARRAY | NDSIZE_5 | RESTART | 
>WDSIZE_32 | DMAEN;
>> +	for (i = 0; i < host->dma_len; i++) {
>> +		host->sg_cpu[i].start_addr = 
>sg_dma_address(&data->sg[i]);
>
>You cannot index sg lists directly anymore. You have to 
>iterate over them using the iteration helpers.

Why?

>> +	if (!data->error)
>> +		data->bytes_xfered = data->blocks * data->blksz;
>> +	else
>> +		data->bytes_xfered = data->blocks * data->blksz - \
>> +				     bfin_read_SDH_DATA_CNT();
>
>This is probably wrong. You need to set bytes_xfered to the 
>number of bytes acked by the card, not the number of bytes 
>sent over the wire (for writes that is). If your hardware can 
>provide that then fine, otherwise set bytes_xfered to 0 on failure.
>
>> +	mmc->ops = &sdh_ops;
>> +	mmc->max_phys_segs = NR_SG;
>> +	mmc->max_seg_size = 1 << 16;
>> +	mmc->max_blk_size = 2 << 11;
>> +	mmc->max_blk_count = 2 << 16;
>
>You forgot max_req_size.
>
>> +out3:
>> +	free_dma(host->dma_ch);
>
>You need a mmc_remove_host() here.
>
>Also check Mike's comments.
 
I will.

Thanks

Cliff
--
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/