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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
