Hopefully, this will be the last lot of enablement patches for Snowball.
The only thing to follow this patch-set is clean-ups and enablement of
drivers/power.
Some of these patches have been on the MLs before, but are yet
to be reviewed/SOB the correct people.
In-Reply-To:
arch/arm/boot/dts/db8500.dtsi | 33 +++++++----
arch/arm/mach-ux500/cpu-db8500.c | 1 -
drivers/i2c/busses/i2c-nomadik.c | 40 ++++++++++++-
drivers/input/misc/ab8500-ponkey.c | 6 ++
drivers/mfd/Kconfig | 11 +---
drivers/mfd/ab8500-core.c | 114 +++++++++++++++++++++---------------
drivers/mfd/ab8500-gpadc.c | 3 +-
drivers/mfd/db8500-prcmu.c | 7 ++-
drivers/rtc/rtc-ab8500.c | 10 +++-
drivers/usb/otg/ab8500-usb.c | 6 ++
include/linux/mfd/abx500/ab8500.h | 4 ++
11 files changed, 159 insertions(+), 76 deletions(-)
The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/ab8500-gpadc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c
index b86fd8e..b6cbc3ba 100644
--- a/drivers/mfd/ab8500-gpadc.c
+++ b/drivers/mfd/ab8500-gpadc.c
@@ -599,7 +599,8 @@ static int __devinit ab8500_gpadc_probe(struct platform_device *pdev)
/* Register interrupt - SwAdcComplete */
ret = request_threaded_irq(gpadc->irq, NULL,
ab8500_bm_gpswadcconvend_handler,
- IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc", gpadc);
+ IRQF_ONESHOT | IRQF_NO_SUSPEND | IRQF_SHARED,
+ "ab8500-gpadc", gpadc);
if (ret < 0) {
dev_err(gpadc->dev, "Failed to register interrupt, irq: %d\n",
gpadc->irq);
--
1.7.9.5
The AB8500 is soon to have its own IRQ domain. For this to be useful
the driver needs to be initialised earlier in the boot sequence. Here
we move initialisation forward from arch_initcall to core_initcall time.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/ab8500-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index dac0e29..4dc5024 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -1484,7 +1484,7 @@ static void __exit ab8500_core_exit(void)
{
platform_driver_unregister(&ab8500_core_driver);
}
-arch_initcall(ab8500_core_init);
+core_initcall(ab8500_core_init);
module_exit(ab8500_core_exit);
MODULE_AUTHOR("Mattias Wallin, Srinidhi Kasagar, Rabin Vincent");
--
1.7.9.5
During ab8500-core clean-up the Kconfig entry for AB8500_I2C_CORE
was left remnant. This patch simply removes it.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/Kconfig | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index e129c82..170072e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -703,16 +703,6 @@ config AB8500_CORE
the irq_chip parts for handling the Mixed Signal chip events.
This chip embeds various other multimedia funtionalities as well.
-config AB8500_I2C_CORE
- bool "AB8500 register access via PRCMU I2C"
- depends on AB8500_CORE && MFD_DB8500_PRCMU
- default y
- help
- This enables register access to the AB8500 chip via PRCMU I2C.
- The AB8500 chip can be accessed via SPI or I2C. On DB8500 hardware
- the I2C bus is connected to the Power Reset
- and Mangagement Unit, PRCMU.
-
config AB8500_DEBUG
bool "Enable debug info via debugfs"
depends on AB8500_CORE && DEBUG_FS
--
1.7.9.5
Here we move the i2c-nomadik's default settings into the driver
rather than specifying them from platform code. At the time of
this writing we only have one user, the u8500. As new users are
added, it is expected that they will be Device Tree compliant.
If this is the case, we will look up their initialisation values
by compatible entry, then apply them forthwith.
Cc: [email protected]
Acked-by: Linus Walleij <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/i2c/busses/i2c-nomadik.c | 40 +++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a92440d..1ffdf67 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
#include <linux/io.h>
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/of_device.h>
#include <plat/i2c.h>
@@ -899,15 +900,51 @@ static const struct i2c_algorithm nmk_i2c_algo = {
.functionality = nmk_i2c_functionality
};
+static struct nmk_i2c_controller u8500_i2c = {
+ /*
+ * slave data setup time, which is
+ * 250 ns,100ns,10ns which is 14,6,2
+ * respectively for a 48 Mhz
+ * i2c clock
+ */
+ .slsu = 0xe,
+ /* Tx FIFO threshold */
+ .tft = 1,
+ /* Rx FIFO threshold */
+ .rft = 8,
+ /* std. mode operation */
+ .clk_freq = 100000,
+ /* Slave response timeout(ms) */
+ .timeout = 200,
+ .sm = I2C_FREQ_MODE_FAST,
+};
+
+
+static const struct of_device_id nmk_gpio_match[] = {
+ { .compatible = "st,nomadik-i2c", .data = &u8500_i2c, },
+ {}
+};
+
static int __devinit nmk_i2c_probe(struct platform_device *pdev)
{
int ret = 0;
struct resource *res;
- struct nmk_i2c_controller *pdata =
+ const struct nmk_i2c_controller *pdata =
pdev->dev.platform_data;
+ const struct of_device_id *of_id =
+ of_match_device(nmk_gpio_match, &pdev->dev);
struct nmk_i2c_dev *dev;
struct i2c_adapter *adap;
+ if (!pdata) {
+ if (of_id && of_id->data)
+ /* Looks like we're booting via Device Tree. */
+ pdata = of_id->data;
+ else
+ /* No i2c configuration found, using the default. */
+ pdata = &u8500_i2c;
+ }
+
dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -1043,6 +1080,7 @@ static struct platform_driver nmk_i2c_driver = {
.owner = THIS_MODULE,
.name = DRIVER_NAME,
.pm = &nmk_i2c_pm,
+ .of_match_table = nmk_gpio_match,
},
.probe = nmk_i2c_probe,
.remove = __devexit_p(nmk_i2c_remove),
--
1.7.9.5
This patch adds the correct compatible string for use during Device Tree
population. Without it the DB8500 PRCMU will not be probed.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/db8500-prcmu.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 50e83dc5..c702f18 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -3004,11 +3004,16 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
no_irq_return:
return err;
}
+static const struct of_device_id db8500_prcmu_match[] = {
+ { .compatible = "stericsson,db8500-prcmu", },
+ { },
+};
static struct platform_driver db8500_prcmu_driver = {
.driver = {
.name = "db8500-prcmu",
.owner = THIS_MODULE,
+ .of_match_table = db8500_prcmu_match,
},
.probe = db8500_prcmu_probe,
};
--
1.7.9.5
The AB8500 driver has now been provided with IRQ domain support. This
means we can request IRQs from any of it's uses via Device Tree. This
patch advertises the AB8500 as an Interrupt Controller and provides the
correct calls in the format the driver expects.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/db8500.dtsi | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 01f9a9d..55a36ae 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -312,24 +312,26 @@
compatible = "stericsson,ab8500";
reg = <5>; /* mailbox 5 is i2c */
interrupts = <0 40 0x4>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
ab8500-gpadc {
compatible = "stericsson,ab8500-gpadc";
- interrupts = <0 32 0x4
- 0 39 0x4>;
+ interrupts = <32 0x4
+ 39 0x4>;
interrupt-names = "HW_CONV_END", "SW_CONV_END";
vddadc-supply = <&ab8500_ldo_tvout_reg>;
};
ab8500-usb {
compatible = "stericsson,ab8500-usb";
- interrupts = < 0 90 0x4
- 0 96 0x4
- 0 14 0x4
- 0 15 0x4
- 0 79 0x4
- 0 74 0x4
- 0 75 0x4>;
+ interrupts = < 90 0x4
+ 96 0x4
+ 14 0x4
+ 15 0x4
+ 79 0x4
+ 74 0x4
+ 75 0x4>;
interrupt-names = "ID_WAKEUP_R",
"ID_WAKEUP_F",
"VBUS_DET_F",
@@ -344,8 +346,8 @@
ab8500-ponkey {
compatible = "stericsson,ab8500-ponkey";
- interrupts = <0 6 0x4
- 0 7 0x4>;
+ interrupts = <6 0x4
+ 7 0x4>;
interrupt-names = "ONKEY_DBF", "ONKEY_DBR";
};
--
1.7.9.5
Here we add a node for the AB8500 Real Time Clock in all devices
supporting the DB8500. The AB8500 RTC driver makes use of named
interrupts we provide support for this too.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/db8500.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 55a36ae..26f895f 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -315,6 +315,13 @@
interrupt-controller;
#interrupt-cells = <2>;
+ ab8500-rtc {
+ compatible = "stericsson,ab8500-rtc";
+ interrupts = <17 0x4
+ 18 0x4>;
+ interrupt-names = "60S", "ALARM";
+ };
+
ab8500-gpadc {
compatible = "stericsson,ab8500-gpadc";
interrupts = <32 0x4
--
1.7.9.5
During a Device Tree boot, all probing will now be completed on parse
of the Device Tree binary. In the same patch we remove platform
registration of the Real Time Clock.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/db8500.dtsi | 2 +-
arch/arm/mach-ux500/cpu-db8500.c | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 26f895f..e90a449 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -48,7 +48,7 @@
};
rtc@80154000 {
- compatible = "stericsson,db8500-rtc";
+ compatible = "arm,rtc-pl031", "arm,primecell";
reg = <0x80154000 0x1000>;
interrupts = <0 18 0x4>;
};
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 7d5d090..a823993 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -237,7 +237,6 @@ struct device * __init u8500_of_init_devices(void)
parent = db8500_soc_device_init();
- db8500_add_rtc(parent);
db8500_add_usb(parent, usb_db8500_rx_dma_cfg, usb_db8500_tx_dma_cfg);
platform_device_register_data(parent,
"cpufreq-u8500", -1, NULL, 0);
--
1.7.9.5
Without this patch, if Device Tree is enabled the AB8500 USB OTG driver
wouldn't get probed at all, as there is no reference to it from platform
code. This patch ensures the driver is probed during normal DT start-up.
Cc: Felipe Balbi <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
drivers/usb/otg/ab8500-usb.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/otg/ab8500-usb.c b/drivers/usb/otg/ab8500-usb.c
index a84af67..9799ac6 100644
--- a/drivers/usb/otg/ab8500-usb.c
+++ b/drivers/usb/otg/ab8500-usb.c
@@ -569,12 +569,18 @@ static int __devexit ab8500_usb_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id ab8500_usb_match[] = {
+ { .compatible = "stericsson,ab8500-usb", },
+ {},
+};
+
static struct platform_driver ab8500_usb_driver = {
.probe = ab8500_usb_probe,
.remove = __devexit_p(ab8500_usb_remove),
.driver = {
.name = "ab8500-usb",
.owner = THIS_MODULE,
+ .of_match_table = ab8500_usb_match,
},
};
--
1.7.9.5
This patch will allow the ab8500-ponkey driver to be probed during
boot when Device Tree is enabled.
Cc: [email protected]
Acked-by: Linus Walleij <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/input/misc/ab8500-ponkey.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/misc/ab8500-ponkey.c b/drivers/input/misc/ab8500-ponkey.c
index 350fd0c..394aa1f 100644
--- a/drivers/input/misc/ab8500-ponkey.c
+++ b/drivers/input/misc/ab8500-ponkey.c
@@ -131,10 +131,16 @@ static int __devexit ab8500_ponkey_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id ab8500_ponkey_match[] = {
+ { .compatible = "stericsson,ab8500-ponkey", },
+ {}
+};
+
static struct platform_driver ab8500_ponkey_driver = {
.driver = {
.name = "ab8500-poweron-key",
.owner = THIS_MODULE,
+ .of_match_table = ab8500_ponkey_match,
},
.probe = ab8500_ponkey_probe,
.remove = __devexit_p(ab8500_ponkey_remove),
--
1.7.9.5
As the AB8500 is an IRQ controller in its own right, here we provide
the AB8500 driver with IRQ domain support. This is required if we wish
to reference any of its IRQs from a platform's Device Tree.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/Kconfig | 1 +
drivers/mfd/ab8500-core.c | 112 +++++++++++++++++++++----------------
include/linux/mfd/abx500/ab8500.h | 4 ++
3 files changed, 70 insertions(+), 47 deletions(-)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 170072e..e3637c2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -696,6 +696,7 @@ config AB8500_CORE
bool "ST-Ericsson AB8500 Mixed Signal Power Management chip"
depends on GENERIC_HARDIRQS && ABX500_CORE && MFD_DB8500_PRCMU
select MFD_CORE
+ select IRQ_DOMAIN
help
Select this option to enable access to AB8500 power management
chip. This connects to U8500 either on the SSP/SPI bus (deprecated
diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 4dc5024..82cd31d 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -11,6 +11,7 @@
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/irq.h>
+#include <linux/irqdomain.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/module.h>
@@ -361,7 +362,7 @@ static void ab8500_irq_sync_unlock(struct irq_data *data)
static void ab8500_irq_mask(struct irq_data *data)
{
struct ab8500 *ab8500 = irq_data_get_irq_chip_data(data);
- int offset = data->irq - ab8500->irq_base;
+ int offset = data->irq;
int index = offset / 8;
int mask = 1 << (offset % 8);
@@ -371,7 +372,7 @@ static void ab8500_irq_mask(struct irq_data *data)
static void ab8500_irq_unmask(struct irq_data *data)
{
struct ab8500 *ab8500 = irq_data_get_irq_chip_data(data);
- int offset = data->irq - ab8500->irq_base;
+ int offset = data->irq;
int index = offset / 8;
int mask = 1 << (offset % 8);
@@ -501,7 +502,7 @@ static irqreturn_t ab8500_irq(int irq, void *dev)
int bit = __ffs(value);
int line = i * 8 + bit;
- handle_nested_irq(ab8500->irq_base + line);
+ handle_nested_irq(line);
value &= ~(1 << bit);
} while (value);
@@ -510,38 +511,51 @@ static irqreturn_t ab8500_irq(int irq, void *dev)
return IRQ_HANDLED;
}
-static int ab8500_irq_init(struct ab8500 *ab8500)
+/**
+ * ab8500_irq_get_virq(): Map an interrupt on a chip to a virtual IRQ
+ *
+ * Useful for drivers to request their own IRQs.
+ *
+ * @ab8500: ab8500_irq controller to operate on.
+ * @irq: index of the interrupt requested in the chip IRQs
+ */
+int ab8500_irq_get_virq(struct ab8500 *ab8500, int irq)
{
- int base = ab8500->irq_base;
- int irq;
- int num_irqs;
+ if (!ab8500)
+ return -EINVAL;
- if (is_ab9540(ab8500))
- num_irqs = AB9540_NR_IRQS;
- else if (is_ab8505(ab8500))
- num_irqs = AB8505_NR_IRQS;
- else
- num_irqs = AB8500_NR_IRQS;
+ return irq_create_mapping(ab8500->domain, irq);
+}
+EXPORT_SYMBOL_GPL(ab8500_irq_get_virq);
+
+static int ab8500_irq_map(struct irq_domain *d, unsigned int virq,
+ irq_hw_number_t hwirq)
+{
+ struct ab8500 *ab8500 = d->host_data;
- for (irq = base; irq < base + num_irqs; irq++) {
- irq_set_chip_data(irq, ab8500);
- irq_set_chip_and_handler(irq, &ab8500_irq_chip,
- handle_simple_irq);
- irq_set_nested_thread(irq, 1);
+ if (!ab8500)
+ return -EINVAL;
+
+ irq_set_chip_data(virq, ab8500);
+ irq_set_chip_and_handler(virq, &ab8500_irq_chip,
+ handle_simple_irq);
+ irq_set_nested_thread(virq, 1);
#ifdef CONFIG_ARM
- set_irq_flags(irq, IRQF_VALID);
+ set_irq_flags(virq, IRQF_VALID);
#else
- irq_set_noprobe(irq);
+ irq_set_noprobe(virq);
#endif
- }
return 0;
}
-static void ab8500_irq_remove(struct ab8500 *ab8500)
+static struct irq_domain_ops ab8500_irq_ops = {
+ .map = ab8500_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+static int ab8500_irq_init(struct ab8500 *ab8500, struct device_node *np)
{
- int base = ab8500->irq_base;
- int irq;
int num_irqs;
if (is_ab9540(ab8500))
@@ -551,13 +565,22 @@ static void ab8500_irq_remove(struct ab8500 *ab8500)
else
num_irqs = AB8500_NR_IRQS;
- for (irq = base; irq < base + num_irqs; irq++) {
-#ifdef CONFIG_ARM
- set_irq_flags(irq, 0);
-#endif
- irq_set_chip_and_handler(irq, NULL, NULL);
- irq_set_chip_data(irq, NULL);
+ if (np) {
+ ab8500->domain = irq_domain_add_linear(
+ np, num_irqs, &ab8500_irq_ops, ab8500);
+ }
+ else {
+ ab8500->domain = irq_domain_add_legacy(
+ NULL, num_irqs, ab8500->irq_base,
+ 0, &ab8500_irq_ops, ab8500);
+ }
+
+ if (!ab8500->domain) {
+ dev_err(ab8500->dev, "Failed to create irqdomain\n");
+ return -ENOSYS;
}
+
+ return 0;
}
int ab8500_suspend(struct ab8500 *ab8500)
@@ -1233,11 +1256,9 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
if (plat)
ab8500->irq_base = plat->irq_base;
- else if (np)
- ret = of_property_read_u32(np, "stericsson,irq-base", &ab8500->irq_base);
- if (!ab8500->irq_base) {
- dev_info(&pdev->dev, "couldn't find irq-base\n");
+ if (!(ab8500->irq_base || np)) {
+ dev_info(&pdev->dev, "couldn't find irq-base and not doing DT boot\n");
ret = -EINVAL;
goto out_free_ab8500;
}
@@ -1323,7 +1344,7 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
AB8500_SWITCH_OFF_STATUS, &value);
if (ret < 0)
return ret;
- dev_info(ab8500->dev, "switch off status: %#x", value);
+ dev_info(ab8500->dev, "switch off status: %#x\n", value);
if (plat && plat->init)
plat->init(ab8500);
@@ -1352,8 +1373,8 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
for (i = 0; i < ab8500->mask_size; i++)
ab8500->mask[i] = ab8500->oldmask[i] = 0xff;
- if (ab8500->irq_base) {
- ret = ab8500_irq_init(ab8500);
+ if (ab8500->irq_base || np) {
+ ret = ab8500_irq_init(ab8500, np);
if (ret)
goto out_freeoldmask;
@@ -1370,7 +1391,7 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
IRQF_ONESHOT | IRQF_NO_SUSPEND,
"ab8500", ab8500);
if (ret)
- goto out_removeirq;
+ goto out_freeoldmask;
}
if (!np) {
@@ -1417,15 +1438,12 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
&ab8500_attr_group);
if (ret)
dev_err(ab8500->dev, "error creating sysfs entries\n");
- else
- return ret;
+
+ return ret;
out_freeirq:
- if (ab8500->irq_base)
+ if (ab8500->irq_base || np)
free_irq(ab8500->irq, ab8500);
-out_removeirq:
- if (ab8500->irq_base)
- ab8500_irq_remove(ab8500);
out_freeoldmask:
kfree(ab8500->oldmask);
out_freemask:
@@ -1439,16 +1457,16 @@ out_free_ab8500:
static int __devexit ab8500_remove(struct platform_device *pdev)
{
struct ab8500 *ab8500 = platform_get_drvdata(pdev);
+ struct device_node *np = pdev->dev.of_node;
if (is_ab9540(ab8500))
sysfs_remove_group(&ab8500->dev->kobj, &ab9540_attr_group);
else
sysfs_remove_group(&ab8500->dev->kobj, &ab8500_attr_group);
mfd_remove_devices(ab8500->dev);
- if (ab8500->irq_base) {
+ if (ab8500->irq_base || np)
free_irq(ab8500->irq, ab8500);
- ab8500_irq_remove(ab8500);
- }
+
kfree(ab8500->oldmask);
kfree(ab8500->mask);
kfree(ab8500);
diff --git a/include/linux/mfd/abx500/ab8500.h b/include/linux/mfd/abx500/ab8500.h
index 91dd3ef..48f126c 100644
--- a/include/linux/mfd/abx500/ab8500.h
+++ b/include/linux/mfd/abx500/ab8500.h
@@ -227,6 +227,7 @@ enum ab8500_version {
* @irq_lock: genirq bus lock
* @transfer_ongoing: 0 if no transfer ongoing
* @irq: irq line
+ * @irq_domain: irq domain
* @version: chip version id (e.g. ab8500 or ab9540)
* @chip_id: chip revision id
* @write: register write
@@ -247,6 +248,7 @@ struct ab8500 {
atomic_t transfer_ongoing;
int irq_base;
int irq;
+ struct irq_domain *domain;
enum ab8500_version version;
u8 chip_id;
@@ -336,4 +338,6 @@ static inline int is_ab8500_2p0(struct ab8500 *ab)
return (is_ab8500(ab) && (ab->chip_id == AB8500_CUT2P0));
}
+int ab8500_irq_get_virq(struct ab8500 *ab8500, int irq);
+
#endif /* MFD_AB8500_H */
--
1.7.9.5
Now the AB8500 has its own IRQ domain it needs to be initialised earlier
in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to
reflect this change for the PRCMU driver too.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/db8500-prcmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index c702f18..1aa0bb7 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -3023,7 +3023,7 @@ static int __init db8500_prcmu_init(void)
return platform_driver_register(&db8500_prcmu_driver);
}
-arch_initcall(db8500_prcmu_init);
+core_initcall(db8500_prcmu_init);
MODULE_AUTHOR("Mattias Nilsson <[email protected]>");
MODULE_DESCRIPTION("DB8500 PRCM Unit driver");
--
1.7.9.5
The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
Cc: Alessandro Zummo <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
drivers/rtc/rtc-ab8500.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-ab8500.c b/drivers/rtc/rtc-ab8500.c
index 4bcf9ca..b11a2ec 100644
--- a/drivers/rtc/rtc-ab8500.c
+++ b/drivers/rtc/rtc-ab8500.c
@@ -422,7 +422,7 @@ static int __devinit ab8500_rtc_probe(struct platform_device *pdev)
}
err = request_threaded_irq(irq, NULL, rtc_alarm_handler,
- IRQF_NO_SUSPEND, "ab8500-rtc", rtc);
+ IRQF_NO_SUSPEND | IRQF_ONESHOT, "ab8500-rtc", rtc);
if (err < 0) {
rtc_device_unregister(rtc);
return err;
--
1.7.9.5
Without this patch, if Device Tree is enabled the AB8500 RTC wouldn't
get probed at all, as there is no reference to it from platform code.
This patch ensures the driver is probed during normal DT start-up.
Cc: Alessandro Zummo <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
drivers/rtc/rtc-ab8500.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-ab8500.c b/drivers/rtc/rtc-ab8500.c
index b11a2ec..116c23d 100644
--- a/drivers/rtc/rtc-ab8500.c
+++ b/drivers/rtc/rtc-ab8500.c
@@ -17,6 +17,7 @@
#include <linux/mfd/abx500.h>
#include <linux/mfd/abx500/ab8500.h>
#include <linux/delay.h>
+#include <linux/of.h>
#define AB8500_RTC_SOFF_STAT_REG 0x00
#define AB8500_RTC_CC_CONF_REG 0x01
@@ -430,7 +431,6 @@ static int __devinit ab8500_rtc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, rtc);
-
err = ab8500_sysfs_rtc_register(&pdev->dev);
if (err) {
dev_err(&pdev->dev, "sysfs RTC failed to register\n");
@@ -454,10 +454,16 @@ static int __devexit ab8500_rtc_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id ab8500_rtc_match[] = {
+ { .compatible = "stericsson,ab8500-rtc", },
+ {}
+};
+
static struct platform_driver ab8500_rtc_driver = {
.driver = {
.name = "ab8500-rtc",
.owner = THIS_MODULE,
+ .of_match_table = ab8500_rtc_match,
},
.probe = ab8500_rtc_probe,
.remove = __devexit_p(ab8500_rtc_remove),
--
1.7.9.5
On Monday 11 June 2012, Lee Jones wrote:
> The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
>
> Cc: Alessandro Zummo <[email protected]>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
Isn't this a bug fix that should be applied to the stable kernels too?
Arnd
On 11/06/12 16:31, Arnd Bergmann wrote:
> On Monday 11 June 2012, Lee Jones wrote:
>> The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
>>
>> Cc: Alessandro Zummo<[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Lee Jones<[email protected]>
>
> Isn't this a bug fix that should be applied to the stable kernels too?
Possibly. Should we alert Greg (CC'ed - alerted)?
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Jun 11, 2012 at 04:37:18PM +0100, Lee Jones wrote:
> On 11/06/12 16:31, Arnd Bergmann wrote:
> >On Monday 11 June 2012, Lee Jones wrote:
> >>The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
> >>
> >>Cc: Alessandro Zummo<[email protected]>
> >>Cc: [email protected]
> >>Signed-off-by: Lee Jones<[email protected]>
> >
> >Isn't this a bug fix that should be applied to the stable kernels too?
>
> Possibly. Should we alert Greg (CC'ed - alerted)?
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
On 11/06/12 16:51, Greg Kroah-Hartman wrote:
> On Mon, Jun 11, 2012 at 04:37:18PM +0100, Lee Jones wrote:
>> On 11/06/12 16:31, Arnd Bergmann wrote:
>>> On Monday 11 June 2012, Lee Jones wrote:
>>>> The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
>>>>
>>>> Cc: Alessandro Zummo<[email protected]>
>>>> Cc: [email protected]
>>>> Signed-off-by: Lee Jones<[email protected]>
>>>
>>> Isn't this a bug fix that should be applied to the stable kernels too?
>>
>> Possibly. Should we alert Greg (CC'ed - alerted)?
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read Documentation/stable_kernel_rules.txt
> for how to do this properly.
>
> </formletter>
I wasn't submitting, merely asking if it would be suitable for inclusion. :)
If it is, I'd be happy to _properly_ submit this and its ab8500-gpadc counterpart.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Jun 11, 2012 at 04:25:02PM +0100, Lee Jones wrote:
> Here we move the i2c-nomadik's default settings into the driver
> rather than specifying them from platform code. At the time of
> this writing we only have one user, the u8500. As new users are
> added, it is expected that they will be Device Tree compliant.
> If this is the case, we will look up their initialisation values
> by compatible entry, then apply them forthwith.
>
> Cc: [email protected]
> Acked-by: Linus Walleij <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/i2c/busses/i2c-nomadik.c | 40 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index a92440d..1ffdf67 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -23,6 +23,7 @@
> #include <linux/io.h>
> #include <linux/regulator/consumer.h>
> #include <linux/pm_runtime.h>
> +#include <linux/of_device.h>
>
> #include <plat/i2c.h>
>
> @@ -899,15 +900,51 @@ static const struct i2c_algorithm nmk_i2c_algo = {
> .functionality = nmk_i2c_functionality
> };
>
> +static struct nmk_i2c_controller u8500_i2c = {
> + /*
> + * slave data setup time, which is
> + * 250 ns,100ns,10ns which is 14,6,2
> + * respectively for a 48 Mhz
> + * i2c clock
> + */
> + .slsu = 0xe,
> + /* Tx FIFO threshold */
Please put these comments directly after the members they describe.
And make sure you use tabs for indentation all over the patch instead of
spaces. checkpatch.pl will help you to get the formal things right.
> + .tft = 1,
> + /* Rx FIFO threshold */
> + .rft = 8,
> + /* std. mode operation */
> + .clk_freq = 100000,
> + .timeout = 200, /* Slave response timeout(ms) */
> + .sm = I2C_FREQ_MODE_FAST,
> +};
> +
> +
> +static const struct of_device_id nmk_gpio_match[] = {
> + { .compatible = "st,nomadik-i2c", .data = &u8500_i2c, },
> + {}
> +};
> +
> static int __devinit nmk_i2c_probe(struct platform_device *pdev)
> {
> int ret = 0;
> struct resource *res;
> - struct nmk_i2c_controller *pdata =
> + const struct nmk_i2c_controller *pdata =
> pdev->dev.platform_data;
> + const struct of_device_id *of_id =
> + of_match_device(nmk_gpio_match, &pdev->dev);
> struct nmk_i2c_dev *dev;
> struct i2c_adapter *adap;
>
> + if (!pdata) {
> + if (of_id && of_id->data)
> + /* Looks like we're booting via Device Tree. */
> + pdata = of_id->data;
> + else
> + /* No i2c configuration found, using the default. */
> + pdata = &u8500_i2c;
> + }
> +
> dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
> if (!dev) {
> dev_err(&pdev->dev, "cannot allocate memory\n");
> @@ -1043,6 +1080,7 @@ static struct platform_driver nmk_i2c_driver = {
> .owner = THIS_MODULE,
> .name = DRIVER_NAME,
> .pm = &nmk_i2c_pm,
> + .of_match_table = nmk_gpio_match,
> },
> .probe = nmk_i2c_probe,
> .remove = __devexit_p(nmk_i2c_remove),
> --
Thanks,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones <[email protected]> wrote:
> As new users are
> added, it is expected that they will be Device Tree compliant.
> If this is the case, we will look up their initialisation values
> by compatible entry, then apply them forthwith.
(...)
> +static struct nmk_i2c_controller u8500_i2c = {
> + ? ? ? /*
> + ? ? ? ?* slave data setup time, which is
> + ? ? ? ?* 250 ns,100ns,10ns which is 14,6,2
> + ? ? ? ?* respectively for a 48 Mhz
> + ? ? ? ?* i2c clock
> + ? ? ? ?*/
> + ? ? ? .slsu ? ? ? ? ? = 0xe,
> + ? ? ? /* Tx FIFO threshold */
> + ? ? ? .tft ? ? ? ? ? ?= 1,
> + ? ? ? /* Rx FIFO threshold */
> + ? ? ? .rft ? ? ? ? ? ?= 8,
> + ? ? ? /* std. mode operation */
> + ? ? ? .clk_freq ? ? ? = 100000,
> + ? ? ? /* Slave response timeout(ms) */
> + ? ? ? .timeout ? ? ? ?= 200,
> + ? ? ? .sm ? ? ? ? ? ? = I2C_FREQ_MODE_FAST,
> +};
So why don't we create proper device tree bindings for the above platform
data?
For example several driver under
Documentation/devicetree/bindings/i2c/*
define the .clk_freq above as "clock-frequency"
samsung-i2c even has this:
samsung,i2c-sda-delay = <100>;
samsung,i2c-max-bus-freq = <100000>;
Where i2c-sda-delay corresponds to slsu above.
I suspect .sm can be derived from the frequency so it
is "FAST" whenever the frequency > 100000.
This looks to me like a way to push the burden of doing the real
device tree binding for the above to the next user.
(Which will likely be me, so therefore I care a bit ...)
Yours,
Linus Walleij
On Mon, Jun 11, 2012 at 6:01 PM, Lee Jones <[email protected]> wrote:
> I wasn't submitting, merely asking if it would be suitable for inclusion. :)
It is.
> If it is, I'd be happy to _properly_ submit this and its ab8500-gpadc counterpart.
Please do.
Acked-by: Linus Walleij <[email protected]>
Thanks,
Linus Walleij
On Mon, Jun 11, 2012 at 5:24 PM, Lee Jones <[email protected]> wrote:
> The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Thanks,
Linus Walleij
On Mon, Jun 11, 2012 at 5:24 PM, Lee Jones <[email protected]> wrote:
> Without this patch, if Device Tree is enabled the AB8500 RTC wouldn't
> get probed at all, as there is no reference to it from platform code.
> This patch ensures the driver is probed during normal DT start-up.
>
> Cc: Alessandro Zummo <[email protected]>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Thanks,
Linus Walleij
On Mon, Jun 11, 2012 at 5:24 PM, Lee Jones <[email protected]> wrote:
> During ab8500-core clean-up the Kconfig entry for AB8500_I2C_CORE
> was left remnant. This patch simply removes it.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Sam, will you pick this patch for the -rc series?
Yours,
Linus Walleij
On Mon, Jun 11, 2012 at 5:24 PM, Lee Jones <[email protected]> wrote:
> This patch adds the correct compatible string for use during Device Tree
> population. Without it the DB8500 PRCMU will not be probed.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Thanks,
Linus Walleij
On Mon, Jun 11, 2012 at 5:24 PM, Lee Jones <[email protected]> wrote:
> Now the AB8500 has its own IRQ domain
But that does not appear until patch 8 in this series? Are the patches in
the wrong order? Or does this need rewording?
> it needs to be initialised earlier
> in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to
> reflect this change for the PRCMU driver too.
Hm what shall we do when we run out of initlevels? I think this was the
kind of thing that deferred probe should solve. Usually changing this kind
of thing has side effects so I'm a bit hesitant.
Yours,
Linus Walleij
On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones <[email protected]> wrote:
> The AB8500 is soon to have its own IRQ domain. For this to be useful
> the driver needs to be initialised earlier in the boot sequence. Here
> we move initialisation forward from arch_initcall to core_initcall time.
I don't understand why using IRQ domain makes it necessary to
initialize the driver earlier in the init process. Please explain to me,
I need tutoring here.
Thanks,
Linus Walleij
On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones <[email protected]> wrote:
> As the AB8500 is an IRQ controller in its own right, here we provide
> the AB8500 driver with IRQ domain support. This is required if we wish
> to reference any of its IRQs from a platform's Device Tree.
OK..
> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
(...)
> -static int ab8500_irq_init(struct ab8500 *ab8500)
> +/**
> + * ab8500_irq_get_virq(): Map an interrupt on a chip to a virtual IRQ
> + *
> + * Useful for drivers to request their own IRQs.
Check style against Documentation/kernel-doc-nano-HOWTO.txt
verbos explanation follows argument documentation.
> + *
> + * @ab8500: ab8500_irq controller to operate on.
> + * @irq: index of the interrupt requested in the chip IRQs
> + */
> +int ab8500_irq_get_virq(struct ab8500 *ab8500, int irq)
> ?{
> - ? ? ? int base = ab8500->irq_base;
> - ? ? ? int irq;
> - ? ? ? int num_irqs;
> + ? ? ? if (!ab8500)
> + ? ? ? ? ? ? ? return -EINVAL;
>
> - ? ? ? if (is_ab9540(ab8500))
> - ? ? ? ? ? ? ? num_irqs = AB9540_NR_IRQS;
> - ? ? ? else if (is_ab8505(ab8500))
> - ? ? ? ? ? ? ? num_irqs = AB8505_NR_IRQS;
> - ? ? ? else
> - ? ? ? ? ? ? ? num_irqs = AB8500_NR_IRQS;
> + ? ? ? return irq_create_mapping(ab8500->domain, irq);
> +}
> +EXPORT_SYMBOL_GPL(ab8500_irq_get_virq);
(...)
> @@ -1233,11 +1256,9 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
>
> ? ? ? ?if (plat)
> ? ? ? ? ? ? ? ?ab8500->irq_base = plat->irq_base;
> - ? ? ? else if (np)
> - ? ? ? ? ? ? ? ret = of_property_read_u32(np, "stericsson,irq-base", &ab8500->irq_base);
So if we're not using the irq base thing anymore, should you also
delete it from the binding document too? (If there is no binding
doc something is wrong and you need to create it I guess...)
> diff --git a/include/linux/mfd/abx500/ab8500.h b/include/linux/mfd/abx500/ab8500.h
> index 91dd3ef..48f126c 100644
> --- a/include/linux/mfd/abx500/ab8500.h
> +++ b/include/linux/mfd/abx500/ab8500.h
> @@ -227,6 +227,7 @@ enum ab8500_version {
> ?* @irq_lock: genirq bus lock
> ?* @transfer_ongoing: 0 if no transfer ongoing
> ?* @irq: irq line
> + * @irq_domain: irq domain
> ?* @version: chip version id (e.g. ab8500 or ab9540)
> ?* @chip_id: chip revision id
> ?* @write: register write
> @@ -247,6 +248,7 @@ struct ab8500 {
> ? ? ? ?atomic_t ? ? ? ?transfer_ongoing;
> ? ? ? ?int ? ? ? ? ? ? irq_base;
> ? ? ? ?int ? ? ? ? ? ? irq;
> + ? ? ? struct irq_domain ?*domain;
Don't you need to forward-declare struct irq_domain?
I think you're just lucky to have it compiling... (Something
else included <linux/irqdomain.h> on the way here.)
Yours,
Linus Walleij
On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones <[email protected]> wrote:
> Without this patch, if Device Tree is enabled the AB8500 USB OTG driver
> wouldn't get probed at all, as there is no reference to it from platform
> code. This patch ensures the driver is probed during normal DT start-up.
>
> Cc: Felipe Balbi <[email protected]>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Thanks,
Linus Walleij
On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones <[email protected]> wrote:
> The AB8500 driver has now been provided with IRQ domain support. This
> means we can request IRQs from any of it's uses via Device Tree. This
> patch advertises the AB8500 as an Interrupt Controller and provides the
> correct calls in the format the driver expects.
>
> Signed-off-by: Lee Jones <[email protected]>
Looks good to me,
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones <[email protected]> wrote:
> Here we add a node for the AB8500 Real Time Clock in all devices
> supporting the DB8500. The AB8500 RTC driver makes use of named
> interrupts we provide support for this too.
>
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones <[email protected]> wrote:
> During a Device Tree boot, all probing will now be completed on parse
> of the Device Tree binary. In the same patch we remove platform
> registration of the Real Time Clock.
>
> Signed-off-by: Lee Jones <[email protected]>
Oh this is a bug fix really, well it's in the device tree so nevermind.
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On 11/06/12 20:05, Wolfram Sang wrote:
> On Mon, Jun 11, 2012 at 04:25:02PM +0100, Lee Jones wrote:
>> Here we move the i2c-nomadik's default settings into the driver
>> rather than specifying them from platform code. At the time of
>> this writing we only have one user, the u8500. As new users are
>> added, it is expected that they will be Device Tree compliant.
>> If this is the case, we will look up their initialisation values
>> by compatible entry, then apply them forthwith.
>>
>> Cc: [email protected]
>> Acked-by: Linus Walleij<[email protected]>
>> Signed-off-by: Lee Jones<[email protected]>
>> ---
>> drivers/i2c/busses/i2c-nomadik.c | 40 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>> index a92440d..1ffdf67 100644
>> --- a/drivers/i2c/busses/i2c-nomadik.c
>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>> @@ -23,6 +23,7 @@
>> #include<linux/io.h>
>> #include<linux/regulator/consumer.h>
>> #include<linux/pm_runtime.h>
>> +#include<linux/of_device.h>
>>
>> #include<plat/i2c.h>
>>
>> @@ -899,15 +900,51 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>> .functionality = nmk_i2c_functionality
>> };
>>
>> +static struct nmk_i2c_controller u8500_i2c = {
>> + /*
>> + * slave data setup time, which is
>> + * 250 ns,100ns,10ns which is 14,6,2
>> + * respectively for a 48 Mhz
>> + * i2c clock
>> + */
>> + .slsu = 0xe,
>> + /* Tx FIFO threshold */
>
> Please put these comments directly after the members they describe.
>
> And make sure you use tabs for indentation all over the patch instead of
> spaces. checkpatch.pl will help you to get the formal things right.
My apologies. This section of code was a direct copy and paste from
platform code (arch/arm/mach-ux500/board-mop500.c). It must have lost
some formatting en route. I'll make the changes you request and re-post.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 11/06/12 21:37, Linus Walleij wrote:
> On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones<[email protected]> wrote:
>
>> As new users are
>> added, it is expected that they will be Device Tree compliant.
>> If this is the case, we will look up their initialisation values
>> by compatible entry, then apply them forthwith.
> (...)
>> +static struct nmk_i2c_controller u8500_i2c = {
>> + /*
>> + * slave data setup time, which is
>> + * 250 ns,100ns,10ns which is 14,6,2
>> + * respectively for a 48 Mhz
>> + * i2c clock
>> + */
>> + .slsu = 0xe,
>> + /* Tx FIFO threshold */
>> + .tft = 1,
>> + /* Rx FIFO threshold */
>> + .rft = 8,
>> + /* std. mode operation */
>> + .clk_freq = 100000,
>> + /* Slave response timeout(ms) */
>> + .timeout = 200,
>> + .sm = I2C_FREQ_MODE_FAST,
>> +};
>
> So why don't we create proper device tree bindings for the above platform
> data?
>
> For example several driver under
> Documentation/devicetree/bindings/i2c/*
> define the .clk_freq above as "clock-frequency"
>
> samsung-i2c even has this:
> samsung,i2c-sda-delay =<100>;
> samsung,i2c-max-bus-freq =<100000>;
>
> Where i2c-sda-delay corresponds to slsu above.
>
> I suspect .sm can be derived from the frequency so it
> is "FAST" whenever the frequency> 100000.
>
> This looks to me like a way to push the burden of doing the real
> device tree binding for the above to the next user.
> (Which will likely be me, so therefore I care a bit ...)
On the contrary. This will avoid any added Device Tree complexity and
ensure that no extra vendor specific bindings are required. When a new
user wishes to use the driver, all they (you) have to do is create a new
struct with the platform specific information and add the entry to
nmk_gpio_match[], simples.
I've even added the logic to extract any information you provide via
nmk_gpio_match[] for use as platform data. This keeps it both out of
platform code and the Device Tree binary. Everyone's a winner. :)
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 11/06/12 22:33, Linus Walleij wrote:
> On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones<[email protected]> wrote:
>
>> As the AB8500 is an IRQ controller in its own right, here we provide
>> the AB8500 driver with IRQ domain support. This is required if we wish
>> to reference any of its IRQs from a platform's Device Tree.
>
> OK..
>
>> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> (...)
>> -static int ab8500_irq_init(struct ab8500 *ab8500)
>> +/**
>> + * ab8500_irq_get_virq(): Map an interrupt on a chip to a virtual IRQ
>> + *
>> + * Useful for drivers to request their own IRQs.
>
> Check style against Documentation/kernel-doc-nano-HOWTO.txt
> verbos explanation follows argument documentation.
Ah, good to know.
I just followed other examples in this case. I'll swap them over.
>> + *
>> + * @ab8500: ab8500_irq controller to operate on.
>> + * @irq: index of the interrupt requested in the chip IRQs
>> + */
>> +int ab8500_irq_get_virq(struct ab8500 *ab8500, int irq)
>> {
>> - int base = ab8500->irq_base;
>> - int irq;
>> - int num_irqs;
>> + if (!ab8500)
>> + return -EINVAL;
>>
>> - if (is_ab9540(ab8500))
>> - num_irqs = AB9540_NR_IRQS;
>> - else if (is_ab8505(ab8500))
>> - num_irqs = AB8505_NR_IRQS;
>> - else
>> - num_irqs = AB8500_NR_IRQS;
>> + return irq_create_mapping(ab8500->domain, irq);
>> +}
>> +EXPORT_SYMBOL_GPL(ab8500_irq_get_virq);
> (...)
>> @@ -1233,11 +1256,9 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
>>
>> if (plat)
>> ab8500->irq_base = plat->irq_base;
>> - else if (np)
>> - ret = of_property_read_u32(np, "stericsson,irq-base",&ab8500->irq_base);
>
> So if we're not using the irq base thing anymore, should you also
> delete it from the binding document too? (If there is no binding
> doc something is wrong and you need to create it I guess...)
No. A document is not required now, as we are using standard bindings.
>> diff --git a/include/linux/mfd/abx500/ab8500.h b/include/linux/mfd/abx500/ab8500.h
>> index 91dd3ef..48f126c 100644
>> --- a/include/linux/mfd/abx500/ab8500.h
>> +++ b/include/linux/mfd/abx500/ab8500.h
>> @@ -227,6 +227,7 @@ enum ab8500_version {
>> * @irq_lock: genirq bus lock
>> * @transfer_ongoing: 0 if no transfer ongoing
>> * @irq: irq line
>> + * @irq_domain: irq domain
>> * @version: chip version id (e.g. ab8500 or ab9540)
>> * @chip_id: chip revision id
>> * @write: register write
>> @@ -247,6 +248,7 @@ struct ab8500 {
>> atomic_t transfer_ongoing;
>> int irq_base;
>> int irq;
>> + struct irq_domain *domain;
>
> Don't you need to forward-declare struct irq_domain?
> I think you're just lucky to have it compiling... (Something
> else included<linux/irqdomain.h> on the way here.)
You're right. I'll make the changes and resubmit.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 11/06/12 22:43, Linus Walleij wrote:
> On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones<[email protected]> wrote:
>
>> During a Device Tree boot, all probing will now be completed on parse
>> of the Device Tree binary. In the same patch we remove platform
>> registration of the Real Time Clock.
>>
>> Signed-off-by: Lee Jones<[email protected]>
>
> Oh this is a bug fix really, well it's in the device tree so nevermind.
It is? How so?
> Acked-by: Linus Walleij<[email protected]>
>
> Yours,
> Linus Walleij
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 11/06/12 22:01, Linus Walleij wrote:
> On Mon, Jun 11, 2012 at 5:24 PM, Lee Jones<[email protected]> wrote:
>
>> Now the AB8500 has its own IRQ domain
>
> But that does not appear until patch 8 in this series? Are the patches in
> the wrong order? Or does this need rewording?
The patches are in the correct order. This needs to be done _before_ we
provide the AB8500 with its own domain. I guess the line should
reference future tense. We are only talking two patches in the future,
is it that important? I will change the wording slightly if it is.
>> it needs to be initialised earlier
>> in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to
>> reflect this change for the PRCMU driver too.
>
> Hm what shall we do when we run out of initlevels? I think this was the
> kind of thing that deferred probe should solve. Usually changing this kind
> of thing has side effects so I'm a bit hesitant.
Nothing seems to be broken by it. My Snowball still comes up and
everything that worked before continues to do so. How would you like me
to take this forward?
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 11/06/12 22:03, Linus Walleij wrote:
> On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones<[email protected]> wrote:
>
>> The AB8500 is soon to have its own IRQ domain. For this to be useful
>> the driver needs to be initialised earlier in the boot sequence. Here
>> we move initialisation forward from arch_initcall to core_initcall time.
>
> I don't understand why using IRQ domain makes it necessary to
> initialize the driver earlier in the init process. Please explain to me,
> I need tutoring here.
Perhaps I need to invest some time into deferring each user's probe()s
instead. Leave it with me I'll do some investigation.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 11/06/12 22:01, Linus Walleij wrote:
> On Mon, Jun 11, 2012 at 5:24 PM, Lee Jones<[email protected]> wrote:
>
>> Now the AB8500 has its own IRQ domain
>
> But that does not appear until patch 8 in this series? Are the patches in
> the wrong order? Or does this need rewording?
>
>> it needs to be initialised earlier
>> in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to
>> reflect this change for the PRCMU driver too.
>
> Hm what shall we do when we run out of initlevels? I think this was the
> kind of thing that deferred probe should solve. Usually changing this kind
> of thing has side effects so I'm a bit hesitant.
Ah yes, I remember now. The IRQ domain needs to be in place _before_ the
Device Tree is parsed by the Open Firmware subsystem. If it's not the
error "no irq domain found" is triggered and the IRQs are never mapped.
I'd be happy to take a second opinion, but I believe this (and the other
core_initcall patch) is required.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 11/06/12 22:03, Linus Walleij wrote:
> On Mon, Jun 11, 2012 at 5:25 PM, Lee Jones<[email protected]> wrote:
>
>> The AB8500 is soon to have its own IRQ domain. For this to be useful
>> the driver needs to be initialised earlier in the boot sequence. Here
>> we move initialisation forward from arch_initcall to core_initcall time.
>
> I don't understand why using IRQ domain makes it necessary to
> initialize the driver earlier in the init process. Please explain to me,
> I need tutoring here.
See the other email.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Lee Jones <[email protected]>
Date: Tue, 17 Apr 2012 16:04:13 +0100
Subject: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver
Here we move the i2c-nomadik's default settings into the driver
rather than specifying them from platform code. At the time of
this writing we only have one user, the u8500. As new users are
added, it is expected that they will be Device Tree compliant.
If this is the case, we will look up their initialisation values
by compatible entry, then apply them forthwith.
Cc: [email protected]
Acked-by: Linus Walleij <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/i2c/busses/i2c-nomadik.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a92440d..23fde19 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
#include <linux/io.h>
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/of_device.h>
#include <plat/i2c.h>
@@ -899,15 +900,44 @@ static const struct i2c_algorithm nmk_i2c_algo = {
.functionality = nmk_i2c_functionality
};
+static struct nmk_i2c_controller u8500_i2c = {
+ /*
+ * Slave data setup time; 250ns, 100ns, and 10ns, which
+ * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
+ */
+ .slsu = 0xe,
+ .tft = 1, /* Tx FIFO threshold */
+ .rft = 8, /* Rx FIFO threshold */
+ .clk_freq = 100000, /* std. mode operation */
+ .timeout = 200, /* Slave response timeout(ms) */
+ .sm = I2C_FREQ_MODE_FAST,
+};
+
+static const struct of_device_id nmk_gpio_match[] = {
+ { .compatible = "st,nomadik-i2c", .data = &u8500_i2c, },
+ {}
+};
+
static int __devinit nmk_i2c_probe(struct platform_device *pdev)
{
int ret = 0;
struct resource *res;
- struct nmk_i2c_controller *pdata =
+ const struct nmk_i2c_controller *pdata =
pdev->dev.platform_data;
+ const struct of_device_id *of_id =
+ of_match_device(nmk_gpio_match, &pdev->dev);
struct nmk_i2c_dev *dev;
struct i2c_adapter *adap;
+ if (!pdata) {
+ if (of_id && of_id->data)
+ /* Looks like we're booting via Device Tree. */
+ pdata = of_id->data;
+ else
+ /* No i2c configuration found, using the default. */
+ pdata = &u8500_i2c;
+ }
+
dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -1043,6 +1073,7 @@ static struct platform_driver nmk_i2c_driver = {
.owner = THIS_MODULE,
.name = DRIVER_NAME,
.pm = &nmk_i2c_pm,
+ .of_match_table = nmk_gpio_match,
},
.probe = nmk_i2c_probe,
.remove = __devexit_p(nmk_i2c_remove),
--
1.7.9.5
From: Lee Jones <[email protected]>
Date: Tue, 22 May 2012 15:25:09 +0100
Subject: [PATCH 01/14] rtc: ab8500-rtc: IRQF_ONESHOT when requesting a
threaded IRQ
The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
Cc: Alessandro Zummo <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
drivers/rtc/rtc-ab8500.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-ab8500.c b/drivers/rtc/rtc-ab8500.c
index 4bcf9ca..b11a2ec 100644
--- a/drivers/rtc/rtc-ab8500.c
+++ b/drivers/rtc/rtc-ab8500.c
@@ -422,7 +422,7 @@ static int __devinit ab8500_rtc_probe(struct platform_device *pdev)
}
err = request_threaded_irq(irq, NULL, rtc_alarm_handler,
- IRQF_NO_SUSPEND, "ab8500-rtc", rtc);
+ IRQF_NO_SUSPEND | IRQF_ONESHOT, "ab8500-rtc", rtc);
if (err < 0) {
rtc_device_unregister(rtc);
return err;
--
1.7.9.5
On Tue, Jun 12, 2012 at 09:52:39AM +0100, Lee Jones wrote:
> From: Lee Jones <[email protected]>
> Date: Tue, 17 Apr 2012 16:04:13 +0100
> Subject: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver
>
> Here we move the i2c-nomadik's default settings into the driver
> rather than specifying them from platform code. At the time of
> this writing we only have one user, the u8500. As new users are
> added, it is expected that they will be Device Tree compliant.
> If this is the case, we will look up their initialisation values
> by compatible entry, then apply them forthwith.
>
> Cc: [email protected]
> Acked-by: Linus Walleij <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
Did you use checkpatch?
============
Applying: i2c: Add Device Tree support to the Nomadik I2C driver
CHECKPATCH
WARNING: please, no space before tabs
#37: FILE: drivers/i2c/busses/i2c-nomadik.c:909:
+^I.tft = 1, ^I /* Tx FIFO threshold */$
WARNING: please, no space before tabs
#38: FILE: drivers/i2c/busses/i2c-nomadik.c:910:
+^I.rft = 8, ^I /* Rx FIFO threshold */$
WARNING: please, no space before tabs
#40: FILE: drivers/i2c/busses/i2c-nomadik.c:912:
+^I.timeout = 200, ^I /* Slave response timeout(ms) */$
ERROR: code indent should use tabs where possible
#56: FILE: drivers/i2c/busses/i2c-nomadik.c:927:
+ const struct of_device_id *of_id =$
WARNING: please, no spaces at the start of a line
#56: FILE: drivers/i2c/busses/i2c-nomadik.c:927:
+ const struct of_device_id *of_id =$
ERROR: code indent should use tabs where possible
#57: FILE: drivers/i2c/busses/i2c-nomadik.c:928:
+ of_match_device(nmk_gpio_match, &pdev->dev);$
WARNING: please, no spaces at the start of a line
#57: FILE: drivers/i2c/busses/i2c-nomadik.c:928:
+ of_match_device(nmk_gpio_match, &pdev->dev);$
total: 2 errors, 5 warnings, 0 checks, 59 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
Your patch has style problems, please review.
============
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Lee Jones <[email protected]>
Date: Tue, 22 May 2012 15:13:13 +0100
Subject: [PATCH 03/14] mfd: ab8500-gpadc: Enable IRQF_ONESHOT when requesting
a threaded IRQ
The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
Cc: Samuel Ortiz <[email protected]>
Cc: [email protected]
Acked-by: Linus Walleij <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/ab8500-gpadc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c
index b86fd8e..b6cbc3ba 100644
--- a/drivers/mfd/ab8500-gpadc.c
+++ b/drivers/mfd/ab8500-gpadc.c
@@ -599,7 +599,8 @@ static int __devinit ab8500_gpadc_probe(struct platform_device *pdev)
/* Register interrupt - SwAdcComplete */
ret = request_threaded_irq(gpadc->irq, NULL,
ab8500_bm_gpswadcconvend_handler,
- IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc", gpadc);
+ IRQF_ONESHOT | IRQF_NO_SUSPEND | IRQF_SHARED,
+ "ab8500-gpadc", gpadc);
if (ret < 0) {
dev_err(gpadc->dev, "Failed to register interrupt, irq: %d\n",
gpadc->irq);
--
1.7.9.5
On Tuesday 12 June 2012, Lee Jones wrote:
> On 11/06/12 22:01, Linus Walleij wrote:
> > Hm what shall we do when we run out of initlevels? I think this was the
> > kind of thing that deferred probe should solve. Usually changing this kind
> > of thing has side effects so I'm a bit hesitant.
>
> Ah yes, I remember now. The IRQ domain needs to be in place before the
> Device Tree is parsed by the Open Firmware subsystem. If it's not the
> error "no irq domain found" is triggered and the IRQs are never mapped.
>
> I'd be happy to take a second opinion, but I believe this (and the other
> core_initcall patch) is required.
It's still just a hack. The real solution that we discussed last time it
came up is to defer the translation of irq numbers until device driver
probe time, and bail out with -EPROBE_DEFER if you try to use a device
whose interrupt is not available yet.
Arnd
On 12/06/12 13:59, Arnd Bergmann wrote:
> On Tuesday 12 June 2012, Lee Jones wrote:
>> On 11/06/12 22:01, Linus Walleij wrote:
>>> Hm what shall we do when we run out of initlevels? I think this was the
>>> kind of thing that deferred probe should solve. Usually changing this kind
>>> of thing has side effects so I'm a bit hesitant.
>>
>> Ah yes, I remember now. The IRQ domain needs to be in place before the
>> Device Tree is parsed by the Open Firmware subsystem. If it's not the
>> error "no irq domain found" is triggered and the IRQs are never mapped.
>>
>> I'd be happy to take a second opinion, but I believe this (and the other
>> core_initcall patch) is required.
>
> It's still just a hack. The real solution that we discussed last time it
> came up is to defer the translation of irq numbers until device driver
> probe time, and bail out with -EPROBE_DEFER if you try to use a device
> whose interrupt is not available yet.
Ah yes, this:
> /*
> * FIXME: Should we set up the GPIO domain here?
> *
> * The problem is that we cannot put the interrupt resources into the platform
> * device until the irqdomain has been added. Right now, we set the GIC interrupt
> * domain from init_irq(), then load the gpio driver from
> * core_initcall(nmk_gpio_init) and add the platform devices from
> * arch_initcall(customize_machine).
> *
> * This feels fragile because it depends on the gpio device getting probed
> * _before_ any device uses the gpio interrupts.
> */
But your suggestion above involves some heavy refactoring of how Device
Tree works. Meanwhile the ab8500 will not work as an IRQ controller
until we either complete the work or use the same solution as we did for
the Nomadik GPIO controller. I think for now at least we should continue
to work on enablement and leave the 'nice to have's until last. We are
doing nothing new here, just bringing the ab8500 IRQ controller into
line with the Nomadik one.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Jun 12, 2012 at 10:03 AM, Lee Jones <[email protected]> wrote:
> On 11/06/12 22:43, Linus Walleij wrote:
>> Oh this is a bug fix really, well it's in the device tree so nevermind.
>
> It is? How so?
That the device tree was declaring a compatible node for something
that never existed (stericsson,db8500-rtc) while the real node is
"arm,rtc-pl031", "arm,primecell".
Without this fix 3.4 will not have an PL031 RTC I guess?
Yours,
Linus Walleij
On 12/06/12 15:32, Linus Walleij wrote:
> On Tue, Jun 12, 2012 at 10:03 AM, Lee Jones<[email protected]> wrote:
>> On 11/06/12 22:43, Linus Walleij wrote:
>
>>> Oh this is a bug fix really, well it's in the device tree so nevermind.
>>
>> It is? How so?
>
> That the device tree was declaring a compatible node for something
> that never existed (stericsson,db8500-rtc) while the real node is
> "arm,rtc-pl031", "arm,primecell".
>
> Without this fix 3.4 will not have an PL031 RTC I guess?
Think of the old entry as a dummy one, a stub if you will. Quite a few
of the entries were initially loaded into the Device Tree as a best
effort first attempt and had to be changed at a later date. This is no
different. I see this as an enablement patch rather than a bug-fix. In
fact, there would have been a bug if this worked when it was initially
written, as the driver would have probed twice on a DT boot. Once by DT
and once by platform code.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tuesday 12 June 2012, Lee Jones wrote:
> But your suggestion above involves some heavy refactoring of how Device
> Tree works. Meanwhile the ab8500 will not work as an IRQ controller
> until we either complete the work or use the same solution as we did for
> the Nomadik GPIO controller. I think for now at least we should continue
> to work on enablement and leave the 'nice to have's until last. We are
> doing nothing new here, just bringing the ab8500 IRQ controller into
> line with the Nomadik one.
Right, I didn't say we should not take your patch, but we should be
aware that it's not the solution we want to use in the long run.
Arnd
On 12/06/12 15:49, Arnd Bergmann wrote:
> On Tuesday 12 June 2012, Lee Jones wrote:
>> But your suggestion above involves some heavy refactoring of how Device
>> Tree works. Meanwhile the ab8500 will not work as an IRQ controller
>> until we either complete the work or use the same solution as we did for
>> the Nomadik GPIO controller. I think for now at least we should continue
>> to work on enablement and leave the 'nice to have's until last. We are
>> doing nothing new here, just bringing the ab8500 IRQ controller into
>> line with the Nomadik one.
>
> Right, I didn't say we should not take your patch, but we should be
> aware that it's not the solution we want to use in the long run.
I agree with you completely.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Jun 12, 2012 at 9:34 AM, Lee Jones <[email protected]> wrote:
> On 11/06/12 21:37, Linus Walleij wrote:
>>
>> So why don't we create proper device tree bindings for the above platform
>> data?
>>(...)
>> This looks to me like a way to push the burden of doing the real
>> device tree binding for the above to the next user.
>> (Which will likely be me, so therefore I care a bit ...)
>
>
> On the contrary. This will avoid any added Device Tree complexity and ensure
> that no extra vendor specific bindings are required. When a new user wishes
> to use the driver, all they (you) have to do is create a new struct with the
> platform specific information and add the entry to nmk_gpio_match[],
> simples.
>
> I've even added the logic to extract any information you provide via
> nmk_gpio_match[] for use as platform data. This keeps it both out of
> platform code and the Device Tree binary. Everyone's a winner. :)
No. You assume that the platform data is a chip-specific property,
and that it will be the same for all busses on the chip. But it
isn't.
The above platform data is *board specific*, it depends on
what devices have been connected to each bus.
For example the max frequency: this depends on the maximum
frequency any of the devices connected to one very bus
can support.
The other platforms have put this frequency into their device
trees for a reason, and this is it.
So for the four different i2c busses there may need to be
four different platform data sets. How are you going to
distinguish between the four buses?
This is thus broken and needs to have proper bindings.
Yours,
Linus Walleij
On 13/06/12 06:40, Linus Walleij wrote:
> On Tue, Jun 12, 2012 at 9:34 AM, Lee Jones<[email protected]> wrote:
>> On 11/06/12 21:37, Linus Walleij wrote:
>>>
>>> So why don't we create proper device tree bindings for the above platform
>>> data?
>>> (...)
>>> This looks to me like a way to push the burden of doing the real
>>> device tree binding for the above to the next user.
>>> (Which will likely be me, so therefore I care a bit ...)
>>
>>
>> On the contrary. This will avoid any added Device Tree complexity and ensure
>> that no extra vendor specific bindings are required. When a new user wishes
>> to use the driver, all they (you) have to do is create a new struct with the
>> platform specific information and add the entry to nmk_gpio_match[],
>> simples.
>>
>> I've even added the logic to extract any information you provide via
>> nmk_gpio_match[] for use as platform data. This keeps it both out of
>> platform code and the Device Tree binary. Everyone's a winner. :)
>
> No. You assume that the platform data is a chip-specific property,
> and that it will be the same for all busses on the chip. But it
> isn't.
>
> The above platform data is *board specific*, it depends on
> what devices have been connected to each bus.
>
> For example the max frequency: this depends on the maximum
> frequency any of the devices connected to one very bus
> can support.
>
> The other platforms have put this frequency into their device
> trees for a reason, and this is it.
>
> So for the four different i2c busses there may need to be
> four different platform data sets. How are you going to
> distinguish between the four buses?
>
> This is thus broken and needs to have proper bindings.
Board specific is fine, as the data is protected by a board specific
property. Do you mean that the properties are *bus specific*? In which
case I see your point and will apply the correct bindings.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 13, 2012 at 9:01 AM, Lee Jones <[email protected]> wrote:
> Board specific is fine, as the data is protected by a board specific
> property. Do you mean that the properties are *bus specific*? In which case
> I see your point and will apply the correct bindings.
I cannot parse this, the board for me is a SoC, busses and a
number of components connected via e.g. I2C.
Can you define what you mean with a "board specific property"?
It seems you are talking about what I would call an
"SoC-specific property", i.e. something out of a .dtsi file for
a certain SoC, whereas the .dts for an entire board is,
well, for a board, a set of components on a PCB.
The arrangement of accelerometers and battery monitors on a
certain board is board-specific, and it is also by definition
bus-specific.
Yours,
Linus Walleij
On 13/06/12 09:12, Linus Walleij wrote:
> On Wed, Jun 13, 2012 at 9:01 AM, Lee Jones<[email protected]> wrote:
>
>> Board specific is fine, as the data is protected by a board specific
>> property. Do you mean that the properties are *bus specific*? In which case
>> I see your point and will apply the correct bindings.
>
> I cannot parse this, the board for me is a SoC, busses and a
> number of components connected via e.g. I2C.
>
> Can you define what you mean with a "board specific property"?
> It seems you are talking about what I would call an
> "SoC-specific property", i.e. something out of a .dtsi file for
> a certain SoC, whereas the .dts for an entire board is,
> well, for a board, a set of components on a PCB.
>
> The arrangement of accelerometers and battery monitors on a
> certain board is board-specific, and it is also by definition
> bus-specific.
Okay, let's put it another way. We can individualise any board, bus,
platform, machine or system by placing all of the information in a DT
node so long as we plonk it in the correct place within the Device Tree.
However, we have just as much control by keeping them in separate
structs in the C file and selecting the right one using the compatible
sting.
The real item for discussion is; if we have varying configurations for
each of the i2c buses residing on the same SoC, do we really want to use
different compatible strings to identify each one. If the answer is no,
which I think it is, then I need to write the bindings and have
everything individually configurable from Device Tree.
While you're mulling this over, I'm just going to write the bindings anyway.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Lee Jones <[email protected]>
Date: Tue, 17 Apr 2012 16:04:13 +0100
Subject: [PATCH 1/3] i2c: Add Device Tree support to the Nomadik I2C driver
Here we apply the bindings required for successful Device Tree
probing of the i2c-nomadik driver. We also apply a fall-back
configuration in case either one is not provided, or a required
element is missing from the one supplied.
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
drivers/i2c/busses/i2c-nomadik.c | 92 ++++++++++++++++++++++++++++++++++----
1 file changed, 84 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a92440d..b97f52e 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
#include <linux/io.h>
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/of.h>
#include <plat/i2c.h>
@@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
.functionality = nmk_i2c_functionality
};
+static struct nmk_i2c_controller u8500_i2c = {
+ /*
+ * Slave data setup time; 250ns, 100ns, and 10ns, which
+ * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
+ */
+ .slsu = 0xe,
+ .tft = 1, /* Tx FIFO threshold */
+ .rft = 8, /* Rx FIFO threshold */
+ .clk_freq = 100000, /* std. mode operation */
+ .timeout = 200, /* Slave response timeout(ms) */
+ .sm = I2C_FREQ_MODE_FAST,
+};
+
+static int __devinit
+nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
+{
+ of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
+ if (!pdata->clk_freq) {
+ pr_warn("%s: Clock frequency not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
+ if (!pdata->slsu) {
+ pr_warn("%s: Data line delay not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
+ if (!pdata->tft) {
+ pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
+ if (!pdata->rft) {
+ pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
+ if (!pdata->timeout) {
+ pr_warn("%s: Timeout not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
+ pdata->sm = I2C_FREQ_MODE_FAST;
+ else
+ pdata->sm = I2C_FREQ_MODE_STANDARD;
+
+ return 0;
+}
+
static int __devinit nmk_i2c_probe(struct platform_device *pdev)
{
int ret = 0;
struct resource *res;
- struct nmk_i2c_controller *pdata =
- pdev->dev.platform_data;
+ struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
+ struct device_node *np = pdev->dev.of_node;
struct nmk_i2c_dev *dev;
struct i2c_adapter *adap;
+ if (np) {
+ if (!pdata) {
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ ret = nmk_i2c_of_probe(np, pdata);
+ if (ret)
+ kfree(pdata);
+ }
+
+ if (!pdata)
+ /* No i2c configuration found, using the default. */
+ pdata = &u8500_i2c;
+
dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -974,12 +1046,10 @@ static int __devinit nmk_i2c_probe(struct platform_device *pdev)
/* fetch the controller configuration from machine */
dev->cfg.clk_freq = pdata->clk_freq;
- dev->cfg.slsu = pdata->slsu;
- dev->cfg.tft = pdata->tft;
- dev->cfg.rft = pdata->rft;
- dev->cfg.sm = pdata->sm;
-
- i2c_set_adapdata(adap, dev);
+ dev->cfg.slsu = pdata->slsu;
+ dev->cfg.tft = pdata->tft;
+ dev->cfg.rft = pdata->rft;
+ dev->cfg.sm = pdata->sm; i2c_set_adapdata(adap, dev);
dev_info(&pdev->dev,
"initialize %s on virtual base %p\n",
@@ -1038,11 +1108,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id nmk_gpio_match[] = {
+ { .compatible = "st,nomadik-i2c", },
+ {},
+};
+
static struct platform_driver nmk_i2c_driver = {
.driver = {
.owner = THIS_MODULE,
.name = DRIVER_NAME,
.pm = &nmk_i2c_pm,
+ .of_match_table = nmk_gpio_match,
},
.probe = nmk_i2c_probe,
.remove = __devexit_p(nmk_i2c_remove),
--
1.7.9.5
From: Lee Jones <[email protected]>
Date: Wed, 13 Jun 2012 16:21:14 +0100
Subject: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree
for DB8500 based devices
For correct operation of the i2c-nomadik driver, it requires some
information surrounding clock-frequencies, delay times and thresholds.
Here we apply those configurations to be used when a platform is
booted via the device Tree.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/db8500.dtsi | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/arch/arm/boot/dts/db8500.dtsi b/arch/arm/boot/dts/db8500.dtsi
index 00a4c3e..f3050f1 100644
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -455,6 +455,13 @@
#address-cells = <1>;
#size-cells = <0>;
v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <100000>;
+ stericsson,slsu = <0xe>;
+ stericsson,tft = <1>;
+ stericsson,rft = <8>;
+ stericsson,timeout = <200>;
+ stericsson,i2c_freq_mode_fast;
};
i2c@80122000 {
@@ -464,6 +471,13 @@
#address-cells = <1>;
#size-cells = <0>;
v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <100000>;
+ stericsson,slsu = <0xe>;
+ stericsson,tft = <1>;
+ stericsson,rft = <8>;
+ stericsson,timeout = <200>;
+ stericsson,i2c_freq_mode_fast;
};
i2c@80128000 {
@@ -473,6 +487,13 @@
#address-cells = <1>;
#size-cells = <0>;
v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <100000>;
+ stericsson,slsu = <0xe>;
+ stericsson,tft = <1>;
+ stericsson,rft = <8>;
+ stericsson,timeout = <200>;
+ stericsson,i2c_freq_mode_fast;
};
i2c@80110000 {
@@ -482,6 +503,13 @@
#address-cells = <1>;
#size-cells = <0>;
v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <100000>;
+ stericsson,slsu = <0xe>;
+ stericsson,tft = <1>;
+ stericsson,rft = <8>;
+ stericsson,timeout = <200>;
+ stericsson,i2c_freq_mode_fast;
};
i2c@8012a000 {
@@ -491,6 +519,13 @@
#address-cells = <1>;
#size-cells = <0>;
v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <100000>;
+ stericsson,slsu = <0xe>;
+ stericsson,tft = <1>;
+ stericsson,rft = <8>;
+ stericsson,timeout = <200>;
+ stericsson,i2c_freq_mode_fast;
};
ssp@80002000 {
--
1.7.9.5
From: Lee Jones <[email protected]>
Date: Wed, 13 Jun 2012 17:00:02 +0100
Subject: [PATCH 3/3] Documentation: Device Tree binding information for
i2c-nomadik driver
This document describes each of the non-standard Device Tree bindings
used in the Nomadik I2C driver.
Signed-off-by: Lee Jones <[email protected]>
---
Documentation/devicetree/bindings/i2c/nomadik.txt | 33 +++++++++++++++++++++
1 file changed, 33 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/nomadik.txt
diff --git a/Documentation/devicetree/bindings/i2c/nomadik.txt b/Documentation/devicetree/bindings/i2c/nomadik.txt
new file mode 100644
index 0000000..18cea18
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/nomadik.txt
@@ -0,0 +1,33 @@
+I2C for Nomadik based systems
+
+Required (non-standard) properties:
+ - Nil
+
+Recommended (non-standard) properties:
+ - clock-frequency : Maximum bus clock frequency for the device
+ - stericsson,slsu : Data line delay
+ - stericsson,tft : Tx FIFO threshold
+ - stericsson,rft : Rx FIFO threshold
+ - stericsson,timeout : Slave response timeout(ms)
+ - stericsson,i2c_freq_mode_fast : I2C fast mode (omit for standard mode)
+
+Optional (non-standard) properties:
+ - Nil
+
+Example :
+
+i2c@80004000 {
+ compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+ reg = <0x80004000 0x1000>;
+ interrupts = <0 21 0x4>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <100000>;
+ stericsson,slsu = <0xe>;
+ stericsson,tft = <1>;
+ stericsson,rft = <8>;
+ stericsson,timeout = <200>;
+ stericsson,i2c_freq_mode_fast;
+};
--
1.7.9.5
On Tue, 12 Jun 2012 09:57:37 +0100
Lee Jones <[email protected]> wrote:
> The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
What does this mean. The kernel crashes? The registration fails? A
warning is emitted?
When fixing a bug, please fully describe the consequences of that bug.
> Cc: [email protected]
Especially when suggesting a -stable backport.
On 13/06/12 23:25, Andrew Morton wrote:
> On Tue, 12 Jun 2012 09:57:37 +0100
> Lee Jones<[email protected]> wrote:
>
>> The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
>
> What does this mean. The kernel crashes? The registration fails? A
> warning is emitted?
>
> When fixing a bug, please fully describe the consequences of that bug.
>
>> Cc: [email protected]
>
> Especially when suggesting a -stable backport.
No problem. I'll try to remember that for next time.
I see that you've taken the patch into your tree however. So do you want
me to update the commit log and resubmit, or are you going to 'let this
one slide'?
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, 14 Jun 2012 08:14:49 +0100 Lee Jones <[email protected]> wrote:
> On 13/06/12 23:25, Andrew Morton wrote:
> > On Tue, 12 Jun 2012 09:57:37 +0100
> > Lee Jones<[email protected]> wrote:
> >
> >> The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
> >
> > What does this mean. The kernel crashes? The registration fails? A
> > warning is emitted?
> >
> > When fixing a bug, please fully describe the consequences of that bug.
> >
> >> Cc: [email protected]
> >
> > Especially when suggesting a -stable backport.
>
> No problem. I'll try to remember that for next time.
>
> I see that you've taken the patch into your tree however. So do you want
> me to update the commit log and resubmit, or are you going to 'let this
> one slide'?
Please read my emails - they're very nice! "What does this mean. The
kernel crashes? The registration fails? A warning is emitted?". You
answer that, I copy-n-paste into changelog and we're done.
On 14/06/12 08:30, Andrew Morton wrote:
> On Thu, 14 Jun 2012 08:14:49 +0100 Lee Jones<[email protected]> wrote:
>
>> On 13/06/12 23:25, Andrew Morton wrote:
>>> On Tue, 12 Jun 2012 09:57:37 +0100
>>> Lee Jones<[email protected]> wrote:
>>>
>>>> The kernel now forces IRQs to be ONESHOT if no IRQ handler is passed.
>>>
>>> What does this mean. The kernel crashes? The registration fails? A
>>> warning is emitted?
>>>
>>> When fixing a bug, please fully describe the consequences of that bug.
>>>
>>>> Cc: [email protected]
>>>
>>> Especially when suggesting a -stable backport.
>>
>> No problem. I'll try to remember that for next time.
>>
>> I see that you've taken the patch into your tree however. So do you want
>> me to update the commit log and resubmit, or are you going to 'let this
>> one slide'?
>
> Please read my emails - they're very nice! "What does this mean. The
> kernel crashes? The registration fails? A warning is emitted?". You
> answer that, I copy-n-paste into changelog and we're done.
Ah, you're a star!
Registration fails:
> __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> {
<snip>
> if (new->flags & IRQF_ONESHOT) {
<snip>
> } else if (new->handler == irq_default_primary_handler) {
> /*
> * The interrupt was requested with handler = NULL, so
> * we use the default primary handler for it. But it
> * does not have the oneshot flag set. In combination
> * with level interrupts this is deadly, because the
> * default primary handler just wakes the thread, then
> * the irq lines is reenabled, but the device still
> * has the level irq asserted. Rinse and repeat....
> *
> * While this works for edge type interrupts, we play
> * it safe and reject unconditionally because we can't
> * say for sure which type this interrupt really
> * has. The type flags are unreliable as the
> * underlying chip implementation can override them.
> */
> pr_err("Threaded irq requested with handler=NULL and !ONESHOT for irq %d\n", irq);
> ret = -EINVAL;
> goto out_mask;
> }
<snip>
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 13, 2012 at 6:07 PM, Lee Jones <[email protected]> wrote:
> Here we apply the bindings required for successful Device Tree
> probing of the i2c-nomadik driver. We also apply a fall-back
> configuration in case either one is not provided, or a required
> element is missing from the one supplied.
>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
This looks more like I'd expect it, good job! :-D
> +static int __devinit
> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
> +{
> + ? ? ? of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
> + ? ? ? if (!pdata->clk_freq) {
> + ? ? ? ? ? ? ? pr_warn("%s: Clock frequency not found\n", np->full_name);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
> + ? ? ? if (!pdata->slsu) {
> + ? ? ? ? ? ? ? pr_warn("%s: Data line delay not found\n", np->full_name);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
> + ? ? ? if (!pdata->tft) {
> + ? ? ? ? ? ? ? pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
> + ? ? ? if (!pdata->rft) {
> + ? ? ? ? ? ? ? pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
> + ? ? ? if (!pdata->timeout) {
> + ? ? ? ? ? ? ? pr_warn("%s: Timeout not found\n", np->full_name);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
> + ? ? ? ? ? ? ? pdata->sm = I2C_FREQ_MODE_FAST;
> + ? ? ? else
> + ? ? ? ? ? ? ? pdata->sm = I2C_FREQ_MODE_STANDARD;
> +
> + ? ? ? return 0;
> +}
Will this compile fine if CONFIG_OF is not selected?
> ? ? ? ?/* fetch the controller configuration from machine */
> ? ? ? ?dev->cfg.clk_freq = pdata->clk_freq;
> - ? ? ? dev->cfg.slsu ? = pdata->slsu;
> - ? ? ? dev->cfg.tft ? ?= pdata->tft;
> - ? ? ? dev->cfg.rft ? ?= pdata->rft;
> - ? ? ? dev->cfg.sm ? ? = pdata->sm;
> -
> - ? ? ? i2c_set_adapdata(adap, dev);
> + ? ? ? dev->cfg.slsu ? ? = pdata->slsu;
> + ? ? ? dev->cfg.tft ? ? ?= pdata->tft;
> + ? ? ? dev->cfg.rft ? ? ?= pdata->rft;
> + ? ? ? dev->cfg.sm ? ? ? = pdata->sm; ?i2c_set_adapdata(adap, dev);
This looks like an unrelated whitespace fix, but OK...
Yours,
Linus Walleij
On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones <[email protected]> wrote:
> From: Lee Jones <[email protected]>
> Date: Wed, 13 Jun 2012 16:21:14 +0100
> Subject: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree
> ?for DB8500 based devices
>
> For correct operation of the i2c-nomadik driver, it requires some
> information surrounding clock-frequencies, delay times and thresholds.
> Here we apply those configurations to be used when a platform is
> booted via the device Tree.
>
> Signed-off-by: Lee Jones <[email protected]>
Splendid,
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones <[email protected]> wrote:
> This document describes each of the non-standard Device Tree bindings
> used in the Nomadik I2C driver.
>
> Signed-off-by: Lee Jones <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Mon, Jun 11, 2012 at 04:25:01PM +0100, Lee Jones wrote:
> + if (np) {
> + ab8500->domain = irq_domain_add_linear(
> + np, num_irqs, &ab8500_irq_ops, ab8500);
> + }
> + else {
> + ab8500->domain = irq_domain_add_legacy(
> + NULL, num_irqs, ab8500->irq_base,
> + 0, &ab8500_irq_ops, ab8500);
> + }
This is odd, why are you forcing non-DT systems to use a legacy mapping?
The more usual approach is to use a legacy mapping if and only if a base
for the legacy range has been provided, otherwise the system will
needlessly fail to initialise the device...
> + if (!(ab8500->irq_base || np)) {
> + dev_info(&pdev->dev, "couldn't find irq-base and not doing DT boot\n");
...like this. See regmap_irq for an example.
On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:
> Device Tree. However, we have just as much control by keeping them
> in separate structs in the C file and selecting the right one using
> the compatible sting.
You're not understanding Linus' point. The compatible string isn't
useful here because properties like the maximum clock rate of the bus
depend on the board design, not the silicon. The controller may be
perfectly happy to run at a given rate but other devices on the bus or
the electrical engineering of the PCB itself may restrict this further.
On Thu, Jun 14, 2012 at 07:36:36PM +0100, Mark Brown wrote:
> You're not understanding Linus' point. The compatible string isn't
> useful here because properties like the maximum clock rate of the bus
> depend on the board design, not the silicon. The controller may be
> perfectly happy to run at a given rate but other devices on the bus or
> the electrical engineering of the PCB itself may restrict this further.
Sorry, I read the next revision and see this was actually resolved OK.
On 14/06/12 19:36, Mark Brown wrote:
> On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:
>
>> Device Tree. However, we have just as much control by keeping them
>> in separate structs in the C file and selecting the right one using
>> the compatible sting.
>
> You're not understanding Linus' point. The compatible string isn't
> useful here because properties like the maximum clock rate of the bus
> depend on the board design, not the silicon. The controller may be
> perfectly happy to run at a given rate but other devices on the bus or
> the electrical engineering of the PCB itself may restrict this further.
And you're not understanding mine. ;)
You can have multiple compatible strings for a single driver. Which one
you reference from the Device Tree will dictate which group of settings
are used, including variation of clock rates.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 14/06/12 19:46, Mark Brown wrote:
> On Thu, Jun 14, 2012 at 07:36:36PM +0100, Mark Brown wrote:
>
>> You're not understanding Linus' point. The compatible string isn't
>> useful here because properties like the maximum clock rate of the bus
>> depend on the board design, not the silicon. The controller may be
>> perfectly happy to run at a given rate but other devices on the bus or
>> the electrical engineering of the PCB itself may restrict this further.
>
> Sorry, I read the next revision and see this was actually resolved OK.
Yes, I just went ahead created the bindings anyway. I figured it would
be neater (if no more functional) to keep all the variations in DT.
Especially if we had devices which only varied by one or two settings,
which would still require a complete new struct using the previous method.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Lee Jones <[email protected]>
Date: Tue, 17 Apr 2012 16:04:13 +0100
Subject: [PATCH 1/1] i2c: Add Device Tree support to the Nomadik I2C driver
Here we apply the bindings required for successful Device Tree
probing of the i2c-nomadik driver. We also apply a fall-back
configuration in case either one is not provided, or a required
element is missing from the one supplied.
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index a92440d..58e8114 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -23,6 +23,7 @@
#include <linux/io.h>
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/of.h>
#include <plat/i2c.h>
@@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
.functionality = nmk_i2c_functionality
};
+static struct nmk_i2c_controller u8500_i2c = {
+ /*
+ * Slave data setup time; 250ns, 100ns, and 10ns, which
+ * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
+ */
+ .slsu = 0xe,
+ .tft = 1, /* Tx FIFO threshold */
+ .rft = 8, /* Rx FIFO threshold */
+ .clk_freq = 100000, /* std. mode operation */
+ .timeout = 200, /* Slave response timeout(ms) */
+ .sm = I2C_FREQ_MODE_FAST,
+};
+
+static int __devinit
+nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
+{
+ of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
+ if (!pdata->clk_freq) {
+ pr_warn("%s: Clock frequency not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
+ if (!pdata->slsu) {
+ pr_warn("%s: Data line delay not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
+ if (!pdata->tft) {
+ pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
+ if (!pdata->rft) {
+ pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
+ if (!pdata->timeout) {
+ pr_warn("%s: Timeout not found\n", np->full_name);
+ return -EINVAL;
+ }
+
+ if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
+ pdata->sm = I2C_FREQ_MODE_FAST;
+ else
+ pdata->sm = I2C_FREQ_MODE_STANDARD;
+
+ return 0;
+}
+
static int __devinit nmk_i2c_probe(struct platform_device *pdev)
{
int ret = 0;
struct resource *res;
- struct nmk_i2c_controller *pdata =
- pdev->dev.platform_data;
+ struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
+ struct device_node *np = pdev->dev.of_node;
struct nmk_i2c_dev *dev;
struct i2c_adapter *adap;
+ if (np) {
+ if (!pdata) {
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ ret = nmk_i2c_of_probe(np, pdata);
+ if (ret)
+ kfree(pdata);
+ }
+
+ if (!pdata)
+ /* No i2c configuration found, using the default. */
+ pdata = &u8500_i2c;
+
dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&pdev->dev, "cannot allocate memory\n");
@@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id nmk_gpio_match[] = {
+ { .compatible = "st,nomadik-i2c", },
+ {},
+};
+
static struct platform_driver nmk_i2c_driver = {
.driver = {
.owner = THIS_MODULE,
.name = DRIVER_NAME,
.pm = &nmk_i2c_pm,
+ .of_match_table = nmk_gpio_match,
},
.probe = nmk_i2c_probe,
.remove = __devexit_p(nmk_i2c_remove),
--
1.7.9.5
On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
> On 14/06/12 19:36, Mark Brown wrote:
> >You're not understanding Linus' point. The compatible string isn't
> >useful here because properties like the maximum clock rate of the bus
> >depend on the board design, not the silicon. The controller may be
> >perfectly happy to run at a given rate but other devices on the bus or
> >the electrical engineering of the PCB itself may restrict this further.
> And you're not understanding mine. ;)
I think we are (or at least I do)...
> You can have multiple compatible strings for a single driver. Which
> one you reference from the Device Tree will dictate which group of
> settings are used, including variation of clock rates.
I'm certainly well aware of that. The problem is that the compatible
strings should identify a particular IP or piece of silicon and be
unchanging properties of that hardware - for example, working out how
big a FIFO in a device is from the compatible information is totally
sensible and reasonable. Things like the I2C bus clock speed on the
other hand depend not on the individual device but on how it's been
integrated into the system so you'd end up saying things like
"nomadik-i2c-100khz-bus" in the compatible string which isn't great.
If there's something like variation in the maximum speed that can be
supported by the different silicon the driver handles then working that
out from compatible and using it as the default is sensible but that's
not the same thing as providing customisation of the system bus
frequency to the user and if there's only one of those two that's
configurable it should be the system bus frequency.
On 15/06/12 10:32, Mark Brown wrote:
> On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
>> On 14/06/12 19:36, Mark Brown wrote:
>
>>> You're not understanding Linus' point. The compatible string isn't
>>> useful here because properties like the maximum clock rate of the bus
>>> depend on the board design, not the silicon. The controller may be
>>> perfectly happy to run at a given rate but other devices on the bus or
>>> the electrical engineering of the PCB itself may restrict this further.
>
>> And you're not understanding mine. ;)
>
> I think we are (or at least I do)...
>
>> You can have multiple compatible strings for a single driver. Which
>> one you reference from the Device Tree will dictate which group of
>> settings are used, including variation of clock rates.
>
> I'm certainly well aware of that. The problem is that the compatible
> strings should identify a particular IP or piece of silicon and be
> unchanging properties of that hardware - for example, working out how
> big a FIFO in a device is from the compatible information is totally
> sensible and reasonable. Things like the I2C bus clock speed on the
> other hand depend not on the individual device but on how it's been
> integrated into the system so you'd end up saying things like
> "nomadik-i2c-100khz-bus" in the compatible string which isn't great.
>
> If there's something like variation in the maximum speed that can be
> supported by the different silicon the driver handles then working that
> out from compatible and using it as the default is sensible but that's
> not the same thing as providing customisation of the system bus
> frequency to the user and if there's only one of those two that's
> configurable it should be the system bus frequency.
All agreed. I think we're singing off the same hymn-sheet here. :)
I was just saying that it was possible to pick a selection of
configurations from a compatible string alone. I also mentioned that on
reflection it would be neater to specify the properties individually
from Device Tree in case of small variations in configuration,
preventing us from using compatible strings like the one you mentioned.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
> On 14/06/12 19:36, Mark Brown wrote:
>> On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:
>>
>>> Device Tree. However, we have just as much control by keeping them
>>> in separate structs in the C file and selecting the right one using
>>> the compatible sting.
>>
>> You're not understanding Linus' point. The compatible string isn't
>> useful here because properties like the maximum clock rate of the bus
>> depend on the board design, not the silicon. The controller may be
>> perfectly happy to run at a given rate but other devices on the bus or
>> the electrical engineering of the PCB itself may restrict this further.
>
> And you're not understanding mine. ;)
>
> You can have multiple compatible strings for a single driver. Which one
> you reference from the Device Tree will dictate which group of settings
> are used, including variation of clock rates.
However, if you list these settings separately, rather than selecting them
based on a compatible string, it means when other board designs come along,
you don't have to modify the kernel code to provide yet another compatible
to settings translation in the code - you can keep all that information
entirely within DT with no kernel code mods.
I thought that was partly the point of DT...
On 15/06/12 11:32, Russell King - ARM Linux wrote:
> On Thu, Jun 14, 2012 at 07:57:32PM +0100, Lee Jones wrote:
>> On 14/06/12 19:36, Mark Brown wrote:
>>> On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote:
>>>
>>>> Device Tree. However, we have just as much control by keeping them
>>>> in separate structs in the C file and selecting the right one using
>>>> the compatible sting.
>>>
>>> You're not understanding Linus' point. The compatible string isn't
>>> useful here because properties like the maximum clock rate of the bus
>>> depend on the board design, not the silicon. The controller may be
>>> perfectly happy to run at a given rate but other devices on the bus or
>>> the electrical engineering of the PCB itself may restrict this further.
>>
>> And you're not understanding mine. ;)
>>
>> You can have multiple compatible strings for a single driver. Which one
>> you reference from the Device Tree will dictate which group of settings
>> are used, including variation of clock rates.
>
> However, if you list these settings separately, rather than selecting them
> based on a compatible string, it means when other board designs come along,
> you don't have to modify the kernel code to provide yet another compatible
> to settings translation in the code - you can keep all that information
> entirely within DT with no kernel code mods.
>
> I thought that was partly the point of DT...
Right. See my most recent emails on the subject. :)
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 14/06/12 19:32, Mark Brown wrote:
> On Mon, Jun 11, 2012 at 04:25:01PM +0100, Lee Jones wrote:
>
>> + if (np) {
>> + ab8500->domain = irq_domain_add_linear(
>> + np, num_irqs,&ab8500_irq_ops, ab8500);
>> + }
>> + else {
>> + ab8500->domain = irq_domain_add_legacy(
>> + NULL, num_irqs, ab8500->irq_base,
>> + 0,&ab8500_irq_ops, ab8500);
>> + }
>
> This is odd, why are you forcing non-DT systems to use a legacy mapping?
> The more usual approach is to use a legacy mapping if and only if a base
> for the legacy range has been provided, otherwise the system will
> needlessly fail to initialise the device...
Makes sense. I'll re-submit.
>> + if (!(ab8500->irq_base || np)) {
>> + dev_info(&pdev->dev, "couldn't find irq-base and not doing DT boot\n");
>
> ...like this. See regmap_irq for an example.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 12/06/12 15:49, Arnd Bergmann wrote:
> On Tuesday 12 June 2012, Lee Jones wrote:
>> But your suggestion above involves some heavy refactoring of how Device
>> Tree works. Meanwhile the ab8500 will not work as an IRQ controller
>> until we either complete the work or use the same solution as we did for
>> the Nomadik GPIO controller. I think for now at least we should continue
>> to work on enablement and leave the 'nice to have's until last. We are
>> doing nothing new here, just bringing the ab8500 IRQ controller into
>> line with the Nomadik one.
>
> Right, I didn't say we should not take your patch, but we should be
> aware that it's not the solution we want to use in the long run.
So is it only for this and the other one to go in now?
If so, can I have acks for them please, so I may place them in my
up-coming pull-request?
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Do this look better to you Mark?
From: Lee Jones <[email protected]>
Date: Sat, 26 May 2012 06:54:04 +0100
Subject: [PATCH 1/1] mfd: Add IRQ domain support for the AB8500
As the AB8500 is an IRQ controller in its own right, here we provide
the AB8500 driver with IRQ domain support. This is required if we wish
to reference any of its IRQs from a platform's Device Tree.
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/Kconfig | 1 +
drivers/mfd/ab8500-core.c | 114 ++++++++++++++++++++-----------------
include/linux/mfd/abx500/ab8500.h | 5 ++
3 files changed, 69 insertions(+), 51 deletions(-)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 170072e..e3637c2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -696,6 +696,7 @@ config AB8500_CORE
bool "ST-Ericsson AB8500 Mixed Signal Power Management chip"
depends on GENERIC_HARDIRQS && ABX500_CORE && MFD_DB8500_PRCMU
select MFD_CORE
+ select IRQ_DOMAIN
help
Select this option to enable access to AB8500 power management
chip. This connects to U8500 either on the SSP/SPI bus (deprecated
diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 4dc5024..47bf8aa 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -11,6 +11,7 @@
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/irq.h>
+#include <linux/irqdomain.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/module.h>
@@ -361,7 +362,7 @@ static void ab8500_irq_sync_unlock(struct irq_data *data)
static void ab8500_irq_mask(struct irq_data *data)
{
struct ab8500 *ab8500 = irq_data_get_irq_chip_data(data);
- int offset = data->irq - ab8500->irq_base;
+ int offset = data->irq;
int index = offset / 8;
int mask = 1 << (offset % 8);
@@ -371,7 +372,7 @@ static void ab8500_irq_mask(struct irq_data *data)
static void ab8500_irq_unmask(struct irq_data *data)
{
struct ab8500 *ab8500 = irq_data_get_irq_chip_data(data);
- int offset = data->irq - ab8500->irq_base;
+ int offset = data->irq;
int index = offset / 8;
int mask = 1 << (offset % 8);
@@ -501,7 +502,7 @@ static irqreturn_t ab8500_irq(int irq, void *dev)
int bit = __ffs(value);
int line = i * 8 + bit;
- handle_nested_irq(ab8500->irq_base + line);
+ handle_nested_irq(line);
value &= ~(1 << bit);
} while (value);
@@ -510,38 +511,51 @@ static irqreturn_t ab8500_irq(int irq, void *dev)
return IRQ_HANDLED;
}
-static int ab8500_irq_init(struct ab8500 *ab8500)
+/**
+ * ab8500_irq_get_virq(): Map an interrupt on a chip to a virtual IRQ
+ *
+ * @ab8500: ab8500_irq controller to operate on.
+ * @irq: index of the interrupt requested in the chip IRQs
+ *
+ * Useful for drivers to request their own IRQs.
+ */
+int ab8500_irq_get_virq(struct ab8500 *ab8500, int irq)
{
- int base = ab8500->irq_base;
- int irq;
- int num_irqs;
+ if (!ab8500)
+ return -EINVAL;
- if (is_ab9540(ab8500))
- num_irqs = AB9540_NR_IRQS;
- else if (is_ab8505(ab8500))
- num_irqs = AB8505_NR_IRQS;
- else
- num_irqs = AB8500_NR_IRQS;
+ return irq_create_mapping(ab8500->domain, irq);
+}
+EXPORT_SYMBOL_GPL(ab8500_irq_get_virq);
+
+static int ab8500_irq_map(struct irq_domain *d, unsigned int virq,
+ irq_hw_number_t hwirq)
+{
+ struct ab8500 *ab8500 = d->host_data;
- for (irq = base; irq < base + num_irqs; irq++) {
- irq_set_chip_data(irq, ab8500);
- irq_set_chip_and_handler(irq, &ab8500_irq_chip,
- handle_simple_irq);
- irq_set_nested_thread(irq, 1);
+ if (!ab8500)
+ return -EINVAL;
+
+ irq_set_chip_data(virq, ab8500);
+ irq_set_chip_and_handler(virq, &ab8500_irq_chip,
+ handle_simple_irq);
+ irq_set_nested_thread(virq, 1);
#ifdef CONFIG_ARM
- set_irq_flags(irq, IRQF_VALID);
+ set_irq_flags(virq, IRQF_VALID);
#else
- irq_set_noprobe(irq);
+ irq_set_noprobe(virq);
#endif
- }
return 0;
}
-static void ab8500_irq_remove(struct ab8500 *ab8500)
+static struct irq_domain_ops ab8500_irq_ops = {
+ .map = ab8500_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+static int ab8500_irq_init(struct ab8500 *ab8500, struct device_node *np)
{
- int base = ab8500->irq_base;
- int irq;
int num_irqs;
if (is_ab9540(ab8500))
@@ -551,13 +565,22 @@ static void ab8500_irq_remove(struct ab8500 *ab8500)
else
num_irqs = AB8500_NR_IRQS;
- for (irq = base; irq < base + num_irqs; irq++) {
-#ifdef CONFIG_ARM
- set_irq_flags(irq, 0);
-#endif
- irq_set_chip_and_handler(irq, NULL, NULL);
- irq_set_chip_data(irq, NULL);
+ if (ab8500->irq_base) {
+ ab8500->domain = irq_domain_add_legacy(
+ NULL, num_irqs, ab8500->irq_base,
+ 0, &ab8500_irq_ops, ab8500);
+ }
+ else {
+ ab8500->domain = irq_domain_add_linear(
+ np, num_irqs, &ab8500_irq_ops, ab8500);
+ }
+
+ if (!ab8500->domain) {
+ dev_err(ab8500->dev, "Failed to create irqdomain\n");
+ return -ENOSYS;
}
+
+ return 0;
}
int ab8500_suspend(struct ab8500 *ab8500)
@@ -1233,14 +1256,6 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
if (plat)
ab8500->irq_base = plat->irq_base;
- else if (np)
- ret = of_property_read_u32(np, "stericsson,irq-base", &ab8500->irq_base);
-
- if (!ab8500->irq_base) {
- dev_info(&pdev->dev, "couldn't find irq-base\n");
- ret = -EINVAL;
- goto out_free_ab8500;
- }
ab8500->dev = &pdev->dev;
@@ -1323,7 +1338,7 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
AB8500_SWITCH_OFF_STATUS, &value);
if (ret < 0)
return ret;
- dev_info(ab8500->dev, "switch off status: %#x", value);
+ dev_info(ab8500->dev, "switch off status: %#x\n", value);
if (plat && plat->init)
plat->init(ab8500);
@@ -1352,8 +1367,8 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
for (i = 0; i < ab8500->mask_size; i++)
ab8500->mask[i] = ab8500->oldmask[i] = 0xff;
- if (ab8500->irq_base) {
- ret = ab8500_irq_init(ab8500);
+ if (ab8500->irq_base || np) {
+ ret = ab8500_irq_init(ab8500, np);
if (ret)
goto out_freeoldmask;
@@ -1370,7 +1385,7 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
IRQF_ONESHOT | IRQF_NO_SUSPEND,
"ab8500", ab8500);
if (ret)
- goto out_removeirq;
+ goto out_freeoldmask;
}
if (!np) {
@@ -1417,15 +1432,12 @@ static int __devinit ab8500_probe(struct platform_device *pdev)
&ab8500_attr_group);
if (ret)
dev_err(ab8500->dev, "error creating sysfs entries\n");
- else
- return ret;
+
+ return ret;
out_freeirq:
- if (ab8500->irq_base)
+ if (ab8500->irq_base || np)
free_irq(ab8500->irq, ab8500);
-out_removeirq:
- if (ab8500->irq_base)
- ab8500_irq_remove(ab8500);
out_freeoldmask:
kfree(ab8500->oldmask);
out_freemask:
@@ -1439,16 +1451,16 @@ out_free_ab8500:
static int __devexit ab8500_remove(struct platform_device *pdev)
{
struct ab8500 *ab8500 = platform_get_drvdata(pdev);
+ struct device_node *np = pdev->dev.of_node;
if (is_ab9540(ab8500))
sysfs_remove_group(&ab8500->dev->kobj, &ab9540_attr_group);
else
sysfs_remove_group(&ab8500->dev->kobj, &ab8500_attr_group);
mfd_remove_devices(ab8500->dev);
- if (ab8500->irq_base) {
+ if (ab8500->irq_base || np)
free_irq(ab8500->irq, ab8500);
- ab8500_irq_remove(ab8500);
- }
+
kfree(ab8500->oldmask);
kfree(ab8500->mask);
kfree(ab8500);
diff --git a/include/linux/mfd/abx500/ab8500.h b/include/linux/mfd/abx500/ab8500.h
index 91dd3ef..4ae2cd9 100644
--- a/include/linux/mfd/abx500/ab8500.h
+++ b/include/linux/mfd/abx500/ab8500.h
@@ -9,6 +9,7 @@
#include <linux/atomic.h>
#include <linux/mutex.h>
+#include <linux/irqdomain.h>
struct device;
@@ -227,6 +228,7 @@ enum ab8500_version {
* @irq_lock: genirq bus lock
* @transfer_ongoing: 0 if no transfer ongoing
* @irq: irq line
+ * @irq_domain: irq domain
* @version: chip version id (e.g. ab8500 or ab9540)
* @chip_id: chip revision id
* @write: register write
@@ -247,6 +249,7 @@ struct ab8500 {
atomic_t transfer_ongoing;
int irq_base;
int irq;
+ struct irq_domain *domain;
enum ab8500_version version;
u8 chip_id;
@@ -336,4 +339,6 @@ static inline int is_ab8500_2p0(struct ab8500 *ab)
return (is_ab8500(ab) && (ab->chip_id == AB8500_CUT2P0));
}
+int ab8500_irq_get_virq(struct ab8500 *ab8500, int irq);
+
#endif /* MFD_AB8500_H */
--
1.7.9.5
On Mon, Jun 18, 2012 at 10:02:53AM +0100, Lee Jones wrote:
> Do this look better to you Mark?
Reviwed-by: Mark Brown <[email protected]>
On Mon, Jun 18, 2012 at 10:02:53AM +0100, Lee Jones wrote:
> @@ -361,7 +362,7 @@ static void ab8500_irq_sync_unlock(struct irq_data *data)
> static void ab8500_irq_mask(struct irq_data *data)
> {
> struct ab8500 *ab8500 = irq_data_get_irq_chip_data(data);
> - int offset = data->irq - ab8500->irq_base;
> + int offset = data->irq;
Are you sure this is right? I thought irq_data->irq was the Linux IRQ
number, irq_data->hwirq was the interrupt number inside the domain.
On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones <[email protected]> wrote:
> + ? ? ? ? ? ? ? ? ? ? ? clock-frequency = <100000>;
> + ? ? ? ? ? ? ? ? ? ? ? stericsson,slsu = <0xe>;
> + ? ? ? ? ? ? ? ? ? ? ? stericsson,tft = <1>;
> + ? ? ? ? ? ? ? ? ? ? ? stericsson,rft = <8>;
> + ? ? ? ? ? ? ? ? ? ? ? stericsson,timeout = <200>;
> + ? ? ? ? ? ? ? ? ? ? ? stericsson,i2c_freq_mode_fast;
In the context of my patch to the board file, we conclude that the
clock-frequency should be set to 400000 instead.
Yours,
Linus Walleij
On 18/06/12 12:29, Linus Walleij wrote:
> On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones<[email protected]> wrote:
>
>> + clock-frequency =<100000>;
>> + stericsson,slsu =<0xe>;
>> + stericsson,tft =<1>;
>> + stericsson,rft =<8>;
>> + stericsson,timeout =<200>;
>> + stericsson,i2c_freq_mode_fast;
>
> In the context of my patch to the board file, we conclude that the
> clock-frequency should be set to 400000 instead.
Right, I have already changed that in my patch-set.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Jun 18, 2012 at 11:20:03AM +0100, Russell King - ARM Linux wrote:
> > + int offset = data->irq;
> Are you sure this is right? I thought irq_data->irq was the Linux IRQ
> number, irq_data->hwirq was the interrupt number inside the domain.
Yes, that's correct. I'm surprised the code runs without crashing...
On Mon, Jun 18, 2012 at 12:20 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Mon, Jun 18, 2012 at 10:02:53AM +0100, Lee Jones wrote:
>> @@ -361,7 +362,7 @@ static void ab8500_irq_sync_unlock(struct irq_data *data)
>> ?static void ab8500_irq_mask(struct irq_data *data)
>> ?{
>> ? ? ? struct ab8500 *ab8500 = irq_data_get_irq_chip_data(data);
>> - ? ? int offset = data->irq - ab8500->irq_base;
>> + ? ? int offset = data->irq;
>
> Are you sure this is right? ?I thought irq_data->irq was the Linux IRQ
> number, irq_data->hwirq was the interrupt number inside the domain.
Agree, this need to be fixed and respin...
Yours,
Linus Walleij
On 18/06/12 19:56, Mark Brown wrote:
> On Mon, Jun 18, 2012 at 11:20:03AM +0100, Russell King - ARM Linux wrote:
>
>>> + int offset = data->irq;
>
>> Are you sure this is right? I thought irq_data->irq was the Linux IRQ
>> number, irq_data->hwirq was the interrupt number inside the domain.
Thanks. Good catch. I have just sent a new patch-set to the mailing list
where I have sent the fixed- up patch again.
> Yes, that's correct. I'm surprised the code runs without crashing...
Oddly the code ran perfectly.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 20, 2012 at 3:00 PM, Lee Jones <[email protected]> wrote:
> On 18/06/12 19:56, Mark Brown wrote:
>>
>> Yes, that's correct. ?I'm surprised the code runs without crashing...
>
> Oddly the code ran perfectly.
So how did you test it?
The only way to generate an IRQ on the AB8500 with the mainline code is
to push the power button, that should generate an event from the
driver in drivers/input/misc/ab8500-ponkey.c that you can listen to
by doing cat on the proper /dev/input/eventN file.
Please test this scenario before and after the patch and you have some
indication that the IRQdomain is working.
Yours,
Linus Walleij
On 21/06/12 08:37, Linus Walleij wrote:
> On Wed, Jun 20, 2012 at 3:00 PM, Lee Jones<[email protected]> wrote:
>> On 18/06/12 19:56, Mark Brown wrote:
>>>
>>> Yes, that's correct. I'm surprised the code runs without crashing...
>>
>> Oddly the code ran perfectly.
>
> So how did you test it?
>
> The only way to generate an IRQ on the AB8500 with the mainline code is
> to push the power button, that should generate an event from the
> driver in drivers/input/misc/ab8500-ponkey.c that you can listen to
> by doing cat on the proper /dev/input/eventN file.
>
> Please test this scenario before and after the patch and you have some
> indication that the IRQdomain is working.
Yep, works perfectly fall and rise:
> CPU0 CPU1
> 6: 0 2 ab8500 ab8500-ponkey-dbf
> 7: 0 2 ab8500 ab8500-ponkey-dbr
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Mon, Jun 11, 2012 at 04:24:58PM +0100, Lee Jones wrote:
> This patch adds the correct compatible string for use during Device Tree
> population. Without it the DB8500 PRCMU will not be probed.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/mfd/db8500-prcmu.c | 5 +++++
> 1 file changed, 5 insertions(+)
Patch applied now, thanks.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Hi Lee,
On Mon, Jun 11, 2012 at 04:24:59PM +0100, Lee Jones wrote:
> Now the AB8500 has its own IRQ domain it needs to be initialised earlier
> in the boot sequence. As the AB8500 relies on the DB8500 PRCMU we need to
> reflect this change for the PRCMU driver too.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/mfd/db8500-prcmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Hi Lee,
On Mon, Jun 11, 2012 at 04:25:00PM +0100, Lee Jones wrote:
> The AB8500 is soon to have its own IRQ domain. For this to be useful
> the driver needs to be initialised earlier in the boot sequence. Here
> we move initialisation forward from arch_initcall to core_initcall time.
>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/mfd/ab8500-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied as well, thanks.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/