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

[Scott Wood <scottwood@freescale.com>]

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

It provides type-safety, and makes accessing the registers more natural.

> > +	struct device_node *guts;
> > +	uint32_t reg = 0;
> 
> s/uint32_t/u32/

Why?

> 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.

-Scott