[PATCH 0/2] drivers: PL061: Add platform driver probing support

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

According to ACPI SPEC, it supports ARM boards to use "GPIO-signaled
ACPI Events". These events can be used for input events. And to QEMU, it
uses GPIO PL061 controller for input events.

These two patches add platform driver support for PL061 probed by DT or
ACPI.

Shannon Zhao (2):
  drivers: PL061: add support for platform driver probing
  drivers: PL061: add ACPI probing for PL061

 drivers/gpio/gpio-pl061.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

-- 
2.0.4

[PATCH 1/2] drivers: PL061: add support for platform driver probing

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Since PL061 currently only supports AMBA driver, to support using GPIO
PL061 by DT or ACPI, it needs to add support for platform driver.
A DT binding is provided with this patch, ACPI support is added in a
separate one.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 drivers/gpio/gpio-pl061.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 0475613..64c10eb 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm.h>
+#include <linux/platform_device.h>
 
 #define GPIODIR 0x400
 #define GPIOIS  0x404
@@ -241,6 +242,94 @@ static struct irq_chip pl061_irqchip = {
 	.irq_set_type	= pl061_irq_type,
 };
 
+static int pl061_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pl061_platform_data *pdata = dev_get_platdata(dev);
+	struct pl061_gpio *chip;
+	int ret, irq, i, irq_base;
+	struct resource *mem;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem)
+		return -EINVAL;
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL)
+		return -ENOMEM;
+
+	if (pdata) {
+		chip->gc.base = pdata->gpio_base;
+		irq_base = pdata->irq_base;
+		if (irq_base <= 0) {
+			dev_err(&pdev->dev, "invalid IRQ base in pdata\n");
+			return -ENODEV;
+		}
+	} else {
+		chip->gc.base = -1;
+		irq_base = 0;
+	}
+
+	chip->base = devm_ioremap(dev, mem->start, resource_size(mem));
+	if (IS_ERR(chip->base))
+		return PTR_ERR(chip->base);
+
+	spin_lock_init(&chip->lock);
+	if (of_property_read_bool(dev->of_node, "gpio-ranges"))
+		chip->uses_pinctrl = true;
+
+	chip->gc.request = pl061_gpio_request;
+	chip->gc.free = pl061_gpio_free;
+	chip->gc.direction_input = pl061_direction_input;
+	chip->gc.direction_output = pl061_direction_output;
+	chip->gc.get = pl061_get_value;
+	chip->gc.set = pl061_set_value;
+	chip->gc.ngpio = PL061_GPIO_NR;
+	chip->gc.label = dev_name(dev);
+	chip->gc.dev = dev;
+	chip->gc.owner = THIS_MODULE;
+
+	ret = gpiochip_add(&chip->gc);
+	if (ret)
+		return ret;
+
+	/*
+	 * irq_chip support
+	 */
+	writeb(0, chip->base + GPIOIE); /* disable irqs */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "invalid IRQ\n");
+		return -ENODEV;
+	}
+
+	ret = gpiochip_irqchip_add(&chip->gc, &pl061_irqchip,
+				   irq_base, handle_simple_irq,
+				   IRQ_TYPE_NONE);
+	if (ret) {
+		dev_info(&pdev->dev, "could not add irqchip\n");
+		return ret;
+	}
+	gpiochip_set_chained_irqchip(&chip->gc, &pl061_irqchip,
+				     irq, pl061_irq_handler);
+
+	for (i = 0; i < PL061_GPIO_NR; i++) {
+		if (pdata) {
+			if (pdata->directions & (BIT(i)))
+				pl061_direction_output(&chip->gc, i,
+						pdata->values & (BIT(i)));
+			else
+				pl061_direction_input(&chip->gc, i);
+		}
+	}
+
+	platform_set_drvdata(pdev, chip);
+	dev_info(&pdev->dev, "PL061 GPIO chip @%pa registered\n",
+		 &mem->start);
+
+	return 0;
+}
+
 static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	struct device *dev = &adev->dev;
