MPC52xx: sysfs failure on adding new device driver

MPC52xx: sysfs failure on adding new device driver

[Grant Likely]

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
--- linux-2.6.11.orig/.config   2005-06-08 17:22:48.000000000 -0600
+++ linux-2.6.11/.config        2005-06-08 17:29:56.000000000 -0600
@@ -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
diff -ruN linux-2.6.11.orig/.config.old linux-2.6.11/.config.old
--- linux-2.6.11.orig/.config.old       2005-06-08 17:22:31.000000000 -0600
+++ linux-2.6.11/.config.old    2005-06-08 17:22:48.000000000 -0600
@@ -1,7 +1,7 @@
 #
 # Automatically generated make config: don't edit
-# Linux kernel version: 2.6.12-rc2
-# Thu May 19 12:30:43 2005
+# Linux kernel version: 2.6.12-rc6
+# Wed Jun  8 17:22:48 2005
 #
 CONFIG_MMU=3Dy
 CONFIG_GENERIC_HARDIRQS=3Dy
@@ -11,6 +11,7 @@
 CONFIG_PPC=3Dy
 CONFIG_PPC32=3Dy
 CONFIG_GENERIC_NVRAM=3Dy
+CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER=3Dy

 #
 # Code maturity level options
@@ -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
diff -ruN linux-2.6.11.orig/.version linux-2.6.11/.version
--- linux-2.6.11.orig/.version  2005-06-08 17:24:33.000000000 -0600
+++ linux-2.6.11/.version       2005-06-08 17:30:04.000000000 -0600
@@ -1 +1 @@
-1
+2
Files linux-2.6.11.orig/arch/ppc/boot/images/uImage and
linux-2.6.11/arch/ppc/boot/images/uImage differ
Files linux-2.6.11.orig/arch/ppc/boot/images/vmlinux.bin and
linux-2.6.11/arch/ppc/boot/images/vmlinux.bin differ
Files linux-2.6.11.orig/arch/ppc/boot/images/vmlinux.gz and
linux-2.6.11/arch/ppc/boot/images/vmlinux.gz differ
diff -ruN linux-2.6.11.orig/drivers/Kconfig linux-2.6.11/drivers/Kconfig
--- linux-2.6.11.orig/drivers/Kconfig   2005-03-02 00:38:26.000000000 -0700
+++ linux-2.6.11/drivers/Kconfig        2005-06-08 17:27:23.000000000 -0600
@@ -58,4 +58,6 @@

 source "drivers/infiniband/Kconfig"

+source "drivers/spi/Kconfig"
+
 endmenu
diff -ruN linux-2.6.11.orig/drivers/Makefile linux-2.6.11/drivers/Makefile
--- linux-2.6.11.orig/drivers/Makefile  2005-06-08 17:22:08.000000000 -0600
+++ linux-2.6.11/drivers/Makefile       2005-06-08 17:27:13.000000000 -0600
@@ -64,3 +64,4 @@
 obj-$(CONFIG_BLK_DEV_SGIIOC4)  +=3D sn/
 obj-y                          +=3D firmware/
 obj-$(CONFIG_CRYPTO)           +=3D crypto/
