lkml.org 
[lkml]   [2010]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: MFD cell structure and sharing of resources
From
Hi Samuel,

Thanks for your valuable input here!

On 16 December 2010 10:35, Samuel Ortiz <sameo@linux.intel.com> wrote:
>> 2. Continue with Andres' olpc-xo1-pm patch that makes that driver act
>> as a driver for both acpi & pms. When both resources are available, it
>> could probe an olpc-xo1-sci platform device as a child of itself,
>> having the ACPI resource shared on the platform_device level (similar
>> to how MFD shares resources via cells).
> This sounds like an acceptable solution to me.

This is also the one that stuck out to me.

There are complications though.


Firstly, passing down the resources from mfd to olpc-xo1-pm, then from
olpc-xo1-pm using "struct resource" doesn't work - the 2nd passing
down fails because the platform layer checks that the resources are
free.

This can be worked around by using platform_data to pass the required
info from olpc-xo1-pm to olpc-xo1-sci (the base register addresses).


Secondly, the olpc-xo1-pm driver is going to have a couple of sysfs
nodes that will be used by userspace.

Under the current design they appear as e.g.:
/sys/devices/pci0000:00/0000:00:0f.0/cs5535-acpi/wakeup_reason

However, it only appears under cs5535-acpi because that's the device
that appeared second (out of acpi + pms). If the probe order ever
changed, the path would change too.
This could be worked around by storing both pointers (acpi + pms) and
choosing one that the olpc-xo1-pm parts will always live under. But as
this detail represents an interface to userspace we should be careful
and try and get it right first time. That wakeup_reason node is not
really related to cs5535, it's an OLPC-specific thing (since the
wakeups can be caused by things totally separate from the geode hw).
So I'd feel a lot more comfortable if that path was less related to
cs5535.

This problem extends to olpc-xo1-sci which becomes a child of
olpc-xo1-pm, and creates another bunch of sysfs nodes e.g.
/sys/devices/pci0000:00/0000:00:0f.0/cs5535-acpi/olpc-xo1-sci/lid_wake_mode



I have one solution in mind but I'm not sure if it goes beyond your
definition of what a mfd cell should be:

cs5535-mfd creates and registers a MFD cell specifically for
olpc-xo1-pm if it finds itself running on an XO laptop. The cell has
the 2 resources that are needed by that driver, and has name
"olpc-xo1-pm". Therefore our sysfs paths start with:
/sys/devices/pci0000:00/0000:00:0f.0/olpc-xo1-pm/

olpc-xo1-pm is then simplified and doesn't have to pretend to be a
driver for 2 devices. It just waits for that mfd cell to cause it to
be probed, then immediately has both resources available to it.

The first problem is also solved (clean sharing of resources between
olpc-xo1-pm and olpc-xo1-sci). olpc-xo1-pm creates olpc-xo1-sci as a
child device, therefore olpc-xo1-sci can access the resources of its
parent as follows:
platform_get_resource(to_platform_device(pdev->dev.parent),
IORESOURCE_IO, 0);

Attaching a cs5535-mfd patch to illustrate what I mean a bit clearer
(compile tested only). The OLPC-specific code in cs5536-mfd compiles
away to nothing on CONFIG_OLPC=n

Thoughts?

Daniel
diff --git a/arch/x86/platform/olpc/olpc-xo1.c b/arch/x86/platform/olpc/olpc-xo1.c
index 1277756..4723139 100644
--- a/arch/x86/platform/olpc/olpc-xo1.c
+++ b/arch/x86/platform/olpc/olpc-xo1.c
@@ -19,7 +19,7 @@
#include <asm/io.h>
#include <asm/olpc.h>

-#define DRV_NAME "olpc-xo1"
+#define DRV_NAME "olpc-xo1-pm"

/* PMC registers (PMS block) */
#define PM_SCLK 0x10
@@ -72,17 +72,23 @@ static int __devinit olpc_xo1_probe(struct platform_device *pdev)
return -EIO;
}

- if (strcmp(pdev->name, "cs5535-pms") == 0)
- pms_base = res->start;
- else if (strcmp(pdev->name, "cs5535-acpi") == 0)
- acpi_base = res->start;
+ pms_base = res->start;

- /* If we have both addresses, we can override the poweroff hook */
- if (pms_base && acpi_base) {
- pm_power_off = xo1_power_off;
- printk(KERN_INFO "OLPC XO-1 support registered\n");
+ res = platform_get_resource(pdev, IORESOURCE_IO, 1);
+ if (!res) {
+ dev_err(&pdev->dev, "can't fetch device resource info\n");
+ return -EIO;
}

+ if (!request_region(res->start, resource_size(res), DRV_NAME)) {
+ dev_err(&pdev->dev, "can't request region\n");
+ return -EIO;
+ }
+
+ acpi_base = res->start;
+
+ pm_power_off = xo1_power_off;
+ printk(KERN_INFO "OLPC XO-1 support registered\n");
return 0;
}

