[PATCH 0/3] Add SDHCI ACPI driver

[PATCH 0/3] Add SDHCI ACPI driver

[Adrian Hunter]

Hi

Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
so I suggest Rafael takes the patches with Chris' Ack.

Please note that I would prefer this to be queued for 3.8


Adrian Hunter (3):
      PNPACPI: exclude SDHCI devices
      ACPI: add SDHCI to ACPI platform devices
      mmc: sdhci-acpi: add SDHCI ACPI driver

 drivers/acpi/scan.c           |   2 +
 drivers/mmc/host/Kconfig      |  12 ++
 drivers/mmc/host/Makefile     |   1 +
 drivers/mmc/host/sdhci-acpi.c | 304 ++++++++++++++++++++++++++++++++++++++++++
 drivers/pnp/pnpacpi/core.c    |   1 +
 5 files changed, 320 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-acpi.c


Regards
Adrian Hunter

[PATCH 1/3] PNPACPI: exclude SDHCI devices

[Adrian Hunter]

These devices will be handled by a ACPI Platform driver.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/pnp/pnpacpi/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 26b5d4b..5b17cc8 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -43,6 +43,7 @@ static struct acpi_device_id excluded_id_list[] __initdata = {
 	{"PNP0C0F", 0},		/* Link device */
 	{"PNP0000", 0},		/* PIC */
 	{"PNP0100", 0},		/* Timer */
+	{"PNP0D40", 0},		/* SDHCI */
 	{"", 0},
 };
 
-- 
1.7.11.7

[PATCH 2/3] ACPI: add SDHCI to ACPI platform devices

[Adrian Hunter]

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/acpi/scan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 8c4ac6d..67a7fa6 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -35,6 +35,8 @@ static const char *dummy_hid = "device";
  */
 static const struct acpi_device_id acpi_platform_device_ids[] = {
 
+	{ "PNP0D40" },
+
 	{ }
 };
 
-- 
1.7.11.7

[PATCH 3/3] mmc: sdhci-acpi: add SDHCI ACPI driver

[Adrian Hunter]

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/Kconfig      |  12 ++
 drivers/mmc/host/Makefile     |   1 +
 drivers/mmc/host/sdhci-acpi.c | 304 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 317 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-acpi.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9bf10e7..56eac10 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -81,6 +81,18 @@ config MMC_RICOH_MMC
 
 	  If unsure, say Y.
 
+config MMC_SDHCI_ACPI
+	tristate "SDHCI support for ACPI enumerated SDHCI controllers"
+	depends on MMC_SDHCI && ACPI
+	help
+	  This selects support for ACPI enumerated SDHCI controllers,
+	  identified by ACPI Compatibility ID PNP0D40 or specific
+	  ACPI Hardware IDs.
+
+	  If you have a controller with this interface, say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_SDHCI_PLTFM
 	tristate "SDHCI platform and OF driver helper"
 	depends on MMC_SDHCI
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 17ad0a7..0e4960a 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_MMC_MXS)		+= mxs-mmc.o
 obj-$(CONFIG_MMC_SDHCI)		+= sdhci.o
 obj-$(CONFIG_MMC_SDHCI_PCI)	+= sdhci-pci.o
 obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI))	+= sdhci-pci-data.o