@@ -376,6 +465,23 @@ static const struct dev_pm_ops pl061_dev_pm_ops = {
 };
 #endif
 
+static const struct of_device_id pl061_of_match[] = {
+	{ .compatible = "arm,pl061", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pl061_of_match);
+
+static struct platform_driver pl061_gpio_platform_driver = {
+	.driver = {
+		.name	= "pl061_gpio",
+		.of_match_table	= pl061_of_match,
+#ifdef CONFIG_PM
+		.pm	= &pl061_dev_pm_ops,
+#endif
+	},
+	.probe		= pl061_platform_probe,
+};
+
 static struct amba_id pl061_ids[] = {
 	{
 		.id	= 0x00041061,
@@ -399,6 +505,8 @@ static struct amba_driver pl061_gpio_driver = {
 
 static int __init pl061_gpio_init(void)
 {
+	if (platform_driver_register(&pl061_gpio_platform_driver))
+		pr_warn("could not register PL061 platform driver\n");
 	return amba_driver_register(&pl061_gpio_driver);
 }
 module_init(pl061_gpio_init);
-- 
2.0.4

[PATCH 1/2] drivers: PL061: add support for platform driver probing

[Linus Walleij]

On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Since PL061 currently only supports AMBA driver, to support using GPIO
> PL061 by DT or ACPI, it needs to add support for platform driver.
> A DT binding is provided with this patch, ACPI support is added in a
> separate one.
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

I already stated in 0/2 what the problem is with this, and it's obviously adding
a huge codechunk, very similar to the AMBA probe path.

We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices
instead.

Yours,
Linus Walleij

[PATCH 1/2] drivers: PL061: add support for platform driver probing

[Graeme Gregory]

On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote:
> On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com>
> wrote:
> 
> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >
> > Since PL061 currently only supports AMBA driver, to support using GPIO
> > PL061 by DT or ACPI, it needs to add support for platform driver.
> > A DT binding is provided with this patch, ACPI support is added in a
> > separate one.
> >
> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> 
> I already stated in 0/2 what the problem is with this, and it's obviously
> adding
> a huge codechunk, very similar to the AMBA probe path.
> 
> We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices
> instead.
> 
AMBA DT support is a hideous hack, we had kind of hoped to avoid doing
that in ACPI.

If you notice ARM already updated pl011 driver to support platform
device type probing this seems to be the way to go.

Graeme

[PATCH 1/2] drivers: PL061: add support for platform driver probing

[Russell King - ARM Linux]

On Mon, Aug 03, 2015 at 04:13:31PM +0100, Graeme Gregory wrote:
> 
> 
> On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote:
> > On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com>
> > wrote:
> > 
> > > From: Shannon Zhao <shannon.zhao@linaro.org>
> > >
> > > Since PL061 currently only supports AMBA driver, to support using GPIO
> > > PL061 by DT or ACPI, it needs to add support for platform driver.
> > > A DT binding is provided with this patch, ACPI support is added in a
> > > separate one.
> > >
> > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > 
> > I already stated in 0/2 what the problem is with this, and it's obviously
> > adding
> > a huge codechunk, very similar to the AMBA probe path.
> > 
> > We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices
> > instead.
> > 
> AMBA DT support is a hideous hack, we had kind of hoped to avoid doing
> that in ACPI.
> 
> If you notice ARM already updated pl011 driver to support platform
> device type probing this seems to be the way to go.

As the AMBA bus maintainer, NAK on that.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/2] drivers: PL061: add support for platform driver probing

[Linus Walleij]

On Mon, Aug 3, 2015 at 5:13 PM, Graeme Gregory <gg@slimlogic.co.uk> wrote:
> On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote:
>> On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com>
>> wrote:
>>
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> >
>> > Since PL061 currently only supports AMBA driver, to support using GPIO
>> > PL061 by DT or ACPI, it needs to add support for platform driver.
>> > A DT binding is provided with this patch, ACPI support is added in a
>> > separate one.
>> >
>> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> I already stated in 0/2 what the problem is with this, and it's obviously
>> adding
>> a huge codechunk, very similar to the AMBA probe path.
>>
>> We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices
>> instead.
>
> AMBA DT support is a hideous hack, we had kind of hoped to avoid doing
> that in ACPI.

Define what you mean with "hideous hack".

I think it is elegant. It combines the DT aspect of telling where the
device is with the Plug-and-play aspect of reading the 0xB105F00D
magic in the PrimeCells very nicely. With an optional override
mechanism. It's nice.

The runtime PM stuff and pclk handling that Russell and also Ulf
has worked on is very helpful and takes out a lot of codelines
in a lot of drivers. Also very nice.

> If you notice ARM already updated pl011 driver to support platform
> device type probing this seems to be the way to go.

Just because there is a precedent does not mean it is good.

Continuing with this adds 100+ lines of hairy initialization to
every AMBA driver.

What needs to happen is stop this and make ACPI spawn
AMBA devices for PrimeCells, and move the serial port over
to that.

A compromise may be to have something that probes a list
of these platform devices in drivers/acpi/ and spawn a
corresponding AMBA device for it. It will still save codelines in
all other subsystems and lower maintenance for the
subsystem maintainers.

Yours,
Linus Walleij

[PATCH 1/2] drivers: PL061: add support for platform driver probing

[Graeme Gregory]

On Thu, 13 Aug 2015, at 01:14 PM, Linus Walleij wrote:
> On Mon, Aug 3, 2015 at 5:13 PM, Graeme Gregory <gg@slimlogic.co.uk>
> wrote:
> > On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote:
> >> On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com>
> >> wrote:
> >>
> >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >> >
> >> > Since PL061 currently only supports AMBA driver, to support using GPIO
> >> > PL061 by DT or ACPI, it needs to add support for platform driver.
> >> > A DT binding is provided with this patch, ACPI support is added in a
> >> > separate one.
> >> >
> >> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> I already stated in 0/2 what the problem is with this, and it's obviously
> >> adding
> >> a huge codechunk, very similar to the AMBA probe path.
> >>
> >> We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices
> >> instead.
> >
> > AMBA DT support is a hideous hack, we had kind of hoped to avoid doing
> > that in ACPI.
> 
> Define what you mean with "hideous hack".
> 
drivers/of/platform.c search for #ifdef CONFIG_ARM_AMBA

> I think it is elegant. It combines the DT aspect of telling where the
> device is with the Plug-and-play aspect of reading the 0xB105F00D
> magic in the PrimeCells very nicely. With an optional override
> mechanism. It's nice.
> 
> The runtime PM stuff and pclk handling that Russell and also Ulf
> has worked on is very helpful and takes out a lot of codelines
> in a lot of drivers. Also very nice.
> 
I have no objection to AMBA devices that are children of an AMBA bus
object. Its the hack above that is nasty!  It is also un-scalable as it
sets the precedent that the platform.c in both DT and ACPI should learn
about all the different types of device probing.

> > If you notice ARM already updated pl011 driver to support platform
> > device type probing this seems to be the way to go.
> 
> Just because there is a precedent does not mean it is good.
> 
> Continuing with this adds 100+ lines of hairy initialization to
> every AMBA driver.
> 
> What needs to happen is stop this and make ACPI spawn
> AMBA devices for PrimeCells, and move the serial port over
> to that.
> 
> A compromise may be to have something that probes a list
> of these platform devices in drivers/acpi/ and spawn a
> corresponding AMBA device for it. It will still save codelines in
> all other subsystems and lower maintenance for the
> subsystem maintainers.
> 
I am currently working on exactly what you describe above.

Graeme

[PATCH 1/2] drivers: PL061: add support for platform driver probing

[Russell King - ARM Linux]

On Thu, Aug 13, 2015 at 01:25:13PM +0100, Graeme Gregory wrote:
> 
> 
> On Thu, 13 Aug 2015, at 01:14 PM, Linus Walleij wrote:
> > On Mon, Aug 3, 2015 at 5:13 PM, Graeme Gregory <gg@slimlogic.co.uk>
> > wrote:
> > > On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote:
> > >> On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com>
> > >> wrote:
> > >>
> > >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> > >> >
> > >> > Since PL061 currently only supports AMBA driver, to support using GPIO
> > >> > PL061 by DT or ACPI, it needs to add support for platform driver.
> > >> > A DT binding is provided with this patch, ACPI support is added in a
> > >> > separate one.
> > >> >
> > >> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > >>
> > >> I already stated in 0/2 what the problem is with this, and it's obviously
> > >> adding
> > >> a huge codechunk, very similar to the AMBA probe path.
> > >>
> > >> We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices
> > >> instead.
> > >
> > > AMBA DT support is a hideous hack, we had kind of hoped to avoid doing
> > > that in ACPI.
> > 
> > Define what you mean with "hideous hack".
> > 
> drivers/of/platform.c search for #ifdef CONFIG_ARM_AMBA

The way that AMBA support was added to OF was not something I was involved
in.  Don't make the mistake of saying "we're only supporting platform
devices in ACPI because the OF implementation is messed up".

> > I think it is elegant. It combines the DT aspect of telling where the
> > device is with the Plug-and-play aspect of reading the 0xB105F00D
> > magic in the PrimeCells very nicely. With an optional override
> > mechanism. It's nice.

It's also the _right_ way to deal with buses with different properties
from platform devices, especially where some can be probed and identified
via hardware-encoded IDs.

> I have no objection to AMBA devices that are children of an AMBA bus
> object. Its the hack above that is nasty!  It is also un-scalable as it
> sets the precedent that the platform.c in both DT and ACPI should learn
> about all the different types of device probing.

What's actually the nasty thing is that DT and ACPI are wedded to platform
devices, which pushes people down the path of more platform device usage,
when in fact they should be creating their own bus classes.

> > > If you notice ARM already updated pl011 driver to support platform
> > > device type probing this seems to be the way to go.
> > 
> > Just because there is a precedent does not mean it is good.

Exactly.

> > Continuing with this adds 100+ lines of hairy initialization to
> > every AMBA driver.

And that's utterly insane.

> > What needs to happen is stop this and make ACPI spawn
> > AMBA devices for PrimeCells, and move the serial port over
> > to that.
> > 
> > A compromise may be to have something that probes a list
> > of these platform devices in drivers/acpi/ and spawn a
> > corresponding AMBA device for it. It will still save codelines in
> > all other subsystems and lower maintenance for the
> > subsystem maintainers.

Even that isn't nice... but I guess as the madness will be contained
within ACPI, it'll be ACPI's problem to maintain, so up to them how to
deal with it provided the code is contained within ACPI. :)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/2] drivers: PL061: add ACPI probing for PL061

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add the necessary driver boilerplate to let the driver be used when
the respective ACPI table is discovered by the ACPI subsystem.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 drivers/gpio/gpio-pl061.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 64c10eb..41fcf7b 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -25,6 +25,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm.h>
 #include <linux/platform_device.h>
+#include <linux/acpi.h>
 
 #define GPIODIR 0x400
 #define GPIOIS  0x404
@@ -471,10 +472,17 @@ static const struct of_device_id pl061_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, pl061_of_match);
 
