Re: [PATCH v4 0/3] ACPI 5 support for GPIO, SPI and I2C

[Mika Westerberg]

On Thu, Nov 22, 2012 at 11:03:26AM +0100, Rafael J. Wysocki wrote:
> On Thursday, November 22, 2012 10:43:28 AM Linus Walleij wrote:
> > On Wed, Nov 21, 2012 at 10:31 PM, Rafael J. Wysocki  wrote:
> > 
> > > This patchset has been around for quite a while and went through a few
> > > iterations, so I think it's as good as it gets at this point.
> > >
> > > I wonder if the GPIO / SPI / I2C maintainers have any objections against 
> > > it or
> > > would like the patches to be modified somehow?
> > >
> > > If not, then I'd like to take it for v3.8 into the linux-pm.git tree, 
> > > because
> > > the patches depend on some changes already in that tree.  Hopefully, 
> > > that's OK.
> > 
> > For GPIO I want Grant to ACK this.
> > 
> > He has experience with both Device Tree and GPIO, knows what ACPI and
> > UEFI is and is way better suited than me to give feedback on this kind of
> > stuff.
> > 
> > So from my stance it's "neutral, whatever Grant says goes".
> 
> OK, we're waiting for a word from Grant, then. :-)

Grant, do you have any input on this? Do you want us to change something in
order to get these merged?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/7] i2c-designware-pci: use managed functions pcim_* and devm_*

[Mika Westerberg]

From: Andy Shevchenko 

This makes the error handling much more simpler than open-coding everything
and in addition makes the probe function smaller an tidier.

Signed-off-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
---
 drivers/i2c/busses/i2c-designware-pcidrv.c |   68 +++-
 1 file changed, 17 insertions(+), 51 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c 
b/drivers/i2c/busses/i2c-designware-pcidrv.c
index eed149d..aacb64e 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -212,8 +212,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 {
struct dw_i2c_dev *dev;
struct i2c_adapter *adap;
-   unsigned long start, len;
-   void __iomem *base;
int r;
struct  dw_pci_controller *controller;
 
@@ -225,51 +223,30 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
controller = &dw_pci_controllers[id->driver_data];
 
-   r = pci_enable_device(pdev);
+   r = pcim_enable_device(pdev);
if (r) {
dev_err(&pdev->dev, "Failed to enable I2C PCI device (%d)\n",
r);
-   goto exit;
+   return r;
}
 
-   /* Determine the address of the I2C area */
-   start = pci_resource_start(pdev, 0);
-   len = pci_resource_len(pdev, 0);
-   if (!start || len == 0) {
-   dev_err(&pdev->dev, "base address not set\n");
-   r = -ENODEV;
-   goto exit;
-   }
-
-   r = pci_request_region(pdev, 0, DRIVER_NAME);
+   r = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
if (r) {
-   dev_err(&pdev->dev, "failed to request I2C region "
-   "0x%lx-0x%lx\n", start,
-   (unsigned long)pci_resource_end(pdev, 0));
-   goto exit;
-   }
-
-   base = ioremap_nocache(start, len);
-   if (!base) {
dev_err(&pdev->dev, "I/O memory remapping failed\n");
-   r = -ENOMEM;
-   goto err_release_region;
+   return r;
}
 
-
-   dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
-   if (!dev) {
-   r = -ENOMEM;
-   goto err_release_region;
-   }
+   dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
+   if (!dev)
+   return -ENOMEM;
 
init_completion(&dev->cmd_complete);
mutex_init(&dev->lock);
dev->clk = NULL;
dev->controller = controller;
dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
-   dev->base = base;
-   dev->dev = get_device(&pdev->dev);
+   dev->base = pcim_iomap_table(pdev)[0];
+   dev->dev = &pdev->dev;
dev->functionality =
I2C_FUNC_I2C |
I2C_FUNC_SMBUS_BYTE |
@@ -284,7 +261,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
dev->rx_fifo_depth = controller->rx_fifo_depth;
r = i2c_dw_init(dev);
if (r)
-   goto err_iounmap;
+   return r;
 
adap = &dev->adapter;
i2c_set_adapdata(adap, dev);
@@ -296,10 +273,11 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
snprintf(adap->name, sizeof(adap->name), "i2c-designware-pci-%d",
adap->nr);
 
-   r = request_irq(pdev->irq, i2c_dw_isr, IRQF_SHARED, adap->name, dev);
+   r = devm_request_irq(&pdev->dev, pdev->irq, i2c_dw_isr, IRQF_SHARED,
+   adap->name, dev);
if (r) {
dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
-   goto err_iounmap;
+   return r;
}
 
i2c_dw_disable_int(dev);
@@ -307,24 +285,16 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
r = i2c_add_numbered_adapter(adap);
if (r) {
dev_err(&pdev->dev, "failure adding adapter\n");
-   goto err_free_irq;
+   return r;
}
 
+   /* Increase reference counter */
+   get_device(&pdev->dev);
+
pm_runtime_put_noidle(&pdev->dev);
pm_runtime_allow(&pdev->dev);
 
return 0;
-
-err_free_irq:
-   free_irq(pdev->irq, dev);
-err_iounmap:
-   iounmap(dev->base);
-   put_device(&pdev->dev);
-   kfree(dev);
-err_release_region:
-   pci_release_region(pdev, 0);
-exit:
-   return r;
 }
 
 static void i2c_dw_pci_remove(struct pci_dev *pdev)
@@ -337,10 +307,6 @@ static void i2c_dw_pci_remove(struct pci_dev *pdev)
 
i2c_del_adapter(&dev->adapter);
put_device(&pdev->dev);
-
-   free_irq(dev->irq, dev);
-   kfree(dev);
-  

[PATCH 7/7] i2c-designware: switch to use runtime PM autosuspend

[Mika Westerberg]

Using autosuspend helps to reduce the resume latency in situations where
another I2C message is going to be started soon. For example with HID over
I2C touch panels we get several messages in a short period of time while
the touch panel is in use.

Signed-off-by: Mika Westerberg 
---
 drivers/i2c/busses/i2c-designware-core.c|3 ++-
 drivers/i2c/busses/i2c-designware-pcidrv.c  |3 ++-
 drivers/i2c/busses/i2c-designware-platdrv.c |3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 0849648..71d33e6 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -595,7 +595,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
msgs[], int num)
ret = -EIO;
 
 done:
-   pm_runtime_put(dev->dev);
+   pm_runtime_mark_last_busy(dev->dev);
+   pm_runtime_put_autosuspend(dev->dev);
mutex_unlock(&dev->lock);
 
return ret;
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c 
b/drivers/i2c/busses/i2c-designware-pcidrv.c
index aacb64e..c8797e2 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -291,7 +291,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
/* Increase reference counter */
get_device(&pdev->dev);
 
-   pm_runtime_put_noidle(&pdev->dev);
+   pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+   pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_allow(&pdev->dev);
 
return 0;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index a22a852..46a25ca 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -175,9 +175,10 @@ static int dw_i2c_probe(struct platform_device *pdev)
/* Increase reference counter */
get_device(&pdev->dev);
 
+   pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+   pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
-   pm_runtime_put(&pdev->dev);
 
return 0;
 }
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/7] i2c-designware-pci: use dev_err() instead of printk()

[Mika Westerberg]

From: Andy Shevchenko 

With dev_err() we can get the device instance printed as well and is pretty
much standard to use dev_* macros in drivers anyway. In addition correct
indentation of probe() arguments.

Signed-off-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
---
 drivers/i2c/busses/i2c-designware-pcidrv.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c 
b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7c5e383..eed149d 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -208,7 +208,7 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 }
 
 static int i2c_dw_pci_probe(struct pci_dev *pdev,
-const struct pci_device_id *id)
+   const struct pci_device_id *id)
 {
struct dw_i2c_dev *dev;
struct i2c_adapter *adap;
@@ -218,7 +218,7 @@ const struct pci_device_id *id)
struct  dw_pci_controller *controller;
 
if (id->driver_data >= ARRAY_SIZE(dw_pci_controllers)) {
-   printk(KERN_ERR "dw_i2c_pci_probe: invalid driver data %ld\n",
+   dev_err(&pdev->dev, "%s: invalid driver data %ld\n", __func__,
id->driver_data);
return -EINVAL;
}
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/7] i2c-designware: use dynamic adapter numbering on Lynxpoint

[Mika Westerberg]

It is not good idea to mix static and dynamic I2C adapter numbering. In
this particular case on Lynxpoint we had graphics I2C adapter which took
the first numbers preventing the designware I2C driver from using the
adapter numbers it preferred.

Fix this by switching to use dynamic adapter numbering on Intel Lynxpoint.

Signed-off-by: Mika Westerberg 
---
 drivers/i2c/busses/i2c-designware-platdrv.c |9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index c19c4ce..a22a852 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -56,20 +56,11 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 static int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
-   struct acpi_device *adev;
-   int busno, ret;
 
if (!ACPI_HANDLE(&pdev->dev))
return -ENODEV;
 
-   ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
-   if (ret)
-   return -ENODEV;
-
dev->adapter.nr = -1;
-   if (adev->pnp.unique_id && !kstrtoint(adev->pnp.unique_id, 0, &busno))
-   dev->adapter.nr = busno;
-
dev->tx_fifo_depth = 32;
dev->rx_fifo_depth = 32;
return 0;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/7] i2c-designware: enable/disable the controller properly

[Mika Westerberg]

The correct way to disable or enable the controller is to wait until
DW_IC_ENABLE_STATUS register bit matches the bit we program to the
DW_IC_ENABLE register. This procedure is described in the DesignWare I2C
handbook.

By doing this we can be sure that the controller is in correct state once
the function returns.

Signed-off-by: Mika Westerberg 
---
 drivers/i2c/busses/i2c-designware-core.c |   29 +++--
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..0e4f531 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -68,6 +68,7 @@
 #define DW_IC_TXFLR0x74
 #define DW_IC_RXFLR0x78
 #define DW_IC_TX_ABRT_SOURCE   0x80
+#define DW_IC_ENABLE_STATUS0x9c
 #define DW_IC_COMP_PARAM_1 0xf4
 #define DW_IC_COMP_TYPE0xfc
 #define DW_IC_COMP_TYPE_VALUE  0x44570140
@@ -248,6 +249,22 @@ static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, 
int offset)
return ((ic_clk * (tLOW + tf) + 5000) / 1) - 1 + offset;
 }
 
+static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
+{
+   int timeout = 100;
+
+   do {
+   dw_writel(dev, enable, DW_IC_ENABLE);
+   if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
+   return;
+
+   usleep_range(25, 250);
+   } while (timeout-- > 0);
+
+   dev_warn(dev->dev, "timeout in %sabling adapter\n",
+enable ? "en" : "dis");
+}
+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data
@@ -278,7 +295,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
}
 
/* Disable the adapter */
-   dw_writel(dev, 0, DW_IC_ENABLE);
+   __i2c_dw_enable(dev, false);
 
/* set standard and fast speed deviders for high/low periods */
 
@@ -345,7 +362,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
u32 ic_con;
 
/* Disable the adapter */
-   dw_writel(dev, 0, DW_IC_ENABLE);
+   __i2c_dw_enable(dev, false);
 
/* set the slave (target) address */
dw_writel(dev, msgs[dev->msg_write_idx].addr, DW_IC_TAR);
@@ -359,7 +376,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
dw_writel(dev, ic_con, DW_IC_CON);
 
/* Enable the adapter */
-   dw_writel(dev, 1, DW_IC_ENABLE);
+   __i2c_dw_enable(dev, true);
 
/* Enable interrupts */
dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
@@ -565,7 +582,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
msgs[], int num)
/* no error */
if (likely(!dev->cmd_err)) {
/* Disable the adapter */
-   dw_writel(dev, 0, DW_IC_ENABLE);
+   __i2c_dw_enable(dev, false);
ret = num;
goto done;
}
@@ -700,7 +717,7 @@ EXPORT_SYMBOL_GPL(i2c_dw_isr);
 void i2c_dw_enable(struct dw_i2c_dev *dev)
 {
/* Enable the adapter */
-   dw_writel(dev, 1, DW_IC_ENABLE);
+   __i2c_dw_enable(dev, true);
 }
 EXPORT_SYMBOL_GPL(i2c_dw_enable);
 
@@ -713,7 +730,7 @@ EXPORT_SYMBOL_GPL(i2c_dw_is_enabled);
 void i2c_dw_disable(struct dw_i2c_dev *dev)
 {
/* Disable controller */
-   dw_writel(dev, 0, DW_IC_ENABLE);
+   __i2c_dw_enable(dev, false);
 
/* Disable all interupts */
dw_writel(dev, 0, DW_IC_INTR_MASK);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 6/7] i2c-designware: use usleep_range() in the busy-loop

[Mika Westerberg]

This is not an atomic context so there is no need to use mdelay() but
instead use usleep_range().

Signed-off-by: Mika Westerberg 
---
 drivers/i2c/busses/i2c-designware-core.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 0e4f531..0849648 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -350,7 +350,7 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
return -ETIMEDOUT;
}
timeout--;
-   mdelay(1);
+   usleep_range(1000, 1100);
}
 
return 0;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/7] i2c-designware: move to managed functions (devm_*)

[Mika Westerberg]

From: Andy Shevchenko 

This makes the error handling much more simpler than open-coding everything
and in addition makes the probe function smaller and tidier.

Signed-off-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   73 ---
 1 file changed, 20 insertions(+), 53 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0ceb6e1..c19c4ce 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -92,7 +92,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
 {
struct dw_i2c_dev *dev;
struct i2c_adapter *adap;
-   struct resource *mem, *ioarea;
+   struct resource *mem;
int irq, r;
 
/* NOTE: driver uses the static register mapping */
@@ -108,32 +108,27 @@ static int dw_i2c_probe(struct platform_device *pdev)
return irq; /* -ENXIO */
}
 
-   ioarea = request_mem_region(mem->start, resource_size(mem),
-   pdev->name);
-   if (!ioarea) {
-   dev_err(&pdev->dev, "I2C region already claimed\n");
-   return -EBUSY;
-   }
+   dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
+   if (!dev)
+   return -ENOMEM;
 
-   dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
-   if (!dev) {
-   r = -ENOMEM;
-   goto err_release_region;
+   dev->base = devm_ioremap_resource(&pdev->dev, mem);
+   if (IS_ERR(dev->base)) {
+   dev_err(&pdev->dev, "I2C region already claimed\n");
+   return PTR_ERR(dev->base);
}
 
init_completion(&dev->cmd_complete);
mutex_init(&dev->lock);
-   dev->dev = get_device(&pdev->dev);
+   dev->dev = &pdev->dev;
dev->irq = irq;
platform_set_drvdata(pdev, dev);
 
-   dev->clk = clk_get(&pdev->dev, NULL);
+   dev->clk = devm_clk_get(&pdev->dev, NULL);
dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 
-   if (IS_ERR(dev->clk)) {
-   r = -ENODEV;
-   goto err_free_mem;
-   }
+   if (IS_ERR(dev->clk))
+   return PTR_ERR(dev->clk);
clk_prepare_enable(dev->clk);
 
dev->functionality =
@@ -146,13 +141,6 @@ static int dw_i2c_probe(struct platform_device *pdev)
dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
 
-   dev->base = ioremap(mem->start, resource_size(mem));
-   if (dev->base == NULL) {
-   dev_err(&pdev->dev, "failure mapping io resources\n");
-   r = -EBUSY;
-   goto err_unuse_clocks;
-   }
-
/* Try first if we can configure the device from ACPI */
r = dw_i2c_acpi_configure(pdev);
if (r) {
@@ -164,13 +152,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
}
r = i2c_dw_init(dev);
if (r)
-   goto err_iounmap;
+   return r;
 
i2c_dw_disable_int(dev);
-   r = request_irq(dev->irq, i2c_dw_isr, IRQF_SHARED, pdev->name, dev);
+   r = devm_request_irq(&pdev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
+   pdev->name, dev);
if (r) {
dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
-   goto err_iounmap;
+   return r;
}
 
adap = &dev->adapter;
@@ -187,57 +176,35 @@ static int dw_i2c_probe(struct platform_device *pdev)
r = i2c_add_numbered_adapter(adap);
if (r) {
dev_err(&pdev->dev, "failure adding adapter\n");
-   goto err_free_irq;
+   return r;
}
of_i2c_register_devices(adap);
acpi_i2c_register_devices(adap);
 
+   /* Increase reference counter */
+   get_device(&pdev->dev);
+
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
pm_runtime_put(&pdev->dev);
 
return 0;
-
-err_free_irq:
-   free_irq(dev->irq, dev);
-err_iounmap:
-   iounmap(dev->base);
-err_unuse_clocks:
-   clk_disable_unprepare(dev->clk);
-   clk_put(dev->clk);
-   dev->clk = NULL;
-err_free_mem:
-   put_device(&pdev->dev);
-   kfree(dev);
-err_release_region:
-   release_mem_region(mem->start, resource_size(mem));
-
-   return r;
 }
 
 static int dw_i2c_remove(struct platform_device *pdev)
 {
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
-   struct resource *mem;
 
pm_runtime_get_sync(&pdev->dev)

Re: [PATCH v3 00/10] spi/pxa2xx: add Intel Lynxpoint SPI controller support

[Mika Westerberg]

On Mon, Feb 04, 2013 at 05:45:49PM +0800, Mark Brown wrote:
> On Fri, Feb 01, 2013 at 12:22:47PM +0200, Mika Westerberg wrote:
> 
> > Mark, thank you for applying patches 1-4/10. Is there anything you want me
> > to do for the rest of the patches in order to get those merged?
> 
> I stopped at patch 4 mostly because it's very large but also because you
> said you were hoping for an ack from Linus.

Ah, I see.

Linus, do you think these are OK?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 05/10] spi/pxa2xx: break out the private DMA API usage into a separate file

[Mika Westerberg]

On Mon, Feb 04, 2013 at 08:43:42PM +0100, Linus Walleij wrote:
> On Tue, Jan 22, 2013 at 11:26 AM, Mika Westerberg
>  wrote:
> 
> > The PXA SPI driver uses PXA platform specific private DMA implementation
> > which does not work on non-PXA platforms. In order to use this driver on
> > other platforms we break out the private DMA implementation into a separate
> > file that gets compiled only when CONFIG_SPI_PXA2XX_PXADMA is set. The DMA
> > functions are stubbed out if there is no DMA implementation selected (i.e
> > we are building on non-PXA platform).
> >
> > While we are there we can kill the dummy DMA bits in pxa2xx_spi.h as they
> > are not needed anymore for CE4100.
> >
> > Once this is done we can add the generic DMA engine support to the driver
> > that allows usage of any DMA controller that implements DMA engine API.
> >
> > Signed-off-by: Mika Westerberg 
> > ---
> > This patch was already acked by Linus W but since I changed this by
> > breaking out the PXA private implementation into a separate file, I'm
> > hoping to get new ack from him ;-)
> 
> Sorry for the long delay. This is a nice and clean cut.
> Acked-by: Linus Walleij 

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 00/10] spi/pxa2xx: add Intel Lynxpoint SPI controller support

[Mika Westerberg]

Hi Mark/Grant,

On Mon, Feb 04, 2013 at 05:45:49PM +0800, Mark Brown wrote:
> On Fri, Feb 01, 2013 at 12:22:47PM +0200, Mika Westerberg wrote:
> 
> > Mark, thank you for applying patches 1-4/10. Is there anything you want me
> > to do for the rest of the patches in order to get those merged?
> 
> I stopped at patch 4 mostly because it's very large but also because you
> said you were hoping for an ack from Linus.

It is large because I had to break out the private PXA DMA implementation
from the driver and I'm not sure if it can be split into a smaller patches.
The series has been tested on PXA by Lu Cao at Marvell and Linus W acked
the DMA patches.

Do you think you could take this for 3.9 or is there anything (besides the
one patch being large) preventhing this?

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Documentation / acpi: refer to correct file for acpi_platform_device_ids[] table

[Mika Westerberg]

When the ACPI platform device code was converted to the new ACPI scan
handler facility, the the acpi_platform_device_ids[] was moved to
drivers/acpi/acpi_platform.c. Update the documentation accordingly.

Signed-off-by: Mika Westerberg 
---
 Documentation/acpi/enumeration.txt |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/acpi/enumeration.txt 
b/Documentation/acpi/enumeration.txt
index 54469bc..94a6561 100644
--- a/Documentation/acpi/enumeration.txt
+++ b/Documentation/acpi/enumeration.txt
@@ -63,8 +63,8 @@ from ACPI tables.
 Currently the kernel is not able to automatically determine from which ACPI
 device it should make the corresponding platform device so we need to add
 the ACPI device explicitly to acpi_platform_device_ids list defined in
-drivers/acpi/scan.c. This limitation is only for the platform devices, SPI
-and I2C devices are created automatically as described below.
+drivers/acpi/acpi_platform.c. This limitation is only for the platform
+devices, SPI and I2C devices are created automatically as described below.
 
 SPI serial bus support
 ~~
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 00/10] spi/pxa2xx: add Intel Lynxpoint SPI controller support

[Mika Westerberg]

On Fri, Feb 08, 2013 at 10:37:15PM +0100, Linus Walleij wrote:
> On Mon, Feb 4, 2013 at 10:52 AM, Mika Westerberg
>  wrote:
> > On Mon, Feb 04, 2013 at 05:45:49PM +0800, Mark Brown wrote:
> >> On Fri, Feb 01, 2013 at 12:22:47PM +0200, Mika Westerberg wrote:
> >>
> >> > Mark, thank you for applying patches 1-4/10. Is there anything you want 
> >> > me
> >> > to do for the rest of the patches in order to get those merged?
> >>
> >> I stopped at patch 4 mostly because it's very large but also because you
> >> said you were hoping for an ack from Linus.
> >
> > Ah, I see.
> >
> > Linus, do you think these are OK?
> 
> Yes AFAICT, I went in and ACKed them some day ago.

Yeah, I noticed that Mark already applied these. Thanks for you both!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Documentation / acpi: refer to correct file for acpi_platform_device_ids[] table

[Mika Westerberg]

On Sat, Feb 09, 2013 at 12:14:27AM +0100, Rafael J. Wysocki wrote:
> On Friday, February 08, 2013 04:07:01 PM Mika Westerberg wrote:
> > When the ACPI platform device code was converted to the new ACPI scan
> > handler facility, the the acpi_platform_device_ids[] was moved to
> > drivers/acpi/acpi_platform.c. Update the documentation accordingly.
> > 
> > Signed-off-by: Mika Westerberg 
> 
> Applied.

Thanks!

> When you post any patches touching ACPI, please send CCs of them to
> linux-acpi as well.

Sure.

In this case I just CC'd what get_maintainer.pl gave me and somehow it
missed linux-acpi totally.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/7] hid driver transport cleanup

[Mika Westerberg]

On Fri, Feb 08, 2013 at 03:37:29PM +0100, Benjamin Tissoires wrote:
> so, here is the hid drivers cleanup. The aim is to remove as much as possible
> direct calls to usbhid for hid drivers. Thus, other transport layers can use
> the existing hid drivers (like I2C or uhid).

We also found out that there is a dependency to usbhid in many of the
drivers, especially in sensor-hub which caused the system crash while the
driver tried to transmit over USB even though it was behind I2C.

Your series fixes that nicely :)

You can add,

Reviewed-by: Mika Westerberg 

for the whole series and in addition,

Tested-by: Mika Westerberg 

for the sensor-hub part (HID over I2C). I have few more patches to the
sensor-hub myself and I'm going to rebase them on top of this series.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/3] HID: sensor-hub: don't limit the driver only to USB bus

[Mika Westerberg]

