[PATCH 1/3] Input: bu21013_ts - Request a regulator that actually exists

[PATCH 1/3] Input: bu21013_ts - Request a regulator that actually exists

[Lee Jones]

Currently the BU21013 Touch Screen driver requests a regulator by the
name of 'V-TOUCH', which doesn't exist anywhere in the kernel. The
correct name, as referenced in platform regulator code is 'avdd'. Here,
when we request a regulator, we use the correct name instead.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/input/touchscreen/bu21013_ts.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/bu21013_ts.c b/drivers/input/touchscreen/bu21013_ts.c
index 5c487d2..2fae682 100644
--- a/drivers/input/touchscreen/bu21013_ts.c
+++ b/drivers/input/touchscreen/bu21013_ts.c
@@ -461,7 +461,7 @@ static int __devinit bu21013_probe(struct i2c_client *client,
 	bu21013_data->chip = pdata;
 	bu21013_data->client = client;
 
-	bu21013_data->regulator = regulator_get(&client->dev, "V-TOUCH");
+	bu21013_data->regulator = regulator_get(&client->dev, "avdd");
 	if (IS_ERR(bu21013_data->regulator)) {
 		dev_err(&client->dev, "regulator_get failed\n");
 		error = PTR_ERR(bu21013_data->regulator);
-- 
1.7.9.5

Re: [PATCH 1/3] Input: bu21013_ts - Request a regulator that actually exists

[Mark Brown]

On Tue, Nov 27, 2012 at 01:13:08PM +0000, Lee Jones wrote:
> Currently the BU21013 Touch Screen driver requests a regulator by the
> name of 'V-TOUCH', which doesn't exist anywhere in the kernel. The
> correct name, as referenced in platform regulator code is 'avdd'. Here,
> when we request a regulator, we use the correct name instead.

The regulator should be requested using whatever the name for the supply
in the datasheet - AVDD does sound very pluasible but it'd be good to
check.

Re: [PATCH 1/3] Input: bu21013_ts - Request a regulator that actually exists

[Lee Jones]

On Tue, 27 Nov 2012, Mark Brown wrote:

> On Tue, Nov 27, 2012 at 01:13:08PM +0000, Lee Jones wrote:
> > Currently the BU21013 Touch Screen driver requests a regulator by the
> > name of 'V-TOUCH', which doesn't exist anywhere in the kernel. The
> > correct name, as referenced in platform regulator code is 'avdd'. Here,
> > when we request a regulator, we use the correct name instead.
> 
> The regulator should be requested using whatever the name for the supply
> in the datasheet - AVDD does sound very pluasible but it'd be good to
> check.

Right. 'avdd' is correct.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] Input: bu21013_ts - Move GPIO init and exit functions into the driver

[Lee Jones]

These GPIO init and exit functions have no place in platform data.
Instead they should be part of the driver. This patch moves them to
their rightful place, which subsequently elevates platform data of
yet more cruft.

Cc: linux-input@vger.kernel.org
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-stuib.c |   71 +-----------------------------
 drivers/input/touchscreen/bu21013_ts.c   |   69 +++++++++++++++++++++--------
 include/linux/input/bu21013.h            |    9 +---
 3 files changed, 53 insertions(+), 96 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500-stuib.c b/arch/arm/mach-ux500/board-mop500-stuib.c
index 564f57d..7e1f294 100644
--- a/arch/arm/mach-ux500/board-mop500-stuib.c
+++ b/arch/arm/mach-ux500/board-mop500-stuib.c
@@ -77,9 +77,6 @@ static struct i2c_board_info __initdata mop500_i2c0_devices_stuib[] = {
  * BU21013 ROHM touchscreen interface on the STUIBs
  */
 
-/* tracks number of bu21013 devices being enabled */
-static int bu21013_devices;
-
 #define TOUCH_GPIO_PIN  84
 
 #define TOUCH_XMAX	384
@@ -88,73 +85,8 @@ static int bu21013_devices;
 #define PRCMU_CLOCK_OCR		0x1CC
 #define TSC_EXT_CLOCK_9_6MHZ	0x840000
 
-/**
- * bu21013_gpio_board_init : configures the touch panel.
- * @reset_pin: reset pin number
- * This function can be used to configures
- * the voltage and reset the touch panel controller.
- */
-static int bu21013_gpio_board_init(int reset_pin)
-{
-	int retval = 0;
-
-	bu21013_devices++;
-	if (bu21013_devices == 1) {
-		retval = gpio_request(reset_pin, "touchp_reset");
-		if (retval) {
-			printk(KERN_ERR "Unable to request gpio reset_pin");
-			return retval;
-		}
-		retval = gpio_direction_output(reset_pin, 1);
-		if (retval < 0) {
-			printk(KERN_ERR "%s: gpio direction failed\n",
-					__func__);
-			return retval;
-		}
-	}
-
-	return retval;
-}
-
-/**
- * bu21013_gpio_board_exit : deconfigures the touch panel controller
- * @reset_pin: reset pin number
- * This function can be used to deconfigures the chip selection
- * for touch panel controller.
- */
-static int bu21013_gpio_board_exit(int reset_pin)
-{
-	int retval = 0;
-
-	if (bu21013_devices == 1) {
-		retval = gpio_direction_output(reset_pin, 0);
-		if (retval < 0) {
-			printk(KERN_ERR "%s: gpio direction failed\n",
-					__func__);
-			return retval;
-		}
-		gpio_set_value(reset_pin, 0);
-	}
-	bu21013_devices--;
-
-	return retval;
-}
-
-/**
- * bu21013_read_pin_val : get the interrupt pin value
- * This function can be used to get the interrupt pin value for touch panel
- * controller.
- */
-static int bu21013_read_pin_val(void)
-{
-	return gpio_get_value(TOUCH_GPIO_PIN);
-}
-
 static struct bu21013_platform_device tsc_plat_device = {
-	.cs_en = bu21013_gpio_board_init,
-	.cs_dis = bu21013_gpio_board_exit,
-	.irq_read_val = bu21013_read_pin_val,
-	.irq = NOMADIK_GPIO_TO_IRQ(TOUCH_GPIO_PIN),
+	.touch_pin = TOUCH_GPIO_PIN,
 	.touch_x_max = TOUCH_XMAX,
 	.touch_y_max = TOUCH_YMAX,
 	.ext_clk = false,
@@ -171,7 +103,6 @@ static struct i2c_board_info __initdata u8500_i2c3_devices_stuib[] = {
 		I2C_BOARD_INFO("bu21013_tp", 0x5D),
 		.platform_data = &tsc_plat_device,
 	},
-
 };
 
 void __init mop500_stuib_init(void)
diff --git a/drivers/input/touchscreen/bu21013_ts.c b/drivers/input/touchscreen/bu21013_ts.c
index 2fae682..8982bea 100644
--- a/drivers/input/touchscreen/bu21013_ts.c
+++ b/drivers/input/touchscreen/bu21013_ts.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/regulator/consumer.h>
 #include <linux/module.h>
+#include <linux/gpio.h>
 
 #define PEN_DOWN_INTR	0
 #define MAX_FINGERS	2
@@ -148,11 +149,12 @@
 struct bu21013_ts_data {
 	struct i2c_client *client;
 	wait_queue_head_t wait;
-	bool touch_stopped;
 	const struct bu21013_platform_device *chip;
 	struct input_dev *in_dev;
-	unsigned int intr_pin;
 	struct regulator *regulator;
+	unsigned int irq;
+	unsigned int intr_pin;
+	bool touch_stopped;
 };
 
 /**
@@ -262,7 +264,7 @@ static irqreturn_t bu21013_gpio_irq(int irq, void *device_data)
 			return IRQ_NONE;
 		}
 
-		data->intr_pin = data->chip->irq_read_val();
+		data->intr_pin = gpio_get_value(data->chip->touch_pin);
 		if (data->intr_pin == PEN_DOWN_INTR)
 			wait_event_timeout(data->wait, data->touch_stopped,
 					   msecs_to_jiffies(2));
@@ -418,10 +420,33 @@ static void bu21013_free_irq(struct bu21013_ts_data *bu21013_data)
 {
 	bu21013_data->touch_stopped = true;
 	wake_up(&bu21013_data->wait);
-	free_irq(bu21013_data->chip->irq, bu21013_data);
+	free_irq(bu21013_data->irq, bu21013_data);
 }
 
 /**
+ * bu21013_cs_disable() - deconfigures the touch panel controller
+ * @bu21013_data: device structure pointer
+ *
+ * This function is used to deconfigure the chip selection
+ * for touch panel controller.
+ */
+static void bu21013_cs_disable(struct bu21013_ts_data *bu21013_data)
+{
+	int error;
+
+	error = gpio_direction_output(bu21013_data->chip->cs_pin, 0);
+	if (error < 0)
+		dev_warn(&bu21013_data->client->dev,
+			 "%s: gpio direction failed, error: %d\n",
+			 __func__, error);
+	else
+		gpio_set_value(bu21013_data->chip->cs_pin, 0);
+
+	gpio_free(bu21013_data->chip->cs_pin);
+}
+
+
+/**
  * bu21013_probe() - initializes the i2c-client touchscreen driver
  * @client: i2c client structure pointer
  * @id: i2c device id pointer
@@ -430,7 +455,7 @@ static void bu21013_free_irq(struct bu21013_ts_data *bu21013_data)
  * driver and returns integer.
  */
 static int __devinit bu21013_probe(struct i2c_client *client,
-					const struct i2c_device_id *id)
+				   const struct i2c_device_id *id)
 {
 	struct bu21013_ts_data *bu21013_data;
 	struct input_dev *in_dev;
@@ -449,6 +474,11 @@ static int __devinit bu21013_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
+	if (!gpio_is_valid(pdata->touch_pin)) {
+		dev_err(&client->dev, "invalid touch_pin supplied\n");
+		return -EINVAL;
+	}
+
 	bu21013_data = kzalloc(sizeof(struct bu21013_ts_data), GFP_KERNEL);
 	in_dev = input_allocate_device();
 	if (!bu21013_data || !in_dev) {
@@ -460,6 +490,7 @@ static int __devinit bu21013_probe(struct i2c_client *client,
 	bu21013_data->in_dev = in_dev;
 	bu21013_data->chip = pdata;
 	bu21013_data->client = client;
+	bu21013_data->irq = gpio_to_irq(pdata->touch_pin);
 
 	bu21013_data->regulator = regulator_get(&client->dev, "avdd");
 	if (IS_ERR(bu21013_data->regulator)) {
@@ -478,12 +509,11 @@ static int __devinit bu21013_probe(struct i2c_client *client,
 	init_waitqueue_head(&bu21013_data->wait);
 
 	/* configure the gpio pins */
-	if (pdata->cs_en) {
-		error = pdata->cs_en(pdata->cs_pin);
-		if (error < 0) {
-			dev_err(&client->dev, "chip init failed\n");
-			goto err_disable_regulator;
-		}
+	error = gpio_request_one(pdata->cs_pin, GPIOF_OUT_INIT_HIGH,
+				 "touchp_reset");
+	if (error < 0) {
+		dev_err(&client->dev, "Unable to request gpio reset_pin\n");
+		goto err_disable_regulator;
 	}
 
 	/* configure the touch panel controller */
@@ -508,12 +538,13 @@ static int __devinit bu21013_probe(struct i2c_client *client,
 						pdata->touch_y_max, 0, 0);
 	input_set_drvdata(in_dev, bu21013_data);
 
-	error = request_threaded_irq(pdata->irq, NULL, bu21013_gpio_irq,
+	error = request_threaded_irq(bu21013_data->irq, NULL, bu21013_gpio_irq,
 				     IRQF_TRIGGER_FALLING | IRQF_SHARED |
 					IRQF_ONESHOT,
 				     DRIVER_TP, bu21013_data);
 	if (error) {
-		dev_err(&client->dev, "request irq %d failed\n", pdata->irq);
+		dev_err(&client->dev, "request irq %d failed\n",
+			bu21013_data->irq);
 		goto err_cs_disable;
 	}
 
@@ -531,7 +562,7 @@ static int __devinit bu21013_probe(struct i2c_client *client,
 err_free_irq:
 	bu21013_free_irq(bu21013_data);
 err_cs_disable:
-	pdata->cs_dis(pdata->cs_pin);
+	bu21013_cs_disable(bu21013_data);
 err_disable_regulator:
 	regulator_disable(bu21013_data->regulator);
 err_put_regulator:
@@ -555,7 +586,7 @@ static int __devexit bu21013_remove(struct i2c_client *client)
 
 	bu21013_free_irq(bu21013_data);
 
-	bu21013_data->chip->cs_dis(bu21013_data->chip->cs_pin);
+	bu21013_cs_disable(bu21013_data);
 
 	input_unregister_device(bu21013_data->in_dev);
 
@@ -584,9 +615,9 @@ static int bu21013_suspend(struct device *dev)
 
 	bu21013_data->touch_stopped = true;
 	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(bu21013_data->chip->irq);
+		enable_irq_wake(bu21013_data->irq);
 	else
-		disable_irq(bu21013_data->chip->irq);
+		disable_irq(bu21013_data->irq);
 
 	regulator_disable(bu21013_data->regulator);
 
@@ -621,9 +652,9 @@ static int bu21013_resume(struct device *dev)
 	bu21013_data->touch_stopped = false;
 
 	if (device_may_wakeup(&client->dev))
-		disable_irq_wake(bu21013_data->chip->irq);
+		disable_irq_wake(bu21013_data->irq);
 	else
-		enable_irq(bu21013_data->chip->irq);
+		enable_irq(bu21013_data->irq);
 
 	return 0;
 }
diff --git a/include/linux/input/bu21013.h b/include/linux/input/bu21013.h
index 05e0328..3ad1be5 100644
--- a/include/linux/input/bu21013.h
+++ b/include/linux/input/bu21013.h
@@ -9,13 +9,11 @@
 
 /**
  * struct bu21013_platform_device - Handle the platform data
- * @cs_en:	pointer to the cs enable function
- * @cs_dis:	pointer to the cs disable function
- * @irq_read_val:    pointer to read the pen irq value function
  * @touch_x_max: touch x max
  * @touch_y_max: touch y max
  * @cs_pin: chip select pin
  * @irq: irq pin
+ * @touch_pin: touch gpio pin
  * @ext_clk: external clock flag
  * @x_flip: x flip flag
  * @y_flip: y flip flag
@@ -24,13 +22,10 @@
  * This is used to handle the platform data
  */
 struct bu21013_platform_device {
-	int (*cs_en)(int reset_pin);
-	int (*cs_dis)(int reset_pin);
-	int (*irq_read_val)(void);
 	int touch_x_max;
 	int touch_y_max;
 	unsigned int cs_pin;
-	unsigned int irq;
+	unsigned int touch_pin;
 	bool ext_clk;
 	bool x_flip;
 	bool y_flip;
-- 
1.7.9.5

[PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Lee Jones]

Now we can register the BU21013_ts touch screen when booting with
Device Tree enabled. Here we parse all the necessary components
previously expected to be passed from platform data.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/input/touchscreen/bu21013_ts.c |   36 ++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/bu21013_ts.c b/drivers/input/touchscreen/bu21013_ts.c
index 8982bea..dbbd1f8 100644
--- a/drivers/input/touchscreen/bu21013_ts.c
+++ b/drivers/input/touchscreen/bu21013_ts.c
@@ -15,6 +15,8 @@
 #include <linux/regulator/consumer.h>
 #include <linux/module.h>
 #include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 
 #define PEN_DOWN_INTR	0
 #define MAX_FINGERS	2
@@ -454,13 +456,30 @@ static void bu21013_cs_disable(struct bu21013_ts_data *bu21013_data)
  * This function used to initializes the i2c-client touchscreen
  * driver and returns integer.
  */
+static void __devinit bu21013_of_probe(struct device_node *np,
+				       struct bu21013_platform_device *pdata)
+{
+	pdata->y_flip = pdata->x_flip = false;
+
+	pdata->x_flip = of_property_read_bool(np, "rohm,flip-x");
+	pdata->y_flip = of_property_read_bool(np, "rohm,flip-y");
+
+	of_property_read_u32(np, "rohm,touch-max-x", &pdata->touch_x_max);
+	of_property_read_u32(np, "rohm,touch-max-y", &pdata->touch_y_max);
+
+	pdata->touch_pin = of_get_named_gpio(np, "touch-gpio", 0);
+	pdata->cs_pin = of_get_named_gpio(np, "reset-gpio", 0);
+
+	pdata->ext_clk = false;
+}
+
 static int __devinit bu21013_probe(struct i2c_client *client,
 				   const struct i2c_device_id *id)
 {
 	struct bu21013_ts_data *bu21013_data;
 	struct input_dev *in_dev;
-	const struct bu21013_platform_device *pdata =
-					client->dev.platform_data;
+	struct device_node *np = client->dev.of_node;
+	struct bu21013_platform_device *pdata = client->dev.platform_data;
 	int error;
 
 	if (!i2c_check_functionality(client->adapter,
@@ -470,8 +489,17 @@ static int __devinit bu21013_probe(struct i2c_client *client,
 	}
 
 	if (!pdata) {
-		dev_err(&client->dev, "platform data not defined\n");
-		return -EINVAL;
+		if (np) {
+			pdata = devm_kzalloc(&client->dev, sizeof(*pdata),
+					GFP_KERNEL);
+			if (!pdata)
+				return -ENOMEM;
+
+			bu21013_of_probe(np, pdata);
+		} else {
+			dev_err(&client->dev, "no device tree or platform data\n");
+			return -EINVAL;
+		}
 	}
 
 	if (!gpio_is_valid(pdata->touch_pin)) {
-- 
1.7.9.5

Re: [PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Dmitry Torokhov]

Hi Lee,

On Tue, Nov 27, 2012 at 01:13:10PM +0000, Lee Jones wrote:
> Now we can register the BU21013_ts touch screen when booting with
> Device Tree enabled. Here we parse all the necessary components
> previously expected to be passed from platform data.

I applied these 3 patches, but for DT we also need to specify compatible
ID and set up of_match_table pointer. Please send me a follow-up patches
doing that and also describing DT bindings for BU21013.

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Lee Jones]

Hi Dmitry,

> On Tue, Nov 27, 2012 at 01:13:10PM +0000, Lee Jones wrote:
> > Now we can register the BU21013_ts touch screen when booting with
> > Device Tree enabled. Here we parse all the necessary components
> > previously expected to be passed from platform data.
> 
> I applied these 3 patches, but for DT we also need to specify compatible
> ID and set up of_match_table pointer. 

Why do you need a compatible string?

> Please send me a follow-up patches
> doing that and also describing DT bindings for BU21013.

I'll send the documentation as a seperate reply to your email.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Mark Brown]

On Wed, Nov 28, 2012 at 08:57:31AM +0000, Lee Jones wrote:

> > I applied these 3 patches, but for DT we also need to specify compatible
> > ID and set up of_match_table pointer. 

> Why do you need a compatible string?

The I2C subsystem guesses at a compatible string by default but it's
much better to explicitly set one as conflicts do arise from time to
time (eg, Wolfson parts are called WMxxxx but the WM prefix is also used
by at least WonderMedia).

Re: [PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Lee Jones]

On Wed, 28 Nov 2012, Mark Brown wrote:

> On Wed, Nov 28, 2012 at 08:57:31AM +0000, Lee Jones wrote:
> 
> > > I applied these 3 patches, but for DT we also need to specify compatible
> > > ID and set up of_match_table pointer. 
> 
> > Why do you need a compatible string?
> 
> The I2C subsystem guesses at a compatible string by default but it's
> much better to explicitly set one as conflicts do arise from time to
> time (eg, Wolfson parts are called WMxxxx but the WM prefix is also used
> by at least WonderMedia).

It uses the exact device name, rather than guessing. I don't think
you're allowed to have duplicate device names in the system, or there
would be clashes at registration time.

The system is the same for platform data and DT alike.

In platform data we have: 

> struct i2c_board_info mop500_i2c0_devices_stuib[] = {
>         {
>                 .type = "bu21013_tp",
>                 .addr = 0x5C,
>         }
> };

Where (i2c_client)client->name is populated by 'type', which should
then match 'driver.name' exactly.

And in DT we have:

bu21013_tp@0x5c {
        compatible = "rhom,bu21013_tp";
        reg = <0x5c>;
};

Where (i2c_client)client->name is populated by of_modalias_node(), which
uses the compatible string in the DTS(I) file and takes off the
'<vendor>,' which should then match 'driver.name' exactly.

Hence, there should be no need to have a compatible string in any i2c
driver registration information.


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

On Thu, Nov 29, 2012 at 10:08:08AM +0000, Lee Jones wrote:
> On Wed, 28 Nov 2012, Mark Brown wrote:

> > The I2C subsystem guesses at a compatible string by default but it's
> > much better to explicitly set one as conflicts do arise from time to
> > time (eg, Wolfson parts are called WMxxxx but the WM prefix is also used
> > by at least WonderMedia).

> It uses the exact device name, rather than guessing. I don't think
> you're allowed to have duplicate device names in the system, or there
> would be clashes at registration time.

> The system is the same for platform data and DT alike.

Right, which is why this mostly works, but it's still better to provide
an actual compatible string which we can be 100% certain will avoid
conflicts.  This is very low cost when one is already defining DT
bindings.

> Hence, there should be no need to have a compatible string in any i2c
> driver registration information.

We're getting away with it at present (and frankly nobody's going to
build in two different drivers for a chip of the same name for practical
systems anyway).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Lee Jones]

On Thu, 29 Nov 2012, Mark Brown wrote:

> On Thu, Nov 29, 2012 at 10:08:08AM +0000, Lee Jones wrote:
> > On Wed, 28 Nov 2012, Mark Brown wrote:
> 
> > > The I2C subsystem guesses at a compatible string by default but it's
> > > much better to explicitly set one as conflicts do arise from time to
> > > time (eg, Wolfson parts are called WMxxxx but the WM prefix is also used
> > > by at least WonderMedia).
> 
> > It uses the exact device name, rather than guessing. I don't think
> > you're allowed to have duplicate device names in the system, or there
> > would be clashes at registration time.
> 
> > The system is the same for platform data and DT alike.
> 
> Right, which is why this mostly works, but it's still better to provide
> an actual compatible string which we can be 100% certain will avoid
> conflicts.  This is very low cost when one is already defining DT
> bindings.

I know that it's an easy thing to do, but I'm more worried about
what would happen if the an I2C device is registered using the
current way (stripping the compatible string and using it as a
client look-up) and we also provide a compatible entry in the
driver itself. Do you know if there is any danger of duplicate
registration or suchlike?

> > Hence, there should be no need to have a compatible string in any i2c
> > driver registration information.
> 
> We're getting away with it at present (and frankly nobody's going to
> build in two different drivers for a chip of the same name for practical
> systems anyway).

Right. In the same way as we're getting away with it when we
register a platform_device using the register/add routines.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]

On Thu, Nov 29, 2012 at 12:00:00PM +0000, Lee Jones wrote:
> On Thu, 29 Nov 2012, Mark Brown wrote:

> > Right, which is why this mostly works, but it's still better to provide
> > an actual compatible string which we can be 100% certain will avoid
> > conflicts.  This is very low cost when one is already defining DT
> > bindings.

> I know that it's an easy thing to do, but I'm more worried about
> what would happen if the an I2C device is registered using the
> current way (stripping the compatible string and using it as a
> client look-up) and we also provide a compatible entry in the
> driver itself. Do you know if there is any danger of duplicate
> registration or suchlike?

That's totally fine, we try first with compatible properties and only
look for the fallback if there aren't any.

> > > Hence, there should be no need to have a compatible string in any i2c
> > > driver registration information.

> > We're getting away with it at present (and frankly nobody's going to
> > build in two different drivers for a chip of the same name for practical
> > systems anyway).

> Right. In the same way as we're getting away with it when we
> register a platform_device using the register/add routines.

Those are all completely Linux-defined of course which helps as well.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] Input: bu21013_ts - Add support for Device Tree booting

[Lee Jones]

Author: Lee Jones <lee.jones@linaro.org>
Date:   Fri Sep 28 14:35:43 2012 +0100

    Input: bu21013_ts - Add support for Device Tree booting
    
    Now we can register the BU21013_ts touch screen when booting with
    Device Tree enabled. Here we parse all the necessary components
    previously expected to be passed from platform data.
    
    Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
    Cc: linux-input@vger.kernel.org
    Acked-by: Arnd Bergmann <arnd@arndb.de>
    Acked-by: Linus Walleij <linus.walleij@linaro.org>
    Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/Documentation/devicetree/bindings/input/touchscreen/bu21013.txt b/Documentation/devicetree/bindings/input/touchscreen/bu21013.txt
new file mode 100644
index 0000000..ca5a2c8
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/bu21013.txt
@@ -0,0 +1,28 @@
+* Rohm BU21013 Touch Screen
+
+Required properties:
+ - compatible              : "rohm,bu21013_tp"
+ - reg                     :  I2C device address
+
+Optional properties:
+ - touch-gpio              : GPIO pin registering a touch event
+ - <supply_name>-supply    : Phandle to a regulator supply
+ - rohm,touch-max-x        : Maximum outward permitted limit in the X axis
+ - rohm,touch-max-y        : Maximum outward permitted limit in the Y axis
+ - rohm,flip-x             : Flip touch coordinates on the X axis
+ - rohm,flip-y             : Flip touch coordinates on the Y axis
+
+Example:
+
+	i2c@80110000 {
+		bu21013_tp@0x5c {
+			compatible = "rohm,bu21013_tp";
+			reg = <0x5c>;
+			touch-gpio = <&gpio2 20 0x4>;
+			avdd-supply = <&ab8500_ldo_aux1_reg>;
+
+			rohm,touch-max-x = <384>;
+			rohm,touch-max-y = <704>;
+			rohm,flip-y;
+		};
+	};
diff --git a/drivers/input/touchscreen/bu21013_ts.c b/drivers/input/touchscreen/bu21013_ts.c
index 16f29d6..07335ac 100644
--- a/drivers/input/touchscreen/bu21013_ts.c
+++ b/drivers/input/touchscreen/bu21013_ts.c
@@ -15,6 +15,8 @@
 #include <linux/regulator/consumer.h>
 #include <linux/module.h>
 #include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 
 #define PEN_DOWN_INTR	0
 #define MAX_FINGERS	2
@@ -473,13 +475,30 @@ static void bu21013_free_irq(struct bu21013_ts_data *bu21013_data)
  * This function used to initializes the i2c-client touchscreen
  * driver and returns integer.
  */
+static void __devinit bu21013_of_probe(struct device_node *np,
+				struct bu21013_platform_device *pdata)
+{
+	pdata->y_flip = pdata->x_flip = false;
+
+	pdata->x_flip = of_property_read_bool(np, "rohm,flip-x");
+	pdata->y_flip = of_property_read_bool(np, "rohm,flip-y");
+
+	of_property_read_u32(np, "rohm,touch-max-x", &pdata->touch_x_max);
+	of_property_read_u32(np, "rohm,touch-max-y", &pdata->touch_y_max);
+
+	pdata->touch_pin = of_get_named_gpio(np, "touch-gpio", 0);
+	pdata->cs_pin = of_get_named_gpio(np, "reset-gpio", 0);
+
+	pdata->ext_clk = false;
+}
+
 static int __devinit bu21013_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
 	struct bu21013_ts_data *bu21013_data;
 	struct input_dev *in_dev;
-	const struct bu21013_platform_device *pdata =
-					client->dev.platform_data;
+	struct device_node *np = client->dev.of_node;
+	struct bu21013_platform_device *pdata = client->dev.platform_data;
 	int error;
 
 	if (!i2c_check_functionality(client->adapter,
@@ -489,8 +508,17 @@ static int __devinit bu21013_probe(struct i2c_client *client,
 	}
 
 	if (!pdata) {
-		dev_err(&client->dev, "platform data not defined\n");
-		return -EINVAL;
+		if (np) {
+			pdata = devm_kzalloc(&client->dev, sizeof(*pdata),
+					GFP_KERNEL);
+			if (!pdata)
+				return -ENOMEM;
+
+			bu21013_of_probe(np, pdata);
+		} else {
+			dev_err(&client->dev, "no device tree or platform data\n");
+			return -EINVAL;
+		}
 	}
 
 	pdata->irq = gpio_to_irq(pdata->touch_pin);