+static const struct acpi_device_id pl061_acpi_match[] = {
+	{ "ARMH0061",},
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, pl061_acpi_match);
+
 static struct platform_driver pl061_gpio_platform_driver = {
 	.driver = {
 		.name	= "pl061_gpio",
 		.of_match_table	= pl061_of_match,
+		.acpi_match_table = ACPI_PTR(pl061_acpi_match),
 #ifdef CONFIG_PM
 		.pm	= &pl061_dev_pm_ops,
 #endif
-- 
2.0.4

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Russell King - ARM Linux]

On Mon, Aug 03, 2015 at 02:59:56PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> According to ACPI SPEC, it supports ARM boards to use "GPIO-signaled
> ACPI Events". These events can be used for input events. And to QEMU, it
> uses GPIO PL061 controller for input events.
> 
> These two patches add platform driver support for PL061 probed by DT or
> ACPI.

This certainly is incorrect for DT, and is probably wrong for ACPI too.
DT creates amba devices, so binds via the amba device driver.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Shannon Zhao]

Hi Russell,

On 2015/8/3 15:58, Russell King - ARM Linux wrote:
> On Mon, Aug 03, 2015 at 02:59:56PM +0800, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> According to ACPI SPEC, it supports ARM boards to use "GPIO-signaled
>> ACPI Events". These events can be used for input events. And to QEMU, it
>> uses GPIO PL061 controller for input events.
>>
>> These two patches add platform driver support for PL061 probed by DT or
>> ACPI.
> 
> This certainly is incorrect for DT, and is probably wrong for ACPI too.
> DT creates amba devices, so binds via the amba device driver.
> 

