Re: [PATCH v3 2/4] mmc: tmio: Add tuning support

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2966 bytes --]


> +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +	unsigned int num = 0;
> +	int i, ret = 0;
> +	bool *tap;
> +
> +	if (host->init_tuning)
> +		num = host->init_tuning(host);
> +	if (!num) {
> +		/* Tuning is not supported */
> +		ret = 0;

ret is already 0 here. Not sure to remove the redundancy, though. This
is clearer and easier to read.

> +		goto out;
> +	}
> +
> +	tap = kmalloc(num * 2, GFP_KERNEL);
> +	if (!tap) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Issue CMD19 twice for each tap
> +	 */
> +	for (i = 0; i < 2 * num; i++) {
> +		int err;

Maybe drop err here...

> +
> +		if (host->prepare_tuning)
> +			host->prepare_tuning(host, i);
> +
> +		err = mmc_send_tuning(host->mmc, opcode, NULL);
> +		if (err && err != -EILSEQ) {
> +			ret = err;
> +			goto err_free;

... use ret here directly ...

> +		}
> +		tap[i] = (err == 0);
> +
> +		mdelay(1);
> +	}

... and set ret = 0 here ?

> +
> +	if (host->select_tuning)
> +		ret = host->select_tuning(host, tap, num * 2);

I wonder if we shouldn't check the validity of the function pointers
together at the beginning of the function and bail out if some pointer
is missing? The above block skips the OOPS but it doesn't make sense to
not select something, or?

> +
> +err_free:
> +	kfree(tap);
> +out:
> +	if (ret < 0) {
> +		dev_warn(&host->pdev->dev, "Tuning procedure failed\n");
> +		tmio_mmc_hw_reset(mmc);
> +	} else {
> +		host->mmc->retune_period = 0;
> +	}
> +
> +	return ret;
> +
> +}
> +
>  /* Process requests from the MMC layer */
>  static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  {
> @@ -781,6 +851,14 @@ static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  
>  	spin_unlock_irqrestore(&host->lock, flags);
>  
> +	if (mrq->sbc) {
> +		ret = tmio_mmc_start_command(host, mrq->sbc);
> +		if (ret)
> +			goto fail;
> +		host->last_req_ts = jiffies;
> +		host->mrq = mrq;
> +	}

Is this related? Maybe a comment would help?

> +
>  	if (mrq->data) {
>  		ret = tmio_mmc_start_data(host, mrq->data);
>  		if (ret)
> @@ -978,6 +1056,8 @@ static struct mmc_host_ops tmio_mmc_ops = {
>  	.enable_sdio_irq = tmio_mmc_enable_sdio_irq,
>  	.card_busy	= tmio_mmc_card_busy,
>  	.multi_io_quirk	= tmio_multi_io_quirk,
> +	.execute_tuning = tmio_mmc_execute_tuning,
> +	.hw_reset	= tmio_mmc_hw_reset,
>  };
>  
>  static int tmio_mmc_init_ocr(struct tmio_mmc_host *host)
> @@ -1202,6 +1282,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev)
>  	struct mmc_host *mmc = dev_get_drvdata(dev);
>  	struct tmio_mmc_host *host = mmc_priv(mmc);
>  
> +	mmc_retune_timer_stop(host->mmc);
> +	mmc_retune_needed(host->mmc);
> +
>  	tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);

All in all, this looks way better now. Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]