Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC

[Paul Osmialowski <pawelo@xxxxxxxxxxx>]

Hi Linus,

Thanks for all of your comments, I'll consider them during my works on the 
next iteration. However, I have doubts about this one:

On Tue, 14 Jul 2015, Linus Walleij wrote:

> On Tue, Jun 30, 2015 at 2:27 PM, Paul Osmialowski <pawelo@xxxxxxxxxxx> wrote:
> 
> > Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3.
> >
> > Signed-off-by: Paul Osmialowski <pawelo@xxxxxxxxxxx>
> (...)
> > +struct kinetis_sim_regs {
> > +       u32 sopt1;      /* System Options Register 1 */
> > +       u32 rsv0[1024];
> > +       u32 sopt2;      /* System Options Register 2 */
> > +       u32 rsv1;
> > +       u32 sopt4;      /* System Options Register 4 */
> > +       u32 sopt5;      /* System Options Register 5 */
> > +       u32 sopt6;      /* System Options Register 6 */
> > +       u32 sopt7;      /* System Options Register 7 */
> > +       u32 rsv2[2];
> > +       u32 sdid;       /* System Device Identification Register */
> > +       u32 scgc[KINETIS_SIM_CG_NUMREGS];       /* Clock Gating Regs 1...7 */
> > +       u32 clkdiv1;    /* System Clock Divider Register 1 */
> > +       u32 clkdiv2;    /* System Clock Divider Register 2 */
> > +       u32 fcfg1;      /* Flash Configuration Register 1 */
> > +       u32 fcfg2;      /* Flash Configuration Register 2 */
> > +       u32 uidh;       /* Unique Identification Register High */
> > +       u32 uidmh;      /* Unique Identification Register Mid-High */
> > +       u32 uidml;      /* Unique Identification Register Mid Low */
> > +       u32 uidl;       /* Unique Identification Register Low */
> > +       u32 clkdiv3;    /* System Clock Divider Register 3 */
> > +       u32 clkdiv4;    /* System Clock Divider Register 4 */
> > +       u32 mcr;        /* Misc Control Register */
> > +};
> 
> Now there is this design pattern where you copy the datasheet
> register map to a struct again.
> 
> This is not good if there is a second revision of the hardware and some
> registers are shuffled around. IMO it is better to just use #defines
> for register
> offsets, so you can do exceptions later. Else a new hardware revision
> leads to a new struct with new accessor functions etc etc.
> 

I don't see how replacing this structure with bunch of defines could make 
anyones life easier. As registers are shuffled around due to updated 
hardware revision they could be shuffled in this structure too. Doing 
this with buch of defines would require eager and careful adaptation of 
all the defines. I don't see how this could be easier.

Note that I'm not making any instances of the structure (it is used only 
for casting), so shuffling its fields around should not affect the code 
that follows.

After recent purge it is only used within this macro:

#define KINETIS_SIM_PTR(base, reg) \
        (&(((struct kinetis_sim_regs *)(base))->reg))

...and used like this:

        ioread32(KINETIS_SIM_PTR(sim, clkdiv1));

IMHO changing the struct internals does not require subsequent changes in 
any of those.

Thanks,
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html