+obj-$(CONFIG_SPI)              +=3D spi/
diff -ruN linux-2.6.11.orig/drivers/spi/Kconfig
linux-2.6.11/drivers/spi/Kconfig---
linux-2.6.11.orig/drivers/spi/Kconfig       1969-12-31
17:00:00.000000000 -0700
+++ linux-2.6.11/drivers/spi/Kconfig    2005-06-08 17:29:37.000000000 -0600
@@ -0,0 +1,19 @@
+#
+# Character device configuration
+#
+
+menu "SPI support"
+
+config SPI
+       tristate "SPI support"
+       ---help---
+         SPI is a serial bus protocol for connecting between ICs
+
+config SPI_MPC52XX_PSC
+       tristate "SPI bus via MPC5xxx PSC port"
+       depends on SPI
+       help
+         Say Y here if you want SPI via an MPC5xxx PSC port.
+
+endmenu
+
diff -ruN linux-2.6.11.orig/drivers/spi/Makefile
linux-2.6.11/drivers/spi/Makefile
--- linux-2.6.11.orig/drivers/spi/Makefile      1969-12-31
17:00:00.000000000 -0700
+++ linux-2.6.11/drivers/spi/Makefile   2005-06-08 17:29:17.000000000 -0600
@@ -0,0 +1,6 @@
+#
+# Makefile for the spi core.
+#
+
+obj-$(CONFIG_SPI_MPC52XX_PSC)  +=3D spi-mpc5xxx-psc.o
+
diff -ruN linux-2.6.11.orig/drivers/spi/spi-mpc5xxx-psc.c
linux-2.6.11/drivers/spi/spi-mpc5xxx-psc.c
--- linux-2.6.11.orig/drivers/spi/spi-mpc5xxx-psc.c     1969-12-31
17:00:00.000000000 -0700
+++ linux-2.6.11/drivers/spi/spi-mpc5xxx-psc.c  2005-06-08
17:29:07.000000000 -0600
@@ -0,0 +1,60 @@
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/cdev.h>
+
+#include <asm/mpc52xx.h>
+#include <asm/mpc52xx_psc.h>
+
+MODULE_LICENSE("Dual BSD/GPL");
+
+static int __devinit
+spi_mpc52xx_psc_probe(struct device *dev)
+{
+       struct platform_device *pdev =3D to_platform_device(dev);
+       /*struct resource *res =3D pdev->resource;*/
+       int idx =3D pdev->id;
+
+       printk(KERN_ALERT "spi-mpc52xx-psc: probing idx=3D%i\n", idx);
+
+       if (!mpc52xx_match_psc_function(idx, "spi"))
+       {
+               printk(KERN_ALERT "function not matched!\n");
+               return -ENODEV;
+       }
+
+       return 0;
+}
+
+static int
+spi_mpc52xx_psc_remove(struct device *dev)
+{
+       return 0;
+}
+
+static struct device_driver spi_mpc52xx_psc_platform_driver =3D {
+       .name           =3D "mpc52xx-psc",
+       .bus            =3D &platform_bus_type,
+       .probe          =3D spi_mpc52xx_psc_probe,
+       .remove         =3D spi_mpc52xx_psc_remove,
+};
+
+static int __init spi_mpc52xx_psc_init(void)
+{
+       int ret;
+
+       printk(KERN_ALERT "spi_mpc52xx_psc: initializing\n");
+
+       ret =3D driver_register(&spi_mpc52xx_psc_platform_driver);
+       return ret;
+}
+
+static void __exit spi_mpc52xx_psc_exit(void)
+{
+       driver_unregister(&spi_mpc52xx_psc_platform_driver);
+       printk(KERN_ALERT "spi_mpc52xx_psc: exiting\n");
+}
+
+module_init(spi_mpc52xx_psc_init);
+module_exit(spi_mpc52xx_psc_exit);
diff -ruN linux-2.6.11.orig/include/config/spi/mpc52xx/psc.h
linux-2.6.11/include/config/spi/mpc52xx/psc.h
--- linux-2.6.11.orig/include/config/spi/mpc52xx/psc.h  1969-12-31
17:00:00.000000000 -0700
+++ linux-2.6.11/include/config/spi/mpc52xx/psc.h       2005-06-08
17:29:56.000000000 -0600
@@ -0,0 +1 @@
+#define CONFIG_SPI_MPC52XX_PSC 1
diff -ruN linux-2.6.11.orig/include/config/spi.h
linux-2.6.11/include/config/spi.h
--- linux-2.6.11.orig/include/config/spi.h      1969-12-31
17:00:00.000000000 -0700
+++ linux-2.6.11/include/config/spi.h   2005-06-08 17:29:56.000000000 -0600
@@ -0,0 +1 @@
+#define CONFIG_SPI 1
diff -ruN linux-2.6.11.orig/include/linux/autoconf.h
linux-2.6.11/include/linux/autoconf.h
--- linux-2.6.11.orig/include/linux/autoconf.h  2005-06-08
17:22:48.000000000 -0600
+++ linux-2.6.11/include/linux/autoconf.h       2005-06-08
17:29:56.000000000 -0600
@@ -1,7 +1,7 @@
 /*
  * Automatically generated C 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
  */
 #define AUTOCONF_INCLUDED
 #define CONFIG_MMU 1
@@ -461,6 +461,12 @@
 #undef CONFIG_INFINIBAND

 /*
+ * SPI support
+ */
+#define CONFIG_SPI 1
+#define CONFIG_SPI_MPC52XX_PSC 1
+
+/*
  * File systems
  */
 #undef CONFIG_EXT2_FS


----------------------------------------------------------
Boot log follows:
----------------------------------------------------------

