This patch frees stmpe driver from tension of freeing resources :)
devm_* derivatives of multiple routines are used while allocating resources,
which would be freed automatically by kernel.
Signed-off-by: Viresh Kumar <[email protected]>
---
V1->V2:
------
- Rebased over latest for-next from Samuel
- updated additional kzalloc with devm_kzalloc(), first one seen below.
drivers/mfd/stmpe.c | 60 +++++++++++++++++++----------------------------------
1 file changed, 21 insertions(+), 39 deletions(-)
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index ba157d4..c0df4b9 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -1052,17 +1052,17 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
int ret;
if (!pdata) {
- if (np) {
- pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
- if (!pdata)
- return -ENOMEM;
-
- stmpe_of_probe(pdata, np);
- } else
+ if (!np)
return -EINVAL;
+
+ pdata = devm_kzalloc(ci->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ stmpe_of_probe(pdata, np);
}
- stmpe = kzalloc(sizeof(struct stmpe), GFP_KERNEL);
+ stmpe = devm_kzalloc(ci->dev, sizeof(struct stmpe), GFP_KERNEL);
if (!stmpe)
return -ENOMEM;
@@ -1084,11 +1084,12 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
ci->init(stmpe);
if (pdata->irq_over_gpio) {
- ret = gpio_request_one(pdata->irq_gpio, GPIOF_DIR_IN, "stmpe");
+ ret = devm_gpio_request_one(ci->dev, pdata->irq_gpio,
+ GPIOF_DIR_IN, "stmpe");
if (ret) {
dev_err(stmpe->dev, "failed to request IRQ GPIO: %d\n",
ret);
- goto out_free;
+ return ret;
}
stmpe->irq = gpio_to_irq(pdata->irq_gpio);
@@ -1105,48 +1106,37 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
dev_err(stmpe->dev,
"%s does not support no-irq mode!\n",
stmpe->variant->name);
- ret = -ENODEV;
- goto free_gpio;
+ return -ENODEV;
}
stmpe->variant = stmpe_noirq_variant_info[stmpe->partnum];
}
ret = stmpe_chip_init(stmpe);
if (ret)
- goto free_gpio;
+ return ret;
if (stmpe->irq >= 0) {
ret = stmpe_irq_init(stmpe, np);
if (ret)
- goto free_gpio;
+ return ret;
- ret = request_threaded_irq(stmpe->irq, NULL, stmpe_irq,
- pdata->irq_trigger | IRQF_ONESHOT,
+ ret = devm_request_threaded_irq(ci->dev, stmpe->irq, NULL,
+ stmpe_irq, pdata->irq_trigger | IRQF_ONESHOT,
"stmpe", stmpe);
if (ret) {
dev_err(stmpe->dev, "failed to request IRQ: %d\n",
ret);
- goto free_gpio;
+ return ret;
}
}
ret = stmpe_devices_init(stmpe);
- if (ret) {
- dev_err(stmpe->dev, "failed to add children\n");
- goto out_removedevs;
- }
-
- return 0;
+ if (!ret)
+ return 0;
-out_removedevs:
+ dev_err(stmpe->dev, "failed to add children\n");
mfd_remove_devices(stmpe->dev);
- if (stmpe->irq >= 0)
- free_irq(stmpe->irq, stmpe);
-free_gpio:
- if (pdata->irq_over_gpio)
- gpio_free(pdata->irq_gpio);
-out_free:
- kfree(stmpe);
+
return ret;
}
@@ -1154,14 +1144,6 @@ int stmpe_remove(struct stmpe *stmpe)
{
mfd_remove_devices(stmpe->dev);
- if (stmpe->irq >= 0)
- free_irq(stmpe->irq, stmpe);
-
- if (stmpe->pdata->irq_over_gpio)
- gpio_free(stmpe->pdata->irq_gpio);
-
- kfree(stmpe);
-
return 0;
}
--
1.7.12.rc2.18.g61b472e
From: Vipul Kumar Samar <[email protected]>
This patch extends existing DT support for stmpe devices. Bindings are mentioned
in binding document.
Signed-off-by: Vipul Kumar Samar <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
V1->V2:
------
- Partial DT support was already there, which i missed earlier.
- Now, this patch is an enhancement of existing DT support.
Documentation/devicetree/bindings/mfd/stmpe.txt | 127 ++++++++++--
drivers/mfd/stmpe-i2c.c | 23 ++-
drivers/mfd/stmpe-spi.c | 15 ++
drivers/mfd/stmpe.c | 264 +++++++++++++++++++++---
4 files changed, 384 insertions(+), 45 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
index 8f0aeda..44aebf3 100644
--- a/Documentation/devicetree/bindings/mfd/stmpe.txt
+++ b/Documentation/devicetree/bindings/mfd/stmpe.txt
@@ -1,25 +1,124 @@
-* STMPE Multi-Functional Device
+ST Microelectronics STMPE Multi-Functional Device
+-------------------------------------------------
+STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad,
+touchscreen, adc, pwm, rotator.
+
+stmpe:
+-----
Required properties:
- - compatible : "st,stmpe[811|1601|2401|2403]"
- - reg : I2C address of the device
+- compatible: Must be one of: "st,stmpe610", "st,stmpe801", "st,stmpe811",
+ "st,stmpe1601", "st,stmpe2401", "st,stmpe2403",
+- id: device id to distinguish between multiple STMPEs on the same board
+- reg: I2C/SPI address of the device
+
+Optional properties:
+- irq-trigger: IRQ trigger to use for the interrupt to the host
+- irq-invert-polarity: bool, IRQ line is connected with reversed polarity
+- autosleep-timeout: inactivity timeout in milliseconds for autosleep. Valid
+ entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
+- interrupts: interrupt number of the device, if interrupt is not via gpio
+- interrupt-parent: Specifies which IRQ controller we're connected to
+- irq-over-gpio: bool, true if gpio is used to get irq
+- irq-gpios: gpio number over which irq will be requested (significant only if
+ irq-over-gpio is true)
+- i2c-client-wake: Marks the input device as wakable
+
+stmpe-keypad:
+--------------
+Required properties in addition to those specified by the shared matrix-keyboard
+bindings:
+- compatible: Must be "stmpe,keypad"
Optional properties:
- - interrupts : The interrupt outputs from the controller
- - interrupt-controller : Marks the device node as an interrupt controller
- - interrupt-parent : Specifies which IRQ controller we're connected to
- - i2c-client-wake : Marks the input device as wakable
- - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
+- keypad,scan-count: number of key scanning cycles to confirm key data. Maximum
+ is STMPE_KEYPAD_MAX_SCAN_COUNT.
+- keypad,debounce-ms: debounce interval, in ms. Maximum is
+ STMPE_KEYPAD_MAX_DEBOUNCE.
+- keypad,no-autorepeat: bool, disable key autorepeat
+
+stmpe-gpio:
+-----------
+Required properties:
+- compatible: Must be "stmpe,gpio"
+
+Optional properties:
+- norequest-mask: bitmask specifying which GPIOs should _not_ be requestable due
+ to different usage (e.g. touch, keypad) STMPE_GPIO_NOREQ_* macros can be used
+ here.
+
+stmpe-ts:
+-----------
+Required properties:
+- compatible: Must be "stmpe,ts"
+
+Optional properties:
+- sample-time: ADC converstion time in number of clock. (0 -> 36 clocks, 1 ->
+ 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, 5 -> 96 clocks, 6
+ -> 144 clocks), recommended is 4.
+- mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC)
+- ref-sel: ADC reference source (0 -> internal reference, 1 -> external
+ reference)
+- adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz)
+- ave-ctrl: Sample average control (0 -> 1 sample, 1 -> 2 samples, 2 -> 4
+ samples, 3 -> 8 samples)
+- touch-det-delay: Touch detect interrupt delay (0 -> 10 us, 1 -> 50 us, 2 ->
+ 100 us, 3 -> 500 us, 4-> 1 ms, 5 -> 5 ms, 6 -> 10 ms, 7 -> 50 ms) recommended
+ is 3
+- settling: Panel driver settling time (0 -> 10 us, 1 -> 100 us, 2 -> 500 us, 3
+ -> 1 ms, 4 -> 5 ms, 5 -> 10 ms, 6 for 50 ms, 7 -> 100 ms) recommended is 2
+- fraction-z: Length of the fractional part in z (fraction-z ([0..7]) = Count of
+ the fractional part) recommended is 7
+- i-drive: current limit value of the touchscreen drivers (0 -> 20 mA typical 35
+ mA max, 1 -> 50 mA typical 80 mA max)
+
+stmpe-adc:
+-----------
+Required properties:
+- compatible: Must be "stmpe,adc"
+
+stmpe-pwm:
+-----------
+Required properties:
+- compatible: Must be "stmpe,pwm"
+
+stmpe-rotator:
+-----------
+Required properties:
+- compatible: Must be "stmpe,rotator"
Example:
+-------
+stmpe can be present over i2c or spi bus, so its node would be added as child
+node of either of spi or i2c device.
+
+stmpe-devices should be added as child nodes of parent stmpe node.
+spi@e0100000 {
+ <...>
stmpe1601: stmpe1601@40 {
+ status = "okay";
compatible = "st,stmpe1601";
- reg = <0x40>;
- interrupts = <26 0x4>;
- interrupt-parent = <&gpio6>;
- interrupt-controller;
+ reg = <0>;
+
+ <.. spi controller specific data ..>
+
+ id = <0>;
+ irq-over-gpio;
+ irq-gpios = <&gpio1 6 0x4>;
+ irq-trigger = <0x2>;
- i2c-client-wake;
- st,autosleep-timeout = <1024>;
+ stmpe610-ts {
+ compatible = "stmpe,ts";
+ ts,sample-time = <4>;
+ ts,mod-12b = <1>;
+ ts,ref-sel = <0>;
+ ts,adc-freq = <1>;
+ ts,ave-ctrl = <1>;
+ ts,touch-det-delay = <2>;
+ ts,settling = <2>;
+ ts,fraction-z = <7>;
+ ts,i-drive = <1>;
+ };
};
+};
diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c
index 947a06a..aaa2da7 100644
--- a/drivers/mfd/stmpe-i2c.c
+++ b/drivers/mfd/stmpe-i2c.c
@@ -13,6 +13,7 @@
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/types.h>
#include "stmpe.h"
@@ -81,12 +82,28 @@ static const struct i2c_device_id stmpe_i2c_id[] = {
};
MODULE_DEVICE_TABLE(i2c, stmpe_id);
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+ { .compatible = "st,stmpe610", .data = &stmpe_i2c_id[0], },
+ { .compatible = "st,stmpe801", .data = &stmpe_i2c_id[1], },
+ { .compatible = "st,stmpe811", .data = &stmpe_i2c_id[2], },
+ { .compatible = "st,stmpe1601", .data = &stmpe_i2c_id[3], },
+ { .compatible = "st,stmpe2401", .data = &stmpe_i2c_id[4], },
+ { .compatible = "st,stmpe2403", .data = &stmpe_i2c_id[5], },
+ { }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
static struct i2c_driver stmpe_i2c_driver = {
- .driver.name = "stmpe-i2c",
- .driver.owner = THIS_MODULE,
+ .driver = {
+ .name = "stmpe-i2c",
+ .owner = THIS_MODULE,
#ifdef CONFIG_PM
- .driver.pm = &stmpe_dev_pm_ops,
+ .pm = &stmpe_dev_pm_ops,
#endif
+ .of_match_table = of_match_ptr(stmpe_dt_ids),
+ },
.probe = stmpe_i2c_probe,
.remove = __devexit_p(stmpe_i2c_remove),
.id_table = stmpe_i2c_id,
diff --git a/drivers/mfd/stmpe-spi.c b/drivers/mfd/stmpe-spi.c
index 9edfe86..1e2bff0 100644
--- a/drivers/mfd/stmpe-spi.c
+++ b/drivers/mfd/stmpe-spi.c
@@ -11,6 +11,7 @@
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/types.h>
#include "stmpe.h"
@@ -119,6 +120,19 @@ static const struct spi_device_id stmpe_spi_id[] = {
};
MODULE_DEVICE_TABLE(spi, stmpe_id);
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_dt_ids[] = {
+ { .compatible = "st,stmpe610", .data = (void *)STMPE610, },
+ { .compatible = "st,stmpe801", .data = (void *)STMPE801, },
+ { .compatible = "st,stmpe811", .data = (void *)STMPE811, },
+ { .compatible = "st,stmpe1601", .data = (void *)STMPE1601, },
+ { .compatible = "st,stmpe2401", .data = (void *)STMPE2401, },
+ { .compatible = "st,stmpe2403", .data = (void *)STMPE2403, },
+ { }
+};
+MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
+#endif
+
static struct spi_driver stmpe_spi_driver = {
.driver = {
.name = "stmpe-spi",
@@ -126,6 +140,7 @@ static struct spi_driver stmpe_spi_driver = {
#ifdef CONFIG_PM
.pm = &stmpe_dev_pm_ops,
#endif
+ .of_match_table = of_match_ptr(stmpe_dt_ids),
},
.probe = stmpe_spi_probe,
.remove = __devexit_p(stmpe_spi_remove),
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index c0df4b9..d69f24b 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -8,11 +8,14 @@
*/
#include <linux/gpio.h>
+#include <linux/err.h>
#include <linux/export.h>
#include <linux/kernel.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
#include <linux/pm.h>
#include <linux/slab.h>
#include <linux/mfd/core.h>
@@ -981,6 +984,230 @@ static int __devinit stmpe_add_device(struct stmpe *stmpe,
NULL, stmpe->irq_base, stmpe->domain);
}
+#ifdef CONFIG_OF
+static const struct of_device_id stmpe_keypad_ids[] = {
+ { .compatible = "stmpe,keypad" },
+ {},
+};
+
+static const struct of_device_id stmpe_gpio_ids[] = {
+ { .compatible = "stmpe,gpio" },
+ {},
+};
+
+static const struct of_device_id stmpe_ts_ids[] = {
+ { .compatible = "stmpe,ts" },
+ {},
+};
+
+static const struct of_device_id stmpe_adc_ids[] = {
+ { .compatible = "stmpe,adc" },
+ {},
+};
+
+static const struct of_device_id stmpe_pwm_ids[] = {
+ { .compatible = "stmpe,pwm" },
+ {},
+};
+
+static const struct of_device_id stmpe_rotator_ids[] = {
+ { .compatible = "stmpe,rotator" },
+ {},
+};
+
+static struct stmpe_keypad_platform_data *
+get_keyboard_pdata_dt(struct device *dev, struct device_node *np)
+{
+ struct stmpe_keypad_platform_data *pdata;
+ u32 val;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_warn(dev, "stmpe keypad kzalloc fail\n");
+ return NULL;
+ }
+
+ if (!of_property_read_u32(np, "keypad,scan-count", &val))
+ pdata->scan_count = val;
+ if (!of_property_read_u32(np, "keypad,debounce-ms", &val))
+ pdata->debounce_ms = val;
+ if (of_property_read_bool(np, "keypad,no-autorepeat"))
+ pdata->no_autorepeat = true;
+
+ return pdata;
+}
+
+static struct stmpe_gpio_platform_data *get_gpio_pdata_dt(struct device *dev,
+ struct device_node *np)
+{
+ struct stmpe_gpio_platform_data *pdata;
+ u32 val;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_warn(dev, "stmpe gpio kzalloc fail\n");
+ return NULL;
+ }
+
+ if (!of_property_read_u32(np, "gpio,norequest-mask", &val))
+ pdata->norequest_mask = val;
+
+ /* assign gpio numbers dynamically */
+ pdata->gpio_base = -1;
+
+ return pdata;
+}
+
+static struct stmpe_ts_platform_data *get_ts_pdata_dt(struct device *dev,
+ struct device_node *np)
+{
+ struct stmpe_ts_platform_data *pdata;
+ u32 val;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_warn(dev, "stmpe ts kzalloc fail\n");
+ return NULL;
+ }
+
+ if (!of_property_read_u32(np, "ts,sample-time", &val))
+ pdata->sample_time = val;
+ if (!of_property_read_u32(np, "ts,mod-12b", &val))
+ pdata->mod_12b = val;
+ if (!of_property_read_u32(np, "ts,ref-sel", &val))
+ pdata->ref_sel = val;
+ if (!of_property_read_u32(np, "ts,adc-freq", &val))
+ pdata->adc_freq = val;
+ if (!of_property_read_u32(np, "ts,ave-ctrl", &val))
+ pdata->ave_ctrl = val;
+ if (!of_property_read_u32(np, "ts,touch-det-delay", &val))
+ pdata->touch_det_delay = val;
+ if (!of_property_read_u32(np, "ts,settling", &val))
+ pdata->settling = val;
+ if (!of_property_read_u32(np, "ts,fraction-z", &val))
+ pdata->fraction_z = val;
+ if (!of_property_read_u32(np, "ts,i-drive", &val))
+ pdata->i_drive = val;
+
+ return pdata;
+}
+
+static struct stmpe_variant_block *
+match_variant_block(struct stmpe_variant_info *variant,
+ enum stmpe_block blocks)
+{
+ int i;
+
+ for (i = 0; i < variant->num_blocks; i++) {
+ struct stmpe_variant_block *block = &variant->blocks[i];
+
+ if (blocks & block->block)
+ return block;
+ }
+ return NULL;
+}
+
+static int stmpe_probe_config_dt(struct device *dev,
+ struct stmpe_platform_data *pdata, struct device_node *np)
+{
+ enum of_gpio_flags flags;
+
+ pdata->irq_base = irq_alloc_descs(-1, 0, STMPE_NR_IRQS, 0);
+ if (IS_ERR_VALUE(pdata->irq_base)) {
+ dev_err(dev, "%s: irq desc alloc failed\n", __func__);
+ return -ENXIO;
+ }
+
+ if (of_property_read_bool(np, "irq_over_gpio")) {
+ pdata->irq_over_gpio = true;
+
+ pdata->irq_gpio = of_get_named_gpio_flags(np, "irq-gpios", 0,
+ &flags);
+ if (!pdata->irq_gpio)
+ dev_dbg(dev, "unable to get irq_gpio from %s.",
+ __func__);
+ }
+
+ if (of_property_read_u32(np, "irq-trigger", &pdata->irq_trigger))
+ dev_dbg(dev, "unable to get irq_trigger\n");
+
+ if (of_property_read_bool(np, "irq-invert-polarity"))
+ pdata->irq_invert_polarity = true;
+
+ if (!of_property_read_u32(np, "autosleep-timeout",
+ &pdata->autosleep_timeout))
+ pdata->autosleep = true;
+
+ if (of_property_read_u32(np, "id", &pdata->id))
+ dev_dbg(dev, "unable to get id\n");
+
+ return 0;
+}
+
+static int stmpe_of_devices_init(struct stmpe *stmpe)
+{
+ struct stmpe_variant_info *variant = stmpe->variant;
+ struct device_node *nc, *np = stmpe->dev->of_node;
+ struct stmpe_variant_block *block;
+ enum stmpe_block blockid;
+ int ret = -EINVAL;
+
+ for_each_child_of_node(np, nc) {
+ if (of_match_node(stmpe_keypad_ids, nc)) {
+ blockid = STMPE_BLOCK_KEYPAD;
+ stmpe->pdata->keypad = get_keyboard_pdata_dt(stmpe->dev,
+ nc);
+ if (!stmpe->pdata->keypad)
+ return -ENOMEM;
+ } else if (of_match_node(stmpe_gpio_ids, nc)) {
+ blockid = STMPE_BLOCK_GPIO;
+ stmpe->pdata->gpio = get_gpio_pdata_dt(stmpe->dev, nc);
+ if (!stmpe->pdata->gpio)
+ return -ENOMEM;
+ } else if (of_match_node(stmpe_ts_ids, nc)) {
+ blockid = STMPE_BLOCK_TOUCHSCREEN;
+ stmpe->pdata->ts = get_ts_pdata_dt(stmpe->dev, nc);
+ if (!stmpe->pdata->ts)
+ return -ENOMEM;
+ } else if (of_match_node(stmpe_adc_ids, nc)) {
+ blockid = STMPE_BLOCK_ADC;
+ } else if (of_match_node(stmpe_pwm_ids, nc)) {
+ blockid = STMPE_BLOCK_PWM;
+ } else if (of_match_node(stmpe_rotator_ids, nc)) {
+ blockid = STMPE_BLOCK_ROTATOR;
+ } else {
+ dev_warn(stmpe->dev, "no matching device node found\n");
+ return -EINVAL;
+ }
+
+ block = match_variant_block(variant, blockid);
+ if (!block) {
+ dev_err(stmpe->dev, "variant doesn't support blockid: %d\n",
+ blockid);
+ continue;
+ }
+
+ ret = stmpe_add_device(stmpe, block->cell);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
+#else
+static inline int stmpe_probe_config_dt(struct stmpe_platform_data *pdata,
+ struct device_node *np)
+{
+ return -ENODEV;
+}
+
+static inline int stmpe_of_devices_init(struct stmpe *stmpe)
+{
+ return -ENODEV;
+}
+#endif
+
static int __devinit stmpe_devices_init(struct stmpe *stmpe)
{
struct stmpe_variant_info *variant = stmpe->variant;
@@ -1017,32 +1244,6 @@ static int __devinit stmpe_devices_init(struct stmpe *stmpe)
return ret;
}
-void __devinit stmpe_of_probe(struct stmpe_platform_data *pdata,
- struct device_node *np)
-{
- struct device_node *child;
-
- of_property_read_u32(np, "st,autosleep-timeout",
- &pdata->autosleep_timeout);
-
- pdata->autosleep = (pdata->autosleep_timeout) ? true : false;
-
- for_each_child_of_node(np, child) {
- if (!strcmp(child->name, "stmpe_gpio")) {
- pdata->blocks |= STMPE_BLOCK_GPIO;
- }
- if (!strcmp(child->name, "stmpe_keypad")) {
- pdata->blocks |= STMPE_BLOCK_KEYPAD;
- }
- if (!strcmp(child->name, "stmpe_touchscreen")) {
- pdata->blocks |= STMPE_BLOCK_TOUCHSCREEN;
- }
- if (!strcmp(child->name, "stmpe_adc")) {
- pdata->blocks |= STMPE_BLOCK_ADC;
- }
- }
-}
-
/* Called from client specific probe routines */
int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
{
@@ -1059,7 +1260,11 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
if (!pdata)
return -ENOMEM;
- stmpe_of_probe(pdata, np);
+ ret = stmpe_probe_config_dt(ci->dev, pdata, np);
+ if (ret) {
+ dev_err(ci->dev, "probe_config_dt failed\n");
+ return ret;
+ }
}
stmpe = devm_kzalloc(ci->dev, sizeof(struct stmpe), GFP_KERNEL);
@@ -1130,7 +1335,10 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
}
}
- ret = stmpe_devices_init(stmpe);
+ if (np)
+ ret = stmpe_of_devices_init(stmpe);
+ else
+ ret = stmpe_devices_init(stmpe);
if (!ret)
return 0;
--
1.7.12.rc2.18.g61b472e
> > Big fat NACK.
> >
> > You've just overwritten the current implementation with your own.
> > Please take time to understand the mechanisms in place before
> > you submit any changes or additions to it.
>
> :)
>
> My fault. Comments on all overwritten stuff accepted
Okay, great.
> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> +- irq-over-gpio: bool, true if gpio is used to get irq
> >> +- irq-gpios: gpio number over which irq will be requested (significant only if
> >> + irq-over-gpio is true)
> >
> > You don't need these. Use gpio_to_irq() instead.
>
> I am passing gpio numbers here and am doing gpio_to_irq() in driver.
> Didn't get this one :(
For a start you have 'irq-over-gpio' in the binding document and Device Tree
and 'irq_over_gpio' in the code. Has it even been tested?
GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a
look to see how they are handled without adding unnecessary DT bindings.
> >> Optional properties:
> >> - - interrupts : The interrupt outputs from the controller
> >> - - interrupt-controller : Marks the device node as an interrupt controller
> >> - - interrupt-parent : Specifies which IRQ controller we're connected to
> >> - - i2c-client-wake : Marks the input device as wakable
> >> - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
> >
> > And you've removed these why?
>
> No. They are readjusted...
Please don't readjust them.
They are clear and easily readable in their current state. In the new
implementation they are messy and hard to decipher.
> One thing removed is interrupt-controller. I had a doubt on this.
> stmpe, by itself doesn't give any interrupt lines to SoC to freely use
> them. Instead
> gpio controller driver part of it does. And so adding
> interrupt-controller for that is
> the right option.
That's fine, I don't have an issue with that.
> stmpe is an interrupt controller for the IP's which are present inside
> it: gpio, adc.
> But interrupt lines for them are managed by stmpe driver internally. So should
> we really add interrupt-controller for it?
You can't manage IRQ lines internally, you have to go through
the IRQ subsystem. When you request an IRQ via device tree you
will do so like this:
interrupts = <25 0x1>;
interrupt-parent = <&stmpe-gpio>;
In this example the stmpe-gpio node must advertise itself using
the interrupt-controller property.
The STMPE GPIO controller can't be used by Device Tree yet in any case,
because it doesn't have an IRQ domain. This is compulsory, or it won't
work. Have you tried to test this functionality yet?
> >> +- keypad,scan-count: number of key scanning cycles to confirm key data. Maximum
> >> + is STMPE_KEYPAD_MAX_SCAN_COUNT.
> >> +- keypad,debounce-ms: debounce interval, in ms. Maximum is
> >> + STMPE_KEYPAD_MAX_DEBOUNCE.
> >> +- keypad,no-autorepeat: bool, disable key autorepeat
> >
> > See "When adding new bindings, ask yourself" above.
>
> Yes, these are required. This is part of platform data it expects.
Okay, I've just had a look at my tested bindings.
We already have these:
debounce-interval /* This is a generic binding */
st,scan-count /* These are vendor specific */
st,no-autorepeat /* " */
Vendor specific bindings should be "<vendor>,<binding>", rather than
"<type_of_driver>,<binding>"
> >> + reg = <0>;
> >
> > You have reg twice here. Also reg should never be '0'.
>
> For SPI, there are chip selects and there is no reg offset.
I understand the addressing issues, but you have 'reg' twice.
Which one should be used?
> >> + stmpe610-ts {
> >> + compatible = "stmpe,ts";
> >> + ts,sample-time = <4>;
> >> + ts,mod-12b = <1>;
> >> + ts,ref-sel = <0>;
> >> + ts,adc-freq = <1>;
> >> + ts,ave-ctrl = <1>;
> >> + ts,touch-det-delay = <2>;
> >> + ts,settling = <2>;
> >> + ts,fraction-z = <7>;
> >> + ts,i-drive = <1>;
> >
> > Wow! See "When adding new bindings, ask yourself" above.
>
> :)
> They are required. I didn't get your point, sorry.
I didn't go through them, but are you sure that:
1. Can I do without them?
1.1 Can I derive the configuration from other things?
2.2 Are they _really_ required, or am I just blindly copying platform data?
2. Does a similar binding already exist?
3. Can other drivers make use of them?
3.1 If so, create a generic binding
3.2 If not, prepend the binding with "<vendor>,"
There looks like a lot there. So each of those values vary, or can
any of them be hard-coded? Just because they're in platform code,
it doesn't mean they should be in the DT binding. A lot of platform
code is actually crap.
> >> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> >> +static struct stmpe_keypad_platform_data *
> >> +get_keyboard_pdata_dt(struct device *dev, struct device_node *np)
> >> +{
> >> + struct stmpe_keypad_platform_data *pdata;
> >> + u32 val;
> >> +
> >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >> + if (!pdata) {
> >> + dev_warn(dev, "stmpe keypad kzalloc fail\n");
> >> + return NULL;
> >> + }
> >> +
> >> + if (!of_property_read_u32(np, "keypad,scan-count", &val))
> >> + pdata->scan_count = val;
> >> + if (!of_property_read_u32(np, "keypad,debounce-ms", &val))
> >> + pdata->debounce_ms = val;
> >> + if (of_property_read_bool(np, "keypad,no-autorepeat"))
> >> + pdata->no_autorepeat = true;
> >
> > Why are you (re)adding these here? Have you even looked in the driver?
>
> Because i wanted to keep all DT stuff together. Obviously i have seen keypad
> driver earlier :)
If you had, you'd realise that these bindings already exist. ;)
I appreciate that the patches haven't landed yet, but they've been
on the MLs for some time now, and already have some important Acks.
> I am not setting pdata of stmpe here, but pdata of keypad.
Exactly, so why are you doing it in the STMPE driver?
It's keypad data, set it up in the keypad driver.
> >> +static struct stmpe_gpio_platform_data *get_gpio_pdata_dt(struct device *dev,
> >> + struct device_node *np)
> >> +{
> >> + struct stmpe_gpio_platform_data *pdata;
> >> + u32 val;
> >> +
> >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >> + if (!pdata) {
> >> + dev_warn(dev, "stmpe gpio kzalloc fail\n");
> >> + return NULL;
> >> + }
> >> +
> >> + if (!of_property_read_u32(np, "gpio,norequest-mask", &val))
> >> + pdata->norequest_mask = val;
> >> +
> >> + /* assign gpio numbers dynamically */
> >> + pdata->gpio_base = -1;
> >> +
> >> + return pdata;
> >> +}
> >
> > Is this function really required? Even if is is, should it live here
> > or in the STMPE driver?
>
> As said earlier, either i can do DT parsing of sub-modules of stmpe in
> their specific files or in stmpe driver itself. Because currently platform
> data of those sub-modules is passed from stmpe, i kept them here only.
> stmpe sub modules can't live without stmpe driver and so keeping all
> binding stuff here isn't that bad of an idea.
Yes, don't do that.
Do DT parsing in the driver it references, not in the most common denominator.
> >> +static struct stmpe_ts_platform_data *get_ts_pdata_dt(struct device *dev,
> >> + struct device_node *np)
> >> +{
> >> + struct stmpe_ts_platform_data *pdata;
> >> + u32 val;
> >> +
> >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >> + if (!pdata) {
> >> + dev_warn(dev, "stmpe ts kzalloc fail\n");
> >> + return NULL;
> >> + }
> >> +
> >> + if (!of_property_read_u32(np, "ts,sample-time", &val))
> >> + pdata->sample_time = val;
> >> + if (!of_property_read_u32(np, "ts,mod-12b", &val))
> >> + pdata->mod_12b = val;
> >> + if (!of_property_read_u32(np, "ts,ref-sel", &val))
> >> + pdata->ref_sel = val;
> >> + if (!of_property_read_u32(np, "ts,adc-freq", &val))
> >> + pdata->adc_freq = val;
> >> + if (!of_property_read_u32(np, "ts,ave-ctrl", &val))
> >> + pdata->ave_ctrl = val;
> >> + if (!of_property_read_u32(np, "ts,touch-det-delay", &val))
> >> + pdata->touch_det_delay = val;
> >> + if (!of_property_read_u32(np, "ts,settling", &val))
> >> + pdata->settling = val;
> >> + if (!of_property_read_u32(np, "ts,fraction-z", &val))
> >> + pdata->fraction_z = val;
> >> + if (!of_property_read_u32(np, "ts,i-drive", &val))
> >> + pdata->i_drive = val;
> >> +
> >> + return pdata;
> >> +}
> >
> > As above.
> >
> > I'm going to stop my review here. I think you get the idea.
>
> I got most of your worries, but couldn't understand the issue of passing
> all pdata fields as DT bindings.
It's a really big issue with DT enablement. Platform data has been filled
with lots of junk over the years, to blindly move it all over to DT would
be foolish. Now is the time to go through each of them and pull out the
ones that we can do without, can be hard-coded or derived in the driver.
I'm more than sure that at least some of the above can be removed with
some thoughtful coding.
> Thanks for your review though. :)
No problem.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 22 November 2012 16:54, Lee Jones <[email protected]> wrote:
>> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
>> +Optional properties:
>> +- irq-trigger: IRQ trigger to use for the interrupt to the host
>> +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity
>
> Are these new?
>
> When adding new bindings, ask yourself:
I was trying to get information of these two bindings from other sources,
but couldn't find. The only other place can be the interrupts field, right?
Because interrupts field depends on the interrupt controller, which can
be a wide variety of controllers in our case, we can't use that. Also,
the cells of interrupts field will have something that is required to be
programmed in the interrupt controller and not in the user of IC.
But here we are talking about programming stmpe and so these bindings
are very much required in stmpe only.
Comments??
--
viresh
> From: Vipul Kumar Samar <[email protected]>
>
> This patch extends existing DT support for stmpe devices. Bindings are mentioned
> in binding document.
>
> Signed-off-by: Vipul Kumar Samar <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> V1->V2:
> ------
> - Partial DT support was already there, which i missed earlier.
> - Now, this patch is an enhancement of existing DT support.
Big fat NACK.
You've just overwritten the current implementation with your own.
Please take time to understand the mechanisms in place before
you submit any changes or additions to it.
> Documentation/devicetree/bindings/mfd/stmpe.txt | 127 ++++++++++--
> drivers/mfd/stmpe-i2c.c | 23 ++-
> drivers/mfd/stmpe-spi.c | 15 ++
> drivers/mfd/stmpe.c | 264 +++++++++++++++++++++---
> 4 files changed, 384 insertions(+), 45 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> index 8f0aeda..44aebf3 100644
> --- a/Documentation/devicetree/bindings/mfd/stmpe.txt
> +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt
> @@ -1,25 +1,124 @@
> -* STMPE Multi-Functional Device
> +ST Microelectronics STMPE Multi-Functional Device
> +-------------------------------------------------
>
> +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad,
> +touchscreen, adc, pwm, rotator.
> +
> +stmpe:
> +-----
> Required properties:
> - - compatible : "st,stmpe[811|1601|2401|2403]"
> - - reg : I2C address of the device
> +- compatible: Must be one of: "st,stmpe610", "st,stmpe801", "st,stmpe811",
> + "st,stmpe1601", "st,stmpe2401", "st,stmpe2403",
This is messy. Use the current format, just add the new versions.
> +- id: device id to distinguish between multiple STMPEs on the same board
> +- reg: I2C/SPI address of the device
Why is this better? Can't you just add "/SPI" to the current string?
> +Optional properties:
> +- irq-trigger: IRQ trigger to use for the interrupt to the host
> +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity
Are these new?
When adding new bindings, ask yourself:
1. Can I do without them?
1.1 Can I derive the configuration from other things?
2.2 Are they _really_ required, or am I just blindly copying platform data?
2. Does a similar binding already exist?
3. Can other drivers make use of them?
3.1 If so, create a generic binding
3.2 If not, prepend the binding with "<vendor>,"
> +- autosleep-timeout: inactivity timeout in milliseconds for autosleep. Valid
> + entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
> +- interrupts: interrupt number of the device, if interrupt is not via gpio
> +- interrupt-parent: Specifies which IRQ controller we're connected to
> +- irq-over-gpio: bool, true if gpio is used to get irq
> +- irq-gpios: gpio number over which irq will be requested (significant only if
What was wrong with the old format (below)? Now it's messy and hard to read.
Only provide changes which are _better_.
> + irq-over-gpio is true)
You don't need these. Use gpio_to_irq() instead.
> +- i2c-client-wake: Marks the input device as wakable
> +stmpe-keypad:
> +--------------
> +Required properties in addition to those specified by the shared matrix-keyboard
> +bindings:
> +- compatible: Must be "stmpe,keypad"
That's not how the current implementation works.
All you have to do is add child nodes with the names:
stmpe_gpio
stmpe_keypad
stmpe_touchscreen
stmpe_adc
> Optional properties:
> - - interrupts : The interrupt outputs from the controller
> - - interrupt-controller : Marks the device node as an interrupt controller
> - - interrupt-parent : Specifies which IRQ controller we're connected to
> - - i2c-client-wake : Marks the input device as wakable
> - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
And you've removed these why?
> +- keypad,scan-count: number of key scanning cycles to confirm key data. Maximum
> + is STMPE_KEYPAD_MAX_SCAN_COUNT.
> +- keypad,debounce-ms: debounce interval, in ms. Maximum is
> + STMPE_KEYPAD_MAX_DEBOUNCE.
> +- keypad,no-autorepeat: bool, disable key autorepeat
See "When adding new bindings, ask yourself" above.
> +stmpe-gpio:
> +-----------
> +Required properties:
> +- compatible: Must be "stmpe,gpio"
> +
> +Optional properties:
> +- norequest-mask: bitmask specifying which GPIOs should _not_ be requestable due
> + to different usage (e.g. touch, keypad) STMPE_GPIO_NOREQ_* macros can be used
> + here.
> +
> +stmpe-ts:
> +-----------
> +Required properties:
> +- compatible: Must be "stmpe,ts"
> +
> +Optional properties:
> +- sample-time: ADC converstion time in number of clock. (0 -> 36 clocks, 1 ->
> + 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks, 5 -> 96 clocks, 6
> + -> 144 clocks), recommended is 4.
> +- mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC)
> +- ref-sel: ADC reference source (0 -> internal reference, 1 -> external
> + reference)
> +- adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz)
> +- ave-ctrl: Sample average control (0 -> 1 sample, 1 -> 2 samples, 2 -> 4
> + samples, 3 -> 8 samples)
> +- touch-det-delay: Touch detect interrupt delay (0 -> 10 us, 1 -> 50 us, 2 ->
> + 100 us, 3 -> 500 us, 4-> 1 ms, 5 -> 5 ms, 6 -> 10 ms, 7 -> 50 ms) recommended
> + is 3
> +- settling: Panel driver settling time (0 -> 10 us, 1 -> 100 us, 2 -> 500 us, 3
> + -> 1 ms, 4 -> 5 ms, 5 -> 10 ms, 6 for 50 ms, 7 -> 100 ms) recommended is 2
> +- fraction-z: Length of the fractional part in z (fraction-z ([0..7]) = Count of
> + the fractional part) recommended is 7
> +- i-drive: current limit value of the touchscreen drivers (0 -> 20 mA typical 35
> + mA max, 1 -> 50 mA typical 80 mA max)
See "When adding new bindings, ask yourself" above.
> +stmpe-adc:
> +-----------
> +Required properties:
> +- compatible: Must be "stmpe,adc"
> +
> +stmpe-pwm:
> +-----------
> +Required properties:
> +- compatible: Must be "stmpe,pwm"
> +
> +stmpe-rotator:
> +-----------
> +Required properties:
> +- compatible: Must be "stmpe,rotator"
>
> Example:
> +-------
> +stmpe can be present over i2c or spi bus, so its node would be added as child
> +node of either of spi or i2c device.
> +
> +stmpe-devices should be added as child nodes of parent stmpe node.
>
> +spi@e0100000 {
This shouldn't be a child of the SPI device becuase it uses SPI.
Drivers use clocks, regulators, IRQ controller too, but they're
not children of those devices.
> stmpe1601: stmpe1601@40 {
> + status = "okay";
You only need this if it's been disabled somewhere else.
> compatible = "st,stmpe1601";
> - reg = <0x40>;
> - interrupts = <26 0x4>;
> - interrupt-parent = <&gpio6>;
> - interrupt-controller;
Why are you removing these?
> + reg = <0>;
You have reg twice here. Also reg should never be '0'.
> + <.. spi controller specific data ..>
> +
> + id = <0>;
Don't do this. Device IDs are Linux specific.
> + irq-over-gpio;
> + irq-gpios = <&gpio1 6 0x4>;
> + irq-trigger = <0x2>;
See "When adding new bindings, ask yourself" above.
> - i2c-client-wake;
> - st,autosleep-timeout = <1024>;
> + stmpe610-ts {
> + compatible = "stmpe,ts";
> + ts,sample-time = <4>;
> + ts,mod-12b = <1>;
> + ts,ref-sel = <0>;
> + ts,adc-freq = <1>;
> + ts,ave-ctrl = <1>;
> + ts,touch-det-delay = <2>;
> + ts,settling = <2>;
> + ts,fraction-z = <7>;
> + ts,i-drive = <1>;
Wow! See "When adding new bindings, ask yourself" above.
> + };
> };
> +};
> diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c
> index 947a06a..aaa2da7 100644
> --- a/drivers/mfd/stmpe-i2c.c
> +++ b/drivers/mfd/stmpe-i2c.c
> @@ -13,6 +13,7 @@
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> #include <linux/types.h>
> #include "stmpe.h"
>
> @@ -81,12 +82,28 @@ static const struct i2c_device_id stmpe_i2c_id[] = {
> };
> MODULE_DEVICE_TABLE(i2c, stmpe_id);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id stmpe_dt_ids[] = {
> + { .compatible = "st,stmpe610", .data = &stmpe_i2c_id[0], },
> + { .compatible = "st,stmpe801", .data = &stmpe_i2c_id[1], },
> + { .compatible = "st,stmpe811", .data = &stmpe_i2c_id[2], },
> + { .compatible = "st,stmpe1601", .data = &stmpe_i2c_id[3], },
> + { .compatible = "st,stmpe2401", .data = &stmpe_i2c_id[4], },
> + { .compatible = "st,stmpe2403", .data = &stmpe_i2c_id[5], },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
> +#endif
>
> static struct i2c_driver stmpe_i2c_driver = {
> - .driver.name = "stmpe-i2c",
> - .driver.owner = THIS_MODULE,
> + .driver = {
> + .name = "stmpe-i2c",
> + .owner = THIS_MODULE,
> #ifdef CONFIG_PM
> - .driver.pm = &stmpe_dev_pm_ops,
> + .pm = &stmpe_dev_pm_ops,
If you want to change these, you need to submit a seperate patch, as
they have nothing to do with DT:ing these driver.
> #endif
> + .of_match_table = of_match_ptr(stmpe_dt_ids),
> + },
> .probe = stmpe_i2c_probe,
> .remove = __devexit_p(stmpe_i2c_remove),
> .id_table = stmpe_i2c_id,
> diff --git a/drivers/mfd/stmpe-spi.c b/drivers/mfd/stmpe-spi.c
> index 9edfe86..1e2bff0 100644
> --- a/drivers/mfd/stmpe-spi.c
> +++ b/drivers/mfd/stmpe-spi.c
> @@ -11,6 +11,7 @@
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> #include <linux/types.h>
> #include "stmpe.h"
>
> @@ -119,6 +120,19 @@ static const struct spi_device_id stmpe_spi_id[] = {
> };
> MODULE_DEVICE_TABLE(spi, stmpe_id);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id stmpe_dt_ids[] = {
> + { .compatible = "st,stmpe610", .data = (void *)STMPE610, },
> + { .compatible = "st,stmpe801", .data = (void *)STMPE801, },
> + { .compatible = "st,stmpe811", .data = (void *)STMPE811, },
> + { .compatible = "st,stmpe1601", .data = (void *)STMPE1601, },
> + { .compatible = "st,stmpe2401", .data = (void *)STMPE2401, },
> + { .compatible = "st,stmpe2403", .data = (void *)STMPE2403, },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, stmpe_dt_ids);
> +#endif
>
> static struct spi_driver stmpe_spi_driver = {
> .driver = {
> .name = "stmpe-spi",
> @@ -126,6 +140,7 @@ static struct spi_driver stmpe_spi_driver = {
> #ifdef CONFIG_PM
> .pm = &stmpe_dev_pm_ops,
> #endif
> + .of_match_table = of_match_ptr(stmpe_dt_ids),
> },
> .probe = stmpe_spi_probe,
> .remove = __devexit_p(stmpe_spi_remove),
> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index c0df4b9..d69f24b 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -8,11 +8,14 @@
> */
>
> #include <linux/gpio.h>
> +#include <linux/err.h>
> #include <linux/export.h>
> #include <linux/kernel.h>
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> #include <linux/pm.h>
> #include <linux/slab.h>
> #include <linux/mfd/core.h>
> @@ -981,6 +984,230 @@ static int __devinit stmpe_add_device(struct stmpe *stmpe,
> NULL, stmpe->irq_base, stmpe->domain);
> }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id stmpe_keypad_ids[] = {
> + { .compatible = "stmpe,keypad" },
> + {},
> +};
> +
> +static const struct of_device_id stmpe_gpio_ids[] = {
> + { .compatible = "stmpe,gpio" },
> + {},
> +};
> +
> +static const struct of_device_id stmpe_ts_ids[] = {
> + { .compatible = "stmpe,ts" },
> + {},
> +};
> +
> +static const struct of_device_id stmpe_adc_ids[] = {
> + { .compatible = "stmpe,adc" },
> + {},
> +};
> +
> +static const struct of_device_id stmpe_pwm_ids[] = {
> + { .compatible = "stmpe,pwm" },
> + {},
> +};
> +
> +static const struct of_device_id stmpe_rotator_ids[] = {
> + { .compatible = "stmpe,rotator" },
> + {},
> +};
There really is no need for all these.
Please use the current implementation instead.
> +static struct stmpe_keypad_platform_data *
> +get_keyboard_pdata_dt(struct device *dev, struct device_node *np)
> +{
> + struct stmpe_keypad_platform_data *pdata;
> + u32 val;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + dev_warn(dev, "stmpe keypad kzalloc fail\n");
> + return NULL;
> + }
> +
> + if (!of_property_read_u32(np, "keypad,scan-count", &val))
> + pdata->scan_count = val;
> + if (!of_property_read_u32(np, "keypad,debounce-ms", &val))
> + pdata->debounce_ms = val;
> + if (of_property_read_bool(np, "keypad,no-autorepeat"))
> + pdata->no_autorepeat = true;
Why are you (re)adding these here? Have you even looked in the driver?
See: drivers/input/keyboard/stmpe-keypad.c
> + return pdata;
> +}
> +
> +static struct stmpe_gpio_platform_data *get_gpio_pdata_dt(struct device *dev,
> + struct device_node *np)
> +{
> + struct stmpe_gpio_platform_data *pdata;
> + u32 val;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + dev_warn(dev, "stmpe gpio kzalloc fail\n");
> + return NULL;
> + }
> +
> + if (!of_property_read_u32(np, "gpio,norequest-mask", &val))
> + pdata->norequest_mask = val;
> +
> + /* assign gpio numbers dynamically */
> + pdata->gpio_base = -1;
> +
> + return pdata;
> +}
Is this function really required? Even if is is, should it live here
or in the STMPE driver?
See: drivers/gpio/gpio-stmpe.c
> +static struct stmpe_ts_platform_data *get_ts_pdata_dt(struct device *dev,
> + struct device_node *np)
> +{
> + struct stmpe_ts_platform_data *pdata;
> + u32 val;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + dev_warn(dev, "stmpe ts kzalloc fail\n");
> + return NULL;
> + }
> +
> + if (!of_property_read_u32(np, "ts,sample-time", &val))
> + pdata->sample_time = val;
> + if (!of_property_read_u32(np, "ts,mod-12b", &val))
> + pdata->mod_12b = val;
> + if (!of_property_read_u32(np, "ts,ref-sel", &val))
> + pdata->ref_sel = val;
> + if (!of_property_read_u32(np, "ts,adc-freq", &val))
> + pdata->adc_freq = val;
> + if (!of_property_read_u32(np, "ts,ave-ctrl", &val))
> + pdata->ave_ctrl = val;
> + if (!of_property_read_u32(np, "ts,touch-det-delay", &val))
> + pdata->touch_det_delay = val;
> + if (!of_property_read_u32(np, "ts,settling", &val))
> + pdata->settling = val;
> + if (!of_property_read_u32(np, "ts,fraction-z", &val))
> + pdata->fraction_z = val;
> + if (!of_property_read_u32(np, "ts,i-drive", &val))
> + pdata->i_drive = val;
> +
> + return pdata;
> +}
As above.
I'm going to stop my review here. I think you get the idea.
> +static struct stmpe_variant_block *
> +match_variant_block(struct stmpe_variant_info *variant,
> + enum stmpe_block blocks)
> +{
> + int i;
> +
> + for (i = 0; i < variant->num_blocks; i++) {
> + struct stmpe_variant_block *block = &variant->blocks[i];
> +
> + if (blocks & block->block)
> + return block;
> + }
> + return NULL;
> +}
> +
> +static int stmpe_probe_config_dt(struct device *dev,
> + struct stmpe_platform_data *pdata, struct device_node *np)
> +{
> + enum of_gpio_flags flags;
> +
> + pdata->irq_base = irq_alloc_descs(-1, 0, STMPE_NR_IRQS, 0);
> + if (IS_ERR_VALUE(pdata->irq_base)) {
> + dev_err(dev, "%s: irq desc alloc failed\n", __func__);
> + return -ENXIO;
> + }
> +
> + if (of_property_read_bool(np, "irq_over_gpio")) {
> + pdata->irq_over_gpio = true;
> +
> + pdata->irq_gpio = of_get_named_gpio_flags(np, "irq-gpios", 0,
> + &flags);
> + if (!pdata->irq_gpio)
> + dev_dbg(dev, "unable to get irq_gpio from %s.",
> + __func__);
> + }
> +
> + if (of_property_read_u32(np, "irq-trigger", &pdata->irq_trigger))
> + dev_dbg(dev, "unable to get irq_trigger\n");
> +
> + if (of_property_read_bool(np, "irq-invert-polarity"))
> + pdata->irq_invert_polarity = true;
> +
> + if (!of_property_read_u32(np, "autosleep-timeout",
> + &pdata->autosleep_timeout))
> + pdata->autosleep = true;
> +
> + if (of_property_read_u32(np, "id", &pdata->id))
> + dev_dbg(dev, "unable to get id\n");
> +
> + return 0;
> +}
> +
> +static int stmpe_of_devices_init(struct stmpe *stmpe)
> +{
> + struct stmpe_variant_info *variant = stmpe->variant;
> + struct device_node *nc, *np = stmpe->dev->of_node;
> + struct stmpe_variant_block *block;
> + enum stmpe_block blockid;
> + int ret = -EINVAL;
> +
> + for_each_child_of_node(np, nc) {
> + if (of_match_node(stmpe_keypad_ids, nc)) {
> + blockid = STMPE_BLOCK_KEYPAD;
> + stmpe->pdata->keypad = get_keyboard_pdata_dt(stmpe->dev,
> + nc);
> + if (!stmpe->pdata->keypad)
> + return -ENOMEM;
> + } else if (of_match_node(stmpe_gpio_ids, nc)) {
> + blockid = STMPE_BLOCK_GPIO;
> + stmpe->pdata->gpio = get_gpio_pdata_dt(stmpe->dev, nc);
> + if (!stmpe->pdata->gpio)
> + return -ENOMEM;
> + } else if (of_match_node(stmpe_ts_ids, nc)) {
> + blockid = STMPE_BLOCK_TOUCHSCREEN;
> + stmpe->pdata->ts = get_ts_pdata_dt(stmpe->dev, nc);
> + if (!stmpe->pdata->ts)
> + return -ENOMEM;
> + } else if (of_match_node(stmpe_adc_ids, nc)) {
> + blockid = STMPE_BLOCK_ADC;
> + } else if (of_match_node(stmpe_pwm_ids, nc)) {
> + blockid = STMPE_BLOCK_PWM;
> + } else if (of_match_node(stmpe_rotator_ids, nc)) {
> + blockid = STMPE_BLOCK_ROTATOR;
> + } else {
> + dev_warn(stmpe->dev, "no matching device node found\n");
> + return -EINVAL;
> + }
> +
> + block = match_variant_block(variant, blockid);
> + if (!block) {
> + dev_err(stmpe->dev, "variant doesn't support blockid: %d\n",
> + blockid);
> + continue;
> + }
> +
> + ret = stmpe_add_device(stmpe, block->cell);
> + if (ret)
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +#else
> +static inline int stmpe_probe_config_dt(struct stmpe_platform_data *pdata,
> + struct device_node *np)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int stmpe_of_devices_init(struct stmpe *stmpe)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> static int __devinit stmpe_devices_init(struct stmpe *stmpe)
> {
> struct stmpe_variant_info *variant = stmpe->variant;
> @@ -1017,32 +1244,6 @@ static int __devinit stmpe_devices_init(struct stmpe *stmpe)
> return ret;
> }
>
> -void __devinit stmpe_of_probe(struct stmpe_platform_data *pdata,
> - struct device_node *np)
> -{
> - struct device_node *child;
> -
> - of_property_read_u32(np, "st,autosleep-timeout",
> - &pdata->autosleep_timeout);
> -
> - pdata->autosleep = (pdata->autosleep_timeout) ? true : false;
> -
> - for_each_child_of_node(np, child) {
> - if (!strcmp(child->name, "stmpe_gpio")) {
> - pdata->blocks |= STMPE_BLOCK_GPIO;
> - }
> - if (!strcmp(child->name, "stmpe_keypad")) {
> - pdata->blocks |= STMPE_BLOCK_KEYPAD;
> - }
> - if (!strcmp(child->name, "stmpe_touchscreen")) {
> - pdata->blocks |= STMPE_BLOCK_TOUCHSCREEN;
> - }
> - if (!strcmp(child->name, "stmpe_adc")) {
> - pdata->blocks |= STMPE_BLOCK_ADC;
> - }
> - }
> -}
> -
> /* Called from client specific probe routines */
> int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
> {
> @@ -1059,7 +1260,11 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
> if (!pdata)
> return -ENOMEM;
>
> - stmpe_of_probe(pdata, np);
> + ret = stmpe_probe_config_dt(ci->dev, pdata, np);
> + if (ret) {
> + dev_err(ci->dev, "probe_config_dt failed\n");
> + return ret;
> + }
> }
>
> stmpe = devm_kzalloc(ci->dev, sizeof(struct stmpe), GFP_KERNEL);
> @@ -1130,7 +1335,10 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
> }
> }
>
> - ret = stmpe_devices_init(stmpe);
> + if (np)
> + ret = stmpe_of_devices_init(stmpe);
> + else
> + ret = stmpe_devices_init(stmpe);
> if (!ret)
> return 0;
>
> --
> 1.7.12.rc2.18.g61b472e
>
>
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 22 November 2012 21:16, Lee Jones <[email protected]> wrote:
>> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
>> >> +- irq-over-gpio: bool, true if gpio is used to get irq
>> >> +- irq-gpios: gpio number over which irq will be requested (significant only if
>> >> + irq-over-gpio is true)
>> >
>> > You don't need these. Use gpio_to_irq() instead.
>>
>> I am passing gpio numbers here and am doing gpio_to_irq() in driver.
>> Didn't get this one :(
>
> For a start you have 'irq-over-gpio' in the binding document and Device Tree
> and 'irq_over_gpio' in the code. Has it even been tested?
>
> GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a
> look to see how they are handled without adding unnecessary DT bindings.
I already knew it, should have picked that up. :(
>> stmpe is an interrupt controller for the IP's which are present inside
>> it: gpio, adc.
>> But interrupt lines for them are managed by stmpe driver internally. So should
>> we really add interrupt-controller for it?
>
> You can't manage IRQ lines internally, you have to go through
> the IRQ subsystem. When you request an IRQ via device tree you
> will do so like this:
By that i meant, there is no external node which would have stmpe as
interrupt controller. Because all of them would be its child node.
This is guaranteed because stmpe is an external device is present on board.
So, it will have its entry in board dts file, and so wouldn't be
scattered in different
files.
> The STMPE GPIO controller can't be used by Device Tree yet in any case,
> because it doesn't have an IRQ domain. This is compulsory, or it won't
> work. Have you tried to test this functionality yet?
I don't have SPEAr board to test it anymore. I have moved out of ST now and
working in linaro as ARM asignee. Just pushing these as an part time activity.
Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about.
> Okay, I've just had a look at my tested bindings.
>
> We already have these:
>
> debounce-interval /* This is a generic binding */
> st,scan-count /* These are vendor specific */
> st,no-autorepeat /* " */
>
> Vendor specific bindings should be "<vendor>,<binding>", rather than
> "<type_of_driver>,<binding>"
Will take care of these.
>> >> + reg = <0>;
>> >
>> > You have reg twice here. Also reg should never be '0'.
>>
>> For SPI, there are chip selects and there is no reg offset.
>
> I understand the addressing issues, but you have 'reg' twice.
>
> Which one should be used?
I haven't replied on that, because it was accepted. Only one
definition should be there for reg.
> I didn't go through them, but are you sure that:
>
> 1. Can I do without them?
> 1.1 Can I derive the configuration from other things?
> 2.2 Are they _really_ required, or am I just blindly copying platform data?
> 2. Does a similar binding already exist?
> 3. Can other drivers make use of them?
> 3.1 If so, create a generic binding
> 3.2 If not, prepend the binding with "<vendor>,"
I will go through them again.
>> >> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
>> Because i wanted to keep all DT stuff together. Obviously i have seen keypad
>> driver earlier :)
>
> If you had, you'd realise that these bindings already exist. ;)
Ok. I had seen it during my V1 and missed these bindings. will fix them now.
--
viresh
On Thu, 22 Nov 2012, Viresh Kumar wrote:
> This patch frees stmpe driver from tension of freeing resources :)
> devm_* derivatives of multiple routines are used while allocating resources,
> which would be freed automatically by kernel.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
>
> V1->V2:
> ------
> - Rebased over latest for-next from Samuel
> - updated additional kzalloc with devm_kzalloc(), first one seen below.
>
> drivers/mfd/stmpe.c | 60 +++++++++++++++++++----------------------------------
> 1 file changed, 21 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index ba157d4..c0df4b9 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -1052,17 +1052,17 @@ int __devinit stmpe_probe(struct stmpe_client_info *ci, int partnum)
> int ret;
>
> if (!pdata) {
> - if (np) {
> - pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> - if (!pdata)
> - return -ENOMEM;
> -
> - stmpe_of_probe(pdata, np);
> - } else
> + if (!np)
I'm assuming you've reversed the semantics here for >80 chars reasons?
I see no harm in it though.
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 22 November 2012 16:54, Lee Jones <[email protected]> wrote:
> Big fat NACK.
>
> You've just overwritten the current implementation with your own.
> Please take time to understand the mechanisms in place before
> you submit any changes or additions to it.
:)
My fault. Comments on all overwritten stuff accepted
>> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
>> +- irq-over-gpio: bool, true if gpio is used to get irq
>> +- irq-gpios: gpio number over which irq will be requested (significant only if
>> + irq-over-gpio is true)
>
> You don't need these. Use gpio_to_irq() instead.
I am passing gpio numbers here and am doing gpio_to_irq() in driver.
Didn't get this one :(
>> Optional properties:
>> - - interrupts : The interrupt outputs from the controller
>> - - interrupt-controller : Marks the device node as an interrupt controller
>> - - interrupt-parent : Specifies which IRQ controller we're connected to
>> - - i2c-client-wake : Marks the input device as wakable
>> - - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
>
> And you've removed these why?
No. They are readjusted...
One thing removed is interrupt-controller. I had a doubt on this.
stmpe, by itself doesn't give any interrupt lines to SoC to freely use
them. Instead
gpio controller driver part of it does. And so adding
interrupt-controller for that is
the right option.
stmpe is an interrupt controller for the IP's which are present inside
it: gpio, adc.
But interrupt lines for them are managed by stmpe driver internally. So should
we really add interrupt-controller for it?
>> +- keypad,scan-count: number of key scanning cycles to confirm key data. Maximum
>> + is STMPE_KEYPAD_MAX_SCAN_COUNT.
>> +- keypad,debounce-ms: debounce interval, in ms. Maximum is
>> + STMPE_KEYPAD_MAX_DEBOUNCE.
>> +- keypad,no-autorepeat: bool, disable key autorepeat
>
> See "When adding new bindings, ask yourself" above.
Yes, these are required. This is part of platform data it expects.
>> +stmpe-ts:
>> +-----------
> See "When adding new bindings, ask yourself" above.
Same. Can you explicitly point out, which bindings you didn't like.
>> +spi@e0100000 {
>
> This shouldn't be a child of the SPI device becuase it uses SPI.
>
> Drivers use clocks, regulators, IRQ controller too, but they're
> not children of those devices.
Yes.
>> + reg = <0>;
>
> You have reg twice here. Also reg should never be '0'.
For SPI, there are chip selects and there is no reg offset.
>> + stmpe610-ts {
>> + compatible = "stmpe,ts";
>> + ts,sample-time = <4>;
>> + ts,mod-12b = <1>;
>> + ts,ref-sel = <0>;
>> + ts,adc-freq = <1>;
>> + ts,ave-ctrl = <1>;
>> + ts,touch-det-delay = <2>;
>> + ts,settling = <2>;
>> + ts,fraction-z = <7>;
>> + ts,i-drive = <1>;
>
> Wow! See "When adding new bindings, ask yourself" above.
:)
They are required. I didn't get your point, sorry.
>> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
>> +static struct stmpe_keypad_platform_data *
>> +get_keyboard_pdata_dt(struct device *dev, struct device_node *np)
>> +{
>> + struct stmpe_keypad_platform_data *pdata;
>> + u32 val;
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata) {
>> + dev_warn(dev, "stmpe keypad kzalloc fail\n");
>> + return NULL;
>> + }
>> +
>> + if (!of_property_read_u32(np, "keypad,scan-count", &val))
>> + pdata->scan_count = val;
>> + if (!of_property_read_u32(np, "keypad,debounce-ms", &val))
>> + pdata->debounce_ms = val;
>> + if (of_property_read_bool(np, "keypad,no-autorepeat"))
>> + pdata->no_autorepeat = true;
>
> Why are you (re)adding these here? Have you even looked in the driver?
Because i wanted to keep all DT stuff together. Obviously i have seen keypad
driver earlier :)
I am not setting pdata of stmpe here, but pdata of keypad.
>> +static struct stmpe_gpio_platform_data *get_gpio_pdata_dt(struct device *dev,
>> + struct device_node *np)
>> +{
>> + struct stmpe_gpio_platform_data *pdata;
>> + u32 val;
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata) {
>> + dev_warn(dev, "stmpe gpio kzalloc fail\n");
>> + return NULL;
>> + }
>> +
>> + if (!of_property_read_u32(np, "gpio,norequest-mask", &val))
>> + pdata->norequest_mask = val;
>> +
>> + /* assign gpio numbers dynamically */
>> + pdata->gpio_base = -1;
>> +
>> + return pdata;
>> +}
>
> Is this function really required? Even if is is, should it live here
> or in the STMPE driver?
As said earlier, either i can do DT parsing of sub-modules of stmpe in
their specific files or in stmpe driver itself. Because currently platform
data of those sub-modules is passed from stmpe, i kept them here only.
stmpe sub modules can't live without stmpe driver and so keeping all
binding stuff here isn't that bad of an idea.
>> +static struct stmpe_ts_platform_data *get_ts_pdata_dt(struct device *dev,
>> + struct device_node *np)
>> +{
>> + struct stmpe_ts_platform_data *pdata;
>> + u32 val;
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata) {
>> + dev_warn(dev, "stmpe ts kzalloc fail\n");
>> + return NULL;
>> + }
>> +
>> + if (!of_property_read_u32(np, "ts,sample-time", &val))
>> + pdata->sample_time = val;
>> + if (!of_property_read_u32(np, "ts,mod-12b", &val))
>> + pdata->mod_12b = val;
>> + if (!of_property_read_u32(np, "ts,ref-sel", &val))
>> + pdata->ref_sel = val;
>> + if (!of_property_read_u32(np, "ts,adc-freq", &val))
>> + pdata->adc_freq = val;
>> + if (!of_property_read_u32(np, "ts,ave-ctrl", &val))
>> + pdata->ave_ctrl = val;
>> + if (!of_property_read_u32(np, "ts,touch-det-delay", &val))
>> + pdata->touch_det_delay = val;
>> + if (!of_property_read_u32(np, "ts,settling", &val))
>> + pdata->settling = val;
>> + if (!of_property_read_u32(np, "ts,fraction-z", &val))
>> + pdata->fraction_z = val;
>> + if (!of_property_read_u32(np, "ts,i-drive", &val))
>> + pdata->i_drive = val;
>> +
>> + return pdata;
>> +}
>
> As above.
>
> I'm going to stop my review here. I think you get the idea.
I got most of your worries, but couldn't understand the issue of passing
all pdata fields as DT bindings.
Thanks for your review though. :)
--
viresh
On 22 November 2012 15:57, Lee Jones <[email protected]> wrote:
> I'm assuming you've reversed the semantics here for >80 chars reasons?
Not for 80 chars reason :)
I did it to decrease nesting level of if/else statements :)
--
viresh
On 22 November 2012 15:57, Lee Jones <[email protected]> wrote:
> On Thu, 22 Nov 2012, Viresh Kumar wrote:
>
>> This patch frees stmpe driver from tension of freeing resources :)
>> devm_* derivatives of multiple routines are used while allocating resources,
>> which would be freed automatically by kernel.
>>
>> Signed-off-by: Viresh Kumar <[email protected]>
> Acked-by: Lee Jones <[email protected]>
Hi Sameo,
Please apply this patch only from this series. I will not resend it in
V3 of my set.
--
viresh
On Thu, Nov 22, 2012 at 10:31:48PM +0530, Viresh Kumar wrote:
> On 22 November 2012 21:16, Lee Jones <[email protected]> wrote:
> > The STMPE GPIO controller can't be used by Device Tree yet in
> > any case, because it doesn't have an IRQ domain. This is
> > compulsory, or it won't work. Have you tried to test this
> > functionality yet?
>
> I don't have SPEAr board to test it anymore. I have moved out of
> ST now and working in linaro as ARM asignee. Just pushing these
> as an part time activity.
>
> Though ST guys would have tested stmpe, but stmpe-gpio, i am not
> sure about.
Let me bring some more information here. I totally understand
Jones concerns, but the way stmpe (and may be other mfd devices)
are handled is this that the parent block (i.e. stmpe) decides on
the variants (say by probing device itself) and then prepares
associated data for the (probed) variant and creates a platform
device for the same.
For the interrupts case also, it is stmpe which registers the
irq domain. This is because, stmpe driver probes variant and
populates its platform data and stmpe-gpio may not be aware of the
variant it serves. At the same time, it (stmpe) needs few of the
(virtual) interrupts for its internal purpose also.
Hence stmpe passes irq_base to the stmpe-gpio driver while
allocating and registering irq domain by itself.
With this approach we have tested the functionality on SPEAr
platform.
--
regards
Shiraz
On 22 November 2012 16:54, Lee Jones <[email protected]> wrote:
>> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
>> stmpe1601: stmpe1601@40 {
>> + id = <0>;
>
> Don't do this. Device IDs are Linux specific.
Hi Lee,
This is id of the mfd device that we need to pass to mfd_add_device()
and is used in following:
pdev = platform_device_alloc(cell->name, id + cell->id);
This is required when we have multiple instances of MFD device present
on board. How do you want me to handle this ?
--
viresh
On Thu, 22 Nov 2012, Viresh Kumar wrote:
> On 22 November 2012 21:16, Lee Jones <[email protected]> wrote:
> >> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> >> +- irq-over-gpio: bool, true if gpio is used to get irq
> >> >> +- irq-gpios: gpio number over which irq will be requested (significant only if
> >> >> + irq-over-gpio is true)
> >> >
> >> > You don't need these. Use gpio_to_irq() instead.
> >>
> >> I am passing gpio numbers here and am doing gpio_to_irq() in driver.
> >> Didn't get this one :(
> >
> > For a start you have 'irq-over-gpio' in the binding document and Device Tree
> > and 'irq_over_gpio' in the code. Has it even been tested?
> >
> > GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a
> > look to see how they are handled without adding unnecessary DT bindings.
>
> I already knew it, should have picked that up. :(
>
> >> stmpe is an interrupt controller for the IP's which are present inside
> >> it: gpio, adc.
> >> But interrupt lines for them are managed by stmpe driver internally. So should
> >> we really add interrupt-controller for it?
> >
> > You can't manage IRQ lines internally, you have to go through
> > the IRQ subsystem. When you request an IRQ via device tree you
> > will do so like this:
>
> By that i meant, there is no external node which would have stmpe as
> interrupt controller. Because all of them would be its child node.
>
> This is guaranteed because stmpe is an external device is present on board.
> So, it will have its entry in board dts file, and so wouldn't be
> scattered in different
> files.
It doesn't matter how it's wired up.
If another node references it as it's IRQ controller you have to
declare it as one using the interrupt-controller binding.
> > The STMPE GPIO controller can't be used by Device Tree yet in any case,
> > because it doesn't have an IRQ domain. This is compulsory, or it won't
> > work. Have you tried to test this functionality yet?
>
> I don't have SPEAr board to test it anymore. I have moved out of ST now and
> working in linaro as ARM asignee. Just pushing these as an part time activity.
Surely you can't push patches which haven't been tested?!
Try to get yourself some hardware. Does anyone near you have an HREF?
In the mean-time, I will write you an IRQ domain.
> Though ST guys would have tested stmpe, but stmpe-gpio, i am not sure about.
It needs to be tested before being accpeted.
> > I didn't go through them, but are you sure that:
> >
> > 1. Can I do without them?
> > 1.1 Can I derive the configuration from other things?
> > 2.2 Are they _really_ required, or am I just blindly copying platform data?
> > 2. Does a similar binding already exist?
> > 3. Can other drivers make use of them?
> > 3.1 If so, create a generic binding
> > 3.2 If not, prepend the binding with "<vendor>,"
>
> I will go through them again.
Thanks.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 23 Nov 2012, Shiraz Hashim wrote:
> On Thu, Nov 22, 2012 at 10:31:48PM +0530, Viresh Kumar wrote:
> > On 22 November 2012 21:16, Lee Jones <[email protected]> wrote:
> > > The STMPE GPIO controller can't be used by Device Tree yet in
> > > any case, because it doesn't have an IRQ domain. This is
> > > compulsory, or it won't work. Have you tried to test this
> > > functionality yet?
> >
> > I don't have SPEAr board to test it anymore. I have moved out of
> > ST now and working in linaro as ARM asignee. Just pushing these
> > as an part time activity.
> >
> > Though ST guys would have tested stmpe, but stmpe-gpio, i am not
> > sure about.
>
> Let me bring some more information here. I totally understand
> Jones concerns, but the way stmpe (and may be other mfd devices)
> are handled is this that the parent block (i.e. stmpe) decides on
> the variants (say by probing device itself) and then prepares
> associated data for the (probed) variant and creates a platform
> device for the same.
I realise this, but now we're using Device Tree, there's no need
to stuff pdata in the parent driver now. It's better that the
child devices are self sufficient.
> For the interrupts case also, it is stmpe which registers the
> irq domain. This is because, stmpe driver probes variant and
> populates its platform data and stmpe-gpio may not be aware of the
> variant it serves. At the same time, it (stmpe) needs few of the
> (virtual) interrupts for its internal purpose also.
I know. I wrote the IRQ domain for STMPE. ;)
STMPE needs its own one too, which I will work on now.
STMPE-GPIO doesn't need to be aware of anything, the device
which wishes to use its GPIOs/IRQs will reference it from
Device Tree in the manor previously explained.
> Hence stmpe passes irq_base to the stmpe-gpio driver while
> allocating and registering irq domain by itself.
Not anymore it doesn't. There are no irq_base:s with DT. All
IRQs are dynamic, hence why STMPE requires its own domain to
play with. I can fix that.
> With this approach we have tested the functionality on SPEAr
> platform.
You'll need to test it again with the new DT approach too. :)
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 23 Nov 2012, Viresh Kumar wrote:
> On 22 November 2012 16:54, Lee Jones <[email protected]> wrote:
> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> stmpe1601: stmpe1601@40 {
>
> >> + id = <0>;
> >
> > Don't do this. Device IDs are Linux specific.
>
> Hi Lee,
>
> This is id of the mfd device that we need to pass to mfd_add_device()
> and is used in following:
MFD devices are Linux specific, whereas DT is cross-platform. Thus
you can't put it in the DTS(I) files.
> pdev = platform_device_alloc(cell->name, id + cell->id);
>
> This is required when we have multiple instances of MFD device present
> on board. How do you want me to handle this ?
There are lots of examples of this already. I have to leave something
to the imagination, or I'll be requesting a cut of your salary. :D
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 23 Nov 2012, Viresh Kumar wrote:
> On 22 November 2012 16:54, Lee Jones <[email protected]> wrote:
> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> +Optional properties:
> >> +- irq-trigger: IRQ trigger to use for the interrupt to the host
> >> +- irq-invert-polarity: bool, IRQ line is connected with reversed polarity
> >
> > Are these new?
> >
> > When adding new bindings, ask yourself:
>
> I was trying to get information of these two bindings from other sources,
> but couldn't find. The only other place can be the interrupts field, right?
Bingo. Continue along this thread.
> Because interrupts field depends on the interrupt controller, which can
> be a wide variety of controllers in our case, we can't use that. Also,
> the cells of interrupts field will have something that is required to be
> programmed in the interrupt controller and not in the user of IC.
>
> But here we are talking about programming stmpe and so these bindings
> are very much required in stmpe only.
Not true.
Hint: `git grep triggered -- Documentation/devicetree/`
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, 22 Nov 2012, Viresh Kumar wrote:
> On 22 November 2012 15:57, Lee Jones <[email protected]> wrote:
> > On Thu, 22 Nov 2012, Viresh Kumar wrote:
> >
> >> This patch frees stmpe driver from tension of freeing resources :)
> >> devm_* derivatives of multiple routines are used while allocating resources,
> >> which would be freed automatically by kernel.
> >>
> >> Signed-off-by: Viresh Kumar <[email protected]>
>
> > Acked-by: Lee Jones <[email protected]>
>
> Hi Sameo,
>
> Please apply this patch only from this series. I will not resend it in
> V3 of my set.
That's not how it works.
Apply my Acked-by yourself and re-send the patch-set as a whole.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 23 Nov 2012, Lee Jones wrote:
> On Thu, 22 Nov 2012, Viresh Kumar wrote:
>
> > On 22 November 2012 15:57, Lee Jones <[email protected]> wrote:
> > > On Thu, 22 Nov 2012, Viresh Kumar wrote:
> > >
> > >> This patch frees stmpe driver from tension of freeing resources :)
> > >> devm_* derivatives of multiple routines are used while allocating resources,
> > >> which would be freed automatically by kernel.
> > >>
> > >> Signed-off-by: Viresh Kumar <[email protected]>
> >
> > > Acked-by: Lee Jones <[email protected]>
> >
> > Hi Sameo,
> >
> > Please apply this patch only from this series. I will not resend it in
> > V3 of my set.
>
> That's not how it works.
>
> Apply my Acked-by yourself and re-send the patch-set as a whole.
Sorry, you have another option. Break it away from the patch-set,
apply my Acked-by, then resubmit as a single patch.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 23 November 2012 15:10, Lee Jones <[email protected]> wrote:
> That's not how it works.
>
> Apply my Acked-by yourself and re-send the patch-set as a whole.
I don't thinks so. This patch is not doing anything with DT stuff and so
can be applied separately. Related patches should be posted together
though.
--
viresh
Hi Viresh,
On Thu, Nov 22, 2012 at 10:40:29AM +0530, Viresh Kumar wrote:
> This patch frees stmpe driver from tension of freeing resources :)
> devm_* derivatives of multiple routines are used while allocating resources,
> which would be freed automatically by kernel.
>
> Signed-off-by: Viresh Kumar <[email protected]>
Applied, with Lee's Ack.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
On 23 November 2012 15:06, Lee Jones <[email protected]> wrote:
> On Fri, 23 Nov 2012, Viresh Kumar wrote:
>> pdev = platform_device_alloc(cell->name, id + cell->id);
>>
>> This is required when we have multiple instances of MFD device present
>> on board. How do you want me to handle this ?
>
> There are lots of examples of this already. I have to leave something
> to the imagination, or I'll be requesting a cut of your salary. :D
My manager already reduced my salary by 20% after reading this mail :(
Ok, this is what my understanding of whole this is. Platform devices are
named like:
- pdev-name: if id passed in pdev.id is -1
- pdev-name.0[1|2|...]: if id passed is 0[1|2|...]
- pdev-name.<dynamically allocated by kernel>: if id passed is -2
Now, we don't declare cell->id fields and they are currently zero and so
value is passed from pdata->id field. So, for example with multiple instances
of stmpe on a board, we have:
- stmpe-0: //Name just for reference...
- stmpe-gpio.0
- stmpe-ts.0
- stmpe-1:
- stmpe-gpio.1
- stmpe-ts.1
- stmpe-2:
- stmpe-gpio.2
- stmpe-ts.2
I main idea is to distinguish various instances of sub modules, like stmpe-gpio.
And this works well with non-DT support we have currently.
With DT, i am not sure how should we pass id field to mfd_add_devices(). If
we pass it -1, then multiple instances will have same name: "stmpe-gpio"
Sorry, for my lack of knowledge. Don't send another mail with salary cut
suggestion as that will make it -40% in total ;)
Thanks for reviewing :)
--
viresh
On Fri, 23 Nov 2012, Viresh Kumar wrote:
> On 23 November 2012 15:06, Lee Jones <[email protected]> wrote:
> > On Fri, 23 Nov 2012, Viresh Kumar wrote:
> >> pdev = platform_device_alloc(cell->name, id + cell->id);
> >>
> >> This is required when we have multiple instances of MFD device present
> >> on board. How do you want me to handle this ?
> >
> > There are lots of examples of this already. I have to leave something
> > to the imagination, or I'll be requesting a cut of your salary. :D
>
> My manager already reduced my salary by 20% after reading this mail :(
Ah, Good.
Tell your manager I'll send my offshore bank details over soon. :)
> Ok, this is what my understanding of whole this is. Platform devices are
> named like:
> - pdev-name: if id passed in pdev.id is -1
> - pdev-name.0[1|2|...]: if id passed is 0[1|2|...]
> - pdev-name.<dynamically allocated by kernel>: if id passed is -2
>
> Now, we don't declare cell->id fields and they are currently zero and so
> value is passed from pdata->id field. So, for example with multiple instances
> of stmpe on a board, we have:
>
> - stmpe-0: //Name just for reference...
> - stmpe-gpio.0
> - stmpe-ts.0
> - stmpe-1:
> - stmpe-gpio.1
> - stmpe-ts.1
> - stmpe-2:
> - stmpe-gpio.2
> - stmpe-ts.2
>
> I main idea is to distinguish various instances of sub modules, like stmpe-gpio.
> And this works well with non-DT support we have currently.
Yes, when !DT, then passing ID is no problem.
> With DT, i am not sure how should we pass id field to mfd_add_devices(). If
> we pass it -1, then multiple instances will have same name: "stmpe-gpio"
No, in DT devices named as part of the hiearchy, so you'd have:
soc-u9500/i2c@80004000/stmpe1601@40/stmpe_keypad
soc-u9500/i2c@80004000/stmpe1601@41/stmpe_keypad
... etc
The only time we need to be concerned is if one stmpe device can
handle more than one keypad, gpio controller, etc. Which I don't
think is the case.
> Sorry, for my lack of knowledge. Don't send another mail with salary cut
> suggestion as that will make it -40% in total ;)
Ah, I promise I'll spend it wisely. :)
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 23 November 2012 21:13, Lee Jones <[email protected]> wrote:
> No, in DT devices named as part of the hiearchy, so you'd have:
>
> soc-u9500/i2c@80004000/stmpe1601@40/stmpe_keypad
> soc-u9500/i2c@80004000/stmpe1601@41/stmpe_keypad
> ... etc
Obviously. How could i miss this naming :(
Okay, so i need to pass -1 and that would be enough. Right?
--
viresh