Re: [PATCH 05/18] MMC: OMAP: Introduce new multislot structure and change driver to use it

[Roel Kluin <12o3l@tiscali.nl>]

Carlos Aguiar wrote:
> From: Juha Yrjola <juha.yrjola@solidboot.com>
> 
> Introduce new MMC multislot structure and change driver to use it.

> diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c

It could be that I misunderstand, but...

> @@ -897,19 +1037,106 @@ static const struct mmc_host_ops mmc_omap_ops = {
>  	.get_ro		= mmc_omap_get_ro,
>  };
>  
> -static int __init mmc_omap_probe(struct platform_device *pdev)
> +static int __init mmc_omap_new_slot(struct mmc_omap_host *host, int id)
>  {
> -	struct omap_mmc_conf *minfo = pdev->dev.platform_data;
> +	struct mmc_omap_slot *slot = NULL;
>  	struct mmc_host *mmc;
> +	int r;
> +
> +	mmc = mmc_alloc_host(sizeof(struct mmc_omap_slot), host->dev);

since you mmc_alloc_host here...

> +	if (mmc == NULL)
> +		return -ENOMEM;
> +
> +	slot = mmc_priv(mmc);
> +	slot->host = host;
> +	slot->mmc = mmc;
> +	slot->id = id;
> +	slot->pdata = &host->pdata->slots[id];
> +
> +	host->slots[id] = slot;
> +
> +	mmc->caps = MMC_CAP_MULTIWRITE | MMC_CAP_MMC_HIGHSPEED |
> +		    MMC_CAP_SD_HIGHSPEED;
> +	if (host->pdata->conf.wire4)
> +		mmc->caps |= MMC_CAP_4_BIT_DATA;
> +
> +	mmc->ops = &mmc_omap_ops;
> +	mmc->f_min = 400000;
> +
> +	if (cpu_class_is_omap2())
> +		mmc->f_max = 48000000;
> +	else
> +		mmc->f_max = 24000000;
> +	if (host->pdata->max_freq)
> +		mmc->f_max = min(host->pdata->max_freq, mmc->f_max);
> +	mmc->ocr_avail = slot->pdata->ocr_mask;
> +
> +	/* Use scatterlist DMA to reduce per-transfer costs.
> +	 * NOTE max_seg_size assumption that small blocks aren't
> +	 * normally used (except e.g. for reading SD registers).
> +	 */
> +	mmc->max_phys_segs = 32;
> +	mmc->max_hw_segs = 32;
> +	mmc->max_blk_size = 2048;	/* BLEN is 11 bits (+1) */
> +	mmc->max_blk_count = 2048;	/* NBLK is 11 bits (+1) */
> +	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
> +	mmc->max_seg_size = mmc->max_req_size;
> +
> +	r = mmc_add_host(mmc);
> +	if (r < 0)
> +		return r;

shouldn't you mmc_free_host(mmc) here?

> +
> +	if (slot->pdata->name != NULL) {
> +		r = device_create_file(&mmc->class_dev,
> +					&dev_attr_slot_name);
> +		if (r < 0)
> +			goto err_remove_host;
> +	}
> +
> +	if (slot->pdata->get_ro != NULL) {
> +		r = device_create_file(&mmc->class_dev,
> +					&dev_attr_ro);
> +	}
> +
> +	return 0;
> +
> +err_remove_slot_name:

This label has no goto, so the 2 lines below are dead code...
Ok, now I see It's in the next patch. (maybe better to put these lines
in that patch?)

> +	if (slot->pdata->name != NULL)
> +		device_remove_file(&mmc->class_dev, &dev_attr_ro);
> +err_remove_host:
> +	mmc_remove_host(mmc);

and maybe mmc_free_host(mmc) here?

> +	return r;
> +}

from mmc_omap_probe()

+	for (i = 0; i < pdata->nr_slots; i++) {
+		ret = mmc_omap_new_slot(host, i);
+		if (ret < 0) {
+			while (--i >= 0)
+				mmc_omap_remove_slot(host->slots[i]);

mmc_omap_remove_slot() does a mmc_free_host(), but note the decrement of i:
the current isn't freed.