OF-platform PATA driver
cover OF-platform PATA driver
Hi all,
Here is the PATA Platform driver using OF infrastructure.
Mostly it's just a wrapper around a bit modified pata_platform driver.
Patches are well split for the easier review:
First one factors out platform_device specific bits and modifies pata_platform to be a library-alike driver (with platform_device default binding).
Second patch is OF-driver itself which is using pata_platform "library".
Third patch is PowerPC specific, but I'm still Cc'ing linux-ide, just to show how we're using it.
As an alternative approach we can use plain pata_platform driver, but I'm not sure how Linux OF bindings' ideologists will or will not like it.
So, these patches are strongly Request For Comments. Feel free to train your nitpicking skills ;-), and/or vote for the option you most pleased about (or suggest another?).
Thanks.
---
Down here is "alternative approach".
Probably board-neutral version may be placed somewhere in the drivers/of/...? But who will call it: board file, or device_initcall for all boards?
arch/powerpc/platforms/83xx/mpc834x_itx.c +73 -0
@@ -24,6 +24,7 @@#include <linux/seq_file.h> #include <linux/root_dev.h> #include <linux/of_platform.h> +#include <linux/pata_platform.h> #include <asm/system.h> #include <asm/atomic.h>
@@ -102,6 +103,78 @@ static int __init mpc834x_itx_probe(void)return of_flat_dt_is_compatible (root,"MPC834xMITX" ); } +static int __init mpc834x_itx_pata_init (void ) +{ +struct device_node *np; +unsigned int i; + +if (!machine_is (mpc834x_itx)) +return 0 ; + +for (np =NULL , i =0 ; + (np =of_find_compatible_node (np,NULL ,"pata-platform" )); + i++) { +int ret; +struct resource res[3 ]; +const u32 *ioport_shift; +struct platform_device *pdev; +struct pata_platform_info pdata = {}; + +memset (res,0 ,sizeof (res)); + + ret =of_address_to_resource (np,0 , &res[0 ]); +if (ret) { +printk (KERN_ERR "pata.%d: unable to get IO address " +"from the device tree\n" , i); + goto err0; + } + + ret =of_address_to_resource (np,1 , &res[1 ]); +if (ret) { +printk (KERN_ERR "pata.%d: unable to get CTL address " +"from the device tree\n" , i); + goto err0; + } + + ret =of_irq_to_resource (np,0 , &res[2 ]); +if (ret ==NO_IRQ ) { +printk (KERN_ERR "pata.%d: no IRQ\n" , i); + goto err0; + } + + ioport_shift =of_get_property (np,"ioport-shift" ,NULL ); +if (ioport_shift) + pdata.ioport_shift = *ioport_shift; + + pdev =platform_device_alloc ("pata_platform" , i); +if (!pdev) + goto err0; + + ret =platform_device_add_data (pdev, &pdata,sizeof (pdata)); +if (ret) + goto err1; + + ret =platform_device_add_resources (pdev, res,ARRAY_SIZE (res)); +if (ret) + goto err1; + + ret =platform_device_register (pdev); +if (ret) + goto err1; + +continue ; +err1: +printk (KERN_ERR "pata.%d: registration failed\n" , i); +platform_device_del (pdev);/* will free everything */ +err0: +/* Even if some device failed, try others */ +continue ; + } + +return 0 ; +} +device_initcall (mpc834x_itx_pata_init); +define_machine (mpc834x_itx) { .name = "MPC834x ITX", .probe = mpc834x_itx_probe,
PATCH 1/3 pata_platform: make probe and remove functions device type neutral
Split pata_platform_{probe,remove} into two pieces:
1. pata_platform_{probe,remove} -- platform_device-dependant bits
2. __ptata_platform_{probe,remove} -- device type neutral bits.
This is done to not duplicate code for the OF-platform driver.
--- drivers/ata/pata_platform.c | 139 +++++++++++++++++++++++++------------------ drivers/ata/pata_platform.h | 12 ++++ 2 files changed, 94 insertions(+), 57 deletions(-) create mode 100644 drivers/ata/pata_platform.h
drivers/ata/pata_platform.c +82 -57
@@ -93,14 +93,9 @@ static struct ata_port_operations pata_platform_port_ops = { };static void pata_platform_setup_port (struct ata_ioports *ioaddr, -struct pata_platform_info *info) +unsigned int shift) { -unsigned int shift =0 ; -/* Fixup the port shift for platforms that need it */ -if (info && info->ioport_shift) - shift = info->ioport_shift; - ioaddr->data_addr = ioaddr->cmd_addr + (ATA_REG_DATA << shift); ioaddr->error_addr = ioaddr->cmd_addr + (ATA_REG_ERR << shift); ioaddr->feature_addr = ioaddr->cmd_addr + (ATA_REG_FEATURE << shift);
@@ -114,8 +109,12 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr, }/** -* pata_platform_probe - attach a platform interface -* @pdev: platform device +* __pata_platform_probe - attach a platform interface +* @dev: device +* @io_res: Resource representing I/O base +* @ctl_res: Resource representing CTL base +* @irq_res: Resource representing IRQ and its flags +* @ioport_shift: I/O port shift * * Register a platform bus IDE interface. Such interfaces are PIO and we * assume do not support IRQ sharing.
@@ -135,42 +134,17 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,* * If no IRQ resource is present, PIO polling mode is used instead. */ -static int __devinit pata_platform_probe (struct platform_device *pdev) +int __devinit __pata_platform_probe (struct device *dev, +struct resource *io_res, +struct resource *ctl_res, +struct resource *irq_res, +unsigned int ioport_shift) { -struct resource *io_res, *ctl_res;struct ata_host *host;struct ata_port *ap; -struct pata_platform_info *pp_info;unsigned int mmio; -int irq; - -/* -* Simple resource validation .. -*/ -if ((pdev->num_resources !=3 ) && (pdev->num_resources !=2 )) { -dev_err (&pdev->dev,"invalid number of resources\n" ); -return -EINVAL ; - } - -/* -* Get the I/O base first -*/ - io_res =platform_get_resource (pdev,IORESOURCE_IO ,0 ); -if (io_res ==NULL ) { - io_res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); -if (unlikely (io_res ==NULL )) -return -EINVAL ; - } - -/* -* Then the CTL base -*/ - ctl_res =platform_get_resource (pdev,IORESOURCE_IO ,1 ); -if (ctl_res ==NULL ) { - ctl_res =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); -if (unlikely (ctl_res ==NULL )) -return -EINVAL ; - } +int irq =0 ; +int irq_flags =0 ;/* * Check for MMIO
@@ -181,14 +155,15 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)/* * And the IRQ */ - irq =platform_get_irq (pdev,0 ); -if (irq <0 ) - irq =0 ;/* no irq */ +if (irq_res && irq_res->start >0 ) { + irq = irq_res->start; + irq_flags = irq_res->flags; + }/* * Now that that's out of the way, wire up the port.. */ - host =ata_host_alloc (&pdev->dev,1 ); + host =ata_host_alloc (dev,1 );if (!host)return -ENOMEM ; ap = host->ports[0 ];
@@ -209,25 +184,24 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)* Handle the MMIO case */ if (mmio) { - ap->ioaddr.cmd_addr =devm_ioremap (&pdev->dev, io_res->start, + ap->ioaddr.cmd_addr =devm_ioremap (dev, io_res->start, io_res->end - io_res->start +1 ); - ap->ioaddr.ctl_addr =devm_ioremap (&pdev->dev, ctl_res->start, + ap->ioaddr.ctl_addr =devm_ioremap (dev, ctl_res->start, ctl_res->end - ctl_res->start +1 ); }else { - ap->ioaddr.cmd_addr =devm_ioport_map (&pdev->dev, io_res->start, + ap->ioaddr.cmd_addr =devm_ioport_map (dev, io_res->start, io_res->end - io_res->start +1 ); - ap->ioaddr.ctl_addr =devm_ioport_map (&pdev->dev, ctl_res->start, + ap->ioaddr.ctl_addr =devm_ioport_map (dev, ctl_res->start, ctl_res->end - ctl_res->start +1 ); }if (!ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr) { -dev_err (&pdev->dev,"failed to map IO/CTL base\n" ); +dev_err (dev,"failed to map IO/CTL base\n" );return -ENOMEM ; } ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr; - pp_info = pdev->dev.platform_data; -pata_platform_setup_port (&ap->ioaddr, pp_info); +pata_platform_setup_port (&ap->ioaddr, ioport_shift); ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport", (unsigned long long)io_res->start,
@@ -235,26 +209,77 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)/* activate */ return ata_host_activate (host, irq, irq ? ata_interrupt :NULL , - pp_info ? pp_info->irq_flags :0 , - &pata_platform_sht); + irq_flags, &pata_platform_sht); } +EXPORT_SYMBOL_GPL (__pata_platform_probe);/** -* pata_platform_remove - unplug a platform interface -* @pdev: platform device +* __pata_platform_remove - unplug a platform interface +* @dev: device * * A platform bus ATA device has been unplugged. Perform the needed * cleanup. Also called on module unload for any active devices. */ -static int __devexit pata_platform_remove(struct platform_device *pdev) +int __devexit __pata_platform_remove(struct device *dev) { -struct device *dev = &pdev->dev;struct ata_host *host =dev_get_drvdata (dev);ata_host_detach (host);return 0 ; } +EXPORT_SYMBOL_GPL (__pata_platform_remove); + +static int __devinit pata_platform_probe (struct platform_device *pdev) +{ +struct resource *io_res; +struct resource *ctl_res; +struct resource *irq_res; +struct pata_platform_info *pp_info = pdev->dev.platform_data; + +/* +* Simple resource validation .. +*/ +if ((pdev->num_resources !=3 ) && (pdev->num_resources !=2 )) { +dev_err (&pdev->dev,"invalid number of resources\n" ); +return -EINVAL ; + } + +/* +* Get the I/O base first +*/ + io_res =platform_get_resource (pdev,IORESOURCE_IO ,0 ); +if (io_res ==NULL ) { + io_res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (unlikely (io_res ==NULL )) +return -EINVAL ; + } + +/* +* Then the CTL base +*/ + ctl_res =platform_get_resource (pdev,IORESOURCE_IO ,1 ); +if (ctl_res ==NULL ) { + ctl_res =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); +if (unlikely (ctl_res ==NULL )) +return -EINVAL ; + } + +/* +* And the IRQ +*/ + irq_res =platform_get_resource (pdev,IORESOURCE_IRQ ,0 ); +if (irq_res) + irq_res->flags = pp_info ? pp_info->irq_flags :0 ; + +return __pata_platform_probe (&pdev->dev, io_res, ctl_res, irq_res, + pp_info ? pp_info->ioport_shift :0 ); +} + +static int __devexit pata_platform_remove(struct platform_device *pdev) +{ +return __pata_platform_remove (&pdev->dev); +}static struct platform_driver pata_platform_driver = { .probe = pata_platform_probe,
drivers/ata/pata_platform.h new file +12 -0
@@ -0,0 +1,12 @@ +#ifndef __DRIVERS_ATA_PATA_PLATFORM_H +#define __DRIVERS_ATA_PATA_PLATFORM_H + +extern int __devinit __pata_platform_probe (struct device *dev, +struct resource *io_res, +struct resource *ctl_res, +struct resource *irq_res, +unsigned int ioport_shift); + +extern int __devexit __pata_platform_remove(struct device *dev); + +#endif /* __DRIVERS_ATA_PATA_PLATFORM_H */
Signature · 1 line
1.5.2.2
PATCH 2/3 pata_of_platform: OF-Platform PATA device driver
In reply to Anton Vorontsov--- drivers/ata/Kconfig | 10 +++++ drivers/ata/Makefile | 1 + drivers/ata/pata_of_platform.c | 88 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 0 deletions(-) create mode 100644 drivers/ata/pata_of_platform.c
drivers/ata/Kconfig +10 -0
@@ -614,6 +614,16 @@ config PATA_PLATFORM If unsure, say N. +config PATA_OF_PLATFORM + tristate "OpenFirmware platform device PATA support" + depends on PATA_PLATFORM && PPC_OF + help + This option enables support for generic directly connected ATA + devices commonly found on embedded systems with OpenFirmware + bindings. + + If unsure, say N. + config PATA_ICSIDE tristate "Acorn ICS PATA support" depends on ARM && ARCH_ACORN
drivers/ata/Makefile +1 -0
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_IXP4XX_CF) += pata_ixp4xx_cf.o obj-$(CONFIG_PATA_SCC) += pata_scc.o obj-$(CONFIG_PATA_BF54X) += pata_bf54x.o obj-$(CONFIG_PATA_PLATFORM) += pata_platform.o +obj-$(CONFIG_PATA_OF_PLATFORM) += pata_of_platform.o obj-$(CONFIG_PATA_ICSIDE) += pata_icside.o # Should be last but two libata driver obj-$(CONFIG_PATA_ACPI) += pata_acpi.o
drivers/ata/pata_of_platform.c new file +88 -0
88 lines not on this page · raw message
Signature · 1 line
1.5.2.2
PATCH 3/3 MPC8349E-mITX: introduce pata node, make use pata_of_platform driver
In reply to Anton Vorontsov--- arch/powerpc/boot/dts/mpc8349emitx.dts | 9 +++++++-- arch/powerpc/platforms/83xx/mpc834x_itx.c | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)
arch/powerpc/boot/dts/mpc8349emitx.dts +7 -2
@@ -249,6 +249,11 @@ device_type = "pci"; }; - - + pata@f0000000 { + compatible = "fsl,mpc8349emitx", "pata-platform"; + reg = <f0000000 10 f000020c 4>; + ioport-shift = <1>; + interrupts = <17 8>; + interrupt-parent = <&ipic>; + }; };
arch/powerpc/platforms/83xx/mpc834x_itx.c +17 -0
@@ -23,6 +23,7 @@#include <linux/delay.h> #include <linux/seq_file.h> #include <linux/root_dev.h> +#include <linux/of_platform.h> #include <asm/system.h> #include <asm/atomic.h>
@@ -37,6 +38,22 @@#include "mpc83xx.h" +static struct of_device_id mpc834x_ids[] = { + { .compatible ="pata-platform" , }, + {}, +}; + +static int __init mpc834x_declare_of_platform_devices (void ) +{ +if (!machine_is (mpc834x_itx)) +return 0 ; + +of_platform_bus_probe (NULL , mpc834x_ids,NULL ); + +return 0 ; +} +device_initcall (mpc834x_declare_of_platform_devices); +/* ************************************************************************ * * Setup the architecture
Signature · 1 line
1.5.2.2
Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce pata node, make use pata_of_platform driver
Quoted 6 lines · Anton Vorontsov
arch/powerpc/platforms/83xx/mpc834x_itx.c +5
-0+static struct of_device_id mpc834x_ids[] = { + { .compatible = "pata-platform", }, + {}, +}; +
arch/powerpc/platforms/83xx/mpc834x_itx.c line 55
Quoted 10 lines
arch/powerpc/platforms/83xx/mpc834x_itx.c +10
-0+static int __init mpc834x_declare_of_platform_devices (void ) +{ + if (!machine_is(mpc834x_itx)) + return 0; + + of_platform_bus_probe(NULL, mpc834x_ids, NULL); + + return 0; +} +device_initcall (mpc834x_declare_of_platform_devices);
This is not really how of_platform_bus_probe was meant to be used. Instead of listing the device you want to probe, you should list all buses that potentially contain a device that you are probing.
Normally, an ata controller is not a top-level device but instead the child of an SOC bus device, and then you just probe all SOC devices, which means their children get added to the device tree.
In your case, that would probably mean that you have to another entry in the "ranges" of the soc8349 node, or add a second socXXXX node that has the 0xf0000000 ranges, depending on the actual layout of the SOC.
Arnd <><
Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce pata node, make use pata_of_platform driver
Quoted 13 lines · Arnd Bergmann
Quoted 12 lines · Anton Vorontsov
arch/powerpc/platforms/83xx/mpc834x_itx.c +11
-0+static struct of_device_id mpc834x_ids[] = { + { .compatible = "pata-platform", }, + {}, +}; + +static int __init mpc834x_declare_of_platform_devices (void ) +{ + if (!machine_is(mpc834x_itx)) + return 0; + + of_platform_bus_probe(NULL, mpc834x_ids, NULL);
Quoted 8 lines
arch/powerpc/platforms/83xx/mpc834x_itx.c line 55
Quoted 4 lines
arch/powerpc/platforms/83xx/mpc834x_itx.c +4
-0+ + return 0; +} +device_initcall (mpc834x_declare_of_platform_devices);This is not really how
of_platform_bus_probewas meant to be used. Instead of listing the device you want to probe, you should list all buses that potentially contain a device that you are probing.
Yup, I sort of knew it. For mpc8349emitx, pata node should be in the localbus node. But there is no localbus node yet... I'll fix that issue in the next round of these patches.
Much thanks,
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2
Quoted 20 lines · Anton Vorontsov
Hi all,
Here is the PATA Platform driver using OF infrastructure.
Mostly it's just a wrapper around a bit modified
pata_platformdriver.Patches are well split for the easier review:
First one factors out
platform_devicespecific bits and modifiespata_platformto be a library-alike driver (withplatform_devicedefault binding).Second patch is OF-driver itself which is using
pata_platform"library".Third patch is PowerPC specific, but I'm still Cc'ing linux-ide, just to show how we're using it.
Quoted 7 lines
As an alternative approach we can use plain
pata_platformdriver, but I'm not sure how Linux OF bindings' ideologists will or will not like it.So, these patches are strongly Request For Comments. Feel free to train your nitpicking skills ;-), and/or vote for the option you most pleased about (or suggest another?).
Seems reasonable from a libata point of view...
(pata_platform maintainer CC'd)
Jeff
Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Anton Vorontsov
Here is the PATA Platform driver using OF infrastructure.
Mostly it's just a wrapper around a bit modified
pata_platformdriver.
Quoted 6 lines
Quoted 6 lines
Patches are well split for the easier review:
First one factors out
platform_devicespecific bits and modifiespata_platformto be a library-alike driver (withplatform_devicedefault binding).
The only issue I have here is that this library-like version has subtle semantic changes that will break existing drivers.
irq_flags exists in struct pata_platform_info precisely for the IRQ resource IRQ flags (as opposed to the IORESOURCE flags, which are what the IRQ resource flags refer to instead). This change takes that for granted and just assumes we're going to be using the res->flags, which is both an invalid assumption, and will utterly break blackfin and others that depend on it.
Incidentally, we already have an include/linux/pata_platform.h. If this is going to be library-like, through the prototypes in there, rather than splitting them up betewen include/linux and drivers/ata. We don't need two headers.
These patches basically look fine to me otherwise, though it would be nice if the semantic-changing bits had been abstracted. So as long as the old irq_flags behaviour is maintained and that irq_res->flags stuff is ripped out, I'll add my Acked-by as well.
Quoted 7 lines · Paul Mundt
Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Anton Vorontsov
Here is the PATA Platform driver using OF infrastructure.
Mostly it's just a wrapper around a bit modified
pata_platformdriver.
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Patches are well split for the easier review:
First one factors out
platform_devicespecific bits and modifiespata_platformto be a library-alike driver (withplatform_devicedefault binding).The only issue I have here is that this library-like version has subtle semantic changes that will break existing drivers.
Actually I've tried to keep semantics intact:
+static int __devinit pata_platform_probe(struct platform_device *pdev) [...]
+ /*
+ * And the IRQ
+ */
+ irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (irq_res)
+ irq_res->flags = pp_info ? pp_info->irq_flags : 0;
[...]
So, I'm passing flags from the platform data. Did you overlook these bits, or I'm still changing semantics somewhere else?
Quoted 11 lines
irq_flagsexists in structpata_platform_infoprecisely for the IRQ resource IRQ flags (as opposed to the IORESOURCE flags, which are what the IRQ resource flags refer to instead). This change takes that for granted and just assumes we're going to be using theres->flags, which is both an invalid assumption, and will utterly break blackfin and others that depend on it.Incidentally, we already have an
include/linux/pata_platform.h. If this is going to be library-like, through the prototypes in there, rather than splitting them up beteweninclude/linuxanddrivers/ata. We don't need two headers.
My intention was: keep "private" declarations in the drivers/ata/ and "public" in the include/linux/ -- to not confuse pata_platform users by __pata_platform_* internal stuff. But okay, I'll merge them.
Quoted 4 lines
These patches basically look fine to me otherwise, though it would be nice if the semantic-changing bits had been abstracted. So as long as the old
irq_flagsbehaviour is maintained and thatirq_res->flagsstuff is ripped out, I'll add my Acked-by as well.
Much thanks,
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2
Quoted 29 lines · Anton Vorontsov
Quoted 7 lines · Paul Mundt
Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Anton Vorontsov
Here is the PATA Platform driver using OF infrastructure.
Mostly it's just a wrapper around a bit modified
pata_platformdriver.Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Patches are well split for the easier review:
First one factors out
platform_devicespecific bits and modifiespata_platformto be a library-alike driver (withplatform_devicedefault binding).The only issue I have here is that this library-like version has subtle semantic changes that will break existing drivers.
Actually I've tried to keep semantics intact:
(quoted diff) +7
-0+static int __devinit pata_platform_probe(struct platform_device *pdev) + /* + * And the IRQ + */ + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + if (irq_res) + irq_res->flags = pp_info ? pp_info->irq_flags : 0;
Quoted 3 lines
So, I'm passing flags from the platform data. Did you overlook these bits, or I'm still changing semantics somewhere else?
Oh, I overlooked that. Using irq_res->flags as a temporary for pp_info->irq_flags seems a bit hacky, but as long as pp_info->irq_flags semantics are intact, I'm not too violently opposed to this anyways.
Quoted 9 lines
Quoted 4 lines
Incidentally, we already have an
include/linux/pata_platform.h. If this is going to be library-like, through the prototypes in there, rather than splitting them up beteweninclude/linuxanddrivers/ata. We don't need two headers.My intention was: keep "private" declarations in the
drivers/ata/and "public" in theinclude/linux/-- to not confusepata_platformusers by __pata_platform_* internal stuff. But okay, I'll merge them.
I suppose that depends on whether the intent is that all pata_platform users will be stuck in drivers/ata or not. If this is treated as more of a library, implementations can simply bury themselves in arch/ land if they feel like it.
Quoted 9 lines · Anton Vorontsov
As an alternative approach we can use plain
pata_platformdriver, but I'm not sure how Linux OF bindings' ideologists will or will not like it.So, these patches are strongly Request For Comments. Feel free to train your nitpicking skills ;-), and/or vote for the option you most pleased about (or suggest another?).
Quoted 8 lines
Thanks.
---
Down here is "alternative approach".Probably board-neutral version may be placed somewhere in the
drivers/of/...? But who will call it: board file, ordevice_initcallfor all boards?
.../...
I'm fine with either approaches in fact. I think it's fair enough when there is existing platform devices to avoid duplicating the whole thing for an of_platform_device (unless you find a nice way to split to a library like you just did) and instead, have a "constructor" that constructs the platform device based on the OF node.
Now, as you pointed out, the question is where to call it... the initcall has the problem of potentially being called for the wrong board and the thing having duplicate OF device and platform device created for it...
One of the thing I've been brewing in my mind is the idea of reworking the current code that create OF platform devices when walking busses down, and have it instead call registered constructor functions. The default ones would then create OF platform devices but one could override that with constructors creating something else (such as a platform device).
Now, it's not trivial, as there is a bit of a mixup on whether a "bus" should create all children and thus define their type (PCI would do such a thing) or whether the type is defined by the device object itself which typically would be the case for "generic" busses such as SoCs or PLBs when creating platform devices. I haven't quite found out a nice way to enable both possibilities... if you have ideas, please share.
In the meantime, maybe it's best to merge the of_platform_device approach since Jeff agrees with it.
Cheers,
Ben.
Quoted 20 lines · On Fri, 23 Nov 2007 20:52:29 +0300 Anton Vorontsov wrote:
Hi all,
Here is the PATA Platform driver using OF infrastructure.
Mostly it's just a wrapper around a bit modified
pata_platformdriver.Patches are well split for the easier review:
First one factors out
platform_devicespecific bits and modifiespata_platformto be a library-alike driver (withplatform_devicedefault binding).Second patch is OF-driver itself which is using
pata_platform"library".Third patch is PowerPC specific, but I'm still Cc'ing linux-ide, just to show how we're using it.
Quoted 8 lines
As an alternative approach we can use plain
pata_platformdriver, but I'm not sure how Linux OF bindings' ideologists will or will not like it.So, these patches are strongly Request For Comments. Feel free to train your nitpicking skills ;-), and/or vote for the option you most pleased about (or suggest another?).
I vote for of_platform_device, since we seem to dodge the clash with platform_device here.
The code looks good, so I'm going to ack all 3 patches and if there will be no more nagging, I'm inclined to ask Kumar to queue this for the next merge window.
Signature · 1 line
Sincerely, Vitaly
Quoted 4 lines · Anton Vorontsov
Here is the PATA Platform driver using OF infrastructure.
Mostly it's just a wrapper around a bit modified
pata_platformdriver.
Thanks a lot for doing this. Patches 2/3 are what I tried to get people to do for some time now but was too lazy to do myself.
As a further thought, do the drivers now still need to be pata specific, or should the OF part be called ata_of_platform instead and also be used for sata devices?
Arnd <><
Quoted 6 lines · Arnd Bergmann
Quoted 4 lines · Anton Vorontsov
Here is the PATA Platform driver using OF infrastructure.
Mostly it's just a wrapper around a bit modified
pata_platformdriver.
Quoted 6 lines
Thanks a lot for doing this. Patches 2/3 are what I tried to get people to do for some time now but was too lazy to do myself.
As a further thought, do the drivers now still need to be pata specific, or should the OF part be called
ata_of_platforminstead and also be used for sata devices?
Ugh, I don't know much about sata, it should act just as a pata in the very basic usage, IIRC. So it's worth trying, but I don't have any platform satas to try... :-/
Hereby, I'd rather stick with pata name, as it's never too late to simply rename things later.
Thanks,
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2