+obj-$(CONFIG_MMC_SDHCI_ACPI)	+= sdhci-acpi.o
 obj-$(CONFIG_MMC_SDHCI_PXAV3)	+= sdhci-pxav3.o
 obj-$(CONFIG_MMC_SDHCI_PXAV2)	+= sdhci-pxav2.o
 obj-$(CONFIG_MMC_SDHCI_S3C)	+= sdhci-s3c.o
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
new file mode 100644
index 0000000..f582fe7
--- /dev/null
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -0,0 +1,304 @@
+/*
+ * Secure Digital Host Controller Interface ACPI driver.
+ *
+ * Copyright (c) 2012, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/export.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/dma-mapping.h>
+#include <linux/compiler.h>
+#include <linux/stddef.h>
+#include <linux/bitops.h>
+#include <linux/types.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/acpi.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/pm.h>
+#include <linux/mmc/sdhci.h>
+
+#include "sdhci.h"
+
+enum {
+	SDHCI_ACPI_SD_CD	= BIT(0),
+	SDHCI_ACPI_RUNTIME_PM	= BIT(1),
+};
+
+struct sdhci_acpi_chip {
+	const struct	sdhci_ops *ops;
+	unsigned int	quirks;
+	unsigned int	quirks2;
+	unsigned long	caps;
+	unsigned int	caps2;
+	mmc_pm_flag_t	pm_caps;
+};
+
+struct sdhci_acpi_slot {
+	const struct	sdhci_acpi_chip *chip;
+	unsigned int	quirks;
+	unsigned int	quirks2;
+	unsigned long	caps;
+	unsigned int	caps2;
+	mmc_pm_flag_t	pm_caps;
+	unsigned int	flags;
+};
+
+struct sdhci_acpi_host {
+	struct sdhci_host		*host;
+	const struct sdhci_acpi_slot	*slot;
+	struct platform_device		*pdev;
+	bool				use_runtime_pm;
+};
+
+static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
+{
+	return c->slot && (c->slot->flags & flag);
+}
+
+static int sdhci_acpi_enable_dma(struct sdhci_host *host)
+{
+	return 0;
+}
+
+static const struct sdhci_ops sdhci_acpi_ops_dflt = {
+	.enable_dma = sdhci_acpi_enable_dma,
+};
+
+static const struct acpi_device_id sdhci_acpi_ids[] = {
+	{ "PNP0D40" },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
+
+static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid)
+{
+	const struct acpi_device_id *id;
+
+	for (id = sdhci_acpi_ids; id->id[0]; id++)
+		if (!strcmp(id->id, hid))
+			return (const struct sdhci_acpi_slot *)id->driver_data;
+	return NULL;
+}
+
+static int __devinit sdhci_acpi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	acpi_handle handle = dev->acpi_handle;
+	struct acpi_device *device;
+	struct sdhci_acpi_host *c;
+	struct sdhci_host *host;
+	struct resource *iomem;
+	resource_size_t len;
+	const char *hid;
+	int err;
+
+	if (acpi_bus_get_device(handle, &device))
+		return -ENODEV;
+
+	if (acpi_bus_get_status(device) || !device->status.present)
+		return -ENODEV;
+
+	hid = acpi_device_hid(device);
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!iomem)
+		return -ENOMEM;
+
+	len = resource_size(iomem);
+	if (len < 0x100)
+		dev_err(dev, "Invalid iomem size!\n");
+
+	if (!devm_request_mem_region(dev, iomem->start, len, dev_name(dev)))
+		return -ENOMEM;
+
+	host = sdhci_alloc_host(dev, sizeof(struct sdhci_acpi_host));
+	if (IS_ERR(host))
+		return PTR_ERR(host);
+
+	c = sdhci_priv(host);
+	c->host = host;
+	c->slot = sdhci_acpi_get_slot(hid);
+	c->pdev = pdev;
+	c->use_runtime_pm = sdhci_acpi_flag(c, SDHCI_ACPI_RUNTIME_PM);
+
+	platform_set_drvdata(pdev, c);
+
+	host->hw_name	= "ACPI";
+	host->ops	= &sdhci_acpi_ops_dflt;
+	host->irq	= platform_get_irq(pdev, 0);
+
+	host->ioaddr = devm_ioremap_nocache(dev, iomem->start,
+					    resource_size(iomem));
+	if (host->ioaddr == NULL) {
+		err = -ENOMEM;
+		goto err_free;
+	}
+
+	if (!dev->dma_mask) {
+		u64 dma_mask;
+
+		if (sdhci_readl(host, SDHCI_CAPABILITIES) & SDHCI_CAN_64BIT) {
+			/* 64-bit DMA is not supported at present */
+			dma_mask = DMA_BIT_MASK(32);
+		} else {
+			dma_mask = DMA_BIT_MASK(32);
+		}
+
+		dev->dma_mask = &dev->coherent_dma_mask;
+		dev->coherent_dma_mask = dma_mask;
+	}
+
+	if (c->slot) {
+		if (c->slot->chip) {
+			host->ops            = c->slot->chip->ops;
+			host->quirks        |= c->slot->chip->quirks;
+			host->quirks2       |= c->slot->chip->quirks2;
+			host->mmc->caps     |= c->slot->chip->caps;
+			host->mmc->caps2    |= c->slot->chip->caps2;
+			host->mmc->pm_caps  |= c->slot->chip->pm_caps;
+		}
+		host->quirks        |= c->slot->quirks;
+		host->quirks2       |= c->slot->quirks2;
+		host->mmc->caps     |= c->slot->caps;
+		host->mmc->caps2    |= c->slot->caps2;
+		host->mmc->pm_caps  |= c->slot->pm_caps;
+	}
+
+	err = sdhci_add_host(host);
+	if (err)
+		goto err_free;
+
+	if (c->use_runtime_pm) {
+		pm_suspend_ignore_children(dev, 1);
+		pm_runtime_set_autosuspend_delay(dev, 50);
+		pm_runtime_use_autosuspend(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return 0;
+
+err_free:
+	platform_set_drvdata(pdev, NULL);
+	sdhci_free_host(c->host);
+	return err;
+}
+
+static int __devexit sdhci_acpi_remove(struct platform_device *pdev)
+{
+	struct sdhci_acpi_host *c = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int dead;
+
+	if (c->use_runtime_pm) {
+		pm_runtime_get_sync(dev);
+		pm_runtime_disable(dev);
+		pm_runtime_put_noidle(dev);
+	}
+
+	dead = (sdhci_readl(c->host, SDHCI_INT_STATUS) == ~0);
+	sdhci_remove_host(c->host, dead);
+	platform_set_drvdata(pdev, NULL);
+	sdhci_free_host(c->host);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+
+static int sdhci_acpi_suspend(struct device *dev)
+{
+	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
+
+	return sdhci_suspend_host(c->host);
+}
+
+static int sdhci_acpi_resume(struct device *dev)
+{
+	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
+
+	return sdhci_resume_host(c->host);
+}
+
+#else
+
+#define sdhci_acpi_suspend	NULL
+#define sdhci_acpi_resume	NULL
+
+#endif
+
+#ifdef CONFIG_PM_RUNTIME
+
+static int sdhci_acpi_runtime_suspend(struct device *dev)
+{
+	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
+
+	return sdhci_runtime_suspend_host(c->host);
+}
+
+static int sdhci_acpi_runtime_resume(struct device *dev)
+{
+	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
+
+	return sdhci_runtime_resume_host(c->host);
+}
+
+static int sdhci_acpi_runtime_idle(struct device *dev)
+{
+	return 0;
+}
+
+#else
+
+#define sdhci_acpi_runtime_suspend	NULL
+#define sdhci_acpi_runtime_resume	NULL
+#define sdhci_acpi_runtime_idle		NULL
+
+#endif
+
+static const struct dev_pm_ops sdhci_acpi_pm_ops = {
+	.suspend		= sdhci_acpi_suspend,
+	.resume			= sdhci_acpi_resume,
+	.runtime_suspend	= sdhci_acpi_runtime_suspend,
+	.runtime_resume		= sdhci_acpi_runtime_resume,
+	.runtime_idle		= sdhci_acpi_runtime_idle,
+};
+
+static struct platform_driver sdhci_acpi_driver = {
+	.driver = {
+		.name			= "sdhci-acpi",
+		.owner			= THIS_MODULE,
+		.acpi_match_table	= sdhci_acpi_ids,
+		.pm			= &sdhci_acpi_pm_ops,
+	},
+	.probe	= sdhci_acpi_probe,
+	.remove	= __devexit_p(sdhci_acpi_remove),
+};
+
+module_platform_driver(sdhci_acpi_driver);
+
+MODULE_DESCRIPTION("Secure Digital Host Controller Interface ACPI driver");
+MODULE_AUTHOR("Adrian Hunter");
+MODULE_LICENSE("GPL v2");
-- 
1.7.11.7

Re: [PATCH 0/3] Add SDHCI ACPI driver

[Chris Ball]

Hi,

On Thu, Nov 22 2012, Adrian Hunter wrote:
> Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
> so I suggest Rafael takes the patches with Chris' Ack.
>
> Please note that I would prefer this to be queued for 3.8

Looks fine:

Acked-by: Chris Ball <cjb@laptop.org>

I have some dumb questions, though -- what kind of platforms ship with
these devices?  Do they ever have the controller on PCI too, and what
happens with sdhci-pci vs. sdhci-acpi in that case?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/3] Add SDHCI ACPI driver

[Adrian Hunter]

On 22/11/12 15:55, Chris Ball wrote:
> Hi,
> 
> On Thu, Nov 22 2012, Adrian Hunter wrote:
>> Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
>> so I suggest Rafael takes the patches with Chris' Ack.
>>
>> Please note that I would prefer this to be queued for 3.8
> 
> Looks fine:
> 
> Acked-by: Chris Ball <cjb@laptop.org>

Thank you!

> 
> I have some dumb questions, though -- what kind of platforms ship with
> these devices?  Do they ever have the controller on PCI too, and what
> happens with sdhci-pci vs. sdhci-acpi in that case?

Since the arrival of ACPI5, platform devices can be configured using ACPI
tables.  PCI can also be used, but the firmware ensures that the same
device is not enumerated via both ACPI and PCI.

Rafael can you take these patches?

Re: [PATCH 0/3] Add SDHCI ACPI driver

[Rafael J. Wysocki]

On Thursday, November 22, 2012 04:46:10 PM Adrian Hunter wrote:
> On 22/11/12 15:55, Chris Ball wrote:
> > Hi,
> > 
> > On Thu, Nov 22 2012, Adrian Hunter wrote:
> >> Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
> >> so I suggest Rafael takes the patches with Chris' Ack.
> >>
> >> Please note that I would prefer this to be queued for 3.8
> > 
> > Looks fine:
> > 
> > Acked-by: Chris Ball <cjb@laptop.org>
> 
> Thank you!
> 
> > 
> > I have some dumb questions, though -- what kind of platforms ship with
> > these devices?  Do they ever have the controller on PCI too, and what
> > happens with sdhci-pci vs. sdhci-acpi in that case?
> 
> Since the arrival of ACPI5, platform devices can be configured using ACPI
> tables.  PCI can also be used, but the firmware ensures that the same
> device is not enumerated via both ACPI and PCI.
> 
> Rafael can you take these patches?

Well, I'd prefer pnpacpi/core.c to actually use acpi_platform_device_ids[]
directly in addition to excluded_id_list[], so that duplicate entries don't
have to be added to the both of them.

Also, I wonder if you really don't want to use ACPI PM and if you don't,
then why?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/3] Add SDHCI ACPI driver

