Re: [RFC] clk: shmobile: r8a7795: Add SD divider support

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

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

On Fri, Jan 22, 2016 at 01:26:26PM +0100, Dirk Behme wrote:
> From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> 
> This patch adds SD[0..3] clock divider support for R-Car Gen3 SoC.
> 
> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

So, I tested this patch and it basically works. I say basically because
the SDHI code currently does not change the clock rate, only
en-/disables it. UHS support will need to change the clock later.

One thing I noticed: SD0-2 are 50MHz like the docs say. SD3 is 200MHz
and I couldn't find a reason for that when having a glimpse. Dirk, can
you check?

> +		if (i >= clock->div_num) {
> +			pr_err("%s: 0x%4x is not support of division ratio.\n",
> +				__func__, sd_fc);
> +			return -ENODATA;
> +		}
...
> +	if (i >= clock->div_num) {
> +		pr_err("%s: 0x%4x is not support of division ratio.\n",
> +			__func__, sd_fc);
> +		return 0;
> +	}
...
> +	if (i >= clock->div_num) {
> +		pr_err("%s: Not support divider range : div=%d (%lu/%lu).\n",
> +			__func__, div, parent_rate, rate);
> +		return -EINVAL;
> +	}

For the above code blocks:

a) I'd think that a consistent -EINVAL would be a proper return value.

b) The driver doesn't do much error printouts, so I wonder if those
messages above are favourable. If so, they should probably print which
sd clock is affected?

Thanks,

   Wolfram


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