diff mbox

[RESEND,1/2] extcon: gpio: Add dt support for the driver

Message ID 1415201388-32060-2-git-send-email-george.cherian@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

George Cherian Nov. 5, 2014, 3:29 p.m. UTC 
Add device tree support to extcon-gpio driver.
Add devicetree binding documentation

While at that
	- Cleanup the pdata as there are no users for the same.
	- Convert the driver to use gpiod_* API's.
	- Some gpio's can sleep while reading, so always use gpio_get_value_cansleep
	to get data. This fixes warning from gpiolib due to wrong API usage.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 .../devicetree/bindings/extcon/extcon-gpio.txt     | 21 ++++++
 drivers/extcon/extcon-gpio.c                       | 83 +++++++---------------
 include/linux/extcon/extcon-gpio.h                 | 59 ---------------
 3 files changed, 46 insertions(+), 117 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
 delete mode 100644 include/linux/extcon/extcon-gpio.h

Comments

Felipe Balbi Nov. 5, 2014, 6:30 p.m. UTC | #1 
On Wed, Nov 05, 2014 at 08:59:47PM +0530, George Cherian wrote:
> Add device tree support to extcon-gpio driver.
> Add devicetree binding documentation
> 
> While at that
> 	- Cleanup the pdata as there are no users for the same.

should be a patch of its own.

> 	- Convert the driver to use gpiod_* API's.

should be a patch of its own.

> 	- Some gpio's can sleep while reading, so always use gpio_get_value_cansleep

doesn't explain the fact that you have such a platform. Make it clear
that you have a platform supported in mainline which uses a GPIO from an
external i2c I/O expander and because of that you need to use can_sleep.

Should also be a patch of its own, as it was before.
Chanwoo Choi Nov. 12, 2014, 1:10 a.m. UTC | #2 
Hi George,

How do you test this patch? This patch cannot test it 
because this patch has not compatible string for binding through DT.

You have to add following of_device_id for extcon-gpio:

+#if defined(CONFIG_OF)
+static const struct of_device_id gpio_extcon_of_match[] = {
+       { .compatible = "extcon-gpio", },
+       {},
+};
+MODULE_DEVICE_TABLE(of, gpio_extcon_of_match);
+#endif
+
 static struct platform_driver gpio_extcon_driver = {
        .probe          = gpio_extcon_probe,
        .remove         = gpio_extcon_remove,
        .driver         = {
                .name   = "extcon-gpio",
                .owner  = THIS_MODULE,
+               .of_match_table = of_match_ptr(gpio_extcon_of_match),
                .pm     = &gpio_extcon_pm_ops,
        },
 };


On 11/06/2014 12:29 AM, George Cherian wrote:
> Add device tree support to extcon-gpio driver.
> Add devicetree binding documentation
> 
> While at that
> 	- Cleanup the pdata as there are no users for the same.
> 	- Convert the driver to use gpiod_* API's.
> 	- Some gpio's can sleep while reading, so always use gpio_get_value_cansleep
> 	to get data. This fixes warning from gpiolib due to wrong API usage.
> 
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>  .../devicetree/bindings/extcon/extcon-gpio.txt     | 21 ++++++
>  drivers/extcon/extcon-gpio.c                       | 83 +++++++---------------
>  include/linux/extcon/extcon-gpio.h                 | 59 ---------------
>  3 files changed, 46 insertions(+), 117 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
>  delete mode 100644 include/linux/extcon/extcon-gpio.h
> 
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> new file mode 100644
> index 0000000..30aa2e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> @@ -0,0 +1,21 @@
> +GPIO based EXTCON
> +
> +EXTCON GPIO
> +-----------
> +
> +Required Properties:
> + - compatible: should be:
> +	* "linux,extcon-gpio"

I prefer to use simply "extcon-gpio" compatible" instead of "linux,extcon-gpio".

> + - gpios: specifies the gpio pin used.
> +
> +Optional Properties:
> + - debounce: Debounce time for GPIO IRQ in ms
> +
> +

Remove unneeded blank line.

