Re: [PATCH 2/8] arm: orion5x: Add clk support for mv88f5181

[Jamie Lentin <jm-Pj/HzkgeCk7QXOPxS62xeg@public.gmane.org>]

On Fri, 26 Aug 2016, LABBE Corentin wrote:

> Hello
>
> I have some minor comments below
>
> [...]
>> +
>> +static u32 __init mv88f5181_get_tclk_freq(void __iomem *sar)
>> +{
>> +	u32 opt = (readl(sar) >> SAR_MV88F5181_TCLK_FREQ) &
>> +		SAR_MV88F5181_TCLK_FREQ_MASK;
>
> Checkpatch complain about a missing blank line after declaration.
> And spliting the read and the & operation will prevent this line breaking

It isn't here, although I'm not arguing the line is missing. Is there an 
extra switch I'm missing?

>> +	if (opt == 0)
>> +		return 133333333;
>> +	else if (opt == 1)
>> +		return 150000000;
>> +	else if (opt == 2)
>> +		return 166666667;
>> +	else
>> +		return 0;
>> +}
>
> Why not using a switch here ?

If you look at this in context[0], this is one of several code-as-config 
declarations, none of which use a switch. IMO it's more useful to make the 
similarity as obvious as possible if this is ever refactored.

Obviously the rest of the file could also be refactored with switch 
statements, but instead of doing that it seems more tempting to move the 
config into the DT binding, e.g.

   cpu-freq-addr = <4>;
   cpu-freq-mask = <0xf>;
   cpu-freqs = <0 333333333 400000000 400000000 500000000>;
   clk-ratios = <0 1 1 2 1 2 1 3 1 3>;

...this is completely off the top of my head mind, so could easily be 
missing something obvious / some prior art.

[0] http://lxr.free-electrons.com/source/drivers/clk/mvebu/orion.c

>> +
>> +#define SAR_MV88F5181_CPU_FREQ       4
>> +#define SAR_MV88F5181_CPU_FREQ_MASK  0xf
>> +
>> +static u32 __init mv88f5181_get_cpu_freq(void __iomem *sar)
>> +{
>> +	u32 opt = (readl(sar) >> SAR_MV88F5181_CPU_FREQ) &
>> +		SAR_MV88F5181_CPU_FREQ_MASK;
>> +	if (opt == 0)
>> +		return 333333333;
>> +	else if (opt == 1 || opt == 2)
>> +		return 400000000;
>> +	else if (opt == 3)
>> +		return 500000000;
>> +	else
>> +		return 0;
>> +}
>
> Same comments
>
>> +
>> +static void __init mv88f5181_get_clk_ratio(void __iomem *sar, int id,
>> +					   int *mult, int *div)
>> +{
>> +	u32 opt = (readl(sar) >> SAR_MV88F5181_CPU_FREQ) &
>> +		SAR_MV88F5181_CPU_FREQ_MASK;
>> +	if (opt == 0 || opt == 1) {
>> +		*mult = 1;
>> +		*div  = 2;
>> +	} else if (opt == 2 || opt == 3) {
>> +		*mult = 1;
>> +		*div  = 3;
>> +	} else {
>> +		*mult = 0;
>> +		*div  = 1;
>> +	}
>> +}
>
> Same comments
>
> Regards
>
>

-- 
Jamie Lentin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html