Oh, sorry. The changes for DT are not necessary. But for ACPI I think it
needs to convert pl061 to platform device since ACPI doesn't support
AMBA devices. The Pl011 also does the same thing to support ACPI
probing. See drivers/tty/serial/amba-pl011.c

Thanks,
-- 
Shannon

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Russell King - ARM Linux]

On Mon, Aug 03, 2015 at 05:26:05PM +0800, Shannon Zhao wrote:
> Hi Russell,
> 
> On 2015/8/3 15:58, Russell King - ARM Linux wrote:
> > On Mon, Aug 03, 2015 at 02:59:56PM +0800, Shannon Zhao wrote:
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> According to ACPI SPEC, it supports ARM boards to use "GPIO-signaled
> >> ACPI Events". These events can be used for input events. And to QEMU, it
> >> uses GPIO PL061 controller for input events.
> >>
> >> These two patches add platform driver support for PL061 probed by DT or
> >> ACPI.
> > 
> > This certainly is incorrect for DT, and is probably wrong for ACPI too.
> > DT creates amba devices, so binds via the amba device driver.
> > 
> 
> Oh, sorry. The changes for DT are not necessary. But for ACPI I think it
> needs to convert pl061 to platform device since ACPI doesn't support
> AMBA devices. The Pl011 also does the same thing to support ACPI
> probing. See drivers/tty/serial/amba-pl011.c