We now have two transport mediums: USB and I2C, where sensor hubs can
exists. So instead of constraining the driver to only these two we let it
to match any HID bus as long as the group is HID_GROUP_SENSOR_HUB.

Signed-off-by: Mika Westerberg 
---
 drivers/hid/hid-sensor-hub.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 2643bce9..c01f10d 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -603,7 +603,8 @@ static void sensor_hub_remove(struct hid_device *hdev)
 }
 
 static const struct hid_device_id sensor_hub_devices[] = {
-   { HID_DEVICE(BUS_USB, HID_GROUP_SENSOR_HUB, HID_ANY_ID, HID_ANY_ID) },
+   { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, HID_ANY_ID,
+HID_ANY_ID) },
{ }
 };
 MODULE_DEVICE_TABLE(hid, sensor_hub_devices);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/3] HID: extend autodetect to handle I2C sensors as well

[Mika Westerberg]

Since the advent of HID over I2C protocol, it is possible to have sensor
hubs behind I2C bus as well. We can autodetect this in a same way than USB
sensor hubs.

Signed-off-by: Mika Westerberg 
---
 drivers/hid/hid-core.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index eb2ee11..fe3a59e 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -729,7 +729,7 @@ static int hid_scan_report(struct hid_device *hid)
item.type == HID_ITEM_TYPE_MAIN &&
item.tag == HID_MAIN_ITEM_TAG_BEGIN_COLLECTION &&
(item_udata(&item) & 0xff) == HID_COLLECTION_PHYSICAL &&
-   hid->bus == BUS_USB)
+   (hid->bus == BUS_USB || hid->bus == BUS_I2C))
hid->group = HID_GROUP_SENSOR_HUB;
}
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/3] HID: sensor-hub: get rid of unused sensor_hub_grabbed_usages[] table

[Mika Westerberg]

This table is not used anywhere in the driver so kill it.

Signed-off-by: Mika Westerberg 
---
 drivers/hid/hid-sensor-hub.c |5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index c06e933..2643bce9 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -608,11 +608,6 @@ static const struct hid_device_id sensor_hub_devices[] = {
 };
 MODULE_DEVICE_TABLE(hid, sensor_hub_devices);
 
-static const struct hid_usage_id sensor_hub_grabbed_usages[] = {
-   { HID_ANY_ID, HID_ANY_ID, HID_ANY_ID },
-   { HID_ANY_ID - 1, HID_ANY_ID - 1, HID_ANY_ID - 1 }
-};
-
 static struct hid_driver sensor_hub_driver = {
.name = "hid-sensor-hub",
.id_table = sensor_hub_devices,
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/7] hid driver transport cleanup

[Mika Westerberg]

On Mon, Feb 11, 2013 at 12:19:13PM +0100, Benjamin Tissoires wrote:
> On Mon, Feb 11, 2013 at 11:13 AM, Mika Westerberg
>  wrote:
> > On Fri, Feb 08, 2013 at 03:37:29PM +0100, Benjamin Tissoires wrote:
> >> so, here is the hid drivers cleanup. The aim is to remove as much as 
> >> possible
> >> direct calls to usbhid for hid drivers. Thus, other transport layers can 
> >> use
> >> the existing hid drivers (like I2C or uhid).
> >
> > We also found out that there is a dependency to usbhid in many of the
> > drivers, especially in sensor-hub which caused the system crash while the
> > driver tried to transmit over USB even though it was behind I2C.
> >
> > Your series fixes that nicely :)
> >
> > You can add,
> >
> > Reviewed-by: Mika Westerberg 
> >
> > for the whole series and in addition,
> >
> > Tested-by: Mika Westerberg 
> >
> > for the sensor-hub part (HID over I2C). I have few more patches to the
> > sensor-hub myself and I'm going to rebase them on top of this series.
> 
> Thanks Mika for reviewing.
> 
> I just wanted to warn you that the HID over I2C part is still lacking
> the support of requests through hid_hw_request(). So the functions
> sensor_hub_set_feature(), sensor_hub_get_feature() and
> sensor_hub_input_attr_get_raw_value() won't work, in a silently way
> :(.

OK.

> I started writing the patch but stopped at some point due to lack of
> ways to test it.

Well, I have the HW here so if you have something please send to me. I can
test it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] HID: sensor-hub: don't limit the driver only to USB bus

[Mika Westerberg]

On Mon, Feb 11, 2013 at 03:54:05PM +, Pandruvada, Srinivas wrote:
> So finally we can use HID sensor hub over I2C.
> Did you try with any I2C sensor hub?

Yes, we have I2C HID sensor hub here and I tested with it. However, I'm not
that familiar how it should be used except that I can see the sensors under
/sys/bus/platform/devices/HID-SENSOR* and there are iio devices under each
platform device.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] dw_dmac: adjust slave_id accordingly to request line base

[Mika Westerberg]

On Tue, Feb 12, 2013 at 07:53:34AM -0800, Vinod Koul wrote:
> On Tue, Jan 29, 2013 at 10:29:43AM +0530, Viresh Kumar wrote:
> > Next time, please direct these mails to my Linaro id :)
> > 
> > On Mon, Jan 28, 2013 at 4:34 PM, Andy Shevchenko
> >  wrote:
> > > On some hardware configurations we have got the request line with the 
> > > offset.
> > > The patch introduces convert_slave_id() helper for that cases. The 
> > > request line
> > > base is got from the platform device resources provided by the 
> > > IORESOURCE_DMA
> > > type.
> > 
> > @Vinod: Is IORESOURCE_DMA suitable for this purpose?
> Looks unlikely...
> 
> But why do we need this in first place?
> 
> I know this is due to common platform probe. Clearly one size not fitting all.
> If you had dedicated PCI probe you wouldnt worrry right?

This is all related to new ACPI 5.0 features for devices enumerated from
ACPI namespace, such as SPI controller.

ACPI 5.0 allows BIOS to specify DMA request lines and channels used by a
slave device with FixedDMA() descriptors, such as:

FixedDMA (0x0010, 0x, Width32bit,) // TX
FixedDMA (0x0011, 0x0001, Width32bit,) // RX

The first field (0x10, 0x11) is the request line assigned to the device. In
this particular case it is the SPI controller.

Now there is only one DMA controller in this platform but the request lines
still start from 0x10 (16).

Then there is another ACPI table called CSRT (Core System Resources Table)
that assings request line range to the DMA controller itself. This range
starts from 0x10 (16) for this platform.

So what we do is that we parse this CSRT table, create platform device for
the DMA controller of found and pass the request line range as
IORESOURCE_DMA resource with the platform device.

In this patch then Andy uses that resource to map the FixedDMA() request
line to the actual hardware request line, which is for the SPI case:

0x10 -> 0
0x11 -> 1

Unfortunately we didn't find any better way to pass this information than
IORESOURCE_DMA. However, if you have better solution we certainly can
change that :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] dw_dmac: adjust slave_id accordingly to request line base

[Mika Westerberg]

On Tue, Feb 12, 2013 at 09:34:35AM -0800, Vinod Koul wrote:
> On Tue, Feb 12, 2013 at 06:43:51PM +0200, Mika Westerberg wrote:
> > On Tue, Feb 12, 2013 at 07:53:34AM -0800, Vinod Koul wrote:
> > > On Tue, Jan 29, 2013 at 10:29:43AM +0530, Viresh Kumar wrote:
> > > > Next time, please direct these mails to my Linaro id :)
> > > > 
> > > > On Mon, Jan 28, 2013 at 4:34 PM, Andy Shevchenko
> > > >  wrote:
> > > > > On some hardware configurations we have got the request line with the 
> > > > > offset.
> > > > > The patch introduces convert_slave_id() helper for that cases. The 
> > > > > request line
> > > > > base is got from the platform device resources provided by the 
> > > > > IORESOURCE_DMA
> > > > > type.
> > > > 
> > > > @Vinod: Is IORESOURCE_DMA suitable for this purpose?
> > > Looks unlikely...
> > > 
> > > But why do we need this in first place?
> > > 
> > > I know this is due to common platform probe. Clearly one size not fitting 
> > > all.
> > > If you had dedicated PCI probe you wouldnt worrry right?
> > 
> > This is all related to new ACPI 5.0 features for devices enumerated from
> > ACPI namespace, such as SPI controller.
> > 
> > ACPI 5.0 allows BIOS to specify DMA request lines and channels used by a
> > slave device with FixedDMA() descriptors, such as:
> > 
> > FixedDMA (0x0010, 0x, Width32bit,) // TX
> > FixedDMA (0x0011, 0x0001, Width32bit,) // RX
> > 
> > The first field (0x10, 0x11) is the request line assigned to the device. In
> > this particular case it is the SPI controller.
> > 
> > Now there is only one DMA controller in this platform but the request lines
> > still start from 0x10 (16).
> > 
> > Then there is another ACPI table called CSRT (Core System Resources Table)
> > that assings request line range to the DMA controller itself. This range
> > starts from 0x10 (16) for this platform.
> > 
> > So what we do is that we parse this CSRT table, create platform device for
> > the DMA controller of found and pass the request line range as
> > IORESOURCE_DMA resource with the platform device.
> Okay, who parses the CSRT table and how is this presented to OS.

ACPI code, found under drivers/acpi/csrt.c handles this. Note that this
code is heading 3.9 so it is not in mainline yet but available in
linux-pm.git/linux-next branch. I can dig the commit id if you are
interested.

> Can we do this in platform data and complete the base calculation.

Do you mean platform data for the dmac driver?

The CSRT parser code can't know what the platform data requirements for the
particular dmac driver are. It only creates the platform devices and adds
MMIO, IRQ and possible DMA resources. It is up to the (dmac) driver to use
this information for whatever purposes in needs.

> In ACPI 5, is DMA a PCI device or platfrom device?

It is platform device.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] gpiolib-acpi: introduce acpi_get_gpio_by_index() helper

[Mika Westerberg]

Instead of open-coding ACPI GPIO resource lookup in each driver, we provide
a helper function analogous to Device Tree version that allows drivers to
specify which GPIO resource they are interested (using an index to the GPIO
resources). The function then finds out the correct resource, translates
the ACPI GPIO number to the corresponding Linux GPIO number and returns
that.

Signed-off-by: Mika Westerberg 
---
 Documentation/acpi/enumeration.txt |   32 ++-
 drivers/gpio/gpiolib-acpi.c|   77 
 include/linux/acpi_gpio.h  |   17 
 3 files changed, 125 insertions(+), 1 deletion(-)

diff --git a/Documentation/acpi/enumeration.txt 
b/Documentation/acpi/enumeration.txt
index 94a6561..b0d5410 100644
--- a/Documentation/acpi/enumeration.txt
+++ b/Documentation/acpi/enumeration.txt
@@ -199,6 +199,8 @@ the device to the driver. For example:
{
Name (SBUF, ResourceTemplate()
{
+   ...
+   // Used to power on/off the device
GpioIo (Exclusive, PullDefault, 0x, 0x,
IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
0x00, ResourceConsumer,,)
@@ -206,10 +208,20 @@ the device to the driver. For example:
// Pin List
0x0055
}
+
+   // Interrupt for the device
+   GpioInt (Edge, ActiveHigh, ExclusiveAndWake, PullNone,
+0x, "\\_SB.PCI0.GPI0", 0x00, 
ResourceConsumer,,)
+   {
+   // Pin list
+   0x0058
+   }
+
...
 
-   Return (SBUF)
}
+
+   Return (SBUF)
}
 
 These GPIO numbers are controller relative and path "\\_SB.PCI0.GPI0"
@@ -220,6 +232,24 @@ The driver can do this by including  
and then calling
 acpi_get_gpio(path, gpio). This will return the Linux GPIO number or
 negative errno if there was no translation found.
 
+In a simple case of just getting the Linux GPIO number from device
+resources one can use acpi_get_gpio_by_index() helper function. It takes
+pointer to the device and index of the GpioIo/GpioInt descriptor in the
+device resources list. For example:
+
+   int gpio_irq, gpio_power;
+   int ret;
+
+   gpio_irq = acpi_get_gpio_by_index(dev, 1, NULL);
+   if (gpio_irq < 0)
+   /* handle error */
+
+   gpio_power = acpi_get_gpio_by_index(dev, 0, NULL);
+   if (gpio_power < 0)
+   /* handle error */
+
+   /* Now we can use the GPIO numbers */
+
 Other GpioIo parameters must be converted first by the driver to be
 suitable to the gpiolib before passing them.
 
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index a063eb0..b66df3b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -54,6 +54,83 @@ int acpi_get_gpio(char *path, int pin)
 }
 EXPORT_SYMBOL_GPL(acpi_get_gpio);
 
+struct acpi_gpio_lookup {
+   struct acpi_gpio_info info;
+   int index;
+   int gpio;
+   int n;
+};
+
+static int acpi_find_gpio(struct acpi_resource *ares, void *data)
+{
+   struct acpi_gpio_lookup *lookup = data;
+
+   if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
+   return 1;
+
+   if (lookup->n++ == lookup->index && lookup->gpio < 0) {
+   const struct acpi_resource_gpio *agpio = &ares->data.gpio;
+
+   lookup->gpio = acpi_get_gpio(agpio->resource_source.string_ptr,
+agpio->pin_table[0]);
+   lookup->info.gpioint =
+   agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
+   }
+
+   return 1;
+}
+
+/**
+ * acpi_get_gpio_by_index() - get a GPIO number from device resources
+ * @dev: pointer to a device to get GPIO from
+ * @index: index of GpioIo/GpioInt resource (starting from %0)
+ * @info: info pointer to fill in (optional)
+ *
+ * Function goes through ACPI resources for @dev and based on @index looks
+ * up a GpioIo/GpioInt resource, translates it to the Linux GPIO number,
+ * and returns it. @index matches GpioIo/GpioInt resources only so if there
+ * are total %3 GPIO resources, the index goes from %0 to %2.
+ *
+ * If the GPIO cannot be translated or there is an error, negative errno is
+ * returned.
+ *
+ * Note: if the GPIO resource has multiple entries in the pin list, this
+ * function only returns the first.
+ */
+int acpi_get_gpio_by_index(struct device *dev, int index,
+  struct acpi_gpio_info *info)
+{
+   struct acpi_gpio_lookup lookup;
+   struct list_head resource_list;
+   struct a

Re: GPS driver for Linux - kernel or user-space driver?

[Mika Westerberg]

(Adding Lv, he has been working on this).

On Wed, Apr 03, 2013 at 06:44:51PM -0700, Marcel Holtmann wrote:
> Hi Greg,
> 
>  I've been approached by a developer at Sony who wants to publish an
>  open source driver for a Sony GPS receiver module.
> >>> 
> >>> What does the device look like?  USB device?  UART?  Something else?
> >>> 
>  I've looked in the kernel source, and only see one standalone GPS
>  driver, for Garmin.  It appears that most GPS support in Linux is done
>  via user-space drivers.  Many GPS hardware modules appear to be
>  accessed via a serial line, or USB/serial port.  The Sony
>  module is pretty much the same, accepting commands and delivering
>  data via a uart from the chip.
>  
>  I planning to recommend writing a user-space driver (based on
>  gpsd and/or the Android GPS HAL specification).  But I'm worried
>  I'm missing something.  Is this the correct approach, or is there
>  an established kernel API for GPS modules - such that I should
>  recommend that this developer writes a kernel module instead
>  of, or in addition to, the user-space support for the hardware?
> >>> 
> >>> If it's just a uart-like device, just write a serial driver and drive it
> >>> from gpsd.  That way seems to be the simplest and then the kernel just
> >>> becomes a dumb-pipe, which is fine.
> >> 
> >> the one thing that still bugs me is that detecting an UART with GPS
> >> functionality behind it is extremely bad experience. If we could get
> >> something like DEVTYPE=gps support for the TTY layer, that would be
> >> helpful.
> > 
> > I would love that, but there's tons of USB gps devices that we don't
> > know if they are a USB serial device, or a GPS device as the device id
> > is the same (companies just embed a usb to serial chip in their
> > devices.)
> > 
> > So, someone has to guess, right now it's userspace.
> > 
> > Unless you have a better idea?
> 
> we could at least try to fix the good ones where we have proper vendor
> and product IDs dedicated for these devices. Or where things can be
> labeled through device tree or some similar methods. The less guess work
> userspace has to do, the smooth the experience will be.

There is one attempt to do this in case of ACPI here:

http://www.spinics.net/lists/linux-acpi/msg42951.html

Basically it adds a new sysfs attribute "peripheral_type" which then has
the ACPI modalias for the device behind UART. That should help userspace to
find out which device it is. The same attribute should work with DeviceTree
as well.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] gpiolib-acpi: introduce acpi_get_gpio_by_index() helper

[Mika Westerberg]

On Thu, Apr 04, 2013 at 11:19:53AM +0200, Benjamin Tissoires wrote:
> Hi Mika,
> 
> On Wed, Apr 3, 2013 at 12:56 PM, Mika Westerberg
>  wrote:
> > Instead of open-coding ACPI GPIO resource lookup in each driver, we provide
> > a helper function analogous to Device Tree version that allows drivers to
> > specify which GPIO resource they are interested (using an index to the GPIO
> > resources). The function then finds out the correct resource, translates
> > the ACPI GPIO number to the corresponding Linux GPIO number and returns
> > that.
> >
> > Signed-off-by: Mika Westerberg 
> > ---
> >  Documentation/acpi/enumeration.txt |   32 ++-
> >  drivers/gpio/gpiolib-acpi.c|   77 
> > 
> >  include/linux/acpi_gpio.h  |   17 
> >  3 files changed, 125 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/acpi/enumeration.txt 
> > b/Documentation/acpi/enumeration.txt
> > index 94a6561..b0d5410 100644
> > --- a/Documentation/acpi/enumeration.txt
> > +++ b/Documentation/acpi/enumeration.txt
> > @@ -199,6 +199,8 @@ the device to the driver. For example:
> > {
> > Name (SBUF, ResourceTemplate()
> > {
> > +   ...
> > +   // Used to power on/off the device
> > GpioIo (Exclusive, PullDefault, 0x, 0x,
> > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
> > 0x00, ResourceConsumer,,)
> > @@ -206,10 +208,20 @@ the device to the driver. For example:
> > // Pin List
> > 0x0055
> > }
> > +
> > +   // Interrupt for the device
> > +   GpioInt (Edge, ActiveHigh, ExclusiveAndWake, 
> > PullNone,
> > +0x, "\\_SB.PCI0.GPI0", 0x00, 
> > ResourceConsumer,,)
> 
> Sorry for coming late in the GPIO ACPI discussion, but when I see this
> documentation, I wonder:
> wouldn't it be feasible to find the correct GPIO by its type? Here, we
> have a GpioIo and a GpioInt, and I bet this would be sometime more
> useful to request the first GpioInt without knowing the correct order
> of declarations.

Why not. But then again you can always check the type returned in the
acpi_gpio_info structure and pick the first GpioInt (if you have multiple
GPIO resources).

> It may be feasible by walking the tree, but a helper would be of great
> help (thinking at i2c-hid here, which can not rely on indexes in the
> DSDT).

Well, index is the only thing we can rely on unfortunately. There's nothing
like names or anything like that.

What I've seen from ACPI enumerated i2c-hid devices there is only one GPIO
resource (GpioInt) declared.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] gpiolib-acpi: introduce acpi_get_gpio_by_index() helper

[Mika Westerberg]

On Thu, Apr 04, 2013 at 11:42:11AM +0200, Benjamin Tissoires wrote:
> On Thu, Apr 4, 2013 at 11:38 AM, Mika Westerberg
>  wrote:
> > On Thu, Apr 04, 2013 at 11:19:53AM +0200, Benjamin Tissoires wrote:
> >> Hi Mika,
> >>
> >> On Wed, Apr 3, 2013 at 12:56 PM, Mika Westerberg
> >>  wrote:
> >> > Instead of open-coding ACPI GPIO resource lookup in each driver, we 
> >> > provide
> >> > a helper function analogous to Device Tree version that allows drivers to
> >> > specify which GPIO resource they are interested (using an index to the 
> >> > GPIO
> >> > resources). The function then finds out the correct resource, translates
> >> > the ACPI GPIO number to the corresponding Linux GPIO number and returns
> >> > that.
> >> >
> >> > Signed-off-by: Mika Westerberg 
> >> > ---
> >> >  Documentation/acpi/enumeration.txt |   32 ++-
> >> >  drivers/gpio/gpiolib-acpi.c|   77 
> >> > 
> >> >  include/linux/acpi_gpio.h  |   17 
> >> >  3 files changed, 125 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/Documentation/acpi/enumeration.txt 
> >> > b/Documentation/acpi/enumeration.txt
> >> > index 94a6561..b0d5410 100644
> >> > --- a/Documentation/acpi/enumeration.txt
> >> > +++ b/Documentation/acpi/enumeration.txt
> >> > @@ -199,6 +199,8 @@ the device to the driver. For example:
> >> > {
> >> > Name (SBUF, ResourceTemplate()
> >> > {
> >> > +   ...
> >> > +   // Used to power on/off the device
> >> > GpioIo (Exclusive, PullDefault, 0x, 0x,
> >> > IoRestrictionOutputOnly, 
> >> > "\\_SB.PCI0.GPI0",
> >> > 0x00, ResourceConsumer,,)
> >> > @@ -206,10 +208,20 @@ the device to the driver. For example:
> >> > // Pin List
> >> > 0x0055
> >> > }
> >> > +
> >> > +   // Interrupt for the device
> >> > +   GpioInt (Edge, ActiveHigh, ExclusiveAndWake, 
> >> > PullNone,
> >> > +0x, "\\_SB.PCI0.GPI0", 0x00, 
> >> > ResourceConsumer,,)
> >>
> >> Sorry for coming late in the GPIO ACPI discussion, but when I see this
> >> documentation, I wonder:
> >> wouldn't it be feasible to find the correct GPIO by its type? Here, we
> >> have a GpioIo and a GpioInt, and I bet this would be sometime more
> >> useful to request the first GpioInt without knowing the correct order
> >> of declarations.
> >
> > Why not. But then again you can always check the type returned in the
> > acpi_gpio_info structure and pick the first GpioInt (if you have multiple
> > GPIO resources).
> >
> >> It may be feasible by walking the tree, but a helper would be of great
> >> help (thinking at i2c-hid here, which can not rely on indexes in the
> >> DSDT).
> >
> > Well, index is the only thing we can rely on unfortunately. There's nothing
> > like names or anything like that.
> >
> > What I've seen from ACPI enumerated i2c-hid devices there is only one GPIO
> > resource (GpioInt) declared.
> 
> Ok, thanks for the answer. I guess the idea would be to pick the index
> 0, check the type, and try indexes 1 or 2 if it's not GpioInt. I bet
> there will be devices with more than one Gpio as most of I2C input
> device have a reset line (except if Microsoft forces them not to have
> one).

One option is to provide acpi_get_gpio_all() that returns all GPIOs and
their corresponding types. That should allow clients like i2c-hid to find
the right GPIO (I'm hoping that there will be only one GpioInt associated
with these devices).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] gpiolib-acpi: introduce acpi_get_gpio_by_index() helper

[Mika Westerberg]

On Thu, Apr 04, 2013 at 12:01:23PM +0200, Benjamin Tissoires wrote:
> > One option is to provide acpi_get_gpio_all() that returns all GPIOs and
> > their corresponding types. That should allow clients like i2c-hid to find
> > the right GPIO (I'm hoping that there will be only one GpioInt associated
> > with these devices).
> 
> That could do the trick.

Great.

> However, I won't be able to test it. I still don't have access to any
> ACPI 5 device with i2c-hid devices...

I have few such devices but none of them has GpioInt resources (they have
the interrupt line routed to ioapic), so I can't test the i2c-hid with
GpioInt either :-/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] dw_dmac: adjust slave_id accordingly to request line base

[Mika Westerberg]

On Tue, Jan 29, 2013 at 10:29:43AM +0530, Viresh Kumar wrote:
> Next time, please direct these mails to my Linaro id :)
> 
> On Mon, Jan 28, 2013 at 4:34 PM, Andy Shevchenko
>  wrote:
> > On some hardware configurations we have got the request line with the 
> > offset.
> > The patch introduces convert_slave_id() helper for that cases. The request 
> > line
> > base is got from the platform device resources provided by the 
> > IORESOURCE_DMA
> > type.
> 
> @Vinod: Is IORESOURCE_DMA suitable for this purpose?