[Mika Westerberg]

On Thu, Nov 22, 2012 at 10:24:33PM +0100, Rafael J. Wysocki wrote:
> On Thursday, November 22, 2012 04:46:10 PM Adrian Hunter wrote:
> > On 22/11/12 15:55, Chris Ball wrote:
> > > Hi,
> > > 
> > > On Thu, Nov 22 2012, Adrian Hunter wrote:
> > >> Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
> > >> so I suggest Rafael takes the patches with Chris' Ack.
> > >>
> > >> Please note that I would prefer this to be queued for 3.8
> > > 
> > > Looks fine:
> > > 
> > > Acked-by: Chris Ball <cjb@laptop.org>
> > 
> > Thank you!
> > 
> > > 
> > > I have some dumb questions, though -- what kind of platforms ship with
> > > these devices?  Do they ever have the controller on PCI too, and what
> > > happens with sdhci-pci vs. sdhci-acpi in that case?
> > 
> > Since the arrival of ACPI5, platform devices can be configured using ACPI
> > tables.  PCI can also be used, but the firmware ensures that the same
> > device is not enumerated via both ACPI and PCI.
> > 
> > Rafael can you take these patches?
> 
> Well, I'd prefer pnpacpi/core.c to actually use acpi_platform_device_ids[]
> directly in addition to excluded_id_list[], so that duplicate entries don't
> have to be added to the both of them.

