From: Grant Likely <hidden> Date: 2005-06-08 23:51:58
I'm working on an MPC52xx SPI device driver using one of the PSC.=20
However, when I call driver_register() I get a failure (-17, EEXISTS)
with a traceback (posted below).
I've tracked it down to failing when trying to create a sysfs entry
for the driver. It fails because sysfs tries to create a directory
that already exists (mpc52xx_psc). The directory was already created
when the psc serial port device driver was registered.
From what I can tell, I should be able to register more than one
driver for a particular device name (mpc52xx_psc). Otherwise I would
need to change arch/ppc/syslib/mpc52xx_devices.c to have a different
name for each psc. If I change the sysfs code to ignore the failure
to create a directory then the driver seems to register fine.
I've attached a simple patch that reproduces the problem with stock
linux-2.6.12-rc6. Am I doing something wrong here or is this a bug in
the sysfs code for the platform bus?
Thanks in advance,
g.
----------------------------------------------------------
Patch follows:
----------------------------------------------------------
diff -ruN linux-2.6.11.orig/.config linux-2.6.11/.config
@@ -1,7 +1,7 @@ # # Automatically generated make config: don't edit # Linux kernel version: 2.6.12-rc6-# Wed Jun 8 17:22:48 2005+# Wed Jun 8 17:29:56 2005 # CONFIG_MMU=3Dy CONFIG_GENERIC_HARDIRQS=3Dy
@@ -460,6 +460,12 @@ # CONFIG_INFINIBAND is not set #+# SPI support+#+CONFIG_SPI=3Dy+CONFIG_SPI_MPC52XX_PSC=3Dy++# # File systems # # CONFIG_EXT2_FS is not set
@@ -35,6 +36,8 @@ CONFIG_KALLSYMS=3Dy # CONFIG_KALLSYMS_ALL is not set # CONFIG_KALLSYMS_EXTRA_PASS is not set+CONFIG_PRINTK=3Dy+CONFIG_BUG=3Dy CONFIG_BASE_FULL=3Dy CONFIG_FUTEX=3Dy CONFIG_EPOLL=3Dy
@@ -62,6 +65,7 @@ # CONFIG_POWER4 is not set # CONFIG_8xx is not set # CONFIG_E500 is not set+CONFIG_PPC_FPU=3Dy # CONFIG_ALTIVEC is not set # CONFIG_TAU is not set # CONFIG_CPU_FREQ is not set
@@ -110,6 +114,7 @@ CONFIG_BINFMT_ELF=3Dy # CONFIG_BINFMT_MISC is not set # CONFIG_CMDLINE_BOOL is not set+CONFIG_ISA_DMA_API=3Dy # # Bus options
@@ -347,7 +352,6 @@ # CONFIG_SERIO_LIBPS2 is not set # CONFIG_SERIO_RAW is not set # CONFIG_GAMEPORT is not set-CONFIG_SOUND_GAMEPORT=3Dy # # Character devices
@@ -0,0 +1,19 @@+#+# Character device configuration+#++menu"SPI support"++configSPI+tristate"SPI support"+---help---+SPIisaserialbusprotocolforconnectingbetweenICs++configSPI_MPC52XX_PSC+tristate"SPI bus via MPC5xxx PSC port"+depends onSPI+help+SayYhereifyouwantSPIviaanMPC5xxxPSCport.++endmenu+
From: Mark Chambers <hidden> Date: 2005-06-09 11:20:26
I'm working on an MPC52xx SPI device driver using one of the PSC.
However, when I call driver_register() I get a failure (-17, EEXISTS)
with a traceback (posted below).
Hey Grant,
This sounds exactly like a problem I had with PSC with devfs. I posted
a patch here on 5/27, I bet you'll find it's a similar problem.
(Let me know if you don't have access to that patch, I'll forward you my
copy)
Mark Chambers
I'm working on an MPC52xx SPI device driver using one of the PSC.
However, when I call driver_register() I get a failure (-17, EEXISTS)
with a traceback (posted below).
I've tracked it down to failing when trying to create a sysfs entry
for the driver. It fails because sysfs tries to create a directory
that already exists (mpc52xx_psc). The directory was already created
when the psc serial port device driver was registered.
quoted
From what I can tell, I should be able to register more than one
driver for a particular device name (mpc52xx_psc).
I always assumed that yes.
But now looking more closely, I'm not sure what I based that assumption
on ... And if not the case that's indeed a problem because that's what's
used to support the different function supported by the PSCs.
Otherwise I would
need to change arch/ppc/syslib/mpc52xx_devices.c to have a different
name for each psc.
No you shouldn't have to touch that. The
mpc52xx_match_psc_function(idx, "spi") is there to know which driver
should be used for what PSC and you're using it correctly so it _should_
work.
If I change the sysfs code to ignore the failure
to create a directory then the driver seems to register fine.
A "better" quick-fix would be to change the platform_match
(drivers/platform.c) to support "sub-fonctions". For example when using
mpc52xx_psc.spi it only matches what's before the dot (if any) with the
device name.
That changes the semantic of the driver names for the platform bus
however, making the dot a "special" char.
Sylvain
I'm working on an MPC52xx SPI device driver using one of the PSC.
However, when I call driver_register() I get a failure (-17, EEXISTS)
with a traceback (posted below).
Hey Grant,
This sounds exactly like a problem I had with PSC with devfs. I posted
a patch here on 5/27, I bet you'll find it's a similar problem.
(Let me know if you don't have access to that patch, I'll forward you my
copy)
Don't think so. Your patch applies to the 2.4 while Grant problem is
specific to the way the different functions of PSCs are handled to work
the 2.6 ppc_sys model.
btw, if not yet integrated you should send your patch to Wolfang, he's
the one maintaining that tree.
Sylvain
From: Grant Likely <hidden> Date: 2005-06-09 14:54:44
On 6/9/05, Sylvain Munaut [off-list ref] wrote:
=20
Grant Likely wrote:
quoted
quoted
From what I can tell, I should be able to register more than one
driver for a particular device name (mpc52xx_psc).
=20
I always assumed that yes.
But now looking more closely, I'm not sure what I based that assumption
on ... And if not the case that's indeed a problem because that's what's
used to support the different function supported by the PSCs.
I was assuming so too, and it seems that the device structure would
support it. Who would know the answer to this?
=20
quoted
Otherwise I would
need to change arch/ppc/syslib/mpc52xx_devices.c to have a different
name for each psc.
=20
No you shouldn't have to touch that. The
mpc52xx_match_psc_function(idx, "spi") is there to know which driver
should be used for what PSC and you're using it correctly so it _should_
work.
I thought so, if I disable the mpc52xx_uart driver then my driver will
register correctly. I agree that it is not desireable to touch
mpc52xx_devices.c
=20
quoted
If I change the sysfs code to ignore the failure
to create a directory then the driver seems to register fine.
=20
A "better" quick-fix would be to change the platform_match
(drivers/platform.c) to support "sub-fonctions". For example when using
mpc52xx_psc.spi it only matches what's before the dot (if any) with the
device name.
... so that a different directory will be created in sysfs for each
driver? That's got possibilities.
=20
That changes the semantic of the driver names for the platform bus
however, making the dot a "special" char.
Who needs to be asked about this? Should I take this discussion over
the the LKML?
Thanks,
g.
From: Kumar Gala <hidden> Date: 2005-06-09 15:20:38
quoted
That changes the semantic of the driver names for the platform bus
however, making the dot a "special" char.
Who needs to be asked about this? Should I take this discussion over
the the LKML?
GregKH would be the person to talk to about the driver core and taking
this to LKML would be useful. We probably have a similar issue with
CPM comm channels.
I always assumed multiple drivers would get probed and the drivers
would fail in probe if they were not suppose to bind to the given
channel/psc, etc..
- kumar
From: Grant Likely <hidden> Date: 2005-06-09 18:48:39
On 6/9/05, Kumar Gala [off-list ref] wrote:
=20
quoted
quoted
That changes the semantic of the driver names for the platform bus
however, making the dot a "special" char.
Who needs to be asked about this? Should I take this discussion over
the the LKML?
=20
GregKH would be the person to talk to about the driver core and taking
this to LKML would be useful. We probably have a similar issue with
CPM comm channels.
=20
I always assumed multiple drivers would get probed and the drivers
would fail in probe if they were not suppose to bind to the given
channel/psc, etc..
=20
I took another look at the sysfs layout, and it doesn't really look
like it is intended to support multiple drivers with the same name.=20
:-( I'll ask GregKH about it.
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.
Thoughts? Will this scheme negatively affect portability?
Here's a working example patch:
--------------------------------------------------------------------------
diff -ruN linux-2.6.12rc6.orig/arch/ppc/platforms/lite5200.c
linux-2.6.12rc6.spi2/arch/ppc/platforms/lite5200.c
=20
+=09/* Assign driver names to PSC devices */
+=09ppc_sys_platform_devices[MPC52xx_PSC1].name =3D "mpc52xx-psc.uart";
+=09ppc_sys_platform_devices[MPC52xx_PSC2].name =3D "mpc52xx-psc.uart";
+=09ppc_sys_platform_devices[MPC52xx_PSC3].name =3D "mpc52xx-psc.spi";
+
=09/* Powersave */
=09/* This is provided as an example on how to do it. But you
=09 need to be aware that NAP disable bus snoop and that may
diff -ruN linux-2.6.12rc6.orig/arch/ppc/syslib/mpc52xx_setup.c
linux-2.6.12rc6.spi2/arch/ppc/syslib/mpc52xx_setup.c
@@ -29,8 +29,9 @@ /* Platform device Usage : * * Since PSCs can have multiple function, the correct driver for each one- * is selected by calling mpc52xx_match_psc_function(...). The function- * handled by this driver is "uart".+ * is selected based on the name assigned to the psc. By convention, the+ * function is appended to the device name in the board setup code. For+ * example, this uart psc driver will only bind to mpc52xx_psc.uart device=
s.
*
* The driver init all necessary registers to place the PSC in uart
mode without
* DCD. However, the pin multiplexing aren't changed and should be set eit=
her
@@ -0,0 +1,19 @@+#+# Character device configuration+#++menu "SPI support"++config SPI+=09tristate "SPI support"+=09---help---+=09 SPI is a serial bus protocol for connecting between ICs++config SPI_MPC52XX_PSC+=09tristate "SPI bus via MPC5xxx PSC port"+=09depends on SPI+=09help+=09 Say Y here if you want SPI via an MPC5xxx PSC port.++endmenu+
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.
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");
and place it in the ppc_sys code so that other platforms havin such
"multi usage" device all have an uniform way of handling that. Galak ?
Sylvain
=20
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.
=20
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
=20
ppc_sys_assign_subfn(MPC52xx_PSC1,"uart");
=20
=20
and place it in the ppc_sys code so that other platforms havin such
"multi usage" device all have an uniform way of handling that. Galak ?
Hmm, yes... I like this better. I was also uncomfortable with
messing with the table directly. A function like that can make sure
that the table is not modified after it is registered with the
platform bus; or if it is, make sure that a driver has not yet been
assigned and that sysfs is properly updated. It would protect against
doing something stupid like:
ppc_sys_platform_devices[MPC52xx_MSCAN1].name =3D "mpc52xx-psc.uart";
g.
From: Grant Likely <hidden> Date: 2005-06-13 19:08:54
On 6/10/05, Sylvain Munaut [off-list ref] wrote:
Hi Grant
=20
quoted
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.
=20
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.
=20
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
=20
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 =3D "mpc52xx-psc"; func=3D"uart";
newname=3Dkmalloc(strlen(.name) + 1 + strlen(func), GFP_KERNEL)
strcpy(newname, .name);
newname[strlen(.name)] =3D ':';
strcpy(newname+strlen(.name)+1, func);
.name =3D buff; /* Note original value of .name is not freed; it was
statically allocated */
after: .name =3D "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.=20
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 invasi=
ve.
Thoughts?
g.