We had a discusssion about this with Andy as well. The thing is that there
is no way in current resource to pass DMA request line numbers supported by
the controller to the driver in a generic way. We on the other hand have to
deal this somehow as we have a shared DMA controller on Lynxpoint where the
offset will start from 16 (but it might be something else as well).

Is there something which limits the usage of IORESOUCE_DMA to be only
usable for ISA DMA channels?

We have made supporting code that currently sits in linux-pm/linux-next
tree that parses an ACPI CSRT table and creates the platform devices with
suitable resources here: 13176bbf183c8 (ACPI: add support for CSRT table).

> > Signed-off-by: Mika Westerberg 
> > Signed-off-by: Andy Shevchenko 
> > ---
> >  drivers/dma/dw_dmac.c  |   13 +
> >  drivers/dma/dw_dmac_regs.h |1 +
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> > index 3155c11..8484ca1 100644
> > --- a/drivers/dma/dw_dmac.c
> > +++ b/drivers/dma/dw_dmac.c
> > @@ -995,6 +995,13 @@ static inline void convert_burst(u32 *maxburst)
> > *maxburst = 0;
> >  }
> >
> > +static inline void convert_slave_id(struct dw_dma_chan *dwc)
> > +{
> > +   struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > +
> > +   dwc->dma_sconfig.slave_id -= dw->request_line_base;
> > +}
> > +
> >  static int
> >  set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
> >  {
> > @@ -1009,6 +1016,7 @@ set_runtime_config(struct dma_chan *chan, struct 
> > dma_slave_config *sconfig)
> >
> > convert_burst(&dwc->dma_sconfig.src_maxburst);
> > convert_burst(&dwc->dma_sconfig.dst_maxburst);
> > +   convert_slave_id(dwc);
> >
> > return 0;
> >  }
> > @@ -1717,6 +1725,11 @@ static int dw_probe(struct platform_device *pdev)
> > memcpy(dw->data_width, pdata->data_width, 4);
> > }
> >
> > +   /* Get the base request line if set */
> > +   io = platform_get_resource(pdev, IORESOURCE_DMA, 0);
> > +   if (io)
> > +   dw->request_line_base = (unsigned int)io->start;
> > +
> 
> How will it work in case of DT?

Can't the DT version do the same thing and pass IORESOURCE_DMA to the
driver? Or we can check dev->of_node and parse it directly from DT instead
of resource.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] ACPI / platform: Use struct acpi_scan_handler for creating devices

[Mika Westerberg]

On Mon, Jan 28, 2013 at 02:01:14PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> Currently, the ACPI namespace scanning code creates platform device
> objects for ACPI device nodes whose IDs match the contents of the
> acpi_platform_device_ids[] table.  However, this adds a superfluous
> special case into acpi_bus_device_attach() and makes it more
> difficult to follow than it has to be.  It also will make it more
> difficult to implement removal code for those platform device objects
> in the future.
> 
> For the above reasons, introduce a struct acpi_scan_handler object
> for creating platform devices and move the code related to that from
> acpi_bus_device_attach() to the .attach() callback of that object.
> Also move the acpi_platform_device_ids[] table to acpi_platform.c.
> 
> Signed-off-by: Rafael J. Wysocki 

I've tested this with Haswell machine and once you fix the problem pointed
out by Yasuaki Ishimat (returning always when ACPI_PLATFORM_CLK is set) the
platform device creation works well. This is a nice cleanup and localizes
the hard coded platform device table in one file making maintenance bit
easier.

Feel free to add:

Reviewed-by: Mika Westerberg 
Tested-by: Mika Westerberg 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] ACPI / platform: Use struct acpi_scan_handler for creating devices

[Mika Westerberg]

On Mon, Jan 28, 2013 at 02:01:14PM +0100, Rafael J. Wysocki wrote:
> +/* Flags for acpi_create_platform_device */
> +#define ACPI_PLATFORM_CLKBIT(0)
> +
> +/*
> + * The following ACPI IDs are known to be suitable for representing as
> + * platform devices.
> + */
> +static const struct acpi_device_id acpi_platform_device_ids[] = {
> +
> + { "PNP0D40" },
> +
> + /* Haswell LPSS devices */
> + { "INT33C0", ACPI_PLATFORM_CLK },
> + { "INT33C1", ACPI_PLATFORM_CLK },
> + { "INT33C2", ACPI_PLATFORM_CLK },
> + { "INT33C3", ACPI_PLATFORM_CLK },
> + { "INT33C4", ACPI_PLATFORM_CLK },
> + { "INT33C5", ACPI_PLATFORM_CLK },
> + { "INT33C6", ACPI_PLATFORM_CLK },
> + { "INT33C7", ACPI_PLATFORM_CLK },
> +
> + { }
> +};

Now that we have everything the platform support code needs in a single
file, should we instead of setting flags and comparing strings like
"INT33C" to find out are we running on Lynxpoint, pass function pointer
that gets called when corresponding device gets created? Something like:

{ "INT33C0", lpt_clks_init },
...

Or do you think we need to keep the flags still?

I can prepare a patch if this turns out to be sensible thing to do.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] ACPI / scan: Fix acpi_bus_get_device() check in acpi_match_device()

[Mika Westerberg]

On Wed, Jan 30, 2013 at 11:03:07PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> Since acpi_bus_get_device() returns int and not acpi_status, change
> acpi_match_device() so that it doesn't apply ACPI_FAILURE() to the
> return value of acpi_bus_get_device().
> 
> Signed-off-by: Rafael J. Wysocki 

Reviewed-by: Mika Westerberg 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] ACPI / scan: Clean up acpi_bus_get_parent()

[Mika Westerberg]

On Wed, Jan 30, 2013 at 11:04:03PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> Make acpi_bus_get_parent() more straightforward and remove an
> unnecessary local variable ret from it.
> 
> Signed-off-by: Rafael J. Wysocki 

Reviewed-by: Mika Westerberg 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 00/10] spi/pxa2xx: add Intel Lynxpoint SPI controller support

[Mika Westerberg]

On Fri, Feb 01, 2013 at 02:32:03PM +0800, zergmk2 wrote:
> I've verified your patch and spi bus worked properly on PXA910 platform.
> I used a debug tool to send data on spi bus, and I connected spi out pin
> and spi in pin.
> So when I read data from spi bus, it should be what I sent from spi bus.
> The test showed the expected result.
> Tested-by: Lu Cao 

Thank you very much for testing!

Mark, thank you for applying patches 1-4/10. Is there anything you want me
to do for the rest of the patches in order to get those merged?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] i2c-designware: add Intel Lynxpoint support

[Mika Westerberg]

On Thu, Jan 24, 2013 at 08:28:46AM +0100, Wolfram Sang wrote:
> I can't tell much about ACPI usage, but rest looks good to me. Applied
> to next.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] gpio: gpio-ich: fix ichx_gpio_check_available() return what callers expect

[Mika Westerberg]

ichx_gpio_check_available() returns either 0 or -ENXIO depending on whether
the given GPIO is available or not. However, callers of this function treat
the return value as boolean:

...
if (!ichx_gpio_check_available(gpio, nr))
return -ENXIO;

which erroneusly fails when the GPIO is available and not vice versa.

Fix this by making the function return boolean as expected by the callers.

Signed-off-by: Mika Westerberg 
---
 drivers/gpio/gpio-ich.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
index 6f2306d..f9dbd50 100644
--- a/drivers/gpio/gpio-ich.c
+++ b/drivers/gpio/gpio-ich.c
@@ -128,9 +128,9 @@ static int ichx_read_bit(int reg, unsigned nr)
return data & (1 << bit) ? 1 : 0;
 }
 
-static int ichx_gpio_check_available(struct gpio_chip *gpio, unsigned nr)
+static bool ichx_gpio_check_available(struct gpio_chip *gpio, unsigned nr)
 {
-   return (ichx_priv.use_gpio & (1 << (nr / 32))) ? 0 : -ENXIO;
+   return ichx_priv.use_gpio & (1 << (nr / 32));
 }
 
 static int ichx_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] HID: make sensor autodetection independent of underlying bus

[Mika Westerberg]

Instead of limiting HID sensors to USB and I2C busses we can just make
everything that has usage page of HID_UP_SENSOR to be included in
HID_GROUP_SENSOR_HUB group. This allows the sensor-hub to work over
bluetooth (and other transports) as well.

Reported-by: Alexander Holler 
Signed-off-by: Mika Westerberg 
---
 drivers/hid/hid-core.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index fe3a59e..f89e902 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -728,8 +728,7 @@ static int hid_scan_report(struct hid_device *hid)
} else if (page == HID_UP_SENSOR &&
item.type == HID_ITEM_TYPE_MAIN &&
item.tag == HID_MAIN_ITEM_TAG_BEGIN_COLLECTION &&
-   (item_udata(&item) & 0xff) == HID_COLLECTION_PHYSICAL &&
-   (hid->bus == BUS_USB || hid->bus == BUS_I2C))
+   (item_udata(&item) & 0xff) == HID_COLLECTION_PHYSICAL)
hid->group = HID_GROUP_SENSOR_HUB;
}
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -next] spi: fix return value check in ce4100_spi_probe()

[Mika Westerberg]

On Fri, Feb 22, 2013 at 10:52:35AM +0800, Wei Yongjun wrote:
> From: Wei Yongjun 
> 
> In case of error, the function platform_device_register_full()
> returns ERR_PTR() and never returns NULL. The NULL test in the
> return value check should be replaced with IS_ERR().
> 
> Signed-off-by: Wei Yongjun 

Nice catch!

Acked-by: Mika Westerberg 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/6] Introduce acpi_match_device_id().

[Mika Westerberg]

On Mon, Oct 01, 2012 at 01:56:17PM +, Zhang, Rui wrote:
> 
> 
> > -Original Message-
> > From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
> > Sent: Monday, October 01, 2012 2:38 PM
> > To: Zhang, Rui
> > Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
> > Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> > Subject: Re: [RFC PATCH 1/6] Introduce acpi_match_device_id().
> > Importance: High
> > 
> > On Sat, Sep 29, 2012 at 01:31:52PM +, Zhang, Rui wrote:
> > >
> > > > > +{
> > > > > + acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> > > >
> > > > If the device is not bound to an ACPI handle this will return NULL.
> > > > And I don't see you doing that in this series meaning that..
> > > >
> > >
> > >
> > > You're right, I should set pdev->archdata.acpi_handle to the I2C
> > > controller in i2c_root.c.
> > 
> > There already is an API for that - check drivers/acpi/glue.c.
> 
> Do you mean acpi_bind?

No, I mean register_acpi_bus_type(). You can see an example of this looking
PCI, USB, .., drivers.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver

[Mika Westerberg]

On Mon, Oct 01, 2012 at 02:19:49PM +, Zhang, Rui wrote:
> 
> 
> > -Original Message-
> > From: linux-i2c-ow...@vger.kernel.org [mailto:linux-i2c-
> > ow...@vger.kernel.org] On Behalf Of Mika Westerberg
> > Sent: Monday, October 01, 2012 2:55 PM
> > To: Zhang, Rui
> > Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
> > Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> > Subject: Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller
> > enumeration driver
> > Importance: High
> > 
> > On Fri, Sep 28, 2012 at 03:40:32PM +0800, Zhang Rui wrote:
> > > +acpi_status __init i2c_enumerate_slave(acpi_handle handle, u32 level,
> > > +void *data, void **return_value) {
> > > + int result;
> > > + acpi_status status;
> > > + struct acpi_buffer buffer;
> > > + struct acpi_resource *resource;
> > > + struct acpi_resource_gpio *gpio;
> > > + struct acpi_resource_i2c_serialbus *i2c;
> > > + int i;
> > > + struct acpi_i2c_root *root = data;
> > > + struct i2c_board_info info;
> > > + struct acpi_device *device;
> > > +
> > > + if (acpi_bus_get_device(handle, &device))
> > > + return AE_OK;
> > > +
> > > + status = acpi_get_current_resources(handle, &buffer);
> > > + if (ACPI_FAILURE(status)) {
> > > + dev_err(&device->dev, "Failed to get ACPI resources\n");
> > > + return AE_OK;
> > > + }
> > > +
> > > + for (i = 0; i < buffer.length; i += sizeof(struct acpi_resource))
> > {
> > > + resource = (struct acpi_resource *)(buffer.pointer + i);
> > > +
> > > + switch (resource->type) {
> > > + case ACPI_RESOURCE_TYPE_GPIO:
> > > + gpio = &resource->data.gpio;
> > > +
> > > + if (gpio->connection_type ==
> > ACPI_RESOURCE_GPIO_TYPE_INT) {
> > > + result =
> > > + acpi_device_get_gpio_irq
> > > + (gpio->resource_source.string_ptr,
> > > +  gpio->pin_table[0], &info.irq);
> > 
> > acpi_device_get_gpio_irq() is not defined in this patch series?
> > 
> ACPI GPIO controller patch has already been sent out, but in ACPI mailing 
> list only.

It would have been good idea to mention this in the cover letter at least.

> 
> > Also you need to do the gpio_request()/gpio_to_irq() things somewhere.
> > Are they handled in acpi_device_get_gpio_irq()?
> >
> Yep.
>  
> > How about GpioIo resources?
> > 
> This is not covered in this patch set, but will be in the next patch set.
> 
> > > + if (result)
> > > + dev_err(&device->dev,
> > > + "Failed to get IRQ\n");
> > > + }
> > > + break;
> > > + case ACPI_RESOURCE_TYPE_SERIAL_BUS:
> > > + i2c = &resource->data.i2c_serial_bus;
> > > +
> > > + info.addr = i2c->slave_address;
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > + }
> > > +
> > > + add_slave(root, &info);
> > > +
> > > + kfree(buffer.pointer);
> > > + return AE_OK;
> > > +}
> > > +
> > > +static int __devinit acpi_i2c_root_add(struct acpi_device *device) {
> > > + acpi_status status;
> > > + struct acpi_i2c_root *root;
> > > + struct resource *resources;
> > > + int result;
> > > +
> > > + if (!device->pnp.unique_id) {
> > > + dev_err(&device->dev,
> > > + "Unsupported ACPI I2C controller. No UID\n");
> > 
> > Where does this restriction come from? As far as I understand UID is
> > optional.
> >
> 
> _UID is optional.
> But it seems to be required for SPB buses that need ACPI device enumeration.
> At least this is true for the ACPI 5 compatible ACPI tables I have.

Yes but if some vendor is not setting it (as it is optional) you still want
your code to work, right?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal

[Mika Westerberg]

On Mon, Oct 01, 2012 at 02:30:00PM +, Zhang, Rui wrote:
> 
> 
> > -Original Message-
> > From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
> > Sent: Monday, October 01, 2012 2:45 PM
> > To: Zhang, Rui
> > Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
> > Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> > Subject: Re: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal
> > Importance: High
> > 
> > On Fri, Sep 28, 2012 at 03:37:43PM +0800, Zhang Rui wrote:
> > >
> > > the main idea is that, for Serial Buses like I2C and SPI, we
> > enumerate
> > > the controller as a platform device, and then enumerate the slaves
> > via
> > > i2c/spi_register_board_info. And then, when the controller is really
> > > probed and enabled in the platform driver, the SPI/I2C bus code will
> > > enumerate I2C/SPI slaves automatically.
> > > And for the other devices, we will enumerate all of them as platform
> > > devices, which is not covered in this patch set yet.
> > 
> > Can you show some example how we could use this new code for example
> > with an existing I2C/SPI slave driver?
> 
> This is just prototype patch set that I want to illustrate my idea
> on ACPI device enumeration, to get more thoughts on this.
> So no example driver so far.

But surely you have thought how this should be done? Otherwise we only see
a part of the solution here. What if this enumeration you introduce here
doesn't play well when I2C/SPI are added?

> 
> > Let's say the device uses few
> > GPIOs, one for interrupt and other for triggering firmware download. In
> > addition to that it needs a special parameters that can be extracted
> > running the "_DSM" method of the device.
> > 
> > Normally the driver would get this stuff from the platform data or from
> > Device Tree but how it is done with these patches?
> 
> Can you show me an example driver that gets the special parameters from 
> Device Tree?

drivers/misc/eeprom/at25.c but there are much more if you search for DT
enabled drivers.

Now, I don't know what is the proper way to pass parameters in ACPI but
"_DSM" seems to be one that is being used.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/5] driver core / ACPI: Move ACPI support to core device and driver types

[Mika Westerberg]

On Wed, Oct 31, 2012 at 10:06:00PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, October 31, 2012 08:33:53 PM Luck, Tony wrote:
> > > By "tested" I mean "run with some new devices that use the ACPI 
> > > enumeration
> > > provided here, on x86".  Sorry for being too vague.
> > 
> > Do you or Mika have access to an ia64 box to test.
> 
> I don't.  I'm not sure about Mika.

I also don't have access to any ia64 machine.

As Rafael said, I tested this on x86 machine only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/3] ACPI 5 support for GPIO, SPI and I2C

[Mika Westerberg]

Hi,

With ACPI 5 we can now describe how devices are connected to their bus
using new resources: SPISerialBus and I2CSerialBus. Also it is now possible
to add GPIO connections for the devices with the help of GpioIO and GpioInt
resources.

This series adds support for these new resources.

The series based on the ACPI 5 enumeration support patches that are available
on Rafael's linux-next branch:

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git 
linux-next

Specifically patches from this thread:

https://lkml.org/lkml/2012/10/31/154

Since these patches depend on the above patches on Rafael's linux-next branch
I suggest that these be merged via that branch, if there are no objections.

The series follows the Device Tree way so that it would be easy to add ACPI
support for the existing SPI and I2C drivers if one is familiar how the
corresponding DT support is done.

For GPIO we introduce a function that maps between ACPI GPIO numbers and Linux
ones - acpi_get_gpio().

SPI slave devices gets enumerated automatically if the master device has
master->dev.acpi_handle set (this is analogous to master->dev.of_mode). The
platform bus code in Rafael's branch assigns the ACPI handle to the master
device.

I2C slave devices can be enumerated by calling acpi_i2c_register_devices() in
the adapter driver.

Thanks,
Mika

Mathias Nyman (1):
  gpio / ACPI: add ACPI support

Mika Westerberg (2):
  spi / ACPI: add ACPI enumeration support
  i2c / ACPI: add ACPI enumeration support

 drivers/acpi/Kconfig|6 ++
 drivers/acpi/Makefile   |1 +
 drivers/acpi/acpi_i2c.c |  234 +++
 drivers/gpio/Kconfig|4 +
 drivers/gpio/Makefile   |1 +
 drivers/gpio/gpiolib-acpi.c |   60 +++
 drivers/i2c/i2c-core.c  |9 ++
 drivers/spi/spi.c   |  231 +-
 include/linux/acpi_gpio.h   |   19 
 include/linux/acpi_i2c.h|   29 ++
 10 files changed, 593 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/acpi_i2c.c
 create mode 100644 drivers/gpio/gpiolib-acpi.c
 create mode 100644 include/linux/acpi_gpio.h
 create mode 100644 include/linux/acpi_i2c.h

-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/3] i2c / ACPI: add ACPI enumeration support

[Mika Westerberg]

ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate
and configure the I2C slave devices behind the I2C controller. This patch
adds helper functions to support I2C slave enumeration.

An ACPI enabled I2C controller driver only needs to call 
acpi_i2c_register_devices()
in order to get its slave devices enumerated, created and bound to the
corresponding ACPI handle.

Signed-off-by: Mika Westerberg 
Acked-by: Rafael J. Wysocki 
---
 drivers/acpi/Kconfig |6 ++
 drivers/acpi/Makefile|1 +
 drivers/acpi/acpi_i2c.c  |  234 ++
 drivers/i2c/i2c-core.c   |9 ++
 include/linux/acpi_i2c.h |   29 ++
 5 files changed, 279 insertions(+)
 create mode 100644 drivers/acpi/acpi_i2c.c
 create mode 100644 include/linux/acpi_i2c.h

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 119d58d..0300bf6 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -181,6 +181,12 @@ config ACPI_DOCK
  This driver supports ACPI-controlled docking stations and removable
  drive bays such as the IBM Ultrabay and the Dell Module Bay.
 
+config ACPI_I2C
+   def_tristate I2C
+   depends on I2C
+   help
+ ACPI I2C enumeration support.
+
 config ACPI_PROCESSOR
tristate "Processor"
select THERMAL
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index a7badb5..8573346 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_ACPI_HED)+= hed.o
 obj-$(CONFIG_ACPI_EC_DEBUGFS)  += ec_sys.o
 obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)+= bgrt.o
+obj-$(CONFIG_ACPI_I2C) += acpi_i2c.o
 
 # processor has its own "processor." module_param namespace
 processor-y:= processor_driver.o processor_throttling.o
diff --git a/drivers/acpi/acpi_i2c.c b/drivers/acpi/acpi_i2c.c
new file mode 100644
index 000..dc6997e
--- /dev/null
+++ b/drivers/acpi/acpi_i2c.c
@@ -0,0 +1,234 @@
+/*
+ * ACPI I2C enumeration support
+ *
+ * Copyright (C) 2012, Intel Corporation
+ * Author: Mika Westerberg 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+
+struct acpi_i2c {
+   acpi_status (*callback)(struct acpi_device *, void *);
+   void *data;
+};
+
+static acpi_status acpi_i2c_enumerate_device(acpi_handle handle, u32 level,
+void *data, void **return_value)
+{
+   struct acpi_i2c *acpi_i2c = data;
+   struct acpi_device *adev;
+
+   if (acpi_bus_get_device(handle, &adev))
+   return AE_OK;
+   if (acpi_bus_get_status(adev) || !adev->status.present)
+   return AE_OK;
+
+   return acpi_i2c->callback(adev, acpi_i2c->data);
+}
+
+static acpi_status acpi_i2c_enumerate(acpi_handle handle,
+   acpi_status (*callback)(struct acpi_device *, void *), void *data)
+{
+   struct acpi_i2c acpi_i2c;
+
+   acpi_i2c.callback = callback;
+   acpi_i2c.data = data;
+
+   return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+  acpi_i2c_enumerate_device, NULL,
+  &acpi_i2c, NULL);
+}
+
+struct acpi_i2c_device_info {
+   struct i2c_board_info board;
+   int triggering;
+   int polarity;
+   int gsi;
+   bool valid;
+};
+
+static acpi_status acpi_i2c_add_resources(struct acpi_resource *res, void 
*data)
+{
+   struct acpi_i2c_device_info *info = data;
+   struct acpi_resource_i2c_serialbus *sb;
+
+   switch (res->type) {
+   case ACPI_RESOURCE_TYPE_SERIAL_BUS:
+   sb = &res->data.i2c_serial_bus;
+   if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+   info->board.addr = sb->slave_address;
+   if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+   info->board.flags |= I2C_CLIENT_TEN;
+
+   /*
+* The info is valid once we have found the
+* I2CSerialBus resource.
+*/
+   info->valid = true;
+   }
+   break;
+
+   case ACPI_RESOURCE_TYPE_IRQ:
+   info->gsi = res->data.irq.interrupts[0];
+   info->triggering = res->data.irq.triggering;
+   info->polarity = res->data.irq.polarity;
+   break;
+
+   case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+   info->gsi = res->data.extended_irq.interrupts[0];
+   info->triggering = res->data.extended_irq.triggering;
+   info->polarity = res->data.extended_irq.polarity;
+  

