RFC: [PATCH] platform device driver model support

[Kumar Gala]

On Jan 12, 2005, at 2:36 AM, Eugene Surovegin wrote:

> On Wed, Jan 12, 2005 at 01:43:09AM -0600, Kumar Gala wrote:
>  >
> > Please take a look at the following patch.  It adds driver model 
> support
> > via platform devices to 85xx.  This is originally based on patches 
> from
> > Jason M.
> >
> > The idea behind the code is that for a give family: 4xx, 8xx, 82xx, 
> 83xx,
> > 85xx, 86xx we will have structure defns for the following:
>  >
> > enum ppc_soc_devices
>  > in asm-ppc/<family.h>:
> >   list of all unique devices in the family
>  >
> > struct platform_device soc_platform_devices[]
> > in arch/ppc/platforms/<family>/<family>_devices.c:
> >   describes all platform devices that exist in the family
>  >
> > struct soc_spec soc_specs[]
>  > in arch/ppc/platforms/<family>/<family>_soc.c:
> >   describes each unique chip in the family and what devices it has
>
> Well, there is a problem right here at least for 4xx.
> Current OCP implementation is much more flexible IMHO.
>
> For 4xx is not uncommon when you have the same "logical" device at the
> different places with different "properties" (e.h. different channel,
> etc).
>
> Your case (85xx) looks simpler - all you need is a list of devices
> which particular SoC supports, without significant differences in
> "properties". This will not work that easy for 4xx.
>
> In fact, I don't see any gain (at least for 4xx) in all these changes.
> It makes 4xx much more tangled IMHO, because we'll still need all
> those ibm405gp.c, ibm405ep.c, ibm440gp.c etc.
>
> Please note, using platform_device is orthogonal to the way we
> describe each SoC (this is what I don't quite like in your patch), and
> I don't have any strong objections to using platform_device instead of
> OCP or feature_call or whatever for communication with device drivers.

I need to understand a bit more about how 4xx does things.  When I 
started the SOC stuff, it was freescale specific.  I agree that its 
orthogonal to the use of platform device.  Does this some like a bad 
idea for the fsl case?

> > Plus the following functions:
>  >
> > identify_soc_by_id() -- determine soc by an int id
> > identify_soc_by_name() -- determin soc by name (useful in some 82xx 
> cases)
>  > ppc_soc_get_pdata() -- get platform_data pointer so board code can 
> modify
>  > ppc_soc_update_paddr() -- update iomem resources with a given paddr
>
> IMHO, ppc_soc_update_paddr - is a very confusing name, in fact, from
> first read I though it _changes_ paddr to the new value, not _adds_ it
> :)
>
> Probably this function should be made 85xx specific as I cannot come
> up with any use for it in 4xx (we don't have anything similar to
> CCSRBAR ;).

Not an issue, any suggestions on renaming to make it clear what it 
does?  Also, I can make this fsl_increment_paddr() since its useful to 
more than on family of fsl processors.

> [snip]
>
> > +
>  > +struct platform_device soc_platform_devices[] = {
>  > +     [MPC85xx_TSEC1] = {
>  > +             .name = "fsl-gianfar",
>  > +             .id     = 1,
>  > +             .dev.platform_data = &mpc85xx_tsec1_pdata,
> > +             .num_resources   = 4,
>  > +             .resource = (struct resource[]) {
>  > +                     {
>  > +                             .start  = MPC85xx_ENET1_OFFSET,
> > +                             .end    = MPC85xx_ENET1_OFFSET +
>  > +                                             MPC85xx_ENET1_SIZE - 
> 1,
>  > +                             .flags  = IORESOURCE_MEM,
> > +                     },
>  > +                     {
>  > +                             .name   = "tx",
>  > +                             .start  = MPC85xx_IRQ_TSEC1_TX,
> > +                             .end    = MPC85xx_IRQ_TSEC1_TX,
> > +                             .flags  = IORESOURCE_IRQ,
> > +                     },
>  > +                     {
>  > +                             .name   = "rx",
>  > +                             .start  = MPC85xx_IRQ_TSEC1_RX,
> > +                             .end    = MPC85xx_IRQ_TSEC1_RX,
> > +                             .flags  = IORESOURCE_IRQ,
> > +                     },
>  > +                     {
>  > +                             .name   = "error",
>  > +                             .start  = MPC85xx_IRQ_TSEC1_ERROR,
> > +                             .end    = MPC85xx_IRQ_TSEC1_ERROR,
> > +                             .flags  = IORESOURCE_IRQ,
> > +                     },
>  [snip]
>
>
>
> I already wrote about this but repeat again :(.
>
>  Why put all these defines (e.g. for memory regions) into header when
> the only user is this particular file. It doesn't help readability and
>  only obfuscates sources (and 4xx is a very good example of such mess
> :)

Understood, I forgot about this.  I've got now issue with changing it, 
just trying to minimize changes.

thanks

- kumar