## Booting image at 00100000 ...
   Image Name:   Linux-2.6.12-rc6-lite5200
   Created:      2005-06-08  23:30:09 UTC
   Image Type:   PowerPC Linux Kernel Image (gzip compressed)
   Data Size:    571225 Bytes =3D 557.8 kB
   Load Address: 00000000
   Entry Point:  00000000
   Verifying Checksum ... OK
   Uncompressing Kernel Image ... OK
id mach(): done
MMU:enter
MMU:hw init
MMU:mapin
MMU:setio
MMU:exit
setup_arch: enter
setup_arch: bootmem
arch: exit
Linux version 2.6.12-rc6-lite5200 (glikely@trillian) (gcc version 3.4.3) #2=
 Wed
Jun 8 17:30:04 MDT 2005
Built 1 zonelists
Kernel command line: console=3DttyS0,115200
mtdparts=3Dphys_mapped_flash:14M(jffs2),1M(kernel),1M(boot)
root=3D/dev/mtdblock0 rootfstype=3Djffs2 rw
PID hash table entries: 512 (order: 9, 8192 bytes)
Dentry cache hash table entries: 16384 (order: 4, 65536 bytes)
Inode-cache hash table entries: 8192 (order: 3, 32768 bytes)
Memory: 63616k available (908k kernel code, 256k data, 72k init, 0k highmem=
)
Mount-cache hash table entries: 512
Linux NoNET1.0 for Linux 2.6

PCI: Probing PCI hardware
JFFS2 version 2.2. (C) 2001-2003 Red Hat, Inc.
Serial: MPC52xx PSC driver
ttyS0 at MMIO 0xf0002000 (irq =3D 39) is a MPC52xx PSC
io scheduler noop registered
io scheduler anticipatory registered
io scheduler deadline registered
io scheduler cfq registered
physmap flash device: 1000000 at ff000000
phys_mapped_flash: Found 1 x8 devices at 0x0 in 8-bit bank
phys_mapped_flash: Found 1 x8 devices at 0x800000 in 8-bit bank
 Amd/Fujitsu Extended Query Table at 0x0040
phys_mapped_flash: CFI does not contain boot bank location. Assuming top.
number of CFI chips: 2
cfi_cmdset_0002: Disabling erase-suspend-program due to code brokenness.
3 cmdlinepart partitions found on MTD device phys_mapped_flash
Creating 3 MTD partitions on "phys_mapped_flash":
0x00000000-0x00e00000 : "jffs2"
0x00e00000-0x00f00000 : "kernel"
0x00f00000-0x01000000 : "boot"
mice: PS/2 mouse device common for all mice
spi_mpc52xx_psc: initializing
kobject_register failed for mpc52xx-psc (-17)
Call trace:
 [c0099128] kobject_register+0x60/0x78
 [c00bc5ac] bus_add_driver+0x78/0x178
 [c00bccc8] driver_register+0x30/0x40
 [c0125b90] spi_mpc52xx_psc_init+0x24/0x34
 [c0003944] init+0x7c/0x22c
 [c0006a04] kernel_thread+0x44/0x60
VFS: Mounted root (jffs2 filesystem).
Freeing unused kernel memory: 72k init
ifconfig: socket: Function not implemented
route: socket: Function not implemented
/etc/rcS: 7: /usr/sbin/telnetd: not f\uffff

BusyBox v1.00 (2005.05.19-06:22+0000) Built-in shell (ash)
Enter 'help' for a list of built-in commands.

/ #

Re: MPC52xx: sysfs failure on adding new device driver

[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).

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

Re: MPC52xx: sysfs failure on adding new device driver

[Sylvain Munaut]

Grant Likely wrote:
> 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.
> 
>>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

Re: MPC52xx: sysfs failure on adding new device driver

[Sylvain Munaut]

Hi Mark

Mark Chambers wrote:
>>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

Re: MPC52xx: sysfs failure on adding new device driver

[Wolfgang Denk]

In message <42A828B7.4030305@246tNt.com> Sylvain Munaut wrote:
> 
> 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.

I'm subscribed here, too :-)

I've seen the patch, but did not decide yet what to do with it.  Many
drivers  don't  support  devfs  anyway, and in my opinion devfs makes
little sense in embedded systems anyway. Not to mention that it  will
officially  be removed from the kernel tree in less than 4 weeks from
now (see Documentation/feature-removal-schedule.txt).

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Man is the best computer we can put aboard a spacecraft ...  and  the
only one that can be mass produced with unskilled labor.
                                                 -- Wernher von Braun

