Re: [PATCH 1/5] [POWERPC] sysdev: implement FSL GTM support

["Grant Likely" <grant.likely@secretlab.ca>]

On Mon, Apr 21, 2008 at 12:39 PM, Scott Wood <scottwood@freescale.com> wrote:
> On Mon, Apr 21, 2008 at 08:03:31AM -0600, Grant Likely wrote:
>
> > >  +    r) Freescale General-purpose Timers Module
>  > >  +
>  > >  +    Required properties:
>  > >  +      - compatible : should be "fsl,gtm" ("fsl,qe-gtm" in addition for QE
>  > >  +                     GTMs or "fsl,cpm2-gtm" for CPM2 GTMs).
>  >
>  > I don't think this is specific enough.  It is a very real possibility
>  > for Freescale to produce another part with a "general purpose timers
>  > module" that isn't register level compatible (and fsl,i2c is an
>  > example of what not to do).
>
>  If that happens, we'll make up "fsl,gtm2". :-)

:-P

>  >  The compatible string should include the
>  > exact chip version.  Newer parts can also claim compatibility with
>  > older ones.
>  >
>  > Defining a 'generic' compatible value is also known as "making stuff up".  :-)
>
>  Nothing wrong with making stuff up as long as we do it sanely.
>
>  How about something like fsl,gtm-1.0?

The problem I have with that is that there is always the potential
that some rogue hardware will appear that messes up your sane scheme.
Plus there is pretty much no downside to just choosing a device that
the more recent parts reference.

Example:  consider 3 chips using the same i2c core; say MPC8313,
MPC5200 & MPC8548.  compatible lines could look like this:
    compatible = "fsl,mpc8313-i2c","fsl-i2c";
    compatible = "fsl,mpc5200-i2c","fsl-i2c";
    compatible = "fsl,mpc8548-i2c","fsl-i2c";

Or; (assuming 8313 is where it first appeared) it could look like this:

    compatible = "fsl,mpc8313-i2c";
    compatible = "fsl,mpc5200-i2c","fsl,mpc8313-i2c";
    compatible = "fsl,mpc8548-i2c","fsl,mpc8313-i2c";

To the driver it's just a different string ("fsl,mpc8313-i2c" instead
of "fsl-i2c"), but with the second you *always* know what an
mpc8131-i2c core is; the mpc8131 user guide tells you.  Even if you
are strict about your sane naming scheme, you cannot look into the
future and someone will do something wonky on you.

The other benefit is it avoids the temptation to define generic
compatible values for things which will *never* be generic parts.
Case in point; when I did the original mpc5200 bindings I did things
like "mpc52xx-<blah>".  However, there probably never will be another
mpc52xx part; and if there is it will look vastly different from the
current 5200.  So I wasted all kinds of effort trying to be generic
about stuff when it is almost certainly wasted effort and additional
complexity to the naming conventions.

>  > >  +static inline void gtm_ack_timer16(struct gtm_timer *tmr, u16 events)
>  > >  +{
>  > >  +       out_be16(tmr->gtevr, events);
>  > >  +}
>  >
>  > Drop 'inline' and expect gcc to do the right thing.
>
>  Not in a header...

Oops, yeah; not sure what I was smoking.

Cheers,
g.



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.