Re: [PATCH 4/6] mmc: sh_mobile_sdhi: Add UHS-I mode support

[Ben Hutchings <ben.hutchings@codethink.co.uk>]

On Mon, 2015-05-18 at 01:05 +0000, Kuninori Morimoto wrote:
> Hi Ben
> 
> > Implement voltage switch, supporting modes up to SDR-50.
> > 
> > Based on work by Shinobu Uehara, Rob Taylor, William Towle and Ian Molton.
> > 
> > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
> > ---
> 
> I have 1 concern about .start_signal_voltage_switch return value
> 
> > +static int sh_mobile_sdhi_start_signal_voltage_switch(
> > +	struct tmio_mmc_host *host, unsigned char signal_voltage)
> > +{
> > +	struct sh_mobile_sdhi *priv = host_to_priv(host);
> > +	struct pinctrl_state *state;
> > +	int min_uV, max_uV;
> > +	int ret;
> > +
> > +	if (IS_ERR(priv->pinctrl) || IS_ERR(host->mmc->supply.vqmmc))
> > +		return -EOPNOTSUPP;
> (snip)
> > @@ -239,6 +293,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
> >  	host->clk_enable	= sh_mobile_sdhi_clk_enable;
> >  	host->clk_disable	= sh_mobile_sdhi_clk_disable;
> >  	host->multi_io_quirk	= sh_mobile_sdhi_multi_io_quirk;
> > +
> > +	host->start_signal_voltage_switch
> > +				= sh_mobile_sdhi_start_signal_voltage_switch;
> > +
> 
> This sh_mobile_sdhi_start_signal_voltage_switch() will return -EOPNOTSUPP
> if IS_ERR(xx) cases, and it will be used on .tmio_mmc_start_signal_voltage_switch /
> mmc_set_signal_voltage. These will think it is error and goes to error case or try again.
> But, not supported is not error ?

It is if we need to switch to a lower voltage.

> Maybe we need this kind of code somewhere ? (ex. ALSA SoC has similar method)
> 
> 	/* EOPNOTSUPP is not error */
> 	if (ret == -EOPNOTSUPP)
> 		ret = 0;
> 
> I have no objection if my concern never happen.

I think the driver should do something like this if the requested
voltage is MMC_SIGNAL_VOLTAGE_330.  I'll fix that in the next version.

Ben.