[PATCH 1/3] gpio / ACPI: add ACPI support

[Mika Westerberg]

From: Mathias Nyman 

Add support for translating ACPI GPIO pin numbers to Linux GPIO API pins.
Needs a gpio controller driver with the acpi handler hook set.

Drivers can use acpi_get_gpio() to translate ACPI5 GpioIO and GpioInt
resources to Linux GPIO's.

Signed-off-by: Mathias Nyman 
Signed-off-by: Mika Westerberg 
Acked-by: Rafael J. Wysocki 
---
 drivers/gpio/Kconfig|4 +++
 drivers/gpio/Makefile   |1 +
 drivers/gpio/gpiolib-acpi.c |   60 +++
 include/linux/acpi_gpio.h   |   19 ++
 4 files changed, 84 insertions(+)
 create mode 100644 drivers/gpio/gpiolib-acpi.c
 create mode 100644 include/linux/acpi_gpio.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d055cee..2f1905b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -49,6 +49,10 @@ config OF_GPIO
def_bool y
depends on OF && !SPARC
 
+config ACPI_GPIO
+   def_bool y
+   depends on ACPI
+
 config DEBUG_GPIO
bool "Debug GPIO calls"
depends on DEBUG_KERNEL
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 9aeed67..5254b6d 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)+= -DDEBUG
 
 obj-$(CONFIG_GPIOLIB)  += gpiolib.o devres.o
 obj-$(CONFIG_OF_GPIO)  += gpiolib-of.o
+obj-$(CONFIG_ACPI_GPIO)+= gpiolib-acpi.o
 
 # Device drivers. Generally keep list sorted alphabetically
 obj-$(CONFIG_GPIO_GENERIC) += gpio-generic.o
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
new file mode 100644
index 000..ef56ea4
--- /dev/null
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -0,0 +1,60 @@
+/*
+ * ACPI helpers for GPIO API
+ *
+ * Copyright (C) 2012, Intel Corporation
+ * Author: Mathias Nyman 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
+{
+   acpi_handle handle = data;
+   acpi_handle gc_handle;
+
+   if (!gc->dev)
+   return false;
+
+   gc_handle = gc->dev->acpi_handle;
+   if (!gc_handle)
+   return false;
+
+   return gc_handle == handle;
+}
+
+/**
+ * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO 
API
+ * @path:  ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
+ * @pin:   ACPI GPIO pin number (0-based, controller-relative)
+ *
+ * Returns GPIO number to use with Linux generic GPIO API, or errno error value
+ */
+
+int acpi_get_gpio(char *path, int pin)
+{
+   struct gpio_chip *chip;
+   acpi_handle handle;
+   acpi_status status;
+
+   status = acpi_get_handle(NULL, path, &handle);
+   if (ACPI_FAILURE(status))
+   return -ENODEV;
+
+   chip = gpiochip_find(handle, acpi_gpiochip_find);
+   if (!chip)
+   return -ENODEV;
+
+   if (!gpio_is_valid(chip->base + pin))
+   return -EINVAL;
+
+   return chip->base + pin;
+}
+EXPORT_SYMBOL(acpi_get_gpio);
diff --git a/include/linux/acpi_gpio.h b/include/linux/acpi_gpio.h
new file mode 100644
index 000..e025664
--- /dev/null
+++ b/include/linux/acpi_gpio.h
@@ -0,0 +1,19 @@
+#ifndef _LINUX_ACPI_GPIO_H_
+#define _LINUX_ACPI_GPIO_H_
+
+#include 
+
+#ifdef CONFIG_ACPI_GPIO
+
+int acpi_get_gpio(char *path, int pin);
+
+#else /* CONFIG_ACPI_GPIO */
+
+static inline int acpi_get_gpio(char *path, int pin)
+{
+   return -ENODEV;
+}
+
+#endif /* CONFIG_ACPI_GPIO */
+
+#endif /* _LINUX_ACPI_GPIO_H_ */
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
configure the SPI slave devices behind the SPI controller. This patch adds
support for this to the SPI core.

In addition we bind ACPI nodes to SPI devices. This makes it possible for
the slave drivers to get the ACPI handle for further configuration.

Signed-off-by: Mika Westerberg 
Acked-by: Rafael J. Wysocki 
---
 drivers/spi/spi.c |  231 -
 1 file changed, 230 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 84c2861..de22a6e 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static void spidev_release(struct device *dev)
 {
@@ -93,6 +94,10 @@ static int spi_match_device(struct device *dev, struct 
device_driver *drv)
if (of_driver_match_device(dev, drv))
return 1;
 
+   /* Then try ACPI */
+   if (acpi_driver_match_device(dev, drv))
+   return 1;
+
if (sdrv->id_table)
return !!spi_match_id(sdrv->id_table, spi);
 
@@ -888,6 +893,227 @@ static void of_register_spi_devices(struct spi_master 
*master)
 static void of_register_spi_devices(struct spi_master *master) { }
 #endif
 
+#ifdef CONFIG_ACPI
+struct acpi_spi {
+   acpi_status (*callback)(struct acpi_device *, void *);
+   void *data;
+};
+
+static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level,
+void *data, void **return_value)
+{
+   struct acpi_spi *acpi_spi = data;
+   struct acpi_device *adev;
+
+   if (acpi_bus_get_device(handle, &adev))
+   return AE_OK;
+   if (acpi_bus_get_status(adev) || !adev->status.present)
+   return AE_OK;
+
+   return acpi_spi->callback(adev, acpi_spi->data);
+}
+
+static acpi_status acpi_spi_enumerate(acpi_handle handle,
+   acpi_status (*callback)(struct acpi_device *, void *), void *data)
+{
+   struct acpi_spi acpi_spi;
+
+   acpi_spi.callback = callback;
+   acpi_spi.data = data;
+
+   return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+  acpi_spi_enumerate_device, NULL,
+  &acpi_spi, NULL);
+}
+
+struct acpi_spi_device_info {
+   struct spi_device *spi;
+   int triggering;
+   int polarity;
+   int gsi;
+   bool valid;
+};
+
+static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void 
*data)
+{
+   struct acpi_spi_device_info *info = data;
+   struct acpi_resource_spi_serialbus *sb;
+   struct spi_device *spi = info->spi;
+
+   switch (res->type) {
+   case ACPI_RESOURCE_TYPE_SERIAL_BUS:
+   sb = &res->data.spi_serial_bus;
+   if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
+   spi->chip_select = sb->device_selection;
+   spi->max_speed_hz = sb->connection_speed;
+
+   /* Mode (clock phase/polarity/etc. */
+   if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
+   spi->mode |= SPI_CPHA;
+   if (sb->clock_polarity == ACPI_SPI_START_HIGH)
+   spi->mode |= SPI_CPOL;
+   if (sb->device_polarity == ACPI_SPI_ACTIVE_HIGH)
+   spi->mode |= SPI_CS_HIGH;
+
+   /*
+* The info is valid once we have found the
+* SPISerialBus resource.
+*/
+   info->valid = true;
+   }
+   break;
+
+   case ACPI_RESOURCE_TYPE_IRQ:
+   info->gsi = res->data.irq.interrupts[0];
+   info->triggering = res->data.irq.triggering;
+   info->polarity = res->data.irq.polarity;
+   break;
+
+   case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+   info->gsi = res->data.extended_irq.interrupts[0];
+   info->triggering = res->data.extended_irq.triggering;
+   info->polarity = res->data.extended_irq.polarity;
+   break;
+   }
+
+   return AE_OK;
+}
+
+static acpi_status acpi_spi_add_device(struct acpi_device *adev, void *data)
+{
+   struct acpi_spi_device_info info;
+   struct spi_master *master = data;
+   struct spi_device *spi;
+   acpi_status status;
+
+   spi = spi_alloc_device(master);
+   if (!spi) {
+   dev_err(&master->dev, "failed to allocate SPI device\n");
+   return AE_ERROR;
+   }
+
+   memset(&info, 0, sizeof(info));
+   info.spi = spi;
+   info.gsi = -1;
+
+   status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+  

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
>  wrote:
> > ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
> > configure the SPI slave devices behind the SPI controller. This patch adds
> > support for this to the SPI core.
> >
> > In addition we bind ACPI nodes to SPI devices. This makes it possible for
> > the slave drivers to get the ACPI handle for further configuration.
> >
> > Signed-off-by: Mika Westerberg 
> > Acked-by: Rafael J. Wysocki 
> > ---
> >  drivers/spi/spi.c |  231 
> > -
> >  1 file changed, 230 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 84c2861..de22a6e 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -35,6 +35,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  static void spidev_release(struct device *dev)
> >  {
> > @@ -93,6 +94,10 @@ static int spi_match_device(struct device *dev, struct 
> > device_driver *drv)
> > if (of_driver_match_device(dev, drv))
> > return 1;
> >
> > +   /* Then try ACPI */
> > +   if (acpi_driver_match_device(dev, drv))
> > +   return 1;
> > +
> > if (sdrv->id_table)
> > return !!spi_match_id(sdrv->id_table, spi);
> >
> > @@ -888,6 +893,227 @@ static void of_register_spi_devices(struct spi_master 
> > *master)
> >  static void of_register_spi_devices(struct spi_master *master) { }
> >  #endif
> >
> > +#ifdef CONFIG_ACPI
> > +struct acpi_spi {
> > +   acpi_status (*callback)(struct acpi_device *, void *);
> > +   void *data;
> > +};
> > +
> > +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level,
> > +void *data, void 
> > **return_value)
> > +{
> > +   struct acpi_spi *acpi_spi = data;
> > +   struct acpi_device *adev;
> > +
> > +   if (acpi_bus_get_device(handle, &adev))
> > +   return AE_OK;
> > +   if (acpi_bus_get_status(adev) || !adev->status.present)
> > +   return AE_OK;
> > +
> > +   return acpi_spi->callback(adev, acpi_spi->data);
> > +}
> > +
> > +static acpi_status acpi_spi_enumerate(acpi_handle handle,
> > +   acpi_status (*callback)(struct acpi_device *, void *), void *data)
> > +{
> > +   struct acpi_spi acpi_spi;
> > +
> > +   acpi_spi.callback = callback;
> > +   acpi_spi.data = data;
> > +
> > +   return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> > +  acpi_spi_enumerate_device, NULL,
> > +  &acpi_spi, NULL);
> > +}
> > +
> > +struct acpi_spi_device_info {
> > +   struct spi_device *spi;
> > +   int triggering;
> > +   int polarity;
> > +   int gsi;
> > +   bool valid;
> > +};
> > +
> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void 
> > *data)
> > +{
> > +   struct acpi_spi_device_info *info = data;
> > +   struct acpi_resource_spi_serialbus *sb;
> > +   struct spi_device *spi = info->spi;
> > +
> > +   switch (res->type) {
> > +   case ACPI_RESOURCE_TYPE_SERIAL_BUS:
> > +   sb = &res->data.spi_serial_bus;
> > +   if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
> > +   spi->chip_select = sb->device_selection;
> > +   spi->max_speed_hz = sb->connection_speed;
> > +
> > +   /* Mode (clock phase/polarity/etc. */
> > +   if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
> > +   spi->mode |= SPI_CPHA;
> > +   if (sb->clock_polarity == ACPI_SPI_START_HIGH)
> > +   spi->mode |= SPI_CPOL;
> > +   if (sb->device_polarity == ACPI_SPI_ACTIVE_HIGH)
> > +   spi->mode |= SPI_CS_HIGH;
> > +
> > +   /*
> > +* The info is valid once we have found the
> > +* SPISerialBus resource.
> > +*/
> > +   info->valid = tr

Re: [PATCH 3/3] i2c / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Sat, Nov 03, 2012 at 10:52:46PM +0100, Jean Delvare wrote:
> On Sat,  3 Nov 2012 09:46:33 +0200, Mika Westerberg wrote:
> > ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate
> > and configure the I2C slave devices behind the I2C controller. This patch
> > adds helper functions to support I2C slave enumeration.
> > 
> > An ACPI enabled I2C controller driver only needs to call 
> > acpi_i2c_register_devices()
> > in order to get its slave devices enumerated, created and bound to the
> > corresponding ACPI handle.
> 
> I'm very happy to finally see this happen. Out of curiosity, did you
> try that code with an actual ACPI implementation?

Yes, it was tested on a real hardware with ACPI 5 support.

> 
> Light review below:
> 
> > 
> > Signed-off-by: Mika Westerberg 
> > Acked-by: Rafael J. Wysocki 
> > ---
> >  drivers/acpi/Kconfig |6 ++
> >  drivers/acpi/Makefile|1 +
> >  drivers/acpi/acpi_i2c.c  |  234 
> > ++
> >  drivers/i2c/i2c-core.c   |9 ++
> >  include/linux/acpi_i2c.h |   29 ++
> >  5 files changed, 279 insertions(+)
> >  create mode 100644 drivers/acpi/acpi_i2c.c
> >  create mode 100644 include/linux/acpi_i2c.h
> > 
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 119d58d..0300bf6 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -181,6 +181,12 @@ config ACPI_DOCK
> >   This driver supports ACPI-controlled docking stations and removable
> >   drive bays such as the IBM Ultrabay and the Dell Module Bay.
> >  
> > +config ACPI_I2C
> > +   def_tristate I2C
> > +   depends on I2C
> > +   help
> > + ACPI I2C enumeration support.
> > +
> >  config ACPI_PROCESSOR
> > tristate "Processor"
> > select THERMAL
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index a7badb5..8573346 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -69,6 +69,7 @@ obj-$(CONFIG_ACPI_HED)+= hed.o
> >  obj-$(CONFIG_ACPI_EC_DEBUGFS)  += ec_sys.o
> >  obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
> >  obj-$(CONFIG_ACPI_BGRT)+= bgrt.o
> > +obj-$(CONFIG_ACPI_I2C) += acpi_i2c.o
> >  
> >  # processor has its own "processor." module_param namespace
> >  processor-y:= processor_driver.o 
> > processor_throttling.o
> > diff --git a/drivers/acpi/acpi_i2c.c b/drivers/acpi/acpi_i2c.c
> > new file mode 100644
> > index 000..dc6997e
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_i2c.c
> > @@ -0,0 +1,234 @@
> > +/*
> > + * ACPI I2C enumeration support
> > + *
> > + * Copyright (C) 2012, Intel Corporation
> > + * Author: Mika Westerberg 
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> 
> You also need  for dev_err() etc., and  for
> ENODEV etc.

I think  already includes  but I'll double check. At
least this compiles without those headers in place :)

> > +
> > +struct acpi_i2c {
> > +   acpi_status (*callback)(struct acpi_device *, void *);
> > +   void *data;
> > +};
> > +
> > +static acpi_status acpi_i2c_enumerate_device(acpi_handle handle, u32 level,
> > +void *data, void **return_value)
> > +{
> > +   struct acpi_i2c *acpi_i2c = data;
> > +   struct acpi_device *adev;
> > +
> > +   if (acpi_bus_get_device(handle, &adev))
> > +   return AE_OK;
> > +   if (acpi_bus_get_status(adev) || !adev->status.present)
> > +   return AE_OK;
> > +
> > +   return acpi_i2c->callback(adev, acpi_i2c->data);
> > +}
> > +
> > +static acpi_status acpi_i2c_enumerate(acpi_handle handle,
> > +   acpi_status (*callback)(struct acpi_device *, void *), void *data)
> > +{
> > +   struct acpi_i2c acpi_i2c;
> > +
> > +   acpi_i2c.callback = callback;
> > +   acpi_i2c.data = data;
> > +
> > +   return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> > +  acpi_i2c_enumerate_device, NULL,
> > +  &acpi_i2c, NULL);
> > +}
> > +
> > +struct acpi_i2c_device_info {
> &

Re: [PATCH 3/3] i2c / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Sun, Nov 04, 2012 at 09:50:31AM +0100, Jean Delvare wrote:
> On Sun, 4 Nov 2012 09:23:17 +0200, Mika Westerberg wrote:
> > On Sat, Nov 03, 2012 at 10:52:46PM +0100, Jean Delvare wrote:
> > > On Sat,  3 Nov 2012 09:46:33 +0200, Mika Westerberg wrote:
> > > > --- /dev/null
> > > > +++ b/drivers/acpi/acpi_i2c.c
> > > > @@ -0,0 +1,234 @@
> > > > +/*
> > > > + * ACPI I2C enumeration support
> > > > + *
> > > > + * Copyright (C) 2012, Intel Corporation
> > > > + * Author: Mika Westerberg 
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the GNU General Public License version 2 as
> > > > + * published by the Free Software Foundation.
> > > > + */
> > > > +
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > 
> > > You also need  for dev_err() etc., and  for
> > > ENODEV etc.
> > 
> > I think  already includes  but I'll double check. At
> > least this compiles without those headers in place :)
> 
> That's not really the point. You never know which header inclusions
> will be removed from other header files in the future, so you should
> include what you need explicitly. This avoids future build breakages as
> well as build breakages on other architectures.

Right. I've actually seen this problem few times before. I added the
inclusion of  and  to the file.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] ACPI 5 support for GPIO, SPI and I2C

[Mika Westerberg]

On Sun, Nov 04, 2012 at 07:29:10PM +0100, Linus Walleij wrote:
> On Sat, Nov 3, 2012 at 8:46 AM, Mika Westerberg
>  wrote:
> 
> > With ACPI 5 we can now describe how devices are connected to their bus
> > using new resources: SPISerialBus and I2CSerialBus. Also it is now possible
> > to add GPIO connections for the devices with the help of GpioIO and GpioInt
> > resources.
> >
> > This series adds support for these new resources.
> 
> I would very much like Grant to review and merge these patches, since he
> is way more familiar with these concepts than me, but I'll have a quick
> glance for syntax and semantics...

That sounds good, thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Mon, Nov 05, 2012 at 11:31:19AM +0100, Rafael J. Wysocki wrote:
> The general idea is to move the _CRS parsing routine from acpi_platform.c
> to scan.c and make it attach resource objects to struct acpi_device.
> 
> I'm thinking about adding a list head to struct acpi_device pointing to a
> list of entries like:
> 
> struct resource_list_entry {
>   struct list_head node;
>   struct resource *resources;
>   unsigned int count;
> };
> 
> where "resources" is an array of resources (e.g. interrupts) in the given
> entry and count is the size of that array.
> 
> That list would contain common resources like 
> ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
> ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32, 
> ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
> I think adding it would allow us to make acpi_create_platform_device(),
> acpi_spi_add_resources() and acpi_i2c_add_resources() more straightforward 
> (and
> remove some code duplication between the last two routines).

This certainly sounds good to me.

> In addition to that, I'd add an entry containing serial bus information, if
> applicable, for the given struct acpi_device, something like:
> 
> union acpi_resource_serial_bus {
>   struct acpi_resource_common_serialbus;
>   struct acpi_resource_spi_serialbus;
>   struct acpi_resource_i2c_serialbus;
>   struct acpi_resource_uart_serialbus;
> };
> 
> struct acpi_device {
>   ...
>   union acpi_resource_serial_bus *serial;
>   ...
> };

It is also possible to have several serial bus connectors on a single
device (although we've seen only one connector per device but it should not
be limited to that).

> Then, things like acpi_spi_add_resources() and acpi_i2c_add_resources() would
> be able to use struct acpi_device objects directly without running any AML.
> That could be done on top of the current series and I'm going to prepare a 
> patch
> for that in the next few days if I find some time between the LCE sessions.

Cool :-) Let me know if you need any help, like testing the patches etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Mon, Nov 05, 2012 at 11:54:55AM +0100, Mark Brown wrote:
> On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote:
> 
> > +   strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias));
> > +   if (info.gsi >= 0)
> > +   spi->irq = acpi_register_gsi(&adev->dev, info.gsi,
> > +info.triggering, info.polarity);
> > +   request_module(spi->modalias);
> 
> request_module()?  Why?

The DT code does the same. I have no idea why it is there, really :-)

I can remove it in the next version if you think it is not needed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote:
> On Mon, Nov 05, 2012 at 12:56:02PM +0200, Mika Westerberg wrote:
> 
> > > struct acpi_device {
> > >   ...
> > >   union acpi_resource_serial_bus *serial;
> > >   ...
> > > };
> 
> > It is also possible to have several serial bus connectors on a single
> > device (although we've seen only one connector per device but it should not
> > be limited to that).
> 
> I've got practical systems where there are multiple buses physically
> connected, though in practice almost always only one is actually used at
> runtime when it's I2C and SPI there are some systems (usually with other
> buses) where you might want to use more than one bus.  Not sure those
> buses will fit in here though.

Yeah, I just went through DSDT table of one of our machines and found a
device that actually has two I2CSerialBus connectors (and those are to the
same controller). What I'm not sure is that is it used to select between
two different addresses or doest the device really have two physical I2C
connections.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote:
> On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote:
> > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote:
> > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote:
> > > > I've got practical systems where there are multiple buses physically
> > > > connected, though in practice almost always only one is actually used at
> > > > runtime when it's I2C and SPI there are some systems (usually with other
> > > > buses) where you might want to use more than one bus.  Not sure those
> > > > buses will fit in here though.
> > > 
> > > Yeah, I just went through DSDT table of one of our machines and found a
> > > device that actually has two I2CSerialBus connectors (and those are to the
> > > same controller). What I'm not sure is that is it used to select between
> > > two different addresses or doest the device really have two physical I2C
> > > connections.
> > 
> > Neither would make sense from a hardware perspective.
> 
> Well, interesting. :-)

It looks like some PMICs for example have two I2C control interfaces, like
TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C
controller with different address, you have the situation like above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Mon, Nov 05, 2012 at 02:20:52PM +0100, Linus Walleij wrote:
> >
> > It looks like some PMICs for example have two I2C control interfaces, like
> > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C
> > controller with different address, you have the situation like above.
> 
> This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c)
> has this. The reason is usually that the device has more than 256 registers
> to the address space simply is not big enough.
> 
> What we do is refer to the subaddresses as "banks" and they happen
> to always be at the next consecutive address so n+1.
> 
> So the same device appear behind several addresses just to support
> a lot of registers.
> 
> So you're not actually modelling the devices on the other end but the
> multiple addresses of a single device.

That makes sense. Thanks for the explanation.

> If the addresses are consecutive like ours it makes sense
> to just instantiate one device and have the driver know that it will
> also be able to access address +1, +2 ... +n. So is it possible
> to group the consecutive related addresses after each other
> here at the acpi-spi level and just create a single device?

Not sure if it should be done there, unless we really can be sure that we
have a single device with multiple addresses (and that is usually something
that is only known by the actual driver) and not some device that has two
unrelated interfaces connecting the same controller.

> If the addresses are not consecutive I guess you need to have
> one device driver bind to several devices and return -EPROBE_DEFER
> until the driver has been probed for all of them or something like
> that, this is what multi-block GPIO drivers sometimes do.

The addresses in the DSDT table for this particular device are not
consecutive so we could do something like you propose above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote:
> 
> In the ACPI namespace we have device nodes and serial interfaces below them.
> In the above case we see that a single device node supports two different
> interfaces and in that case we probably should create two different
> struct i2c_adapter objects for the same ACPI device node.
> 
> Mika, what do you think?

I agree.

