On Wed, 2015-05-06 at 15:25 -0700, Stephen Boyd wrote:
> On 05/06, Scott Wood wrote:
> > On Wed, 2015-05-06 at 00:02 -0700, Stephen Boyd wrote:
> > > On 04/16, Igal.Liberman wrote:
> > > > +static int get_fm_clk_idx(int fm_id, int *fm_clk_idx)
> > > > +{
> > > > + struct ccsr_guts __iomem *guts_regs = NULL;
> > >
> > > Unnecessary initialization to NULL. Also, marking a structure as
> > > __iomem is odd. Why do we need to use a struct to figure out
> > > offsets for registers? Why not just use #defines? That would
> > > probably also make it easy to avoid the asm include here.
> >
> > Using a struct for registers is quite common:
> > scott@snotra:~/fsl/git/linux/upstream$ git grep struct|grep __iomem|wc -l
> > 3005
>
> $ git grep -E 'struct \w+ __iomem' | wc -l
> 2212
>
> That's slightly inflated, but ok.
>
> Within drivers/clk there aren't any though, hence my apprehension
>
> $ git grep -E 'struct \w+ __iomem' -- drivers/clk/ | wc -l
> 0
I'm not sure why clk should be special. Plus, this is a struct that's
been used by other parts of the kernel since before git history began,
rather than something defined specifically for drivers/clk.
> > It provides type-safety, and makes accessing the registers more natural.
>
> Sure, we can leave the struct as is, but to make this compile on
> ARM we need to figure something out. Move the struct definition
> into include/linux/platform_data/ perhaps?
It's register definition rather than platform data, but yes, it should
go somewhere in include/linux. Or I suppose we could put #ifdef
CONFIG_PPC around the fman stuff.
> > > Also unnecessary initialization.
> >
> > Given the if/else if/else if/... nature of how reg is initialized, this
> > seems like a useful and harmless way of making behavior predictable if
> > there is a bug.
> >
>
> If there's a possibility of a bug due to missed initialization
> perhaps it's a sign the code is too complicated and should be
> broken down into smaller functions.
Well, there's always a possibility. :-)
Though rereading this function, reg is only used in the locations where
it's set -- not after the if/else stuff -- so I no longer think this is
a particularly high risk situation. Plus, GCC's gotten pretty
aggressive about warning about such possibilities.
> For example, this function could be rewritten to have a match table
> with function pointers that return the fm_clk_idx.
Yes, that'd be nice.
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