Re: MPC52xx: sysfs failure on adding new device driver

[Grant Likely]

On 6/9/05, Sylvain Munaut <tnt@246tnt.com> wrote:
>=20
> Grant Likely wrote:
> >>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
> > 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
> > 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.

Re: MPC52xx: sysfs failure on adding new device driver

[Kumar Gala]

>> 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

Re: MPC52xx: sysfs failure on adding new device driver

[Mark Chambers]

Sylvain:
> btw, if not yet integrated you should send your patch to Wolfang, he's 
> the one maintaining that tree.

Wolfgang:
> I've seen the patch, but did not decide yet what to do with it.  Many
> drivers  don't  support  devfs  anyway, and in my opinion devfs makes
> little sense in embedded systems anyway. Not to mention that it  will
> officially  be removed from the kernel tree in less than 4 weeks from
> now (see Documentation/feature-removal-schedule.txt).

This is nonsense.  You know, when I made this patch I said 
to myself, "Self, you're wasting your time, these guys won't take patches 
from somebody they don't know"  But I thought it was enough of a 
no-brainer that it might go through.

Seems I was wrong.

Mark Chambers

Re: MPC52xx: sysfs failure on adding new device driver

[Grant Likely]

On 6/9/05, Mark Chambers <markc@mail.com> wrote:
> Sylvain:
> > btw, if not yet integrated you should send your patch to Wolfang, he's
> > the one maintaining that tree.
>=20
> Wolfgang:
> > I've seen the patch, but did not decide yet what to do with it.  Many
> > drivers  don't  support  devfs  anyway, and in my opinion devfs makes
> > little sense in embedded systems anyway. Not to mention that it  will
> > officially  be removed from the kernel tree in less than 4 weeks from
> > now (see Documentation/feature-removal-schedule.txt).
>=20
> This is nonsense.  You know, when I made this patch I said
> to myself, "Self, you're wasting your time, these guys won't take patches
> from somebody they don't know"  But I thought it was enough of a
> no-brainer that it might go through.
>=20
> Seems I was wrong.
>=20
Don't take things too personally.  (I understand where you're coming
from; I'm virtually unknown around here also).  You have to remember
that devfs is pretty much considered to be a bastard stepchild and
there is a lot of negative opinion surrounding it.  Because of that it
is very hard to get *any* devfs patches to be considered.

Just posting your patch on the ML is important and valuable.  That
makes things easy when somebody else has the same problem.  I
certainly remember seeing your original post, and I made note of it
for if I ever see that problem.

Cheers,
g.

Re: MPC52xx: sysfs failure on adding new device driver

[Grant Likely]

On 6/9/05, Kumar Gala <kumar.gala@freescale.com> wrote:
>=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?
>=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
--- linux-2.6.12rc6.orig/arch/ppc/platforms/lite5200.c=092005-06-08
17:22:05.000000000 -0600
+++ linux-2.6.12rc6.spi2/arch/ppc/platforms/lite5200.c=092005-06-09
12:23:26.000000000 -0600
@@ -50,17 +50,6 @@
 /* Platform specific code                                                 =
  */
 /* =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D */
=20
-/* Supported PSC function in "preference" order */
-struct mpc52xx_psc_func mpc52xx_psc_functions[] =3D {
-=09=09{       .id     =3D 0,
-=09=09=09.func   =3D "uart",
-=09=09},
-=09=09{       .id     =3D -1,   /* End entry */
-=09=09=09.func   =3D NULL,
-=09=09}
-=09};
-
-
 static int
 lite5200_show_cpuinfo(struct seq_file *m)
 {
@@ -198,6 +187,11 @@
 =09isa_io_base=09=09=3D 0;
 =09isa_mem_base=09=09=3D 0;
=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
--- linux-2.6.12rc6.orig/arch/ppc/syslib/mpc52xx_setup.c=092005-06-08
17:22:05.000000000 -0600
+++ linux-2.6.12rc6.spi2/arch/ppc/syslib/mpc52xx_setup.c=092005-06-09
12:11:31.000000000 -0600
@@ -216,15 +216,3 @@
 =09tb_to_us =3D mulhwu_scale_factor(xlbfreq / divisor, 1000000);
 }