Only problem I see is that then we have two I2C adapter devices with the
same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C
core thinks about that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Mon, Nov 05, 2012 at 04:19:20PM +0100, Jean Delvare wrote:
> On Mon, 5 Nov 2012 16:53:15 +0200, Mika Westerberg wrote:
> > On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote:
> > > 
> > > In the ACPI namespace we have device nodes and serial interfaces below 
> > > them.
> > > In the above case we see that a single device node supports two different
> > > interfaces and in that case we probably should create two different
> > > struct i2c_adapter objects for the same ACPI device node.
> > > 
> > > Mika, what do you think?
> > 
> > I agree.
> > 
> > Only problem I see is that then we have two I2C adapter devices with the
> > same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C
> > core thinks about that.
> 
> I2C core fears that you're mixing up everything ;) I2C adapter devices
> are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave
> devices. There's nothing wrong with i2c_clients sharing ->name, that's
> even how device driver matching is achieved. The uniqueness of
> i2c_clients is on their bus_id which is the combination of i2c adapter
> number and slave address (e.g. 0-0050)

Yeah, I mixed I2C adapter and client. Thanks for correcting.

So if we create one I2C adapter from the platform bus code as we do now and
then for each I2CSerialBus connector we create one I2C client (well, the
one that is created when i2c_new_device() is called), everything should
work, right?

Then I suggest that we have a list of serial bus resources in the struct
acpi_device and create the I2C clients based on that.

> i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we
> usually append the base I/O address at the end of the name to guarantee
> that. ACPI will have to come up with something similar.

It should already be unique in case of ACPI. We use ACPI _HID and _UID to
achieve that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Mika Westerberg]

On Mon, Nov 05, 2012 at 10:43:17AM -0700, Bjorn Helgaas wrote:
> > It should already be unique in case of ACPI. We use ACPI _HID and _UID to
> > achieve that.
> 
> Using only _HID and _UID to guarantee uniqueness means you're relying
> on a property of the BIOS, so you're vulnerable to BIOS bugs.
> 
> If there's an ACPI Device for I2C adapters, why wouldn't you just use
> its device name as set in acpi_device_register() (basically a _HID +
> instance number)?

That's a good point - we could change to use that instead (the platform
code in linux-pm tree uses _HID + _UID but I guess it is pretty trivial to
change).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] ACPI, ia64: export acpi_[un]register_gsi()

[Mika Westerberg]

These functions might be called from modules as well so make sure they are
exported.

Signed-off-by: Mika Westerberg 
---
 arch/ia64/kernel/acpi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index 44057885..bd86e39 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -633,6 +633,7 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int 
triggering, int polarity)
  ACPI_EDGE_SENSITIVE) ? IOSAPIC_EDGE :
 IOSAPIC_LEVEL);
 }
+EXPORT_SYMBOL(acpi_register_gsi);
 
 void acpi_unregister_gsi(u32 gsi)
 {
@@ -644,6 +645,7 @@ void acpi_unregister_gsi(u32 gsi)
 
iosapic_unregister_intr(gsi);
 }
+EXPORT_SYMBOL(acpi_unregister_gsi);
 
 static int __init acpi_parse_fadt(struct acpi_table_header *table)
 {
-- 
1.7.12.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] ACPI, x86: export acpi_[un]register_gsi()

[Mika Westerberg]

From: Andy Shevchenko 

These functions might be called from modules as well so make sure they are
exported. In addition we implement empty version of acpi_unregister_gsi() and
remove the one from pci_irq.c.

Signed-off-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
---
Although there are no modules that use this currently, we are working on
ACPI 5 device enumeration support that is going to use these functions and
some of that code can be compiled as a module.

 arch/x86/kernel/acpi/boot.c | 6 ++
 drivers/acpi/pci_irq.c  | 5 -
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e651f7a..2616d2b 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -574,6 +574,12 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int 
trigger, int polarity)
 
return irq;
 }
+EXPORT_SYMBOL(acpi_register_gsi);
+
+void acpi_unregister_gsi(u32 gsi)
+{
+}
+EXPORT_SYMBOL(acpi_unregister_gsi);
 
 void __init acpi_set_irq_model_pic(void)
 {
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 0eefa12..1be25a5 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -495,11 +495,6 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
return 0;
 }
 
-/* FIXME: implement x86/x86_64 version */
-void __attribute__ ((weak)) acpi_unregister_gsi(u32 i)
-{
-}
-
 void acpi_pci_irq_disable(struct pci_dev *dev)
 {
struct acpi_prt_entry *entry;
-- 
1.7.12.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] ACPI, x86: export acpi_[un]register_gsi()

[Mika Westerberg]

On Mon, Oct 29, 2012 at 12:12:10PM +0100, Ingo Molnar wrote:
> 
> * Mika Westerberg  wrote:
> 
> > From: Andy Shevchenko 
> > 
> > These functions might be called from modules as well so make sure they are
> > exported. In addition we implement empty version of acpi_unregister_gsi() 
> > and
> > remove the one from pci_irq.c.
> > 
> > Signed-off-by: Andy Shevchenko 
> > Signed-off-by: Mika Westerberg 
> > ---
> > Although there are no modules that use this currently, we are working on
> > ACPI 5 device enumeration support that is going to use these functions and
> > some of that code can be compiled as a module.
> 
> So why not submit these patches together with those patches?

Ok, we will do that. At the same time we change them to use _GPL version of
the EXPORT_SYMBOL.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4] i2c: enable runtime PM for I2C adapter devices enumerated from ACPI

[Mika Westerberg]

The ACPI specification requires the parent device to be powered on before
any of its children. It can be only powered off when all the children are
already off.

Currently whenever there is no I2C traffic going on, the I2C controller
driver can put the device into low power state transparently to its
children (the I2C client devices). This violates the ACPI specification
because now the parent device is in lower power state than its children.

In order to keep ACPI happy we enable runtime PM for the I2C adapter device
if we find out that the I2C controller was in fact an ACPI device. In
addition to that we attach the I2C client devices to the ACPI power domain
and make sure that they are powered on when the driver ->probe() is called.

Non-ACPI devices are not affected by this change.

This patch is based on the work by Aaron Lu and Lv Zheng.

Signed-off-by: Mika Westerberg 
---
Changes to v3:
 * Check only client ACPI_HANDLE().
 * Fixed bug where we attach the device to ACPI power domain multiple
   times.
 * Functions are renamed to _get_attach/_put_attach to make it clear that
   we do attach/detach as well.

 drivers/i2c/i2c-core.c | 47 ++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 29d3f04..e289b1d 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -236,6 +236,27 @@ int i2c_recover_bus(struct i2c_adapter *adap)
return adap->bus_recovery_info->recover_bus(adap);
 }
 
+static void acpi_i2c_device_pm_get_attach(struct i2c_client *client,
+ bool attach)
+{
+   if (ACPI_HANDLE(&client->dev)) {
+   /* Make sure the adapter is active */
+   pm_runtime_get_sync(&client->adapter->dev);
+   if (attach)
+   acpi_dev_pm_attach(&client->dev, true);
+   }
+}
+
+static void acpi_i2c_device_pm_put_detach(struct i2c_client *client,
+ bool detach)
+{
+   if (ACPI_HANDLE(&client->dev)) {
+   if (detach)
+   acpi_dev_pm_detach(&client->dev, true);
+   pm_runtime_put(&client->adapter->dev);
+   }
+}
+
 static int i2c_device_probe(struct device *dev)
 {
struct i2c_client   *client = i2c_verify_client(dev);
@@ -254,11 +275,15 @@ static int i2c_device_probe(struct device *dev)
client->flags & I2C_CLIENT_WAKE);
dev_dbg(dev, "probe\n");
 
+   acpi_i2c_device_pm_get_attach(client, true);
+
status = driver->probe(client, i2c_match_id(driver->id_table, client));
if (status) {
client->driver = NULL;
i2c_set_clientdata(client, NULL);
}
+
+   acpi_i2c_device_pm_put_detach(client, !!status);
return status;
 }
 
@@ -271,6 +296,8 @@ static int i2c_device_remove(struct device *dev)
if (!client || !dev->driver)
return 0;
 
+   acpi_i2c_device_pm_get_attach(client, false);
+
driver = to_i2c_driver(dev->driver);
if (driver->remove) {
dev_dbg(dev, "remove\n");
@@ -283,6 +310,8 @@ static int i2c_device_remove(struct device *dev)
client->driver = NULL;
i2c_set_clientdata(client, NULL);
}
+
+   acpi_i2c_device_pm_put_detach(client, true);
return status;
 }
 
@@ -294,8 +323,11 @@ static void i2c_device_shutdown(struct device *dev)
if (!client || !dev->driver)
return;
driver = to_i2c_driver(dev->driver);
-   if (driver->shutdown)
+   if (driver->shutdown) {
+   acpi_i2c_device_pm_get_attach(client, false);
driver->shutdown(client);
+   acpi_i2c_device_pm_put_detach(client, false);
+   }
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1263,6 +1295,16 @@ exit_recovery:
bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter);
mutex_unlock(&core_lock);
 
+   /*
+* For ACPI enumerated controllers we must make sure that the
+* controller is powered on before its children. Runtime PM handles
+* this for us once we have enabled it for the adapter device.
+*/
+   if (ACPI_HANDLE(adap->dev.parent)) {
+   pm_runtime_no_callbacks(&adap->dev);
+   pm_runtime_enable(&adap->dev);
+   }
+
return 0;
 
 out_list:
@@ -1427,6 +1469,9 @@ void i2c_del_adapter(struct i2c_adapter *adap)
return;
}
 
+   if (ACPI_HANDLE(adap->dev.parent))
+   pm_runtime_disable(&adap->dev);
+
/* Tell drivers about this removal */
mutex_lock(&core_lock);
bus_for_each_drv(&i2c_bus_type, NULL, adap,
-- 
1.8.4.rc3

--
To unsubs

[PATCH] gpio/lynxpoint: check if the interrupt is enabled in IRQ handler

[Mika Westerberg]

Checking LP_INT_STAT is not enough in the interrupt handler because its
contents get updated regardless of whether the pin has interrupt enabled or
not. This causes the driver to loop forever for GPIOs that are pulled up.

Fix this by checking the interrupt enable bit for the pin as well.

Signed-off-by: Mika Westerberg 
---
 drivers/gpio/gpio-lynxpoint.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-lynxpoint.c b/drivers/gpio/gpio-lynxpoint.c
index 2d9ca60..41b5913 100644
--- a/drivers/gpio/gpio-lynxpoint.c
+++ b/drivers/gpio/gpio-lynxpoint.c
@@ -248,14 +248,15 @@ static void lp_gpio_irq_handler(unsigned irq, struct 
irq_desc *desc)
struct lp_gpio *lg = irq_data_get_irq_handler_data(data);
struct irq_chip *chip = irq_data_get_irq_chip(data);
u32 base, pin, mask;
-   unsigned long reg, pending;
+   unsigned long reg, ena, pending;
unsigned virq;
 
/* check from GPIO controller which pin triggered the interrupt */
for (base = 0; base < lg->chip.ngpio; base += 32) {
reg = lp_gpio_reg(&lg->chip, base, LP_INT_STAT);
+   ena = lp_gpio_reg(&lg->chip, base, LP_INT_ENABLE);
 
-   while ((pending = inl(reg))) {
+   while ((pending = (inl(reg) & inl(ena {
pin = __ffs(pending);
mask = BIT(pin);
/* Clear before handling so we don't lose an edge */
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/5] gpiolib / ACPI: add ACPI support for gpiod_get_index()

[Mika Westerberg]

gpiod_get_index() and gpiod_get() are now the new preferred way to request
GPIOs. Add support for finding the corresponding GPIO descriptor from ACPI
namespace.

Signed-off-by: Mika Westerberg 
---
 drivers/gpio/gpiolib.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e2a2cdd..1e099de 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -2111,6 +2112,12 @@ static struct gpio_desc *of_find_gpio(struct device 
*dev, const char *id,
 }
 #endif
 
+static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
+   unsigned int idx, unsigned long *flags)
+{
+   return acpi_get_gpiod_by_index(dev, idx, NULL);
+}
+
 static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
unsigned int idx, unsigned long *flags)
 {
@@ -2192,6 +2199,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct 
device *dev,
if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) {
dev_dbg(dev, "using device tree for GPIO lookup\n");
desc = of_find_gpio(dev, con_id, idx, &flags);
+   } else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
+   dev_dbg(dev, "using ACPI for GPIO lookup\n");
+   desc = acpi_find_gpio(dev, con_id, idx, &flags);
} else {
dev_dbg(dev, "using lookup tables for GPIO lookup");
desc = gpiod_find(dev, con_id, idx, &flags);
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/5] gpiolib / ACPI: allow passing GPIOF_ACTIVE_LOW for GpioInt resources

[Mika Westerberg]

The ACPI GpioInt resources contain polarity field that is used to specify
whether the interrupt is active high or low. Since gpiolib supports
GPIOF_ACTIVE_LOW we can pass this information in the flags field in
acpi_find_gpio(), analogous to the DeviceTree version.

Signed-off-by: Mika Westerberg 
---
 drivers/gpio/gpiolib-acpi.c |  2 ++
 drivers/gpio/gpiolib.c  | 12 +++-
 include/linux/acpi_gpio.h   |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 333297c..4c6df41 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -251,6 +251,8 @@ static int acpi_find_gpio(struct acpi_resource *ares, void 
*data)
  agpio->pin_table[0]);
lookup->info.gpioint =
agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
+   lookup->info.active_low =
+   agpio->polarity == ACPI_ACTIVE_LOW;
}
 
return 1;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1e099de..b01a231 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2115,7 +2115,17 @@ static struct gpio_desc *of_find_gpio(struct device 
*dev, const char *id,
 static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
unsigned int idx, unsigned long *flags)
 {
-   return acpi_get_gpiod_by_index(dev, idx, NULL);
+   struct acpi_gpio_info info;
+   struct gpio_desc *desc;
+
+   desc = acpi_get_gpiod_by_index(dev, idx, &info);
+   if (IS_ERR(desc))
+   return desc;
+
+   if (info.gpioint && info.active_low)
+   *flags |= GPIOF_ACTIVE_LOW;
+
+   return desc;
 }
 
 static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
diff --git a/include/linux/acpi_gpio.h b/include/linux/acpi_gpio.h
index bbfa83a..af599ec 100644
--- a/include/linux/acpi_gpio.h
+++ b/include/linux/acpi_gpio.h
@@ -10,9 +10,11 @@
 /**
  * struct acpi_gpio_info - ACPI GPIO specific information
  * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
+ * @active_low: in case of @gpioint, the pin is active low
  */
 struct acpi_gpio_info {
bool gpioint;
+   bool active_low;
 };
 
 #ifdef CONFIG_GPIO_ACPI
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/5] gpiolib: convert ACPI GPIO helpers to gpiod_ interfaces

[Mika Westerberg]

Hi all,

This series converts the ACPI GPIO helpers in gpiolib-acpi.c to use the new
and preferred GPIO descriptor based interface as suggested by the GPIO
subsystem maintainer.

The first patch is just a cosmetic fix to move acpi_gpiochip_free_interrupts()
closer to acpi_gpiochip_request_interrupts(). Patches [2-5/5] do the actual
conversion.

The series is based on the latest patches from Alexandre Courbot and I've
tested them on Intel Haswell based machine.

Please review.

Mika Westerberg (5):
  gpiolib / ACPI: move acpi_gpiochip_free_interrupts next to the request 
function
  gpiolib / ACPI: convert to gpiod interfaces
  gpiolib / ACPI: add ACPI support for gpiod_get_index()
  gpiolib / ACPI: allow passing GPIOF_ACTIVE_LOW for GpioInt resources
  gpiolib / ACPI: document the GPIO descriptor based interface

 Documentation/acpi/enumeration.txt |  22 +++
 drivers/gpio/gpiolib-acpi.c| 129 ++---
 drivers/gpio/gpiolib.c |  20 ++
 include/linux/acpi_gpio.h  |  40 +---
 4 files changed, 138 insertions(+), 73 deletions(-)

-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/5] gpiolib / ACPI: move acpi_gpiochip_free_interrupts next to the request function

[Mika Westerberg]

It makes more sense to have these functions close to each other. No
functional changes.

Signed-off-by: Mika Westerberg 
---
 drivers/gpio/gpiolib-acpi.c | 76 ++---
 1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index f2beb72..1745ce5 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -194,6 +194,44 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip 
*chip)
 }
 EXPORT_SYMBOL(acpi_gpiochip_request_interrupts);
 
+/**
+ * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
+ * @chip:  gpio chip
+ *
+ * Free interrupts associated with the _EVT method for the given GPIO chip.
+ *
+ * The remaining ACPI event interrupts associated with the chip are freed
+ * automatically.
+ */
+void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
+{
+   acpi_handle handle;
+   acpi_status status;
+   struct list_head *evt_pins;
+   struct acpi_gpio_evt_pin *evt_pin, *ep;
+
+   if (!chip->dev || !chip->to_irq)
+   return;
+
+   handle = ACPI_HANDLE(chip->dev);
+   if (!handle)
+   return;
+
+   status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins);
+   if (ACPI_FAILURE(status))
+   return;
+
+   list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) {
+   devm_free_irq(chip->dev, evt_pin->irq, evt_pin);
+   list_del(&evt_pin->node);
+   kfree(evt_pin);
+   }
+
+   acpi_detach_data(handle, acpi_gpio_evt_dh);
+   kfree(evt_pins);
+}
+EXPORT_SYMBOL(acpi_gpiochip_free_interrupts);
+
 struct acpi_gpio_lookup {
struct acpi_gpio_info info;
int index;
@@ -271,41 +309,3 @@ int acpi_get_gpio_by_index(struct device *dev, int index,
return lookup.gpio;
 }
 EXPORT_SYMBOL_GPL(acpi_get_gpio_by_index);
-
-/**
- * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
- * @chip:  gpio chip
- *
- * Free interrupts associated with the _EVT method for the given GPIO chip.
- *
- * The remaining ACPI event interrupts associated with the chip are freed
- * automatically.
- */
-void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
-{
-   acpi_handle handle;
-   acpi_status status;
-   struct list_head *evt_pins;
-   struct acpi_gpio_evt_pin *evt_pin, *ep;
-
-   if (!chip->dev || !chip->to_irq)
-   return;
-
-   handle = ACPI_HANDLE(chip->dev);
-   if (!handle)
-   return;
-
-   status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins);
-   if (ACPI_FAILURE(status))
-   return;
-
-   list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) {
-   devm_free_irq(chip->dev, evt_pin->irq, evt_pin);
-   list_del(&evt_pin->node);
-   kfree(evt_pin);
-   }
-
-   acpi_detach_data(handle, acpi_gpio_evt_dh);
-   kfree(evt_pins);
-}
-EXPORT_SYMBOL(acpi_gpiochip_free_interrupts);
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/5] gpiolib / ACPI: document the GPIO descriptor based interface

[Mika Westerberg]

In addition to the existing ACPI specific GPIO interface, document the new
descriptor based GPIO interface in Documentation/acpi/enumeration.txt, so
it is clear that this new interface is preferred over the ACPI specific
version.

Signed-off-by: Mika Westerberg 
---
 Documentation/acpi/enumeration.txt | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/acpi/enumeration.txt 
b/Documentation/acpi/enumeration.txt
index aca4e69..48bb9ab 100644
--- a/Documentation/acpi/enumeration.txt
+++ b/Documentation/acpi/enumeration.txt
@@ -322,3 +322,25 @@ suitable to the gpiolib before passing them.
 
 In case of GpioInt resource an additional call to gpio_to_irq() must be
 done before calling request_irq().
+
+Note that the above API is ACPI specific and not recommended for drivers
+that need to support non-ACPI systems. The recommended way is to use
+the descriptor based GPIO interfaces. The above example looks like this
+when converted to the GPIO desc:
+
+   #include 
+   ...
+
+   struct gpio_desc *irq_desc, *power_desc;
+
+   irq_desc = gpiod_get_index(dev, NULL, 1);
+   if (IS_ERR(irq_desc))
+   /* handle error */
+
+   power_desc = gpiod_get_index(dev, NULL, 0);
+   if (IS_ERR(power_desc))
+   /* handle error */
+
+   /* Now we can use the GPIO descriptors */
+
+See also Documentation/gpio.txt.
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Mika Westerberg]

The new GPIO descriptor based interface is now preferred over the old
integer based one. This patch converts the ACPI GPIO helpers to use this
new interface internally. In addition to that provide compatibility
functions acpi_get_gpio() and acpi_get_gpio_by_index() that convert the
returned GPIO descriptors to integers.

Signed-off-by: Mika Westerberg 
---
 drivers/gpio/gpiolib-acpi.c | 51 +
 include/linux/acpi_gpio.h   | 38 ++---
 2 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 1745ce5..333297c 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -11,7 +11,7 @@
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -33,14 +33,15 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void 
*data)
 }
 
 /**
- * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO 
API
+ * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with 
GPIO API
  * @path:  ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
  * @pin:   ACPI GPIO pin number (0-based, controller-relative)
  *
- * Returns GPIO number to use with Linux generic GPIO API, or errno error value
+ * Returns GPIO descriptor to use with Linux generic GPIO API, or ERR_PTR
+ * error value
  */
 
-int acpi_get_gpio(char *path, int pin)
+struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 {
struct gpio_chip *chip;
acpi_handle handle;
@@ -48,18 +49,15 @@ int acpi_get_gpio(char *path, int pin)
 
status = acpi_get_handle(NULL, path, &handle);
if (ACPI_FAILURE(status))
-   return -ENODEV;
+   return ERR_PTR(-ENODEV);
 
chip = gpiochip_find(handle, acpi_gpiochip_find);
if (!chip)
-   return -ENODEV;
+   return ERR_PTR(-ENODEV);
 
-   if (!gpio_is_valid(chip->base + pin))
-   return -EINVAL;
-
-   return chip->base + pin;
+   return gpio_to_desc(chip->base + pin);
 }
-EXPORT_SYMBOL_GPL(acpi_get_gpio);
+EXPORT_SYMBOL_GPL(acpi_get_gpiod);
 
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
 {
@@ -235,7 +233,7 @@ EXPORT_SYMBOL(acpi_gpiochip_free_interrupts);
 struct acpi_gpio_lookup {
struct acpi_gpio_info info;
int index;
-   int gpio;
+   struct gpio_desc *desc;
int n;
 };
 
@@ -246,11 +244,11 @@ static int acpi_find_gpio(struct acpi_resource *ares, 
void *data)
if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
return 1;
 
-   if (lookup->n++ == lookup->index && lookup->gpio < 0) {
+   if (lookup->n++ == lookup->index && !lookup->desc) {
const struct acpi_resource_gpio *agpio = &ares->data.gpio;
 
-   lookup->gpio = acpi_get_gpio(agpio->resource_source.string_ptr,
-agpio->pin_table[0]);
+   lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
+ agpio->pin_table[0]);
lookup->info.gpioint =
agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
}
@@ -259,24 +257,24 @@ static int acpi_find_gpio(struct acpi_resource *ares, 
void *data)
 }
 
 /**
- * acpi_get_gpio_by_index() - get a GPIO number from device resources
+ * acpi_get_gpiod_by_index() - get a GPIO descriptor from device resources
  * @dev: pointer to a device to get GPIO from
  * @index: index of GpioIo/GpioInt resource (starting from %0)
  * @info: info pointer to fill in (optional)
  *
  * Function goes through ACPI resources for @dev and based on @index looks
- * up a GpioIo/GpioInt resource, translates it to the Linux GPIO number,
+ * up a GpioIo/GpioInt resource, translates it to the Linux GPIO descriptor,
  * and returns it. @index matches GpioIo/GpioInt resources only so if there
  * are total %3 GPIO resources, the index goes from %0 to %2.
  *
- * If the GPIO cannot be translated or there is an error, negative errno is
+ * If the GPIO cannot be translated or there is an error an ERR_PTR is
  * returned.
  *
  * Note: if the GPIO resource has multiple entries in the pin list, this
  * function only returns the first.
  */