> +Eg:
> +
> +	extcon1: am43_usbid_extcon1 {

I want to change the example of extcon-gpio as following:
	"extcon1: am43_usbid_extcon1" -> "usb_extcon: extcon-gpio0"

> +		compatible = "linux,extcon-gpio";

ditto.

> +		gpios = <&gpio3 12 GPIO_ACTIVE_HIGH>;
> +		debounce = <20>;
> +		};

You have to fix indentation of brace.

> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 72f19a3..85795de 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -21,26 +21,23 @@
>  */
>  
>  #include <linux/extcon.h>
> -#include <linux/extcon/extcon-gpio.h>
>  #include <linux/gpio.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> +#include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of_gpio.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/workqueue.h>
>  
>  struct gpio_extcon_data {
>  	struct extcon_dev *edev;
> -	unsigned gpio;
> -	bool gpio_active_low;
> -	const char *state_on;
> -	const char *state_off;
> +	struct gpio_desc *gpiod;
>  	int irq;
>  	struct delayed_work work;
>  	unsigned long debounce_jiffies;
> -	bool check_on_resume;
>  };
>  
>  static void gpio_extcon_work(struct work_struct *work)
> @@ -50,9 +47,7 @@ static void gpio_extcon_work(struct work_struct *work)
>  		container_of(to_delayed_work(work), struct gpio_extcon_data,
>  			     work);
>  
> -	state = gpio_get_value(data->gpio);
> -	if (data->gpio_active_low)
> -		state = !state;
> +	state = gpiod_get_value_cansleep(data->gpiod);
>  	extcon_set_state(data->edev, state);
>  }
>  
> @@ -65,34 +60,16 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
> -{
> -	struct device *dev = edev->dev.parent;
> -	struct gpio_extcon_data *extcon_data = dev_get_drvdata(dev);
> -	const char *state;
> -
> -	if (extcon_get_state(edev))
> -		state = extcon_data->state_on;
> -	else
> -		state = extcon_data->state_off;
> -
> -	if (state)
> -		return sprintf(buf, "%s\n", state);
> -	return -EINVAL;
> -}
> -
>  static int gpio_extcon_probe(struct platform_device *pdev)
>  {
> -	struct gpio_extcon_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct device_node *np = pdev->dev.of_node;
>  	struct gpio_extcon_data *extcon_data;
> +	unsigned int irq_flags;
> +	unsigned int debounce = 0;
>  	int ret;
>  
> -	if (!pdata)
> -		return -EBUSY;
> -	if (!pdata->irq_flags) {
> -		dev_err(&pdev->dev, "IRQ flag is not specified.\n");
> +	if (!np)
>  		return -EINVAL;
> -	}
>  
>  	extcon_data = devm_kzalloc(&pdev->dev, sizeof(struct gpio_extcon_data),
>  				   GFP_KERNEL);
> @@ -104,27 +81,23 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "failed to allocate extcon device\n");
>  		return -ENOMEM;
>  	}
> -	extcon_data->edev->name = pdata->name;
> -
> -	extcon_data->gpio = pdata->gpio;
> -	extcon_data->gpio_active_low = pdata->gpio_active_low;
> -	extcon_data->state_on = pdata->state_on;
> -	extcon_data->state_off = pdata->state_off;
> -	extcon_data->check_on_resume = pdata->check_on_resume;
> -	if (pdata->state_on && pdata->state_off)
> -		extcon_data->edev->print_state = extcon_gpio_print_state;
> -
> -	ret = devm_gpio_request_one(&pdev->dev, extcon_data->gpio, GPIOF_DIR_IN,
> -				    pdev->name);
> -	if (ret < 0)
> -		return ret;
> +	extcon_data->edev->name = np->name;
> +	extcon_data->gpiod = gpiod_get(&pdev->dev, NULL);
> +	if (IS_ERR(extcon_data->gpiod))
> +		return PTR_ERR(extcon_data->gpiod);
>  
> -	if (pdata->debounce) {
> -		ret = gpio_set_debounce(extcon_data->gpio,
> -					pdata->debounce * 1000);
> +	extcon_data->irq = gpiod_to_irq(extcon_data->gpiod);
> +	if (extcon_data->irq < 0)
> +		return extcon_data->irq;
> +
> +	irq_flags = irq_get_trigger_type(extcon_data->irq);

I recommend to use interrupt flags as following:
	irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;

> +	of_property_read_u32(np, "debounce", &debounce);

I don't want to use additional variable ('debounce').
You better to use 'data->debounce_jiffies' direclty instead of 'debounce'
Also, I prefer to check the return value of of_property_read_u32().

> +	if (debounce) {
> +		ret = gpiod_set_debounce(extcon_data->gpiod,
> +					 debounce * 1000);
>  		if (ret < 0)
>  			extcon_data->debounce_jiffies =
> -				msecs_to_jiffies(pdata->debounce);
> +				msecs_to_jiffies(debounce);
>  	}

How about following codes?

	if (of_property_read_u32(np, "debounce", &data->debounce_jiffies))
		data->debounce_jiffies = 0;

	ret = gpiod_set_debounce(data->gpiod, data->debounce_jiffies * 1000);
	if (ret < 0)
		data->debounce_jiffies
			= msecs_to_jiffies(data->debounce_jiffies);


>  
>  	ret = devm_extcon_dev_register(&pdev->dev, extcon_data->edev);
> @@ -132,13 +105,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
> -
> -	extcon_data->irq = gpio_to_irq(extcon_data->gpio);
> -	if (extcon_data->irq < 0)
> -		return extcon_data->irq;
> -
>  	ret = request_any_context_irq(extcon_data->irq, gpio_irq_handler,
> -				      pdata->irq_flags, pdev->name,
> +				      irq_flags, pdev->name,
>  				      extcon_data);
>  	if (ret < 0)
>  		return ret;
> @@ -166,9 +134,8 @@ static int gpio_extcon_resume(struct device *dev)
>  	struct gpio_extcon_data *extcon_data;
>  
>  	extcon_data = dev_get_drvdata(dev);
> -	if (extcon_data->check_on_resume)

Why did you remove 'check_on_resume' flag without any description?

> -		queue_delayed_work(system_power_efficient_wq,
> -			&extcon_data->work, extcon_data->debounce_jiffies);
> +	queue_delayed_work(system_power_efficient_wq,
> +			   &extcon_data->work, extcon_data->debounce_jiffies);
>  
>  	return 0;
>  }
> diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
> deleted file mode 100644
> index 0b17ad4..0000000
> --- a/include/linux/extcon/extcon-gpio.h
> +++ /dev/null
> @@ -1,59 +0,0 @@
> -/*
> - *  External connector (extcon) class generic GPIO driver
> - *
> - * Copyright (C) 2012 Samsung Electronics
> - * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
> - *
> - * based on switch class driver
> - * Copyright (C) 2008 Google, Inc.
> - * Author: Mike Lockwood <lockwood@android.com>
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> -*/
> -#ifndef __EXTCON_GPIO_H__
> -#define __EXTCON_GPIO_H__ __FILE__
> -
> -#include <linux/extcon.h>
> -
> -/**
> - * struct gpio_extcon_platform_data - A simple GPIO-controlled extcon device.
> - * @name:		The name of this GPIO extcon device.
> - * @gpio:		Corresponding GPIO.
> - * @gpio_active_low:	Boolean describing whether gpio active state is 1 or 0
> - *			If true, low state of gpio means active.
> - *			If false, high state of gpio means active.
> - * @debounce:		Debounce time for GPIO IRQ in ms.
> - * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
> - * @state_on:		print_state is overriden with state_on if attached.
> - *			If NULL, default method of extcon class is used.
> - * @state_off:		print_state is overriden with state_off if detached.
> - *			If NUll, default method of extcon class is used.
> - * @check_on_resume:	Boolean describing whether to check the state of gpio
> - *			while resuming from sleep.
> - *
> - * Note that in order for state_on or state_off to be valid, both state_on
> - * and state_off should be not NULL. If at least one of them is NULL,
> - * the print_state is not overriden.
> - */
> -struct gpio_extcon_platform_data {
> -	const char *name;
> -	unsigned gpio;
> -	bool gpio_active_low;
> -	unsigned long debounce;
> -	unsigned long irq_flags;
> -
> -	/* if NULL, "0" or "1" will be printed */
> -	const char *state_on;
> -	const char *state_off;
> -	bool check_on_resume;
> -};
> -
> -#endif /* __EXTCON_GPIO_H__ */
> 

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
new file mode 100644
index 0000000..30aa2e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
@@ -0,0 +1,21 @@ 
+GPIO based EXTCON
+
+EXTCON GPIO
+-----------
+
+Required Properties:
+ - compatible: should be:
+	* "linux,extcon-gpio"
+ - gpios: specifies the gpio pin used.
+
+Optional Properties:
+ - debounce: Debounce time for GPIO IRQ in ms
+
+
+Eg:
+
+	extcon1: am43_usbid_extcon1 {
+		compatible = "linux,extcon-gpio";
+		gpios = <&gpio3 12 GPIO_ACTIVE_HIGH>;
+		debounce = <20>;
+		};
diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 72f19a3..85795de 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -21,26 +21,23 @@ 
 */
 
 #include <linux/extcon.h>