@@ -93,27 +99,19 @@ static int __devexit olpc_xo1_remove(struct platform_device *pdev)
r = platform_get_resource(pdev, IORESOURCE_IO, 0);
release_region(r->start, resource_size(r));

- if (strcmp(pdev->name, "cs5535-pms") == 0)
- pms_base = 0;
- else if (strcmp(pdev->name, "cs5535-acpi") == 0)
- acpi_base = 0;
+ r = platform_get_resource(pdev, IORESOURCE_IO, 1);
+ release_region(r->start, resource_size(r));
+
+ pms_base = 0;
+ acpi_base = 0;

pm_power_off = NULL;
return 0;
}

-static struct platform_driver cs5535_pms_drv = {
+static struct platform_driver xo1_pm_drv = {
.driver = {
- .name = "cs5535-pms",
- .owner = THIS_MODULE,
- },
- .probe = olpc_xo1_probe,
- .remove = __devexit_p(olpc_xo1_remove),
-};
-
-static struct platform_driver cs5535_acpi_drv = {
- .driver = {
- .name = "cs5535-acpi",
+ .name = DRV_NAME,
.owner = THIS_MODULE,
},
.probe = olpc_xo1_probe,
@@ -122,28 +120,17 @@ static struct platform_driver cs5535_acpi_drv = {

static int __init olpc_xo1_init(void)
{
- int r;
-
- r = platform_driver_register(&cs5535_pms_drv);
- if (r)
- return r;
-
- r = platform_driver_register(&cs5535_acpi_drv);
- if (r)
- platform_driver_unregister(&cs5535_pms_drv);
-
- return r;
+ return platform_driver_register(&xo1_pm_drv);
}

static void __exit olpc_xo1_exit(void)
{
- platform_driver_unregister(&cs5535_acpi_drv);
- platform_driver_unregister(&cs5535_pms_drv);
+ platform_driver_unregister(&xo1_pm_drv);
}

MODULE_AUTHOR("Daniel Drake <dsd@laptop.org>");
MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:olpc-xo1");
+MODULE_ALIAS("platform:" DRV_NAME);

module_init(olpc_xo1_init);
module_exit(olpc_xo1_exit);
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3a7b891..203245d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -540,7 +540,7 @@ config AB3550_CORE
config MFD_CS5535
tristate "Support for CS5535 and CS5536 southbridge core functions"
select MFD_CORE
- depends on PCI
+ depends on PCI && X86
---help---
This is the core driver for CS5535/CS5536 MFD functions. This is
necessary for using the board's GPIO and MFGPT functionality.
diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 59ca6f1..9ef5dd9 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -28,6 +28,8 @@
#include <linux/module.h>
#include <linux/pci.h>

+#include <asm/olpc.h>
+
#define DRV_NAME "cs5535-mfd"

enum cs5535_mfd_bars {
@@ -74,6 +76,15 @@ static __devinitdata struct mfd_cell cs5535_mfd_cells[] = {
},
};

+static __devinitdata struct resource cs5535_mfd_resources_xo1[2];
+
+static __devinitdata struct mfd_cell cs5535_mfd_cell_xo1 = {
+ .id = -1,
+ .name = "olpc-xo1-pm",
+ .num_resources = 2,
+ .resources = cs5535_mfd_resources_xo1,
+};
+
static int __devinit cs5535_mfd_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
@@ -106,8 +117,22 @@ static int __devinit cs5535_mfd_probe(struct pci_dev *pdev,
dev_info(&pdev->dev, "%zu devices registered.\n",
ARRAY_SIZE(cs5535_mfd_cells));

+ if (machine_is_olpc()) {
+ cs5535_mfd_resources_xo1[0] = cs5535_mfd_resources[PMS_BAR];
+ cs5535_mfd_resources_xo1[1] = cs5535_mfd_resources[ACPI_BAR];
+ err = mfd_add_devices(&pdev->dev, -1, &cs5535_mfd_cell_xo1, 1,
+ NULL, 0);
+ if (err) {
+ dev_err(&pdev->dev,
+ "MFD add OLPC device fail: %d\n", err);
+ goto err_olpc;
+ }
+ }
+
return 0;

+err_olpc:
+ mfd_remove_devices(&pdev->dev);
err_disable:
pci_disable_device(pdev);
return err;
\
 
 \ /
  Last update: 2010-12-16 16:41    [W:2.263 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site