-int acpi_get_gpio_by_index(struct device *dev, int index,
-  struct acpi_gpio_info *info)
+struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
+ struct acpi_gpio_info *info)
 {
struct acpi_gpio_lookup lookup;
struct list_head resource_list;
@@ -285,27 +283,26 @@ int acpi_get_gpio_by_index(struct device *dev, int index,
int ret;
 
if (!dev)
-   return -EINVAL;
+   return ERR_PTR(-EINVAL);
 
handle = ACPI_HANDLE(d

Re: [PATCH v4] i2c: enable runtime PM for I2C adapter devices enumerated from ACPI

[Mika Westerberg]

On Tue, Oct 01, 2013 at 04:09:42PM +0300, Mika Westerberg wrote:
> The ACPI specification requires the parent device to be powered on before
> any of its children. It can be only powered off when all the children are
> already off.
> 
> Currently whenever there is no I2C traffic going on, the I2C controller
> driver can put the device into low power state transparently to its
> children (the I2C client devices). This violates the ACPI specification
> because now the parent device is in lower power state than its children.
> 
> In order to keep ACPI happy we enable runtime PM for the I2C adapter device
> if we find out that the I2C controller was in fact an ACPI device. In
> addition to that we attach the I2C client devices to the ACPI power domain
> and make sure that they are powered on when the driver ->probe() is called.

It looks like Windows actually powers the I2C controller off independently
of the I2C client power state. We should probably do the same in Linux even
though it is not following what the ACPI spec says (but makes sense with
serial buses like I2C and SPI).

Wolfram, please don't apply this patch - we are going to do one more
iteration but this time we only attach devices to the ACPI power domain and
leave runtime PM alone.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] i2c-designware: configure *CNT values from ACPI

[Mika Westerberg]

Some Intel LPSS I2C devices make the *CNT values available from ACPI
namespace in similar way than the SDA hold value. These values allow
platform/BIOS to specify the most accurate *CNT values for the device
driver to use.

So change the ACPI part of the driver to use these values if they exists
and otherwise use the default method based solely on the input clock speed.

Signed-off-by: Mika Westerberg 
---
This applies on top of following patch which is not yet in i2c tree:

http://permalink.gmane.org/gmane.linux.drivers.i2c/15754

 drivers/i2c/busses/i2c-designware-platdrv.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
index c7cfdac..a1488df 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -59,7 +59,6 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
acpi_handle handle = ACPI_HANDLE(&pdev->dev);
-   acpi_string name;
 
if (!handle)
return -ENODEV;
@@ -68,14 +67,32 @@ static int dw_i2c_acpi_configure(struct platform_device 
*pdev)
dev->tx_fifo_depth = 32;
dev->rx_fifo_depth = 32;
 
-   name = dev->master_cfg & DW_IC_CON_SPEED_FAST ? "FMCN" : "SSCN";
-   if (ACPI_SUCCESS(acpi_evaluate_object(handle, name, NULL, &buf))) {
+   /* Standard speed configuration */
+   if (ACPI_SUCCESS(acpi_evaluate_object(handle, "SSCN", NULL, &buf))) {
union acpi_object *obj = (union acpi_object *)buf.pointer;
 
if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 3) {
const union acpi_object *objs = obj->package.elements;
 
-   dev->sda_hold_time = (u32)objs[2].integer.value;
+   dev->ss_hcnt = (u16)objs[0].integer.value;
+   dev->ss_lcnt = (u16)objs[1].integer.value;
+   if (!(dev->master_cfg & DW_IC_CON_SPEED_FAST))
+   dev->sda_hold_time = (u32)objs[2].integer.value;
+   }
+
+   kfree(buf.pointer);
+   }
+   /* Fast speed configuration */
+   if (ACPI_SUCCESS(acpi_evaluate_object(handle, "FMCN", NULL, &buf))) {
+   union acpi_object *obj = (union acpi_object *)buf.pointer;
+
+   if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 3) {
+   const union acpi_object *objs = obj->package.elements;
+
+   dev->fs_hcnt = (u16)objs[0].integer.value;
+   dev->fs_lcnt = (u16)objs[1].integer.value;
+   if (dev->master_cfg & DW_IC_CON_SPEED_FAST)
+   dev->sda_hold_time = (u32)objs[2].integer.value;
}
 
kfree(buf.pointer);
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

The DesignWare I2C controller has high count (HCNT) and low count (LCNT)
registers for each of the I2C speed modes (standard and fast). These
registers are programmed based on the input clock speed in the driver.

However, that is not always the most accurate way. For example on Intel
BayTrail we have 100MHz input clock and calculated *CNT values result as
measured by one of our team:

 Standard mode: 100.25kHz
 Fast mode: 315.41kHZ

The fast mode speed is over 20% lower than what is expected. It might be
that there are things like strenght of the pull-up resistors, bus
capacitance etc. that are very platform specific and have an effect on the
clock signal.

With this patch we let the platform code to specify more accurate, optimal
*CNT values if they are known beforehand.

Signed-off-by: Mika Westerberg 
---
 drivers/i2c/busses/i2c-designware-core.c | 46 +++-
 drivers/i2c/busses/i2c-designware-core.h | 12 +
 2 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index ad46616..f31469e 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -308,29 +308,39 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
/* set standard and fast speed deviders for high/low periods */
 
/* Standard-mode */
-   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
-   40, /* tHD;STA = tHIGH = 4.0 us */
-   3,  /* tf = 0.3 us */
-   0,  /* 0: DW default, 1: Ideal */
-   0); /* No offset */
-   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
-   47, /* tLOW = 4.7 us */
-   3,  /* tf = 0.3 us */
-   0); /* No offset */
+   if (dev->ss_hcnt && dev->ss_lcnt) {
+   hcnt = dev->ss_hcnt;
+   lcnt = dev->ss_lcnt;
+   } else {
+   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+   40, /* tHD;STA = tHIGH = 4.0 us */
+   3,  /* tf = 0.3 us */
+   0,  /* 0: DW default, 1: Ideal */
+   0); /* No offset */
+   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+   47, /* tLOW = 4.7 us */
+   3,  /* tf = 0.3 us */
+   0); /* No offset */
+   }
dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
/* Fast-mode */
-   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
-   6,  /* tHD;STA = tHIGH = 0.6 us */
-   3,  /* tf = 0.3 us */
-   0,  /* 0: DW default, 1: Ideal */
-   0); /* No offset */
-   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
-   13, /* tLOW = 1.3 us */
-   3,  /* tf = 0.3 us */
-   0); /* No offset */
+   if (dev->fs_hcnt && dev->fs_lcnt) {
+   hcnt = dev->fs_hcnt;
+   lcnt = dev->fs_lcnt;
+   } else {
+   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+   6,  /* tHD;STA = tHIGH = 0.6 us */
+   3,  /* tf = 0.3 us */
+   0,  /* 0: DW default, 1: Ideal */
+   0); /* No offset */
+   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+   13, /* tLOW = 1.3 us */
+   3,  /* tf = 0.3 us */
+   0); /* No offset */
+   }
dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
diff --git a/drivers/i2c/busses/i2c-designware-core.h 
b/drivers/i2c/busses/i2c-designware-core.h
index 912aa22..e8a7565 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -61,6 +61,14 @@
  * @tx_fifo_depth: depth of the hardware tx fifo
  * @rx_fifo_depth: depth of the hardware rx fifo
  * @rx_outstanding: current master-rx elements in tx fifo
+ * @ss_hcnt: standard speed HCNT value
+ * @ss_lcnt: standard speed LCNT value
+ * @fs_hcnt: fast speed HCNT value
+ * @fs_lcnt: fast speed LCNT value
+ *
+ * HCNT and LCNT parameters can be used if the platform knows more

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Mon, Jul 08, 2013 at 03:42:17PM +0200, Christian Ruppert wrote:
> On Mon, Jul 08, 2013 at 02:45:26PM +0300, Mika Westerberg wrote:
> > The DesignWare I2C controller has high count (HCNT) and low count (LCNT)
> > registers for each of the I2C speed modes (standard and fast). These
> > registers are programmed based on the input clock speed in the driver.
> > 
> > However, that is not always the most accurate way. For example on Intel
> > BayTrail we have 100MHz input clock and calculated *CNT values result as
> > measured by one of our team:
> > 
> >  Standard mode: 100.25kHz
> >  Fast mode: 315.41kHZ
> > 
> > The fast mode speed is over 20% lower than what is expected.
> 
> We have observed similar issues and I am wondering if this effect is due
> to the overly-pessimistic formulas in i2c_dw_scl_lcnt and
> i2c_dw_scl_hcnt. The comments in these functions are a bit confusing and
> I don't pretend having fully understood what the intention is. I'm
> having the impression that defining the arguments tf in function of the
> hardware would be the "correct" way to achieve better clock accuracy.

Well, that's not the only timing parameter specified in the I2C spec. We
also have tr among other things (even though the dw driver doesn't use it).

> > It might be
> > that there are things like strenght of the pull-up resistors, bus
> > capacitance etc. that are very platform specific and have an effect on the
> > clock signal.
> 
> I believe tf is supposed to model these things. I just haven't clearly
> understood how. In my understanding, transition times should be
> subtracted rather than added to cycle times or am I getting something
> fundamentally wrong here?

I'm not sure, honestly :-) I believe tf is the same tf that is in the I2C
spec, meaning fall time of both SCL and SDA signals and the spec measures
one clock cycle to be from end of the first tf to the end of the second
(fig. 27 in the I2C spec). If I'm reading it right then it means adding
instead of substracting.

Do you have any suggestions how we could use tf here? At least on Intel
hardware we have an ACPI method that returns directly the optimum *CNT
values.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 4/8] ACPI / hotplug / PCI: Hotplug context objects for bridges and functions

[Mika Westerberg]

On Tue, Jul 09, 2013 at 02:18:12AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> When either a new hotplug brigde or a new hotplug function is added
^^
typo

