MPC52xx: sysfs failure on adding new device driver

[Grant Likely]

On 6/10/05, Sylvain Munaut <tnt at 246tnt.com> wrote:
> Hi Grant
> 
> > In the mean time, here's another option:  Leave
> > arch/ppc/syslib/mpc52xx_devices.c alone, but modify the table in the
> > board setup code to assign specific drivers to the PSC devices before
> > the table is parsed by the platform bus.  This has the added advantage
> > of eliminating the need for mpc52xx_match_psc_function() and it's
> > cousins.
> 
> > +     /* Assign driver names to PSC devices */
> > +     ppc_sys_platform_devices[MPC52xx_PSC1].name = "mpc52xx-psc.uart";
> > +     ppc_sys_platform_devices[MPC52xx_PSC2].name = "mpc52xx-psc.uart";
> > +     ppc_sys_platform_devices[MPC52xx_PSC3].name = "mpc52xx-psc.spi";
> 
> Yes, I kinda like that. That maybe the cleanest way, just 1 line of code
> per device and when no subfn is assigned, nothing is loaded.
> 
> I don't really like messing manually with the ppc_sys_platform
> "internals" outside of the ppc_sys code, but maybe creating a call like
> 
> ppc_sys_assign_subfn(MPC52xx_PSC1,"uart");

I'm continuing to look at this problem and I'm trying to figure out
how pc_sys_assign_subfn() could be implemented.  One issue with this
approach is that the function needs to know what the 'base name' of
the device is.  With the current ppc_sys_platform_device scheme, there
is only the .name field in the platform_device structure.  The first
time ppc_sys_assign_subfn is called on a device it can simply allocate
a new string buffer and concatenate the original value of .name with a
seperator and the function name.

For example (pseudocode):
before:   .name = "mpc52xx-psc";  func="uart";
newname=kmalloc(strlen(.name) + 1 + strlen(func), GFP_KERNEL)
strcpy(newname, .name);
newname[strlen(.name)] = ':';
strcpy(newname+strlen(.name)+1, func);
.name = buff;  /* Note original value of .name is not freed; it was
statically allocated */
after:  .name = "mpc52xx-psc:uart"

If ppc_sys_platform_device is called a second time on the same device,
it needs to free the new buffer, otherwise we have a small memory
leak.  However, the function has no easy way to determine if it is
being called a second time.

I see a few of solutions here:

1. Wrap the 'platform_device' structure with a new structure that
includes a .basename field.  Default declarations of devices should
set .name to NULL and ppc_sys_assign_subfn() will always use .basename
as a prefix.  Any buffer pointed to by .name will always be freed
before assigning new value.

2. Make ppc_sys_assign_subfn() look for the seperator special
character (':' in example).  If the special character is there, make
the assumption that a subfn has already been assigned and the old
value should be freed.  However, this causes problems if the default
device tree wants to specify a default subfunction.

3. Never free the value of .name.  This assumes that once the subfn is
set it will never be changed during runtime and so the function will
only ever replace the original (statically allocated) value of .name. 
I think this option should be avoided; I don't think that the
assumption is appropriate.  There may very well be boards that need to
change the function of a PSC without rebooting.

At the moment, I think option #1 is the cleanest, but it is a little invasive.

Thoughts?
g.