Maybe rather than having every AMBA driver also converted to a platform
driver (which GregKH hates) maybe ACPI should support the AMBA bus
instead?

Greg?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Linus Walleij]

On Mon, Aug 3, 2015 at 11:49 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Aug 03, 2015 at 05:26:05PM +0800, Shannon Zhao wrote:

>> Oh, sorry. The changes for DT are not necessary. But for ACPI I think it
>> needs to convert pl061 to platform device since ACPI doesn't support
>> AMBA devices. The Pl011 also does the same thing to support ACPI
>> probing. See drivers/tty/serial/amba-pl011.c
>
> Maybe rather than having every AMBA driver also converted to a platform
> driver (which GregKH hates) maybe ACPI should support the AMBA bus
> instead?

I agree with Russell, and overall I'm not sure I'm very happy with
commit 0dd1e247fd39aed20fd2baacc62ca44d82534798
"drivers: PL011: add support for the ARM SBSA generic UART"
either.

I can see how this works, but it adds a big chunk of overhead to
every driver that want to support both PrimeCell MMIO probing
and devicetree and ACPI.

Devicetree is spawning awesome AMBA devices for PrimeCells
in drivers/of/platform.c, so why can't ACPI do the same?

The AMBA bus is intelligent and very helpful with things like
runtime suspend/resume. It also supports reading the version
registers to autodetect device characteristics.

Yours,
Linus Walleij

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Greg Kroah-Hartman]