=20
-int mpc52xx_match_psc_function(int psc_idx, const char *func)
-{
-=09struct mpc52xx_psc_func *cf =3D mpc52xx_psc_functions;
-
-=09while ((cf->id !=3D -1) && (cf->func !=3D NULL)) {
-=09=09if ((cf->id =3D=3D psc_idx) && !strcmp(cf->func,func))
-=09=09=09return 1;
-=09=09cf++;
-=09}
-
-=09return 0;
-}
diff -ruN linux-2.6.12rc6.orig/drivers/Kconfig
linux-2.6.12rc6.spi2/drivers/Kconfig
--- linux-2.6.12rc6.orig/drivers/Kconfig=092005-03-02 00:38:26.000000000 -0=
700
+++ linux-2.6.12rc6.spi2/drivers/Kconfig=092005-06-08 17:27:23.000000000 -0=
600
@@ -58,4 +58,6 @@
=20
 source "drivers/infiniband/Kconfig"
=20
+source "drivers/spi/Kconfig"
+
 endmenu
diff -ruN linux-2.6.12rc6.orig/drivers/Makefile
linux-2.6.12rc6.spi2/drivers/Makefile
--- linux-2.6.12rc6.orig/drivers/Makefile=092005-06-08 17:22:08.000000000 -=
0600
+++ linux-2.6.12rc6.spi2/drivers/Makefile=092005-06-08 17:27:13.000000000 -=
0600
@@ -64,3 +64,4 @@
 obj-$(CONFIG_BLK_DEV_SGIIOC4)=09+=3D sn/
 obj-y=09=09=09=09+=3D firmware/
 obj-$(CONFIG_CRYPTO)=09=09+=3D crypto/
+obj-$(CONFIG_SPI)=09=09+=3D spi/
diff -ruN linux-2.6.12rc6.orig/drivers/serial/mpc52xx_uart.c
linux-2.6.12rc6.spi2/drivers/serial/mpc52xx_uart.c
--- linux-2.6.12rc6.orig/drivers/serial/mpc52xx_uart.c=092005-06-08
17:22:23.000000000 -0600
+++ linux-2.6.12rc6.spi2/drivers/serial/mpc52xx_uart.c=092005-06-09
12:14:40.000000000 -0600
@@ -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
@@ -730,9 +731,6 @@
 =09if (idx < 0 || idx >=3D MPC52xx_PSC_MAXNUM)
 =09=09return -EINVAL;
=20
-=09if (!mpc52xx_match_psc_function(idx,"uart"))
-=09=09return -ENODEV;
-
 =09/* Init the port structure */
 =09port =3D &mpc52xx_uart_ports[idx];
=20
@@ -804,7 +802,7 @@
 #endif
