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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
