Re: [v4] clk: qoriq: Add support for the FMan clock

[Scott Wood <scottwood@freescale.com>]

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