-#include <linux/extcon/extcon-gpio.h>
 #include <linux/gpio.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
 
 struct gpio_extcon_data {
 	struct extcon_dev *edev;
-	unsigned gpio;
-	bool gpio_active_low;
-	const char *state_on;
-	const char *state_off;
+	struct gpio_desc *gpiod;
 	int irq;
 	struct delayed_work work;
 	unsigned long debounce_jiffies;
-	bool check_on_resume;
 };
 
 static void gpio_extcon_work(struct work_struct *work)
@@ -50,9 +47,7 @@  static void gpio_extcon_work(struct work_struct *work)
 		container_of(to_delayed_work(work), struct gpio_extcon_data,
 			     work);
 
-	state = gpio_get_value(data->gpio);
-	if (data->gpio_active_low)
-		state = !state;
+	state = gpiod_get_value_cansleep(data->gpiod);
 	extcon_set_state(data->edev, state);
 }
 
@@ -65,34 +60,16 @@  static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
-{
-	struct device *dev = edev->dev.parent;
-	struct gpio_extcon_data *extcon_data = dev_get_drvdata(dev);
-	const char *state;
-
-	if (extcon_get_state(edev))
-		state = extcon_data->state_on;
-	else
-		state = extcon_data->state_off;
-
-	if (state)
-		return sprintf(buf, "%s\n", state);
-	return -EINVAL;
-}
-
 static int gpio_extcon_probe(struct platform_device *pdev)
 {
-	struct gpio_extcon_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct device_node *np = pdev->dev.of_node;
 	struct gpio_extcon_data *extcon_data;
+	unsigned int irq_flags;
+	unsigned int debounce = 0;
 	int ret;
 
-	if (!pdata)
-		return -EBUSY;
-	if (!pdata->irq_flags) {
-		dev_err(&pdev->dev, "IRQ flag is not specified.\n");
+	if (!np)
 		return -EINVAL;
-	}
 
 	extcon_data = devm_kzalloc(&pdev->dev, sizeof(struct gpio_extcon_data),
 				   GFP_KERNEL);
@@ -104,27 +81,23 @@  static int gpio_extcon_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to allocate extcon device\n");
 		return -ENOMEM;
 	}