=20
 static struct device_driver mpc52xx_uart_platform_driver =3D {
-=09.name=09=09=3D "mpc52xx-psc",
+=09.name=09=09=3D "mpc52xx-psc.uart",
 =09.bus=09=09=3D &platform_bus_type,
 =09.probe=09=09=3D mpc52xx_uart_probe,
 =09.remove=09=09=3D mpc52xx_uart_remove,
diff -ruN linux-2.6.12rc6.orig/drivers/spi/Kconfig
linux-2.6.12rc6.spi2/drivers/spi/Kconfig
--- linux-2.6.12rc6.orig/drivers/spi/Kconfig=091969-12-31 17:00:00.00000000=
0 -0700
+++ linux-2.6.12rc6.spi2/drivers/spi/Kconfig=092005-06-08 17:29:37.00000000=
0 -0600
@@ -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
+
diff -ruN linux-2.6.12rc6.orig/drivers/spi/Makefile
linux-2.6.12rc6.spi2/drivers/spi/Makefile
--- linux-2.6.12rc6.orig/drivers/spi/Makefile=091969-12-31
17:00:00.000000000 -0700
+++ linux-2.6.12rc6.spi2/drivers/spi/Makefile=092005-06-08
17:29:17.000000000 -0600
@@ -0,0 +1,6 @@
+#
+# Makefile for the spi core.
+#
+
+obj-$(CONFIG_SPI_MPC52XX_PSC)=09+=3D spi-mpc5xxx-psc.o
+
diff -ruN linux-2.6.12rc6.orig/drivers/spi/spi-mpc5xxx-psc.c
linux-2.6.12rc6.spi2/drivers/spi/spi-mpc5xxx-psc.c
--- linux-2.6.12rc6.orig/drivers/spi/spi-mpc5xxx-psc.c=091969-12-31
17:00:00.000000000 -0700
+++ linux-2.6.12rc6.spi2/drivers/spi/spi-mpc5xxx-psc.c=092005-06-09
12:20:28.000000000 -0600
@@ -0,0 +1,54 @@
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/cdev.h>
+
+#include <asm/mpc52xx.h>
+#include <asm/mpc52xx_psc.h>
+
+MODULE_LICENSE("Dual BSD/GPL");
+
+static int __devinit
+spi_mpc52xx_psc_probe(struct device *dev)
+{
+=09struct platform_device *pdev =3D to_platform_device(dev);
+=09/*struct resource *res =3D pdev->resource;*/
+=09int idx =3D pdev->id;
+
+=09printk(KERN_ALERT "spi-mpc52xx-psc: probing idx=3D%i\n", idx);
+
+=09return 0;
+}
+
+static int=20
+spi_mpc52xx_psc_remove(struct device *dev)
+{
+=09return 0;
+}
+
+static struct device_driver spi_mpc52xx_psc_platform_driver =3D {
+=09.name=09=09=3D "mpc52xx-psc.spi",
+=09.bus=09=09=3D &platform_bus_type,
+=09.probe=09=09=3D spi_mpc52xx_psc_probe,
+=09.remove=09=09=3D spi_mpc52xx_psc_remove,
+};
+
+static int __init spi_mpc52xx_psc_init(void)
+{
+=09int ret;
+
+=09printk(KERN_ALERT "spi_mpc52xx_psc: initializing\n");
+
+=09ret =3D driver_register(&spi_mpc52xx_psc_platform_driver);
+=09return ret;
+}
+
+static void __exit spi_mpc52xx_psc_exit(void)
+{
+=09driver_unregister(&spi_mpc52xx_psc_platform_driver);
+=09printk(KERN_ALERT "spi_mpc52xx_psc: exiting\n");
+}
+
+module_init(spi_mpc52xx_psc_init);
+module_exit(spi_mpc52xx_psc_exit);
diff -ruN linux-2.6.12rc6.orig/include/asm-ppc/mpc52xx.h
linux-2.6.12rc6.spi2/include/asm-ppc/mpc52xx.h
--- linux-2.6.12rc6.orig/include/asm-ppc/mpc52xx.h=092005-06-08
17:22:29.000000000 -0600
+++ linux-2.6.12rc6.spi2/include/asm-ppc/mpc52xx.h=092005-06-09
12:11:19.000000000 -0600
@@ -415,17 +415,6 @@
=20
 extern void mpc52xx_find_bridges(void);
=20
-
-=09/* Matching of PSC function */
-struct mpc52xx_psc_func {
-=09int id;
-=09char *func;
-};
-
-extern int mpc52xx_match_psc_function(int psc_idx, const char *func);
-extern struct  mpc52xx_psc_func mpc52xx_psc_functions[];
-=09/* This array is to be defined in platform file */
-
 #endif /* __ASSEMBLY__ */

Re: MPC52xx: sysfs failure on adding new device driver

[Sylvain Munaut]

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");


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

Re: MPC52xx: sysfs failure on adding new device driver

[Grant Likely]

On 6/10/05, Sylvain Munaut <tnt@246tnt.com> wrote:
> Hi Grant
>=20
> > +     /* Assign driver names to PSC devices */
> > +     ppc_sys_platform_devices[MPC52xx_PSC1].name =3D "mpc52xx-psc.uart=
";
> > +     ppc_sys_platform_devices[MPC52xx_PSC2].name =3D "mpc52xx-psc.uart=
";
> > +     ppc_sys_platform_devices[MPC52xx_PSC3].name =3D "mpc52xx-psc.spi"=
;
>=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.

Re: MPC52xx: sysfs failure on adding new device driver

[Grant Likely]

On 6/10/05, Sylvain Munaut <tnt@246tnt.com> wrote:
> Hi Grant
>=20
> > 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
> > +     /* Assign driver names to PSC devices */
> > +     ppc_sys_platform_devices[MPC52xx_PSC1].name =3D "mpc52xx-psc.uart=
";
> > +     ppc_sys_platform_devices[MPC52xx_PSC2].name =3D "mpc52xx-psc.uart=
";
> > +     ppc_sys_platform_devices[MPC52xx_PSC3].name =3D "mpc52xx-psc.spi"=
;
>=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.