> by the ACPI-based PCI hotplug (acpiphp) code, attach a context object
> to its ACPI handle to store hotplug-related information in it.  To
> start with, put the handle's bridge and function pointers into that
> object.  Count references to the context objects and drop them when
> they are not needed any more.
> 
> First of all, this makes it possible to find out if the given bridge
> has been registered as a function already in a much more
> straightforward way and acpiphp_bridge_handle_to_function() can be
> dropped (Yay!).
>
> This also will allow some more simplifications to be made going
> forward.
> 
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/pci/hotplug/acpiphp.h  |   10 ++
>  drivers/pci/hotplug/acpiphp_glue.c |  154 
> ++---
>  2 files changed, 121 insertions(+), 43 deletions(-)
> 
> Index: linux-pm/drivers/pci/hotplug/acpiphp.h
> ===
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp.h
> +++ linux-pm/drivers/pci/hotplug/acpiphp.h
> @@ -49,6 +49,7 @@
>  #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME , ## 
> arg)
>  #define warn(format, arg...) printk(KERN_WARNING "%s: " format, MY_NAME , ## 
> arg)
>  
> +struct acpiphp_context;
>  struct acpiphp_bridge;
>  struct acpiphp_slot;
>  
> @@ -77,6 +78,7 @@ struct acpiphp_bridge {
>   struct kref ref;
>   acpi_handle handle;
>  
> + struct acpiphp_context *context;
>   /* Ejectable PCI-to-PCI bridge (PCI bridge and PCI function) */
>   struct acpiphp_func *func;
>  
> @@ -119,6 +121,7 @@ struct acpiphp_slot {
>   * typically 8 objects per slot (i.e. for each PCI function)
>   */
>  struct acpiphp_func {
> + struct acpiphp_context *context;
>   struct acpiphp_slot *slot;  /* parent */
>  
>   struct list_head sibling;
> @@ -129,6 +132,13 @@ struct acpiphp_func {
>   u32 flags;  /* see below */
>  };
>  
> +struct acpiphp_context {
> + struct kref kref;
> + acpi_handle handle;
> + struct acpiphp_func *func;
> + struct acpiphp_bridge *bridge;
> +};
> +
>  /*
>   * struct acpiphp_attention_info - device specific attention registration
>   *
> Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> ===
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> @@ -79,6 +79,61 @@ is_pci_dock_device(acpi_handle handle, u
>   }
>  }
>  
> +static void acpiphp_context_handler(acpi_handle handle, void *context)
> +{
> + /* Intentionally empty. */
> +}
> +
> +static struct acpiphp_context *acpiphp_init_context(acpi_handle handle)
> +{
> + struct acpiphp_context *context;
> + acpi_status status;
> +
> + context = kzalloc(sizeof(*context), GFP_KERNEL);
> + if (!context)
> + return NULL;
> +
> + context->handle = handle;
> + kref_init(&context->kref);
> + status = acpi_attach_data(handle, acpiphp_context_handler, context);
> + if (ACPI_FAILURE(status)) {
> + kfree(context);
> + return NULL;
> + }
> + return context;
> +}
> +
> +static struct acpiphp_context *acpiphp_get_context(acpi_handle handle)
> +{
> + struct acpiphp_context *context = NULL;
> + acpi_status status;
> + void *data;
> +
> + status = acpi_get_data(handle, acpiphp_context_handler, &data);
> + if (ACPI_SUCCESS(status)) {
> + context = data;
> + kref_get(&context->kref);
> + } else if (status == AE_NOT_FOUND) {
> + context = acpiphp_init_context(handle);
> + }
> + return context;
> +}
> +
> +static void acpiphp_release_context(struct kref *kref)
> +{
> + struct acpiphp_context *context;
> +
> + context = container_of(kref, struct acpiphp_context, kref);
> + WARN_ON(context->func || context->bridge);
> + acpi_detach_data(context->handle, acpiphp_context_handler);
> + kfree(context);
> +}
> +
> +static void acpiphp_put_context(struct acpiphp_context *context)
> +{
> + kref_put(&context->kref, acpiphp_release_context);
> +}
> +
>  static inline void get_bridge(struct acpiphp_bridge *bridge)
>  {
>   kref_get(&bridge->ref);
> @@ -91,6 +146,7 @@ static inline void put_bridge(struct acp
>  
>  static void free_bridge(struct kref *kref)
>  {
> + struct acpiphp_context *context;
>   struct acpiphp_bridge *bridge;
>   struct acpiphp_slot *slot, *next;
>   struct acpiphp_func *func, *tmp;
> @@ -99,17 +155,24 @@ static void free_bridge(struct kref *kre
>  
>   list_for_each_entry_safe(slot, next, &bridge->slots, node) {
>   list_for_each_entry_safe(func, tmp, &slot

Re: [RFC][PATCH 5/8] ACPI / hotplug / PCI: Unified notify handler for hotplug events

[Mika Westerberg]

On Tue, Jul 09, 2013 at 02:19:04AM +0200, Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/pci/hotplug/acpiphp.h
> ===
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp.h
> +++ linux-pm/drivers/pci/hotplug/acpiphp.h
> @@ -137,6 +137,7 @@ struct acpiphp_context {
>   acpi_handle handle;
>   struct acpiphp_func *func;
>   struct acpiphp_bridge *bridge;
> + bool handler_for_func:1;

Hmm, should it be just plain:

bool handler_for_func;

? What's the reason using bitfields for bool?

>  };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 6/8] ACPI / hotplug / PCI: Drop acpiphp_handle_to_bridge()

[Mika Westerberg]

On Tue, Jul 09, 2013 at 02:20:31AM +0200, Rafael J. Wysocki wrote:
> @@ -953,37 +937,49 @@ static void acpiphp_sanitize_bus(struct
>   * ACPI event handlers
>   */
>  
> -static acpi_status
> -check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
> +static acpi_status check_sub_bridges(acpi_handle handle, u32 lvl, void *data,
> +  void **rv)
>  {
> - struct acpiphp_bridge *bridge;
> - char objname[64];
> - struct acpi_buffer buffer = { .length = sizeof(objname),
> -   .pointer = objname };
> + struct acpiphp_context *context = acpiphp_get_context(handle);
> +
> + if (!context)
> + return AE_OK;
>  
> - bridge = acpiphp_handle_to_bridge(handle);
> - if (bridge) {
> + if (context->bridge) {
> + struct acpiphp_bridge *bridge = context->bridge;
> + char objname[64];
> + struct acpi_buffer buffer = { .length = sizeof(objname),
> +   .pointer = objname };
> +
> + get_bridge(bridge);
>   acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
> - dbg("%s: re-enumerating slots under %s\n",
> - __func__, objname);
> + dbg("%s: re-enumerating slots under %s\n", __func__, objname);

Although not related to this patch directly, how about using
acpi_handle_debug() or similar here?

>   acpiphp_check_bridge(bridge);
>   put_bridge(bridge);
>   }
> + acpiphp_put_context(context);
>   return AE_OK ;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> What I meant is the following: The clock cycle time Tc is composed of
> the four components
> 
>   Tc = Th + Tf + Tl + Tr
> 
> where
>   Th: Time during which the signal is high
>   Tf: Falling edge transition time
>   Tl: Time during which the signal is low
>   Tr: Rising edge transition time
> 
> The I2C specification specifies a minimum for Tl and Th and a range (or
> maximum) for Tr and Tf. A maximum frequency is specified as the
> frequency obtained by adding the minima for Th and Tl to the maxima of
> Tr ant Tf.
> Since as you said, transition times are very much PCB dependent, one way
> to guarantee the max. frequency constraint (and to achieve a constant
> frequency at its max) is to define the constants
> Th' = Th + Tf := Th_min + Tf_max
> Tl' = Tl + Tr := Tl_min + Tr_max
> 
> and to calculate the variables
> Th = Th' - Tf
> Tl = Tl' - Tr
> in function of Tf and Tr of the given PCB.

If I understand the above, it leaves Tf and Tr to be PCB specific and then
these values are passed to the core driver from platform data, right?

I'm thinking that passing directly the "measured" *CNT values from the
platform data makes this even more accurate (given that information is
available). And if you only have the Tf and Tr for your board, you can have
custom calculation done in the platform part of the driver that takes them
into account, and then passes these custom *CNT values to the core.

> > At least on Intel
> > hardware we have an ACPI method that returns directly the optimum *CNT
> > values.
> 
> I don't know ACPI very well and if it deals with register values
> directly your patch is probably the right thing.

Our FW people decided to have a custom ACPI method that returns the correct
values to be used in the Windows I2C driver. It returns direct *CNT
register values that have been measured to be good for a given PCB.

> The timing calculation in the driver seems a bit strange to me, however
> (see above), but I never dared touching it because I never had time to
> dive into the code deep enough and I was just wondering if it was
> possible to fix it at the same time.

It would be good if we can fix this for non-ACPI devices as well. Is it
hard to add similar properties to the driver specific Device Tree bindings?

Wolfram, do you have any input on this?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] i2c-designware: configure *CNT values from ACPI

[Mika Westerberg]

On Mon, Jul 08, 2013 at 02:45:27PM +0300, Mika Westerberg wrote:
> Some Intel LPSS I2C devices make the *CNT values available from ACPI
> namespace in similar way than the SDA hold value. These values allow
> platform/BIOS to specify the most accurate *CNT values for the device
> driver to use.
> 
> So change the ACPI part of the driver to use these values if they exists
> and otherwise use the default method based solely on the input clock speed.
> 
> Signed-off-by: Mika Westerberg 
> ---
> This applies on top of following patch which is not yet in i2c tree:
> 
> http://permalink.gmane.org/gmane.linux.drivers.i2c/15754
> 
>  drivers/i2c/busses/i2c-designware-platdrv.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index c7cfdac..a1488df 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -59,7 +59,6 @@ static int dw_i2c_acpi_configure(struct platform_device 
> *pdev)
>   struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
>   struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
>   acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> - acpi_string name;
>  
>   if (!handle)
>   return -ENODEV;
> @@ -68,14 +67,32 @@ static int dw_i2c_acpi_configure(struct platform_device 
> *pdev)
>   dev->tx_fifo_depth = 32;
>   dev->rx_fifo_depth = 32;
>  
> - name = dev->master_cfg & DW_IC_CON_SPEED_FAST ? "FMCN" : "SSCN";
> - if (ACPI_SUCCESS(acpi_evaluate_object(handle, name, NULL, &buf))) {
> + /* Standard speed configuration */
> + if (ACPI_SUCCESS(acpi_evaluate_object(handle, "SSCN", NULL, &buf))) {
>   union acpi_object *obj = (union acpi_object *)buf.pointer;
>  
>   if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 3) {
>   const union acpi_object *objs = obj->package.elements;
>  
> - dev->sda_hold_time = (u32)objs[2].integer.value;
> + dev->ss_hcnt = (u16)objs[0].integer.value;
> + dev->ss_lcnt = (u16)objs[1].integer.value;
> + if (!(dev->master_cfg & DW_IC_CON_SPEED_FAST))
> + dev->sda_hold_time = (u32)objs[2].integer.value;
> + }
> +
> + kfree(buf.pointer);
> + }
> + /* Fast speed configuration */
> + if (ACPI_SUCCESS(acpi_evaluate_object(handle, "FMCN", NULL, &buf))) {

There is a problem with the above as it reuses buf from the previous call.
I'll post a new revision of the patches with this fixed.

> + union acpi_object *obj = (union acpi_object *)buf.pointer;
> +
> + if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 3) {
> + const union acpi_object *objs = obj->package.elements;
> +
> + dev->fs_hcnt = (u16)objs[0].integer.value;
> + dev->fs_lcnt = (u16)objs[1].integer.value;
> + if (dev->master_cfg & DW_IC_CON_SPEED_FAST)
> + dev->sda_hold_time = (u32)objs[2].integer.value;
>   }
>  
>   kfree(buf.pointer);
> -- 
> 1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] i2c-designware: configure SDA hold time from ACPI

[Mika Westerberg]

On Wed, Jul 03, 2013 at 12:52:40PM +0300, Mika Westerberg wrote:
> Some Intel LPSS I2C devices make the SDA hold time parameter available via
> SSCN (standard mode) and FMCN (fast mode) ACPI methods. If we find that
> such method exist, we evaluate it and pass the returned SDA hold value to
> the i2c-designware core analogous to Device Tree.
> 
> Signed-off-by: Mika Westerberg 

Please disregard this patch. I'll send a new one that combines both setting of
SDA hold time and *CNT values from ACPI.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
> On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote:
> > On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> > > What I meant is the following: The clock cycle time Tc is composed of
> > > the four components
> > > 
> > >   Tc = Th + Tf + Tl + Tr
> > > 
> > > where
> > >   Th: Time during which the signal is high
> > >   Tf: Falling edge transition time
> > >   Tl: Time during which the signal is low
> > >   Tr: Rising edge transition time
> > > 
> > > The I2C specification specifies a minimum for Tl and Th and a range (or
> > > maximum) for Tr and Tf. A maximum frequency is specified as the
> > > frequency obtained by adding the minima for Th and Tl to the maxima of
> > > Tr ant Tf.
> > > Since as you said, transition times are very much PCB dependent, one way
> > > to guarantee the max. frequency constraint (and to achieve a constant
> > > frequency at its max) is to define the constants
> > > Th' = Th + Tf := Th_min + Tf_max
> > > Tl' = Tl + Tr := Tl_min + Tr_max
> > > 
> > > and to calculate the variables
> > > Th = Th' - Tf
> > > Tl = Tl' - Tr
> > > in function of Tf and Tr of the given PCB.
> > 
> > If I understand the above, it leaves Tf and Tr to be PCB specific and then
> > these values are passed to the core driver from platform data, right?
> 
> That would be the idea: Calculate Th' and Tl' in function of the desired
> clock frequency and duty cycle and then adapt these values using
> measured transition times. What prevented me from implementing this
> rather academic approach are the following comments in
> i2c-designware-core.c:
> 
> /*
>  * DesignWare I2C core doesn't seem to have solid strategy to meet
>  * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
>  * will result in violation of the tHD;STA spec.
>  */
> 
> /* ...
>  * This is just experimental rule; the tHD;STA period
>  * turned out to be proportinal to (_HCNT + 3).  With this setting,
>  * we could meet both tHIGH and tHD;STA timing specs.
>  * ...
>  */
> 
> If I interpret this right, the slow down of the clock is intentional to
> meet tHD;STA timing constraints.

Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above
comments apply to some earlier version of the IP that didn't have the SDA
hold register?

> > I'm thinking that passing directly the "measured" *CNT values from the
> > platform data makes this even more accurate (given that information is
> > available). And if you only have the Tf and Tr for your board, you can have
> > custom calculation done in the platform part of the driver that takes them
> > into account, and then passes these custom *CNT values to the core.
> 
> Well, as in the previous discussion on SDA hold time, Tf and Tr are
> physical values whereas the register values are clock dependent and
> probably not appropriate at least for device tree usage (or cases where
> the clock speed might change). If I understand you correctly, ACPI is
> more register oriented and able to cope with this outside of the OS?

Well, ACPI doesn't care what its methods return (unless the methods are
specfied in the ACPI spec). In this case it just returns a package of three
values: HCNT, LCNT and SDA hold time.

I guess it is supposed to work like this:

 * If there is no vendor specific SSCN/FMCN methods for the device we can
   just use the default and calculate *CNT from the clock speed.
 * If there exists a method, we use the values there instead.

I think the Windows driver does something like above. IMHO we should to the
same in Linux driver to be able to take advantage of the measured *CNT
values.

> > > > At least on Intel
> > > > hardware we have an ACPI method that returns directly the optimum *CNT
> > > > values.
> > > 
> > > I don't know ACPI very well and if it deals with register values
> > > directly your patch is probably the right thing.
> > 
> > Our FW people decided to have a custom ACPI method that returns the correct
> > values to be used in the Windows I2C driver. It returns direct *CNT
> > register values that have been measured to be good for a given PCB.
> > 
> > > The timing calculation in the driver seems a bit strange to me, however
> > > (see above), but I never dared touching it because I never had time to
> > > dive into the code deep enough and I was just wondering if it was
> > > possible to fix it at the sam

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote:
> On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
> > On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote:
> > > On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> > > > What I meant is the following: The clock cycle time Tc is composed of
> > > > the four components
> > > > 
> > > >   Tc = Th + Tf + Tl + Tr
> > > > 
> > > > where
> > > >   Th: Time during which the signal is high
> > > >   Tf: Falling edge transition time
> > > >   Tl: Time during which the signal is low
> > > >   Tr: Rising edge transition time
> > > > 
> > > > The I2C specification specifies a minimum for Tl and Th and a range (or
> > > > maximum) for Tr and Tf. A maximum frequency is specified as the
> > > > frequency obtained by adding the minima for Th and Tl to the maxima of
> > > > Tr ant Tf.
> > > > Since as you said, transition times are very much PCB dependent, one way
> > > > to guarantee the max. frequency constraint (and to achieve a constant
> > > > frequency at its max) is to define the constants
> > > > Th' = Th + Tf := Th_min + Tf_max
> > > > Tl' = Tl + Tr := Tl_min + Tr_max
> > > > 
> > > > and to calculate the variables
> > > > Th = Th' - Tf
> > > > Tl = Tl' - Tr
> > > > in function of Tf and Tr of the given PCB.
> > > 
> > > If I understand the above, it leaves Tf and Tr to be PCB specific and then
> > > these values are passed to the core driver from platform data, right?
> > 
> > That would be the idea: Calculate Th' and Tl' in function of the desired
> > clock frequency and duty cycle and then adapt these values using
> > measured transition times. What prevented me from implementing this
> > rather academic approach are the following comments in
> > i2c-designware-core.c:
> > 
> > /*
> >  * DesignWare I2C core doesn't seem to have solid strategy to meet
> >  * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
> >  * will result in violation of the tHD;STA spec.
> >  */
> > 
> > /* ...
> >  * This is just experimental rule; the tHD;STA period
> >  * turned out to be proportinal to (_HCNT + 3).  With this setting,
> >  * we could meet both tHIGH and tHD;STA timing specs.
> >  * ...
> >  */
> > 
> > If I interpret this right, the slow down of the clock is intentional to
> > meet tHD;STA timing constraints.
> 
> Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above
> comments apply to some earlier version of the IP that didn't have the SDA
> hold register?

Scratch that.

I re-read the spec and tHD;STA is hold time for (repeated) start. There is
a constraint that says that the device must internally provide a hold time
of at least 300ns for the SDA signal. Maybe that's the constraint the
comment above is referring to?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] i2c-designware: make *CNT values configurable

[Mika Westerberg]

On Fri, Jul 12, 2013 at 04:56:49PM +0900, Shinya Kuribayashi wrote:
> On 7/11/13 7:13 PM, Mika Westerberg wrote:
> >On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote:
> >>On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote:
> >>>On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote:
> >>>>On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote:
> >>>>>What I meant is the following: The clock cycle time Tc is composed of
> >>>>>the four components
> >>>>>
> >>>>>   Tc = Th + Tf + Tl + Tr
> >>>>>
> >>>>>where
> >>>>>   Th: Time during which the signal is high
> >>>>>   Tf: Falling edge transition time
> >>>>>   Tl: Time during which the signal is low
> >>>>>   Tr: Rising edge transition time
> >>>>>
> >>>>>The I2C specification specifies a minimum for Tl and Th and a range (or
> >>>>>maximum) for Tr and Tf. A maximum frequency is specified as the
> >>>>>frequency obtained by adding the minima for Th and Tl to the maxima of
> >>>>>Tr ant Tf.
> >>>>>Since as you said, transition times are very much PCB dependent, one way
> >>>>>to guarantee the max. frequency constraint (and to achieve a constant
> >>>>>frequency at its max) is to define the constants
> >>>>>Th' = Th + Tf := Th_min + Tf_max
> >>>>>Tl' = Tl + Tr := Tl_min + Tr_max
> >>>>>
> >>>>>and to calculate the variables
> >>>>>Th = Th' - Tf
> >>>>>Tl = Tl' - Tr
> >>>>>in function of Tf and Tr of the given PCB.
> >>>>
> >>>>If I understand the above, it leaves Tf and Tr to be PCB specific and then
> >>>>these values are passed to the core driver from platform data, right?
> >>>
> >>>That would be the idea: Calculate Th' and Tl' in function of the desired
> >>>clock frequency and duty cycle and then adapt these values using
> >>>measured transition times. What prevented me from implementing this
> >>>rather academic approach are the following comments in
> >>>i2c-designware-core.c:
> 
> When we talk about I2C timing specs, we should not bring up "clock speed"
> things.  All we have to do is to strictly meet timing constraints of
> tHIGH, tLOW, tHD;SATA, tr, tf, etc.  The resulting "clock speed" is not
> a goal.

OK, thanks for the explanation.

I'm relatively new with I2C bus even though I've adapted the DW I2C driver
to work on our HW.

> >>>/*
> >>>  * DesignWare I2C core doesn't seem to have solid strategy to meet
> >>>  * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
> >>>  * will result in violation of the tHD;STA spec.
> >>>  */
> >>>
> >>>/* ...
> >>>  * This is just experimental rule; the tHD;STA period
> >>>  * turned out to be proportinal to (_HCNT + 3).  With this setting,
> >>>  * we could meet both tHIGH and tHD;STA timing specs.
> >>>  * ...
> >>>  */
> >>>
> >>>If I interpret this right, the slow down of the clock is intentional to
> >>>meet tHD;STA timing constraints.
> 
> Correct.
> 
> >>Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above
> >>comments apply to some earlier version of the IP that didn't have the SDA
> >>hold register?
> 
> If I remember DesignWare APB I2C spec correctly, SDA hold time register
> doesn't help to meet tHD;STA spec.  Could someone confirm it really so
> with a real hardware, please?

Indeed, SDA hold in the DesignWare I2C is not the same as tHD;STA according
the databook. Unfortunately I don't have means to actually measure that
here.

Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN
methods are measured by our HW guys on a certain board and they have
verified that using those we meet all the I2C timing specs.

In order to take advantage of those we need some way to pass those values
to the i2c designware core. I have two suggestions:
  
  - Use the method outlined in this patch and let the interface driver
override *CNT values.

  - Allow interface drivers to override the function that does calculations
for these values like:

if (dev->std_scl_cnt)
dev->std_scl_cnt(dev, &hcnt, &lcnt);
else
/* Fallback to the calculation based solely on iclk */

Any comments on these?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 11/30] ACPI / hotplug / PCI: Register all devices under the given bridge

[Mika Westerberg]

On Fri, Jul 12, 2013 at 01:50:29AM +0200, Rafael J. Wysocki wrote:
> @@ -1210,6 +1125,35 @@ void acpiphp_enumerate_slots(struct pci_
>*/
>   get_device(&bus->dev);
>  
> + if (!pci_is_root_bus(bridge->pci_bus)) {
> + struct acpiphp_context *context;
> +
> + /*
> +  * This bridge should have been registered as a hotplug function
> +  * under its parent, so the context has to be there.  If not, we
> +  * are in deep goo.
> +  */
> + mutex_lock(&acpiphp_context_lock);
> + context = acpiphp_get_context(handle);
> + if (WARN_ON(!context || !context->func)) {
> + mutex_unlock(&acpiphp_context_lock);
> + put_device(&bus->dev);
> + kfree(bridge);
> + return;
> + }
> + bridge->context = context;
> + context->bridge = bridge;
> + /* Get a reference to the parent bridge. */
> + get_bridge(context->func->slot->bridge);
> + mutex_unlock(&acpiphp_context_lock);
> + }
> +
> + status = acpi_get_handle(bridge->handle, "_EJ0", &handle);
> + if (ACPI_SUCCESS(status)) {
> + dbg("found ejectable p2p bridge\n");
> + bridge->flags |= BRIDGE_HAS_EJ0;
> + }
> +
>   /* must be added to the list prior to calling register_slot */
>   mutex_lock(&bridge_mutex);
>   list_add(&bridge->list, &bridge_list);
> @@ -1220,34 +1164,9 @@ void acpiphp_enumerate_slots(struct pci_
>register_slot, NULL, bridge, NULL);
>   if (ACPI_FAILURE(status)) {
>   acpi_handle_err(bridge->handle, "failed to register slots\n");
> - goto err;
> - }
> -
> - if (pci_is_root_bus(bridge->pci_bus))
> - return;
> -
> - status = acpi_get_handle(bridge->handle, "_EJ0", &handle);
> - if (ACPI_SUCCESS(status)) {
> - dbg("found ejectable p2p bridge\n");
> - bridge->flags |= BRIDGE_HAS_EJ0;
> - }
> - if (context->handler_for_func) {
> - /* Notify handler already installed. */
> - get_bridge(context->func->slot->bridge);
> - return;
> + cleanup_bridge(bridge);
> + put_bridge(bridge);
>   }
> -
> - /* install notify handler for P2P bridges */
> - status = acpi_install_notify_handler(bridge->handle, ACPI_SYSTEM_NOTIFY,
> -  handle_hotplug_event, NULL);

With this removed we now install notify handlers for the slot objects under
this bridge but not for the bridge itself.

In case of Thunderbolt the BIOS will send the event for the root port ACPI
device \_SB_.PCI0.RP05 and with the above change we don't handle it
anymore.

> - if (ACPI_SUCCESS(status))
> - return;
> -
> - acpi_handle_err(bridge->handle, "failed to register notify handler\n");
> -
> - err:
> - cleanup_bridge(bridge);
> - put_bridge(bridge);
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 11/30] ACPI / hotplug / PCI: Register all devices under the given bridge

[Mika Westerberg]

On Fri, Jul 12, 2013 at 02:54:20PM +0300, Mika Westerberg wrote:
> On Fri, Jul 12, 2013 at 01:50:29AM +0200, Rafael J. Wysocki wrote:
> > @@ -1210,6 +1125,35 @@ void acpiphp_enumerate_slots(struct pci_
> >  */
> > get_device(&bus->dev);
> >  
> > +   if (!pci_is_root_bus(bridge->pci_bus)) {
> > +   struct acpiphp_context *context;
> > +
> > +   /*
> > +* This bridge should have been registered as a hotplug function
> > +* under its parent, so the context has to be there.  If not, we
> > +* are in deep goo.
> > +*/
> > +   mutex_lock(&acpiphp_context_lock);
> > +   context = acpiphp_get_context(handle);
> > +   if (WARN_ON(!context || !context->func)) {
> > +   mutex_unlock(&acpiphp_context_lock);
> > +   put_device(&bus->dev);
> > +   kfree(bridge);
> > +   return;
> > +   }
> > +   bridge->context = context;
> > +   context->bridge = bridge;
> > +   /* Get a reference to the parent bridge. */
> > +   get_bridge(context->func->slot->bridge);
> > +   mutex_unlock(&acpiphp_context_lock);
> > +   }
> > +
> > +   status = acpi_get_handle(bridge->handle, "_EJ0", &handle);
> > +   if (ACPI_SUCCESS(status)) {
> > +   dbg("found ejectable p2p bridge\n");
> > +   bridge->flags |= BRIDGE_HAS_EJ0;
> > +   }
> > +
> > /* must be added to the list prior to calling register_slot */
> > mutex_lock(&bridge_mutex);
> > list_add(&bridge->list, &bridge_list);
> > @@ -1220,34 +1164,9 @@ void acpiphp_enumerate_slots(struct pci_
> >  register_slot, NULL, bridge, NULL);
> > if (ACPI_FAILURE(status)) {
> > acpi_handle_err(bridge->handle, "failed to register slots\n");
> > -   goto err;
> > -   }
> > -
> > -   if (pci_is_root_bus(bridge->pci_bus))
> > -   return;
> > -
> > -   status = acpi_get_handle(bridge->handle, "_EJ0", &handle);
> > -   if (ACPI_SUCCESS(status)) {
> > -   dbg("found ejectable p2p bridge\n");
> > -   bridge->flags |= BRIDGE_HAS_EJ0;
> > -   }
> > -   if (context->handler_for_func) {
> > -   /* Notify handler already installed. */
> > -   get_bridge(context->func->slot->bridge);
> > -   return;
> > +   cleanup_bridge(bridge);
> > +   put_bridge(bridge);
> > }
> > -
> > -   /* install notify handler for P2P bridges */
> > -   status = acpi_install_notify_handler(bridge->handle, ACPI_SYSTEM_NOTIFY,
> > -handle_hotplug_event, NULL);
> 
> With this removed we now install notify handlers for the slot objects under
> this bridge but not for the bridge itself.
> 
> In case of Thunderbolt the BIOS will send the event for the root port ACPI
> device \_SB_.PCI0.RP05 and with the above change we don't handle it
> anymore.

Actually there was a bug in patch [23/30] that caused this. Once that is
fixed I can see that the notify handlers get correctly installed. I'll
comment on that patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 23/30] ACPI / hotplug / PCI: Do not exectute _PS0 and _PS3 directly

[Mika Westerberg]

On Fri, Jul 12, 2013 at 02:01:30AM +0200, Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/pci/hotplug/acpiphp.h
> ===
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp.h
> +++ linux-pm/drivers/pci/hotplug/acpiphp.h
> @@ -160,7 +160,6 @@ struct acpiphp_attention_info
>  
>  /* slot flags */
>  
> -#define SLOT_POWEREDON   (0x0001)
>  #define SLOT_ENABLED (0x0002)
>  #define SLOT_MULTIFUNCTION   (0x0004)
>  
> @@ -168,11 +167,7 @@ struct acpiphp_attention_info
>  
>  #define FUNC_HAS_STA (0x0001)
>  #define FUNC_HAS_EJ0 (0x0002)
> -#define FUNC_HAS_PS0 (0x0010)
> -#define FUNC_HAS_PS1 (0x0020)
> -#define FUNC_HAS_PS2 (0x0040)
> -#define FUNC_HAS_PS3 (0x0080)
> -#define FUNC_HAS_DCK(0x0100)
> +#define FUNC_HAS_DCK(0x0003)

These are flags not enum so the above wants to be

#define FUNC_HAS_DCK(0x0004)

otherwise we accidentally match checks like:

/* install notify handler */
if (!(newfunc->flags & FUNC_HAS_DCK)) {
...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 0/30] ACPI / hotplug / PCI: Major rework + Thunderbolt workarounds

[Mika Westerberg]

On Fri, Jul 12, 2013 at 01:34:20AM +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> I've made some progress with my ACPIPHP rework since I posted the series last
> time and here goes an update.
> 
> First off, the previous series was somewhat racy, which should be fixed now.
> Apart from this there's quite some new material on top of the patches I posted
> last time (or rather on top of their new versions) and I integrated the
> Thunderbolt series from Mika with that.  As a result,
> 
> https://patchwork.kernel.org/patch/2817341/
> 
> is required to be applied.

With the above mentioned patch applied + fix for patch [23/30], I tested
this series on Acer Aspire S5 and Intel DZ77RE-75K desktop and Thunderbolt
works just fine :-)

You can add

Tested-by: Mika Westerberg 

to the series.

Nice cleanup!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] PCI / hotplug / ACPI: Get rid of check_sub_bridges()

[Mika Westerberg]

Now that acpiphp_check_bridge() always enumerates devices behind the
bridge, there is no need to do that for each sub-bridge anymore like it is
done in the current ACPI-based PCI hotplug code. Given this we don't need
check_sub_bridges() anymore and can drop the function completely.

This also simplifies the ACPIPHP code a bit.

Signed-off-by: Mika Westerberg 
---
This applies on top of v3.10 with Rafael's ACPIPHP + Thunderbolt series
applied:

http://www.spinics.net/lists/linux-acpi/msg44480.html


 drivers/pci/hotplug/acpiphp_glue.c | 25 -
 1 file changed, 25 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c 
b/drivers/pci/hotplug/acpiphp_glue.c
index 62e09af..741504a 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -760,25 +760,6 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus)
  * ACPI event handlers
  */
 
-static acpi_status
-check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
-{
-   struct acpiphp_bridge *bridge;
-   char objname[64];
-   struct acpi_buffer buffer = { .length = sizeof(objname),
- .pointer = objname };
-
-   bridge = acpiphp_handle_to_bridge(handle);
-   if (bridge) {
-   acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
-   dbg("%s: re-enumerating slots under %s\n",
-   __func__, objname);
-   acpiphp_check_bridge(bridge);
-   put_bridge(bridge);
-   }
-   return AE_OK ;
-}
-
 void acpiphp_check_host_bridge(acpi_handle handle)
 {
struct acpiphp_bridge *bridge;
@@ -788,9 +769,6 @@ void acpiphp_check_host_bridge(acpi_handle handle)
acpiphp_check_bridge(bridge);
put_bridge(bridge);
}
-
-   acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
-   ACPI_UINT32_MAX, check_sub_bridges, NULL, NULL, NULL);
 }
 
 static void hotplug_event(acpi_handle handle, u32 type, void *data)
@@ -818,9 +796,6 @@ static void hotplug_event(acpi_handle handle, u32 type, 
void *data)
dbg("%s: re-enumerating slots under %s\n", __func__, objname);
if (bridge) {
acpiphp_check_bridge(bridge);
-   acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
-   ACPI_UINT32_MAX, check_sub_bridges,
-   NULL, NULL, NULL);
} else {
struct acpiphp_slot *slot = func->slot;
 
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PCI / hotplug / ACPI: Get rid of check_sub_bridges()

[Mika Westerberg]

On Sat, Jul 13, 2013 at 11:47:26PM +0200, Rafael J. Wysocki wrote:
> On Saturday, July 13, 2013 08:09:59 PM Mika Westerberg wrote:
> > Now that acpiphp_check_bridge() always enumerates devices behind the
> > bridge, there is no need to do that for each sub-bridge anymore like it is
> > done in the current ACPI-based PCI hotplug code. Given this we don't need
> > check_sub_bridges() anymore and can drop the function completely.
> > 
> > This also simplifies the ACPIPHP code a bit.
> > 
> > Signed-off-by: Mika Westerberg 
> > ---
> > This applies on top of v3.10 with Rafael's ACPIPHP + Thunderbolt series
> > applied:
> >
> 
> OK, I added it to my bleeding-edge branch along with this series:
> 
> > http://www.spinics.net/lists/linux-acpi/msg44480.html
> 
> rebased on top of some previous ACPI cleanup commits.  I needed to make some
> changes in the process (and fixed up some breakage reported by the auto build
> testing), hopefully I didn't break anything.  If you're in an adventurous 
> mood,
> testing would be welcome. ;-)  [That already includes the majority of 3.11
> material from Linus, though, so unexpected breakage elsewhere may happen.]

Tried the bleeding-edge branch on both of our test machines and Thunderbolt
still works fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] x86: add pin control support to Intel low power subsystem

[Mika Westerberg]

On Fri, Sep 13, 2013 at 05:02:29PM +0300, Mathias Nyman wrote:
> x86 chips with LPSS (low power subsystem) such as Lynxpoint and
> Baytrail have SoC like peripheral support and controllable pins.
> 
> At the moment, Baytrail needs the pinctrl-baytrail driver to let
> peripherals control their gpio resources, but more pincontrol functions
> such as pin muxing and grouping are possible to add later.
> 
> Signed-off-by: Mathias Nyman 

Makes sense, and since there seems to be no way to enable pinctrl by
just selecting it from 'make XXXconfig',

Reviewed-by: Mika Westerberg 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Fri, Sep 13, 2013 at 02:10:43PM -0700, Kevin Hilman wrote:
> >
> > // This makes sure that the controller itself is powered on
> > // (adapter device follows its parent which is the controller). The
> > // controller is attached to the ACPI power domain so it is
> > // brought to D0 now.
> > pm_runtime_get_sync(&client->adapter->dev);
> >
> > // This binds the client device to the ACPI power domain, and in
> > // addition to that brings the client device to D0.
> 
> OK, then here is where the problem is, because you're building ACPI
> assumptions into the core. For non-ACPI devices, this part is a nop, so
> the client device is still powered off, which breaks the assumptions
> below.

We expect that once the driver ->probe() is called, and it doesn't
participate the runtime PM prepared here, the device is regarded as powered
on, runtime PM active.

If the driver participates in runtime PM, it needs to power on the device
and then call pm_runtime_put() to suspend the device.

> > if (ACPI_HANDLE(&client->dev))
> > acpi_dev_pm_attach(&client->dev, true);
> >
> > // Increase the refcount so that client can start runtime PM
> > // transitions when it calls _put().
> > pm_runtime_get_noresume(&client->dev);
> 
> > // Mark the device being active as
> > //  1) In ACPI case we know that is true as we just powered the
> > // device on.
> > //  2) We treat the device by default to be runtime PM active and
> > // powered on (that's in the changelog and should follow what
> > // the PCI bus did).
> > pm_runtime_set_active(&client->dev);
> >
> > // Enable runtime PM but nothing happens yet as long as the client
> > // driver doesn't call _put().
> > pm_runtime_enable(&client->dev);
> >
> > So, yes there might be a disconnect between the runtime PM state and the
> > device HW state now (same is with default to suspended).
> 
> Yes, but until now, default to suspended has been assumed, so any
> changes to that will likely require more thorough auditing of other drivers.