How about having pnpacpi to check if the ACPI device is already bound to a
physical device and skip the device creation? Then we don't need to expose
the acpi_platform_device_ids[] list, and this is what the ->find_device()
code already does so why create the device in the first place?

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 5b17cc8..4dc2e64 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -243,6 +243,10 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	char *pnpid;
 	struct acpi_hardware_id *id;
 
+	/* Skip devices that are already bound */
+	if (device->physical_node_count)
+		return 0;
+
 	/*
 	 * If a PnPacpi device is not present , the device
 	 * driver should not be loaded.

Re: [PATCH 3/3] mmc: sdhci-acpi: add SDHCI ACPI driver

[Mika Westerberg]

On Thu, Nov 22, 2012 at 10:43:50AM +0200, Adrian Hunter wrote:
> +static int __devinit sdhci_acpi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	acpi_handle handle = dev->acpi_handle;

This is not going to work anymore, you'll have to use ACPI_HANDLE(dev) for
this (there was a new macro introduced with the struct acpi_dev_node).

> +	struct acpi_device *device;
> +	struct sdhci_acpi_host *c;
> +	struct sdhci_host *host;
> +	struct resource *iomem;
> +	resource_size_t len;
> +	const char *hid;
> +	int err;
> +
> +	if (acpi_bus_get_device(handle, &device))
> +		return -ENODEV;
> +
> +	if (acpi_bus_get_status(device) || !device->status.present)
> +		return -ENODEV;

This is a bit redundant as the platform code already checks whether the
device is present or not and only creates the platform device in case it is
present.

Re: [PATCH 0/3] Add SDHCI ACPI driver

[Adrian Hunter]

On 23/11/12 11:34, Mika Westerberg wrote:
> On Thu, Nov 22, 2012 at 10:24:33PM +0100, Rafael J. Wysocki wrote:
>> On Thursday, November 22, 2012 04:46:10 PM Adrian Hunter wrote:
>>> On 22/11/12 15:55, Chris Ball wrote:
>>>> Hi,
>>>>
>>>> On Thu, Nov 22 2012, Adrian Hunter wrote:
>>>>> Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
>>>>> so I suggest Rafael takes the patches with Chris' Ack.
>>>>>
>>>>> Please note that I would prefer this to be queued for 3.8
>>>>
>>>> Looks fine:
>>>>
>>>> Acked-by: Chris Ball <cjb@laptop.org>
>>>
>>> Thank you!
>>>
>>>>
>>>> I have some dumb questions, though -- what kind of platforms ship with
>>>> these devices?  Do they ever have the controller on PCI too, and what
>>>> happens with sdhci-pci vs. sdhci-acpi in that case?
>>>
>>> Since the arrival of ACPI5, platform devices can be configured using ACPI
>>> tables.  PCI can also be used, but the firmware ensures that the same
>>> device is not enumerated via both ACPI and PCI.
>>>
>>> Rafael can you take these patches?
>>
>> Well, I'd prefer pnpacpi/core.c to actually use acpi_platform_device_ids[]
>> directly in addition to excluded_id_list[], so that duplicate entries don't
>> have to be added to the both of them.
> 
> How about having pnpacpi to check if the ACPI device is already bound to a
> physical device and skip the device creation? Then we don't need to expose
> the acpi_platform_device_ids[] list, and this is what the ->find_device()
> code already does so why create the device in the first place?

Yes, I was going to suggest that too.  AFAICS pnpacpi has no concept of
multiple physical nodes.  Any objections?


> 
> diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
> index 5b17cc8..4dc2e64 100644
> --- a/drivers/pnp/pnpacpi/core.c
> +++ b/drivers/pnp/pnpacpi/core.c
> @@ -243,6 +243,10 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
>  	char *pnpid;
>  	struct acpi_hardware_id *id;
>  
> +	/* Skip devices that are already bound */
> +	if (device->physical_node_count)
> +		return 0;
> +
>  	/*
>  	 * If a PnPacpi device is not present , the device
>  	 * driver should not be loaded.
> 
> 

Re: [PATCH 0/3] Add SDHCI ACPI driver

[Adrian Hunter]

On 22/11/12 23:24, Rafael J. Wysocki wrote:
> On Thursday, November 22, 2012 04:46:10 PM Adrian Hunter wrote:
>> On 22/11/12 15:55, Chris Ball wrote:
>>> Hi,
>>>
>>> On Thu, Nov 22 2012, Adrian Hunter wrote:
>>>> Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
>>>> so I suggest Rafael takes the patches with Chris' Ack.
>>>>
>>>> Please note that I would prefer this to be queued for 3.8
>>>
>>> Looks fine:
>>>
>>> Acked-by: Chris Ball <cjb@laptop.org>
>>
>> Thank you!
>>
>>>
>>> I have some dumb questions, though -- what kind of platforms ship with
>>> these devices?  Do they ever have the controller on PCI too, and what
>>> happens with sdhci-pci vs. sdhci-acpi in that case?
>>
>> Since the arrival of ACPI5, platform devices can be configured using ACPI
>> tables.  PCI can also be used, but the firmware ensures that the same
>> device is not enumerated via both ACPI and PCI.
>>
>> Rafael can you take these patches?
> 
> Well, I'd prefer pnpacpi/core.c to actually use acpi_platform_device_ids[]
> directly in addition to excluded_id_list[], so that duplicate entries don't
> have to be added to the both of them.
> 
> Also, I wonder if you really don't want to use ACPI PM and if you don't,
> then why?

Mika and Lv Zheng are working on adding it to acpi_platform

Re: [PATCH 0/3] Add SDHCI ACPI driver

[Rafael J. Wysocki]

On Friday, November 23, 2012 12:13:13 PM Adrian Hunter wrote:
> On 23/11/12 11:34, Mika Westerberg wrote:
> > On Thu, Nov 22, 2012 at 10:24:33PM +0100, Rafael J. Wysocki wrote:
> >> On Thursday, November 22, 2012 04:46:10 PM Adrian Hunter wrote:
> >>> On 22/11/12 15:55, Chris Ball wrote:
> >>>> Hi,
> >>>>
> >>>> On Thu, Nov 22 2012, Adrian Hunter wrote:
> >>>>> Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
> >>>>> so I suggest Rafael takes the patches with Chris' Ack.
> >>>>>
> >>>>> Please note that I would prefer this to be queued for 3.8
> >>>>
> >>>> Looks fine:
> >>>>
> >>>> Acked-by: Chris Ball <cjb@laptop.org>
> >>>
> >>> Thank you!
> >>>
> >>>>
> >>>> I have some dumb questions, though -- what kind of platforms ship with
> >>>> these devices?  Do they ever have the controller on PCI too, and what
> >>>> happens with sdhci-pci vs. sdhci-acpi in that case?
> >>>
> >>> Since the arrival of ACPI5, platform devices can be configured using ACPI
> >>> tables.  PCI can also be used, but the firmware ensures that the same
> >>> device is not enumerated via both ACPI and PCI.
> >>>
> >>> Rafael can you take these patches?
> >>
> >> Well, I'd prefer pnpacpi/core.c to actually use acpi_platform_device_ids[]
> >> directly in addition to excluded_id_list[], so that duplicate entries don't
> >> have to be added to the both of them.
> > 
> > How about having pnpacpi to check if the ACPI device is already bound to a
> > physical device and skip the device creation? Then we don't need to expose
> > the acpi_platform_device_ids[] list, and this is what the ->find_device()
> > code already does so why create the device in the first place?
> 
> Yes, I was going to suggest that too.  AFAICS pnpacpi has no concept of
> multiple physical nodes.  Any objections?

Not from me.

Thanks,
Rafael


> > diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
> > index 5b17cc8..4dc2e64 100644
> > --- a/drivers/pnp/pnpacpi/core.c
> > +++ b/drivers/pnp/pnpacpi/core.c
> > @@ -243,6 +243,10 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
> >  	char *pnpid;
> >  	struct acpi_hardware_id *id;
> >  
> > +	/* Skip devices that are already bound */
> > +	if (device->physical_node_count)
> > +		return 0;
> > +
> >  	/*
> >  	 * If a PnPacpi device is not present , the device
> >  	 * driver should not be loaded.
> > 
> > 
> 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/3] Add SDHCI ACPI driver

[Rafael J. Wysocki]

On Friday, November 23, 2012 12:23:11 PM Adrian Hunter wrote:
> On 22/11/12 23:24, Rafael J. Wysocki wrote:
> > On Thursday, November 22, 2012 04:46:10 PM Adrian Hunter wrote:
> >> On 22/11/12 15:55, Chris Ball wrote:
> >>> Hi,
> >>>
> >>> On Thu, Nov 22 2012, Adrian Hunter wrote:
> >>>> Here is SDHCI ACPI driver.  It is dependent on new ACPI Platform support
> >>>> so I suggest Rafael takes the patches with Chris' Ack.
> >>>>
> >>>> Please note that I would prefer this to be queued for 3.8
> >>>
> >>> Looks fine:
> >>>
> >>> Acked-by: Chris Ball <cjb@laptop.org>
> >>
> >> Thank you!
> >>
> >>>
> >>> I have some dumb questions, though -- what kind of platforms ship with
> >>> these devices?  Do they ever have the controller on PCI too, and what
> >>> happens with sdhci-pci vs. sdhci-acpi in that case?
> >>
> >> Since the arrival of ACPI5, platform devices can be configured using ACPI
> >> tables.  PCI can also be used, but the firmware ensures that the same
> >> device is not enumerated via both ACPI and PCI.
> >>
> >> Rafael can you take these patches?
> > 
> > Well, I'd prefer pnpacpi/core.c to actually use acpi_platform_device_ids[]
> > directly in addition to excluded_id_list[], so that duplicate entries don't
> > have to be added to the both of them.
> > 
> > Also, I wonder if you really don't want to use ACPI PM and if you don't,
> > then why?
> 
> Mika and Lv Zheng are working on adding it to acpi_platform

OK

Please address the Mika's comment for [3/3], though.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.