-	extcon_data->edev->name = pdata->name;
-
-	extcon_data->gpio = pdata->gpio;
-	extcon_data->gpio_active_low = pdata->gpio_active_low;
-	extcon_data->state_on = pdata->state_on;
-	extcon_data->state_off = pdata->state_off;
-	extcon_data->check_on_resume = pdata->check_on_resume;
-	if (pdata->state_on && pdata->state_off)
-		extcon_data->edev->print_state = extcon_gpio_print_state;
-
-	ret = devm_gpio_request_one(&pdev->dev, extcon_data->gpio, GPIOF_DIR_IN,
-				    pdev->name);
-	if (ret < 0)
-		return ret;
+	extcon_data->edev->name = np->name;
+	extcon_data->gpiod = gpiod_get(&pdev->dev, NULL);
+	if (IS_ERR(extcon_data->gpiod))
+		return PTR_ERR(extcon_data->gpiod);
 
-	if (pdata->debounce) {
-		ret = gpio_set_debounce(extcon_data->gpio,
-					pdata->debounce * 1000);
+	extcon_data->irq = gpiod_to_irq(extcon_data->gpiod);
+	if (extcon_data->irq < 0)
+		return extcon_data->irq;
+
+	irq_flags = irq_get_trigger_type(extcon_data->irq);
+	of_property_read_u32(np, "debounce", &debounce);
+	if (debounce) {
+		ret = gpiod_set_debounce(extcon_data->gpiod,
+					 debounce * 1000);
 		if (ret < 0)
 			extcon_data->debounce_jiffies =
-				msecs_to_jiffies(pdata->debounce);
+				msecs_to_jiffies(debounce);
 	}
 
 	ret = devm_extcon_dev_register(&pdev->dev, extcon_data->edev);
@@ -132,13 +105,8 @@  static int gpio_extcon_probe(struct platform_device *pdev)
 		return ret;
 
 	INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
-
-	extcon_data->irq = gpio_to_irq(extcon_data->gpio);
-	if (extcon_data->irq < 0)
-		return extcon_data->irq;
-
 	ret = request_any_context_irq(extcon_data->irq, gpio_irq_handler,
-				      pdata->irq_flags, pdev->name,
+				      irq_flags, pdev->name,
 				      extcon_data);
 	if (ret < 0)
 		return ret;
@@ -166,9 +134,8 @@  static int gpio_extcon_resume(struct device *dev)
 	struct gpio_extcon_data *extcon_data;
 
 	extcon_data = dev_get_drvdata(dev);
-	if (extcon_data->check_on_resume)
-		queue_delayed_work(system_power_efficient_wq,
-			&extcon_data->work, extcon_data->debounce_jiffies);
+	queue_delayed_work(system_power_efficient_wq,
+			   &extcon_data->work, extcon_data->debounce_jiffies);
 
 	return 0;
 }
diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
deleted file mode 100644
index 0b17ad4..0000000
--- a/include/linux/extcon/extcon-gpio.h
+++ /dev/null
@@ -1,59 +0,0 @@ 
-/*
- *  External connector (extcon) class generic GPIO driver
- *
- * Copyright (C) 2012 Samsung Electronics
- * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
- *
- * based on switch class driver
- * Copyright (C) 2008 Google, Inc.
- * Author: Mike Lockwood <lockwood@android.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
-*/
-#ifndef __EXTCON_GPIO_H__
-#define __EXTCON_GPIO_H__ __FILE__
-
-#include <linux/extcon.h>
-
-/**
- * struct gpio_extcon_platform_data - A simple GPIO-controlled extcon device.
- * @name:		The name of this GPIO extcon device.
- * @gpio:		Corresponding GPIO.
- * @gpio_active_low:	Boolean describing whether gpio active state is 1 or 0
- *			If true, low state of gpio means active.
- *			If false, high state of gpio means active.
- * @debounce:		Debounce time for GPIO IRQ in ms.
- * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
- * @state_on:		print_state is overriden with state_on if attached.
- *			If NULL, default method of extcon class is used.
- * @state_off:		print_state is overriden with state_off if detached.
- *			If NUll, default method of extcon class is used.
- * @check_on_resume:	Boolean describing whether to check the state of gpio
- *			while resuming from sleep.
- *
- * Note that in order for state_on or state_off to be valid, both state_on
- * and state_off should be not NULL. If at least one of them is NULL,
- * the print_state is not overriden.
- */
-struct gpio_extcon_platform_data {
-	const char *name;
-	unsigned gpio;
-	bool gpio_active_low;
-	unsigned long debounce;
-	unsigned long irq_flags;
-
-	/* if NULL, "0" or "1" will be printed */
-	const char *state_on;
-	const char *state_off;
-	bool check_on_resume;
-};
-
-#endif /* __EXTCON_GPIO_H__ */