I agree. And it looks like I missed few existing drivers as well. I'm going
to update them in the next version of the series.

There's also a less intrusive way of fixing the problem we see with ACPI
enabled I2C devices:

1. In I2C core i2c_device_probe() we power on the I2C controller
and attach the client device to the ACPI power domain. Just like in
this patch but we don't touch the I2C client device runtime PM.

-> This should allow the existing drivers to keep using whatever
runtime PM strategy they have chosen.

2. For ACPI enumerated I2C client devices drivers we need to
implement the runtime PM in order to save power (otherwise the
devices will be left powered on).

and do the same for SPI devices as well.

Then only thing that changes for non-ACPI devices is that the controller
will be powered on during the client device probe (well, and during
remove).

Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Sat, Sep 14, 2013 at 12:10:37AM +, Zheng, Lv wrote:
> Is it possible to install the handler for ACPI_ROOT_OBJECT?
> Can it be achieved by implementing a setup callback?

Yes that can be done. However, that would mean that we always install the
operation region handler even if there is no suitable GPIO driver loaded.
With this patch we install the handler once the GPIO driver for this device
is registered. If nothing is registered no handlers will be installed.

What would be the advantage in doing what you propose?

> Maybe you can also eliminate acpi_attach_data usages by doing so.

I think we still need that for ACPI _EVT handling.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Sun, Sep 15, 2013 at 01:47:44PM +0100, Mark Brown wrote:
> On Sun, Sep 15, 2013 at 09:41:39AM +0300, Mika Westerberg wrote:
> 
> > There's also a less intrusive way of fixing the problem we see with ACPI
> > enabled I2C devices:
> 
> > 1. In I2C core i2c_device_probe() we power on the I2C controller
> > and attach the client device to the ACPI power domain. Just like in
> > this patch but we don't touch the I2C client device runtime PM.
> 
> > -> This should allow the existing drivers to keep using whatever
> > runtime PM strategy they have chosen.
> 
> There should be no explicit need to power on the I2C controller if it's
> implemented the same way the existing ones are - just have it power
> itself on when it is doing a transfer.

The problem is that the ACPI child device can't be in higher power state
than its parent (and this is also what the runtime PM expects). If we don't
power the I2C controller device before we power on the I2C client device
that rule is violated and we get an complaint to the console.

> > 2. For ACPI enumerated I2C client devices drivers we need to
> > implement the runtime PM in order to save power (otherwise the
> > devices will be left powered on).
> 
> > and do the same for SPI devices as well.
> 
> > Then only thing that changes for non-ACPI devices is that the controller
> > will be powered on during the client device probe (well, and during
> > remove).
> 
> > Thoughts?
> 
> This is definitely less intrusive than the current proposal, there's one
> ACPI I2C device binding queued for -next after the merge window (rt5640)
> which will need an update.

OK, thanks for the heads up. I'll check it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Mon, Sep 16, 2013 at 01:21:53AM +, Zheng, Lv wrote:
> > A pseudo device may be created to access the GPIO operation region fields 
> > provided by one GPIO device.
> > The pseudo device may have other functions to access other GPIO operation 
> > region fields provided by other GPIO devices, or even worse to
> > access other ACPI provided value-adds.
> > So hierarchically the pseudo device only requires CPU, thus should not be 
> > under the GPIO device, which means the GPIO operation regions
> > have nothing to do with the GPIO devices' ACPI handle.
> 
> Sorry for the wording.
> It's better to say the GPIO operation region users haven't strict
> relationship to the GPIO operation region providers.
> As the installation is to provide GPIO operation regions to the users, it
> shouldn't relate to the providers' ACPI handle.

If I understand you correctly you mean that there might be multiple users
(different devices) for the same GPIO operation region, right?

That shouldn't be a problem as far as I can tell.

What comes to the hierarchy you refer, I'm not sure if that is a problem
either (unless I'm missing something). The GPIO can be used anywhere in the
ASL, it doesn't have to be descendant of the GPIO device. You only need to
do something like this:

// Assert the GPIO
Store(1, \_SB.PCI0.SHD3)

In other words, use the fully qualified name.

Typically when the GPIO device _REG() is called it sets some variable like
AVBL to true which is then checked in the code that uses the GPIO:

If (LEqual (\_SB.PCI0.GPO0.AVBL, One))
{
Store (Zero, \_SB.PCI0..SHD3)
}

So if there is no driver, this part of the code is never called.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> [...]
> >>>The call to pm_runtime_get_noresume() should make sure that the device is
> >>>in active state (at least in state where it can access the bus) if I'm
> >>>understanding this right.
> >>
> >>I can't see how this would happen. How runtime_resume/runtime_suspend
> >>callbacks would get invoked with this code, if, e.g. originally driver 
> >>called
> >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in 
> >>probe() ?
> >
> >The driver callbacks are not called but if the device has been attached to
> >a power domain (like we do with ACPI) the power domain callbacks get called
> >and it brings the "bus" to such state that we are able to access the
> >device. That also was the reason I used _noresume() but didn't look too
> >close the implementation.
> 
> OK, but if a client driver assumes default inactive power state it
> will expect
> its callbacks to get called. Otherwise exisiting code might break. So, e.g.
> in case of s5p-tv it would rather need to be something like:
> 
>   pm_runtime_put()
> 
>   pm_runtime_get_sync()
>   sii9234_verify_version()
>   pm_runtime_put(dev)

Yes, or even call directly its runtime_resume callback to bring the device
to the active state.

> >>pm_runtime_get_noresume() merely increments usage counter of a device.
> >>It seems that these changes will break the s5p-tv driver. I might be missing
> >>something though.
> >
> >You are right and Kevin also mentioned this. It should be pm_runtime_get(),
> >if I'm not mistaken.
> 
> Note that client drivers usually call pm_runtime_enable() only when
> it is safe
> to call their driver's runtime PM callbacks. By enabling runtime PM
> before the
> client's driver has completely initialized we may risk that the
> callbacks are
> executed with uninitialized data, if I understand things correctly.

I think that calling pm_runtime_enable() on behalf of the client driver
shouldn't cause any problems. There is no PM events queued for that device
yet (and we have prevented that from happening when we called
_get_noresume() for the device).

Only when the client driver calls _put() things start to happen and at that
phase the runtime PM hooks should be usable.

> >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> >>activate a bus controller device manually in the core for when the client's
> >>probe() is executed, since i2c core will activate the bus controller for 
> >>when
> >>transfer is being carried out.
> >>
> >>But I can understand this is needed for ACPI and it shouldn't break existing
> >>drivers, that do runtime PM activate the client device in probe().
> >
> >Indeed, we don't want to break anything (and we still need something like
> >this for ACPI).
> >
> >>Now I'm sure this will break power management of the 
> >>drivers/media/exynos4-is
> >>driver, due to incorrect power sequence (power domain and clocks handling).
> >>I'll try to take care of it in separate patch, as I have some patches 
> >>pending,
> >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> >>drivers/media/i2c/s5k6a3.c.
> >
> >I missed that code when I converted existing users to this method. Sorry
> >about that (I can handle that in the next version).
> >
> >I quickly looked at it and I don't see anything that could break (once
> >converted). What it does is this:
> >
> > pm_runtime_no_callbacks(dev);
> > pm_runtime_enable(dev);
> >
> >changing that to:
> >
> > pm_runtime_no_callbacks(dev);
> > pm_runtime_put(dev);
> >
> >shouldn't cause problems AFAICT.
> 
> Yes, considering this driver in isolation it should be fine.
> 
> However, I observed system suspend issues when the I2C bus controller was
> being activated (which would happen in the I2C core after your changes)
> before some other driver has initialized.
> 
> So to ensure things continue to work the "fimc-isp-i2c" driver would need
> to be registered after the "exynos4-fimc-is" driver has initialized. Or the
> "exynos4-fimc-is" would need to call of_platform_populate() to instantiate
> its all children devices as specified in device tree (see arch/arm/boot/dts/
> exynos4x12.dtsi in -next). "simple-bus" would then have to be not listed in
> the compat

Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote:
> That's definitely an ACPI specific (probably x86 specific ACPI?)
> requirement not a generic one, on some systems it would increase power
> consumption since the controller will need to sit on while the device is
> functioning autonomously.

Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state
than its parent. This is not x86 specific, though I'm not sure if this is
implemented elsewhere.

> Even though the controller power consumption is going to be minimal the
> power domain it is in may be relatively large.  Can't the power domains
> for ACPI deal with this requirement, for example by making the I2C slave
> power domains children of the controller power domain?

We'll look into this. Thanks for the suggestion.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Mon, Sep 16, 2013 at 03:46:16PM +0100, Graeme Gregory wrote:
> On Mon, Sep 16, 2013 at 05:38:12PM +0300, Mika Westerberg wrote:
> > On Mon, Sep 16, 2013 at 11:12:49AM +0100, Mark Brown wrote:
> > > That's definitely an ACPI specific (probably x86 specific ACPI?)
> > > requirement not a generic one, on some systems it would increase power
> > > consumption since the controller will need to sit on while the device is
> > > functioning autonomously.
> > 
> > Yes, the ACPI 5.0 spec says that the device cannot be in higher D-state
> > than its parent. This is not x86 specific, though I'm not sure if this is
> > implemented elsewhere.
> > 
> I do not think this stops the OS fine controlling the power of the device
> though. It is only a mechanism to make sure the tree of D states is vaguely
> sane from the ACPI point of view. What happens in each D state is never
> actually defined in the ACPI spec.

I think there's a pretty good definition of the D-states in chapter 2.3 of
the ACPI 5.0 spec.

In our case the problem is that the I2C controller is in D3Cold (off) and
we try to move the I2C client device to D0 (on) it violates the ACPI spec.

Anyway we are looking if we can somehow make this work in such way that it
doesn't prevent non-ACPI devices from functioning as they expect now.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

[Mika Westerberg]

On Mon, Sep 16, 2013 at 11:35:56PM +, Zheng, Lv wrote:
> > From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
> > Sent: Monday, September 16, 2013 4:11 PM
> > 
> > On Mon, Sep 16, 2013 at 01:21:53AM +, Zheng, Lv wrote:
> > > > A pseudo device may be created to access the GPIO operation region 
> > > > fields provided by one GPIO device.
> > > > The pseudo device may have other functions to access other GPIO 
> > > > operation region fields provided by other GPIO devices, or even worse
> > to
> > > > access other ACPI provided value-adds.
> > > > So hierarchically the pseudo device only requires CPU, thus should not 
> > > > be under the GPIO device, which means the GPIO operation
> > regions
> > > > have nothing to do with the GPIO devices' ACPI handle.
> > >
> > > Sorry for the wording.
> > > It's better to say the GPIO operation region users haven't strict
> > > relationship to the GPIO operation region providers.
> > > As the installation is to provide GPIO operation regions to the users, it
> > > shouldn't relate to the providers' ACPI handle.
> > 
> > If I understand you correctly you mean that there might be multiple users
> > (different devices) for the same GPIO operation region, right?
> 
> No, this is not what I meant.
> Can one vendor device uses two or more GPIO pins from different GPIO ports?

Yes.

> > That shouldn't be a problem as far as I can tell.
> > 
> > What comes to the hierarchy you refer, I'm not sure if that is a problem
> > either (unless I'm missing something). The GPIO can be used anywhere in the
> > ASL, it doesn't have to be descendant of the GPIO device. You only need to
> > do something like this:
> > 
> > // Assert the GPIO
> > Store(1, \_SB.PCI0.SHD3)
> > 
> > In other words, use the fully qualified name.
> 
> Yes, which means this line of code can be everywhere in the namespace.
> It is not required to be under one particular GPIO device as long as there is 
> an operation region defined in that scope.
> 
> The problem is the installation, the first parameter for
> acpi_install_address_space_handler() is a namespace node, the function
> will call _REG for all OperationRegions below the scope whose top most
> node is the specified node.
> The nodes out of this scope are not affected.  So if an OperationRegion
> is defined out of this scope, problem happens.

I suppose for that operation region another GPIO device should be
introduced then, no?

So if we don't have a GPIO driver for the given operation region, what can
we do? We don't want the ASL code to erroneusly think that there is an
operation region available when it is not.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

On Mon, Sep 16, 2013 at 09:07:07PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 16, 2013 11:47:08 AM Mika Westerberg wrote:
> > On Sun, Sep 15, 2013 at 03:48:12PM +0200, Sylwester Nawrocki wrote:
> > > On 09/13/2013 05:40 PM, Mika Westerberg wrote:
> > > [...]
> > > >>>The call to pm_runtime_get_noresume() should make sure that the device 
> > > >>>is
> > > >>>in active state (at least in state where it can access the bus) if I'm
> > > >>>understanding this right.
> > > >>
> > > >>I can't see how this would happen. How runtime_resume/runtime_suspend
> > > >>callbacks would get invoked with this code, if, e.g. originally driver 
> > > >>called
> > > >>pm_runtime_enable(), pm_runtime_get_sync(), pm_runtime_put_sync() in 
> > > >>probe() ?
> > > >
> > > >The driver callbacks are not called but if the device has been attached 
> > > >to
> > > >a power domain (like we do with ACPI) the power domain callbacks get 
> > > >called
> > > >and it brings the "bus" to such state that we are able to access the
> > > >device. That also was the reason I used _noresume() but didn't look too
> > > >close the implementation.
> > > 
> > > OK, but if a client driver assumes default inactive power state it
> > > will expect
> > > its callbacks to get called. Otherwise exisiting code might break. So, 
> > > e.g.
> > > in case of s5p-tv it would rather need to be something like:
> > > 
> > >   pm_runtime_put()
> > > 
> > >   pm_runtime_get_sync()
> > >   sii9234_verify_version()
> > >   pm_runtime_put(dev)
> > 
> > Yes, or even call directly its runtime_resume callback to bring the device
> > to the active state.
> > 
> > > >>pm_runtime_get_noresume() merely increments usage counter of a device.
> > > >>It seems that these changes will break the s5p-tv driver. I might be 
> > > >>missing
> > > >>something though.
> > > >
> > > >You are right and Kevin also mentioned this. It should be 
> > > >pm_runtime_get(),
> > > >if I'm not mistaken.
> > > 
> > > Note that client drivers usually call pm_runtime_enable() only when
> > > it is safe
> > > to call their driver's runtime PM callbacks. By enabling runtime PM
> > > before the
> > > client's driver has completely initialized we may risk that the
> > > callbacks are
> > > executed with uninitialized data, if I understand things correctly.
> > 
> > I think that calling pm_runtime_enable() on behalf of the client driver
> > shouldn't cause any problems. There is no PM events queued for that device
> > yet (and we have prevented that from happening when we called
> > _get_noresume() for the device).
> 
> That only is the case if the device in RPM_ACTIVE when we enable runtime PM 
> for
> it.  If it is RPM_SUSPENDED at that point, it still is possible that the 
> resume
> callback will be executed then.

OK, thanks for the clarification.

> > Only when the client driver calls _put() things start to happen and at that
> > phase the runtime PM hooks should be usable.
> > 
> > > >>As Mark pointed out this is currently unwanted behaviour to runtime PM
> > > >>activate a bus controller device manually in the core for when the 
> > > >>client's
> > > >>probe() is executed, since i2c core will activate the bus controller 
> > > >>for when
> > > >>transfer is being carried out.
> > > >>
> > > >>But I can understand this is needed for ACPI and it shouldn't break 
> > > >>existing
> > > >>drivers, that do runtime PM activate the client device in probe().
> > > >
> > > >Indeed, we don't want to break anything (and we still need something like
> > > >this for ACPI).
> > > >
> > > >>Now I'm sure this will break power management of the 
> > > >>drivers/media/exynos4-is
> > > >>driver, due to incorrect power sequence (power domain and clocks 
> > > >>handling).
> > > >>I'll try to take care of it in separate patch, as I have some patches 
> > > >>pending,
> > > >>that move most of code from drivers/media/exynos4-is/fimc-is-sensor.c to
> > > >>drivers/media/i2

[PATCH] spi/pxa2xx: check status register as well to determine if the device is off

[Mika Westerberg]

The current interrupt handler calls pm_runtime_suspended() to check if the
device is suspended or not. However, runtime PM status of the device is
only set to suspended once all PM runtime suspend hooks have executed.

In case of Intel Lynxpoint we have the device bound to the ACPI power
domain and its runtime suspend hook will put the device to D3hot (or D3cold
if possible). This means that the device is powered off before its state is
set to runtime suspended. While in this state the device might get an
interrupt that is meant for another device (as the interrupt line is
shared), and because the device is powered off accessing its registers will
return 0x that the driver misinterprets as an invalid state.

When this happens user will see messages like below on the console:

  pxa2xx-spi INT33C0:00: bad message state in interrupt handler

Fix this by checking the status register for ~0 and returning IRQ_NONE in
that case.

Signed-off-by: Mika Westerberg 
---
 drivers/spi/spi-pxa2xx.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index e0fd6f6..2cbab8a 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -546,8 +546,17 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
if (pm_runtime_suspended(&drv_data->pdev->dev))
return IRQ_NONE;
 
-   sccr1_reg = read_SSCR1(reg);
+   /*
+* If the device is not yet in RPM suspended state and we get an
+* interrupt that is meant for another device, check if status bits
+* are all set to one. That means that the device is already
+* powered off.
+*/
status = read_SSSR(reg);
+   if (status == ~0)
+   return IRQ_NONE;
+
+   sccr1_reg = read_SSCR1(reg);
 
/* Ignore possible writes if we don't need to write */
if (!(sccr1_reg & SSCR1_TIE))
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices

[Mika Westerberg]

From: Aaron Lu 

This patch adds runtime PM support for the I2C bus in a similar way that
has been done for PCI bus already. This means that the I2C bus core
prepares runtime PM for a client device just before a driver is about to be
bound to it. Devices that are not bound to any driver are not prepared for
runtime PM.

In order to take advantage of this runtime PM support, the client device
driver needs drop the device runtime PM reference count by calling
pm_runtime_put() in its ->probe() callback and possibly implement rest of
the runtime PM callbacks.

However, this does not yet make runtime PM happen for the device, it has to
be explicitly allowed from userspace per each I2C client device. The
reason for this is that things like HID over I2C might not work as smoothly
when runtime PM is active. So we leave it to the user to balance between
performance and power efficiency.

User can allow runtime PM for the client device by running:

# echo auto > /sys/bus/i2c/devices//power/control

and it can be forbidden again by:

# echo on > /sys/bus/i2c/devices//power/control

Status of the device can be monitored by reading files under the device
power directory.

If the driver doesn't support runtime PM (like most of the existing I2C
client drivers), the device in question is regarded as being runtime PM
active and powered on.

The patch adds also runtime PM support for the adapter device because it is
needed to be able to runtime power manage the I2C controller device. The
adapter device is handled along with the I2C controller device (it uses
pm_runtime_no_callbacks()).

Signed-off-by: Aaron Lu 
Signed-off-by: Mika Westerberg 
Acked-by: Rafael J. Wysocki 
---
 drivers/i2c/i2c-core.c | 48 +++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3d44292..8fad5ac 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -254,11 +254,34 @@ static int i2c_device_probe(struct device *dev)
client->flags & I2C_CLIENT_WAKE);
dev_dbg(dev, "probe\n");
 
+   /* Make sure the adapter is active */
+   pm_runtime_get_sync(&client->adapter->dev);
+
+   /*
+* Enable runtime PM for the client device. If the client wants to
+* participate on runtime PM it should call pm_runtime_put() in its
+* probe() callback.
+*
+* User still needs to allow the PM runtime before it can actually
+* happen.
+*/
+   pm_runtime_forbid(&client->dev);
+   pm_runtime_get_noresume(&client->dev);
+   pm_runtime_set_active(&client->dev);
+   pm_runtime_enable(&client->dev);
+
status = driver->probe(client, i2c_match_id(driver->id_table, client));
if (status) {
client->driver = NULL;
i2c_set_clientdata(client, NULL);
+
+   pm_runtime_disable(&client->dev);
+   pm_runtime_set_suspended(&client->dev);
+   pm_runtime_put_noidle(&client->dev);
}
+
+   pm_runtime_put(&client->adapter->dev);
+
return status;
 }
 
@@ -271,6 +294,8 @@ static int i2c_device_remove(struct device *dev)
if (!client || !dev->driver)
return 0;
 
+   pm_runtime_get_sync(&client->adapter->dev);
+
driver = to_i2c_driver(dev->driver);
if (driver->remove) {
dev_dbg(dev, "remove\n");
@@ -283,6 +308,13 @@ static int i2c_device_remove(struct device *dev)
client->driver = NULL;
i2c_set_clientdata(client, NULL);
}
+
+   /* Undo the runtime PM done in i2c_probe() */
+   pm_runtime_disable(&client->dev);
+   pm_runtime_set_suspended(&client->dev);
+   pm_runtime_put_noidle(&client->dev);
+
+   pm_runtime_put(&client->adapter->dev);
return status;
 }
 
@@ -294,8 +326,11 @@ static void i2c_device_shutdown(struct device *dev)
if (!client || !dev->driver)
return;
driver = to_i2c_driver(dev->driver);
-   if (driver->shutdown)
+   if (driver->shutdown) {
+   pm_runtime_get_sync(&client->adapter->dev);
driver->shutdown(client);
+   pm_runtime_put(&client->adapter->dev);
+   }
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1263,6 +1298,15 @@ exit_recovery:
bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter);
mutex_unlock(&core_lock);
 
+   /*
+* Make sure the adapter runtime PM follows the parent device (the
+* host controller) so that we can suspend it once there aren't any
+* active clients anymore.
+*/
+   pm_runtime_set_active(&

[PATCH RESEND 2/2] i2c: attach/detach I2C client device to the ACPI power domain

[Mika Westerberg]

From: Lv Zheng 

If the I2C client device is enumerated from ACPI namespace it might have
ACPI methods that needs to be called in order to transition the device to
a different power states (such as _PSx).

Implement this for I2C client devices by checking if the device has an ACPI
handle and if that's the case, attach it to the ACPI power domain. In
addition we make sure that the device is fully powered when its ->probe()
function gets called.

For non-ACPI devices this patch is a no-op.

Signed-off-by: Lv Zheng 
Signed-off-by: Mika Westerberg 
Acked-by: Rafael J. Wysocki 
---
 drivers/i2c/i2c-core.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 8fad5ac..fdf086b 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -257,6 +257,9 @@ static int i2c_device_probe(struct device *dev)
/* Make sure the adapter is active */
pm_runtime_get_sync(&client->adapter->dev);
 
+   if (ACPI_HANDLE(&client->dev))
+   acpi_dev_pm_attach(&client->dev, true);
+
/*
 * Enable runtime PM for the client device. If the client wants to
 * participate on runtime PM it should call pm_runtime_put() in its
@@ -278,6 +281,9 @@ static int i2c_device_probe(struct device *dev)
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
pm_runtime_put_noidle(&client->dev);
+
+   if (ACPI_HANDLE(&client->dev))
+   acpi_dev_pm_detach(&client->dev, true);
}
 
pm_runtime_put(&client->adapter->dev);
@@ -314,6 +320,9 @@ static int i2c_device_remove(struct device *dev)
pm_runtime_set_suspended(&client->dev);
pm_runtime_put_noidle(&client->dev);
 
+   if (ACPI_HANDLE(&client->dev))
+   acpi_dev_pm_detach(&client->dev, true);
+
pm_runtime_put(&client->adapter->dev);
return status;
 }
-- 
1.8.4.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/