| Message ID | 1472203264-21089-3-git-send-email-jm@lentin.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > + 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 ? > + > +#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
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 > >
On Fri, Aug 26, 2016 at 01:24:18PM +0100, Jamie Lentin wrote: > 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? I agree it is a false positive. Probably the best action is to report it to the checkpatch people. > >>+ 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. Agreed. Please leave it as is, and if anybody does care about this, they can submit a followup patch refactoring all instances. Andrew
diff --git a/Documentation/devicetree/bindings/clock/mvebu-core-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-core-clock.txt index 670c2af..eb985a6 100644 --- a/Documentation/devicetree/bindings/clock/mvebu-core-clock.txt +++ b/Documentation/devicetree/bindings/clock/mvebu-core-clock.txt @@ -52,6 +52,7 @@ Required properties: "marvell,dove-core-clock" - for Dove SoC core clocks "marvell,kirkwood-core-clock" - for Kirkwood SoC (except mv88f6180) "marvell,mv88f6180-core-clock" - for Kirkwood MV88f6180 SoC + "marvell,mv88f5181-core-clock" - for Orion MV88F5181 SoC "marvell,mv88f5182-core-clock" - for Orion MV88F5182 SoC "marvell,mv88f5281-core-clock" - for Orion MV88F5281 SoC "marvell,mv88f6183-core-clock" - for Orion MV88F6183 SoC diff --git a/drivers/clk/mvebu/orion.c b/drivers/clk/mvebu/orion.c index fd12956..a6e5bee 100644 --- a/drivers/clk/mvebu/orion.c +++ b/drivers/clk/mvebu/orion.c @@ -21,6 +21,76 @@ static const struct coreclk_ratio orion_coreclk_ratios[] __initconst = { }; /* + * Orion 5181 + */ + +#define SAR_MV88F5181_TCLK_FREQ 8 +#define SAR_MV88F5181_TCLK_FREQ_MASK 0x3 + +static u32 __init mv88f5181_get_tclk_freq(void __iomem *sar) +{ + u32 opt = (readl(sar) >> SAR_MV88F5181_TCLK_FREQ) & + SAR_MV88F5181_TCLK_FREQ_MASK; + if (opt == 0) + return 133333333; + else if (opt == 1) + return 150000000; + else if (opt == 2) + return 166666667; + else + return 0; +} + +#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; +} + +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; + } +} + +static const struct coreclk_soc_desc mv88f5181_coreclks = { + .get_tclk_freq = mv88f5181_get_tclk_freq, + .get_cpu_freq = mv88f5181_get_cpu_freq, + .get_clk_ratio = mv88f5181_get_clk_ratio, + .ratios = orion_coreclk_ratios, + .num_ratios = ARRAY_SIZE(orion_coreclk_ratios), +}; + +static void __init mv88f5181_clk_init(struct device_node *np) +{ + return mvebu_coreclk_setup(np, &mv88f5181_coreclks); +} + +CLK_OF_DECLARE(mv88f5181_clk, "marvell,mv88f5181-core-clock", mv88f5181_clk_init); + +/* * Orion 5182 */