On Mon, Aug 03, 2015 at 10:49:23AM +0100, Russell King - ARM Linux wrote:
> On Mon, Aug 03, 2015 at 05:26:05PM +0800, Shannon Zhao wrote:
> > Hi Russell,
> > 
> > On 2015/8/3 15:58, Russell King - ARM Linux wrote:
> > > On Mon, Aug 03, 2015 at 02:59:56PM +0800, Shannon Zhao wrote:
> > >> From: Shannon Zhao <shannon.zhao@linaro.org>
> > >>
> > >> According to ACPI SPEC, it supports ARM boards to use "GPIO-signaled
> > >> ACPI Events". These events can be used for input events. And to QEMU, it
> > >> uses GPIO PL061 controller for input events.
> > >>
> > >> These two patches add platform driver support for PL061 probed by DT or
> > >> ACPI.
> > > 
> > > This certainly is incorrect for DT, and is probably wrong for ACPI too.
> > > DT creates amba devices, so binds via the amba device driver.
> > > 
> > 
> > Oh, sorry. The changes for DT are not necessary. But for ACPI I think it
> > needs to convert pl061 to platform device since ACPI doesn't support
> > AMBA devices. The Pl011 also does the same thing to support ACPI
> > probing. See drivers/tty/serial/amba-pl011.c
> 
> Maybe rather than having every AMBA driver also converted to a platform
> driver (which GregKH hates) maybe ACPI should support the AMBA bus
> instead?
> 
> Greg?

The ACPI developers have been doing work to allow a driver to handle
getting the resources from either DT or ACPI no matter what bus it is
on, so converting anything to a platform driver shouldn't be needed.

but I don't really know the details here, and this isn't being sent to
the ACPI mailing list, so I don't know what to suggest...

greg k-h

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Russell King - ARM Linux]

On Mon, Aug 03, 2015 at 09:26:23AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Aug 03, 2015 at 10:49:23AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Aug 03, 2015 at 05:26:05PM +0800, Shannon Zhao wrote:
> > > Hi Russell,
> > > 
> > > On 2015/8/3 15:58, Russell King - ARM Linux wrote:
> > > > On Mon, Aug 03, 2015 at 02:59:56PM +0800, Shannon Zhao wrote:
> > > >> From: Shannon Zhao <shannon.zhao@linaro.org>
> > > >>
> > > >> According to ACPI SPEC, it supports ARM boards to use "GPIO-signaled
> > > >> ACPI Events". These events can be used for input events. And to QEMU, it
> > > >> uses GPIO PL061 controller for input events.
> > > >>
> > > >> These two patches add platform driver support for PL061 probed by DT or
> > > >> ACPI.
> > > > 
> > > > This certainly is incorrect for DT, and is probably wrong for ACPI too.
> > > > DT creates amba devices, so binds via the amba device driver.
> > > > 
> > > 
> > > Oh, sorry. The changes for DT are not necessary. But for ACPI I think it
> > > needs to convert pl061 to platform device since ACPI doesn't support
> > > AMBA devices. The Pl011 also does the same thing to support ACPI
> > > probing. See drivers/tty/serial/amba-pl011.c
> > 
> > Maybe rather than having every AMBA driver also converted to a platform
> > driver (which GregKH hates) maybe ACPI should support the AMBA bus
> > instead?
> > 
> > Greg?
> 
> The ACPI developers have been doing work to allow a driver to handle
> getting the resources from either DT or ACPI no matter what bus it is
> on, so converting anything to a platform driver shouldn't be needed.
> 
> but I don't really know the details here, and this isn't being sent to
> the ACPI mailing list, so I don't know what to suggest...

Greg,

You're the one who's said many times that you'd like to see platform
devices and platform device drivers to go away.  So, when I give you
the opportunity to comment on platform devices being used to augment
existing device drivers such as the AMBA bus drivers (which you've
also previously said is a more preferable way to use the driver model)
why not support that view?

Right now, there seems to be a move to convert _all_ AMBA device drivers
to become dual-drivers: an amba bus driver and a platform driver.  Is
that something you really want to see, irrespective of ACPI issues?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Greg Kroah-Hartman]

