Re: [PATCH/RFC v2 1/3] mmc: tmio: Add tuning support

[Simon Horman <horms@verge.net.au>]

On Mon, May 23, 2016 at 12:53:59AM +0000, Kuninori Morimoto wrote:
> 
> Hi Simon
> 
> Thank you for your patch.
> Picky comment from me :)

Thanks for noticing these things and sorry for not having
done so myself.

> > From: Ai Kyuse <ai.kyuse.uw@renesas.com>
> > 
> > Add tuning support for use with SDR104 mode
> > 
> > Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com>
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> (snip)
> > @@ -160,6 +161,11 @@ struct tmio_mmc_host {
> >  			      unsigned int direction, int blk_size);
> >  	int (*start_signal_voltage_switch)(struct mmc_host *mmc,
> >  					   struct mmc_ios *ios);
> > +	unsigned int (*init_tuning)(struct tmio_mmc_host *host);
> > +	int (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap);
> > +	int (*select_tuning)(struct tmio_mmc_host *host, bool *tap);
> > +	bool (*retuning)(struct tmio_mmc_host *host);
> > +	void (*hw_reset)(struct tmio_mmc_host *host);
> >  };
> 
> There is no (*retuning) code ?

Thanks, I think returning should simply be removed.

> > +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;
> > +		goto out;
> > +	}
> > +
> > +	tap = kmalloc(num * 2, GFP_KERNEL);
> > +	if (tap == NULL) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> 
> 	if (!tap) {
> 		...
> 	}

ok

> > +	/*
> > +	 * Issue CMD19 twice for each tap
> > +	 */
> > +	for (i = 0; i < 2 * num; i++) {
> > +		int err;
> > +
> > +		if (host->prepare_tuning)
> > +			host->prepare_tuning(host, i);
> 
> you want to check return value here ?
> 	int (*prepare_tuning)(xxx);

The implementation of prepare_tuning doesn't seem to ever return an error.
Perhaps it would be best to just change the type to:

	void (*prepare_tuning)(xxx);

> > +		err = mmc_send_tuning(host->mmc, opcode, NULL);
> > +		if (err && err != -EILSEQ) {
> > +			ret = err;
> > +			goto err_free;
> > +		}
> > +		tap[i] = (err == 0);
> > +
> > +		mdelay(1);
> > +	}
> > +
> > +	if (host->select_tuning)
> > +		ret = host->select_tuning(host, tap);
> 
> Here, "tap" was alloc:ed array, but there is no array_size parameter.
> A littile bit strange for me..., but I'm not sure

Right. I think its safe. But its not very clean.
I will add a size parameter.