On Mon, Aug 03, 2015 at 08:09:35PM +0100, Russell King - ARM Linux wrote:
> On Mon, Aug 03, 2015 at 09:26:23AM -0700, Greg Kroah-Hartman wrote:
> > On Mon, Aug 03, 2015 at 10:49:23AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Aug 03, 2015 at 05:26:05PM +0800, Shannon Zhao wrote:
> > > > Hi Russell,
> > > > 
> > > > On 2015/8/3 15:58, Russell King - ARM Linux wrote:
> > > > > On Mon, Aug 03, 2015 at 02:59:56PM +0800, Shannon Zhao wrote:
> > > > >> From: Shannon Zhao <shannon.zhao@linaro.org>
> > > > >>
> > > > >> According to ACPI SPEC, it supports ARM boards to use "GPIO-signaled
> > > > >> ACPI Events". These events can be used for input events. And to QEMU, it
> > > > >> uses GPIO PL061 controller for input events.
> > > > >>
> > > > >> These two patches add platform driver support for PL061 probed by DT or
> > > > >> ACPI.
> > > > > 
> > > > > This certainly is incorrect for DT, and is probably wrong for ACPI too.
> > > > > DT creates amba devices, so binds via the amba device driver.
> > > > > 
> > > > 
> > > > Oh, sorry. The changes for DT are not necessary. But for ACPI I think it
> > > > needs to convert pl061 to platform device since ACPI doesn't support
> > > > AMBA devices. The Pl011 also does the same thing to support ACPI
> > > > probing. See drivers/tty/serial/amba-pl011.c
> > > 
> > > Maybe rather than having every AMBA driver also converted to a platform
> > > driver (which GregKH hates) maybe ACPI should support the AMBA bus
> > > instead?
> > > 
> > > Greg?
> > 
> > The ACPI developers have been doing work to allow a driver to handle
> > getting the resources from either DT or ACPI no matter what bus it is
> > on, so converting anything to a platform driver shouldn't be needed.
> > 
> > but I don't really know the details here, and this isn't being sent to
> > the ACPI mailing list, so I don't know what to suggest...
> 
> Greg,
> 
> You're the one who's said many times that you'd like to see platform
> devices and platform device drivers to go away.

Yes, of course, I hate the things :)

> So, when I give you
> the opportunity to comment on platform devices being used to augment
> existing device drivers such as the AMBA bus drivers (which you've
> also previously said is a more preferable way to use the driver model)
> why not support that view?

Because I don't know the whole story here, sorry.

> Right now, there seems to be a move to convert _all_ AMBA device drivers
> to become dual-drivers: an amba bus driver and a platform driver.  Is
> that something you really want to see, irrespective of ACPI issues?

Ick, no, I don't think that's wise at all.  Why not just make the needed
resources availble to _all_ bus drivers, then it doesn't matter if it's
AMBA, or ACPI, or USB, or anything else?

I thought that is what some recent patches did to the acpi/of core, but
maybe I was mistaken.

thanks,

greg k-h

[PATCH 0/2] drivers: PL061: Add platform driver probing support

[Linus Walleij]

On Fri, Aug 7, 2015 at 4:21 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Aug 03, 2015 at 08:09:35PM +0100, Russell King - ARM Linux wrote:

>> Right now, there seems to be a move to convert _all_ AMBA device drivers
>> to become dual-drivers: an amba bus driver and a platform driver.  Is
>> that something you really want to see, irrespective of ACPI issues?
>
> Ick, no, I don't think that's wise at all.  Why not just make the needed
> resources availble to _all_ bus drivers, then it doesn't matter if it's
> AMBA, or ACPI, or USB, or anything else?

Amen to that.

> I thought that is what some recent patches did to the acpi/of core, but
> maybe I was mistaken.

I think you're referring to the initiative to provide query functions that will
work the same on device trees and ACPI devices, that is to get a key/value
from a certain struct device * no matter if it came from a DT or ACPI.

This is about how the core hardware description spawns devices. While
DT will (correctly) spawn AMBA devices, the ACPI code apparently
doesn't. But it should. of_amba_device_create() even shows exactly
how to do it.

What I fear is that ACPI actually has no way to tell whether a device is
an AMBA device but rather thinks the whole world consists of
platform_devices, and then *THAT* needs to be fixed.

Yours,
Linus Walleij