* [PATCH v4] extcon: gpio: Add the support for Device tree bindings
@ 2016-05-26 11:47 Venkat Reddy Talla
2016-05-26 13:21 ` Chanwoo Choi
2016-05-27 15:29 ` Rob Herring
0 siblings, 2 replies; 14+ messages in thread
From: Venkat Reddy Talla @ 2016-05-26 11:47 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, Rob Herring, Pawel Moll
Cc: Mark Rutland, Ian Campbell, devicetree, Kumar Gala, linux-kernel,
Laxman Dewangan, Venkat Reddy Talla
Add the support for Device tree bindings of extcon-gpio driver.
The extcon-gpio device tree node must include the both 'extcon-id' and
'gpios' property.
For example:
usb_cable: extcon-gpio-0 {
compatible = "extcon-gpio";
extcon-id = <EXTCON_USB>;
gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
}
ta_cable: extcon-gpio-1 {
compatible = "extcon-gpio";
extcon-id = <EXTCON_CHG_USB_DCP>;
gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
debounce-ms = <50>; /* 50 millisecond */
wakeup-source;
}
&dwc3_usb {
extcon = <&usb_cable>;
};
&charger {
extcon = <&ta_cable>;
};
Signed-off-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
---
changes from v3:
- add description for wakeup-source in documentation
- change dts property extcon-gpio name to gpios
- use of_get_named_gpio_flags to get gpio number and flags
Changes from v2:
- Add the more description for 'extcon-id' property in documentation
Changes from v1:
- Create the include/dt-bindings/extcon/extcon.h including the identification
of external connector. These definitions are used in dts file.
- Fix error if CONFIG_OF is disabled.
- Add signed-off tag by Myungjoo Ham
---
---
.../devicetree/bindings/extcon/extcon-gpio.txt | 48 +++++++++
drivers/extcon/extcon-gpio.c | 109 +++++++++++++++++----
include/dt-bindings/extcon/extcon.h | 47 +++++++++
include/linux/extcon/extcon-gpio.h | 8 +-
4 files changed, 189 insertions(+), 23 deletions(-)
create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
create mode 100644 include/dt-bindings/extcon/extcon.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..81f7932
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
@@ -0,0 +1,48 @@
+GPIO Extcon device
+
+This is a virtual device used to generate the specific cable states from the
+GPIO pin.
+
+Required properties:
+- compatible: Must be "extcon-gpio".
+- extcon-id: The extcon support the various type of external connector to check
+ whether connector is attached or detached. The each external connector has
+ the unique number to identify it. So this property includes the unique number
+ which indicates the specific external connector. When external connector is
+ attached or detached, GPIO pin detect the changed state. See include/
+ dt-bindings/extcon/extcon.h which defines the unique number for supported
+ external connector from extcon framework.
+- gpios: GPIO pin to detect the external connector. See gpio binding.
+
+Optional properties:
+- debounce-ms: the debounce dealy for GPIO pin in millisecond.
+- wakeup-source: Boolean, extcon can wake-up the system from suspend.
+ if gpio provided in extcon-gpio DT node is registered as interrupt,
+ then extcon can wake-up the system from suspend if wakeup-source property
+ is available in DT node, if gpio registered as interrupt but wakeup-source
+ is not available in DT node, then system wake-up due to extcon events
+ not supported.
+
+Example: Examples of extcon-gpio node as listed below:
+
+ usb_cable: extcon-gpio-0 {
+ compatible = "extcon-gpio";
+ extcon-id = <EXTCON_USB>;
+ extcon-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
+ }
+
+ ta_cable: extcon-gpio-1 {
+ compatible = "extcon-gpio";
+ extcon-id = <EXTCON_CHG_USB_DCP>;
+ extcon-gpio = <&gpio3 2 GPIO_ACTIVE_LOW>;
+ debounce-ms = <50>; /* 50 millisecond */
+ wakeup-source;
+ }
+
+ &dwc3_usb {
+ extcon = <&usb_cable>;
+ };
+
+ &charger {
+ extcon = <&ta_cable>;
+ };
diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index d023789..592f395 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -1,11 +1,9 @@
/*
* extcon_gpio.c - Single-state GPIO extcon driver based on extcon class
*
- * Copyright (C) 2008 Google, Inc.
- * Author: Mike Lockwood <lockwood@android.com>
- *
- * Modified by MyungJoo Ham <myungjoo.ham@samsung.com> to support extcon
- * (originally switch class is supported)
+ * Copyright (C) 2016 Chanwoo Choi <cw00.choi@samsung.com>, Samsung Electronics
+ * Copyright (C) 2012 MyungJoo Ham <myungjoo.ham@samsung.com>, Samsung Electronics
+ * Copyright (C) 2008 Mike Lockwood <lockwood@android.com>, Google, Inc.
*
* This software is licensed under the terms of the GNU General Public
* License version 2, as published by the Free Software Foundation, and
@@ -25,13 +23,17 @@
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
struct gpio_extcon_data {
struct extcon_dev *edev;
int irq;
+ bool irq_wakeup;
struct delayed_work work;
unsigned long debounce_jiffies;
@@ -49,7 +51,7 @@ static void gpio_extcon_work(struct work_struct *work)
state = gpiod_get_value_cansleep(data->id_gpiod);
if (data->pdata->gpio_active_low)
state = !state;
- extcon_set_state(data->edev, state);
+ extcon_set_cable_state_(data->edev, data->pdata->extcon_id, state);
}
static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
@@ -61,6 +63,42 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static int gpio_extcon_parse_of(struct platform_device *pdev,
+ struct gpio_extcon_data *data)
+{
+ struct gpio_extcon_pdata *pdata;
+ struct device_node *np = pdev->dev.of_node;
+ enum of_gpio_flags flags;
+ int ret;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ ret = device_property_read_u32(&pdev->dev, "extcon-id",
+ &pdata->extcon_id);
+ if (ret < 0)
+ return -EINVAL;
+
+ pdata->gpio = of_get_named_gpio_flags(np, "gpios", 0, &flags);
+ if (pdata->gpio < 0)
+ return -EINVAL;
+
+ if (flags & OF_GPIO_ACTIVE_LOW)
+ pdata->gpio_active_low = true;
+
+ data->irq_wakeup = device_property_read_bool(&pdev->dev,
+ "wakeup-source");
+
+ device_property_read_u32(&pdev->dev, "debounce-ms", &pdata->debounce);
+
+ pdata->irq_flags = (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
+ | IRQF_ONESHOT);
+
+ data->pdata = pdata;
+ return 0;
+}
+
static int gpio_extcon_init(struct device *dev, struct gpio_extcon_data *data)
{
struct gpio_extcon_pdata *pdata = data->pdata;
@@ -96,16 +134,20 @@ static int gpio_extcon_probe(struct platform_device *pdev)
struct gpio_extcon_data *data;
int ret;
- if (!pdata)
- return -EBUSY;
- if (!pdata->irq_flags || pdata->extcon_id > EXTCON_NONE)
- return -EINVAL;
-
- data = devm_kzalloc(&pdev->dev, sizeof(struct gpio_extcon_data),
- GFP_KERNEL);
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
- data->pdata = pdata;
+
+ if (!pdata) {
+ ret = gpio_extcon_parse_of(pdev, data);
+ if (ret < 0)
+ return ret;
+ } else {
+ data->pdata = pdata;
+ }
+
+ if (!data->pdata->irq_flags || data->pdata->extcon_id == EXTCON_NONE)
+ return -EINVAL;
/* Initialize the gpio */
ret = gpio_extcon_init(&pdev->dev, data);
@@ -113,7 +155,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
return ret;
/* Allocate the memory of extcon devie and register extcon device */
- data->edev = devm_extcon_dev_allocate(&pdev->dev, &pdata->extcon_id);
+ data->edev = devm_extcon_dev_allocate(&pdev->dev,
+ &data->pdata->extcon_id);
if (IS_ERR(data->edev)) {
dev_err(&pdev->dev, "failed to allocate extcon device\n");
return -ENOMEM;
@@ -130,7 +173,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
* is attached or detached.
*/
ret = devm_request_any_context_irq(&pdev->dev, data->irq,
- gpio_irq_handler, pdata->irq_flags,
+ gpio_irq_handler,
+ data->pdata->irq_flags,
pdev->name, data);
if (ret < 0)
return ret;
@@ -139,6 +183,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
/* Perform initial detection */
gpio_extcon_work(&data->work.work);
+ if (data->irq_wakeup)
+ device_init_wakeup(&pdev->dev, data->irq_wakeup);
return 0;
}
@@ -152,11 +198,23 @@ static int gpio_extcon_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
+static int gpio_extcon_suspend(struct device *dev)
+{
+ struct gpio_extcon_data *data = dev_get_drvdata(dev);
+
+ if (data->irq_wakeup)
+ enable_irq_wake(data->irq);
+
+ return 0;
+}
+
static int gpio_extcon_resume(struct device *dev)
{
- struct gpio_extcon_data *data;
+ struct gpio_extcon_data *data = dev_get_drvdata(dev);
+
+ if (data->irq_wakeup)
+ disable_irq_wake(data->irq);
- data = dev_get_drvdata(dev);
if (data->pdata->check_on_resume)
queue_delayed_work(system_power_efficient_wq,
&data->work, data->debounce_jiffies);
@@ -165,7 +223,18 @@ static int gpio_extcon_resume(struct device *dev)
}
#endif
-static SIMPLE_DEV_PM_OPS(gpio_extcon_pm_ops, NULL, gpio_extcon_resume);
+#if defined(CONFIG_OF)
+static const struct of_device_id gpio_extcon_of_match[] = {
+ { .compatible = "extcon-gpio", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, gpio_extcon_of_match);
+#else
+#define gpio_extcon_of_match NULL
+#endif
+
+static SIMPLE_DEV_PM_OPS(gpio_extcon_pm_ops,
+ gpio_extcon_suspend, gpio_extcon_resume);
static struct platform_driver gpio_extcon_driver = {
.probe = gpio_extcon_probe,
@@ -173,11 +242,13 @@ static struct platform_driver gpio_extcon_driver = {
.driver = {
.name = "extcon-gpio",
.pm = &gpio_extcon_pm_ops,
+ .of_match_table = gpio_extcon_of_match,
},
};
module_platform_driver(gpio_extcon_driver);
+MODULE_AUTHOR("Chanwoo Choi <cw00.choi@samsung.com>");
MODULE_AUTHOR("Mike Lockwood <lockwood@android.com>");
MODULE_DESCRIPTION("GPIO extcon driver");
MODULE_LICENSE("GPL");
diff --git a/include/dt-bindings/extcon/extcon.h b/include/dt-bindings/extcon/extcon.h
new file mode 100644
index 0000000..dbba588
--- /dev/null
+++ b/include/dt-bindings/extcon/extcon.h
@@ -0,0 +1,47 @@
+/*
+ * This header provides constants for most extcon bindings.
+ *
+ * Most extcon bindings include the unique identification format.
+ * In most cases, the unique id format uses the standard values/macro
+ * defined in this header.
+ */
+
+#ifndef _DT_BINDINGS_EXTCON_EXTCON_H
+#define _DT_BINDINGS_EXTCON_EXTCON_H
+
+/* USB external connector */
+#define EXTCON_USB 1
+#define EXTCON_USB_HOST 2
+
+/* Charging external connector */
+#define EXTCON_CHG_USB_SDP 5 /* Standard Downstream Port */
+#define EXTCON_CHG_USB_DCP 6 /* Dedicated Charging Port */
+#define EXTCON_CHG_USB_CDP 7 /* Charging Downstream Port */
+#define EXTCON_CHG_USB_ACA 8 /* Accessory Charger Adapter */
+#define EXTCON_CHG_USB_FAST 9
+#define EXTCON_CHG_USB_SLOW 10
+
+/* Jack external connector */
+#define EXTCON_JACK_MICROPHONE 20
+#define EXTCON_JACK_HEADPHONE 21
+#define EXTCON_JACK_LINE_IN 22
+#define EXTCON_JACK_LINE_OUT 23
+#define EXTCON_JACK_VIDEO_IN 24
+#define EXTCON_JACK_VIDEO_OUT 25
+#define EXTCON_JACK_SPDIF_IN 26 /* Sony Philips Digital InterFace */
+#define EXTCON_JACK_SPDIF_OUT 27
+
+/* Display external connector */
+#define EXTCON_DISP_HDMI 40 /* High-Definition Multimedia Interface */
+#define EXTCON_DISP_MHL 41 /* Mobile High-Definition Link */
+#define EXTCON_DISP_DVI 42 /* Digital Visual Interface */
+#define EXTCON_DISP_VGA 43 /* Video Graphics Array */
+
+/* Miscellaneous external connector */
+#define EXTCON_DOCK 60
+#define EXTCON_JIG 61
+#define EXTCON_MECHANICAL 62
+
+#define EXTCON_NUM 63
+
+#endif /* _DT_BINDINGS_EXTCON_EXTCON_H */
diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
index 7cacafb..f7e7673 100644
--- a/include/linux/extcon/extcon-gpio.h
+++ b/include/linux/extcon/extcon-gpio.h
@@ -1,8 +1,8 @@
/*
* Single-state GPIO extcon driver based on extcon class
*
- * Copyright (C) 2012 Samsung Electronics
- * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
+ * Copyright (C) 2016 Chanwoo Choi <cw00.choi@samsung.com>, Samsung Electronics
+ * Copyright (C) 2012 MyungJoo Ham <myungjoo.ham@samsung.com>, Samsung Electronics
*
* based on switch class driver
* Copyright (C) 2008 Google, Inc.
@@ -35,10 +35,10 @@
* while resuming from sleep.
*/
struct gpio_extcon_pdata {
- unsigned int extcon_id;
+ u32 extcon_id;
unsigned gpio;
bool gpio_active_low;
- unsigned long debounce;
+ u32 debounce;
unsigned long irq_flags;
bool check_on_resume;
--
2.1.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-26 11:47 [PATCH v4] extcon: gpio: Add the support for Device tree bindings Venkat Reddy Talla
@ 2016-05-26 13:21 ` Chanwoo Choi
2016-05-27 5:13 ` Venkat Reddy Talla
2016-05-27 15:29 ` Rob Herring
1 sibling, 1 reply; 14+ messages in thread
From: Chanwoo Choi @ 2016-05-26 13:21 UTC (permalink / raw)
To: Venkat Reddy Talla
Cc: MyungJoo Ham, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, devicetree, Kumar Gala, linux-kernel,
Laxman Dewangan
Hi Venkat,
There is some miscommunication. On previous my reply, I don't mean
that the author of the patch[1] is changed from me to you.
I'd like you to remain the original author(me) for the patch[1]
without changing the author.
[1] https://lkml.org/lkml/2015/10/21/8
- [PATCH v3] extcon: gpio: Add the support for Device tree bindings
You can use the patch[1] as based patch and you can add new feature on
base patch[1]. Also, If you ok, you can modify the extccon-gpio.c
driver as Rob comment[2].
But, Rob Herring gave me the some comment[2].
[2] https://lkml.org/lkml/2015/10/21/906
Thanks,
Chanwoo Choi
On Thu, May 26, 2016 at 8:47 PM, Venkat Reddy Talla
<vreddytalla@nvidia.com> wrote:
> Add the support for Device tree bindings of extcon-gpio driver.
> The extcon-gpio device tree node must include the both 'extcon-id' and
> 'gpios' property.
>
> For example:
> usb_cable: extcon-gpio-0 {
> compatible = "extcon-gpio";
> extcon-id = <EXTCON_USB>;
> gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
> }
> ta_cable: extcon-gpio-1 {
> compatible = "extcon-gpio";
> extcon-id = <EXTCON_CHG_USB_DCP>;
> gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
> debounce-ms = <50>; /* 50 millisecond */
> wakeup-source;
> }
> &dwc3_usb {
> extcon = <&usb_cable>;
> };
> &charger {
> extcon = <&ta_cable>;
> };
>
> Signed-off-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>
> ---
> changes from v3:
> - add description for wakeup-source in documentation
> - change dts property extcon-gpio name to gpios
> - use of_get_named_gpio_flags to get gpio number and flags
> Changes from v2:
> - Add the more description for 'extcon-id' property in documentation
> Changes from v1:
> - Create the include/dt-bindings/extcon/extcon.h including the identification
> of external connector. These definitions are used in dts file.
> - Fix error if CONFIG_OF is disabled.
> - Add signed-off tag by Myungjoo Ham
> ---
> ---
> .../devicetree/bindings/extcon/extcon-gpio.txt | 48 +++++++++
> drivers/extcon/extcon-gpio.c | 109 +++++++++++++++++----
> include/dt-bindings/extcon/extcon.h | 47 +++++++++
> include/linux/extcon/extcon-gpio.h | 8 +-
> 4 files changed, 189 insertions(+), 23 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> create mode 100644 include/dt-bindings/extcon/extcon.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..81f7932
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> @@ -0,0 +1,48 @@
> +GPIO Extcon device
> +
> +This is a virtual device used to generate the specific cable states from the
> +GPIO pin.
> +
> +Required properties:
> +- compatible: Must be "extcon-gpio".
> +- extcon-id: The extcon support the various type of external connector to check
> + whether connector is attached or detached. The each external connector has
> + the unique number to identify it. So this property includes the unique number
> + which indicates the specific external connector. When external connector is
> + attached or detached, GPIO pin detect the changed state. See include/
> + dt-bindings/extcon/extcon.h which defines the unique number for supported
> + external connector from extcon framework.
> +- gpios: GPIO pin to detect the external connector. See gpio binding.
> +
> +Optional properties:
> +- debounce-ms: the debounce dealy for GPIO pin in millisecond.
> +- wakeup-source: Boolean, extcon can wake-up the system from suspend.
> + if gpio provided in extcon-gpio DT node is registered as interrupt,
> + then extcon can wake-up the system from suspend if wakeup-source property
> + is available in DT node, if gpio registered as interrupt but wakeup-source
> + is not available in DT node, then system wake-up due to extcon events
> + not supported.
> +
> +Example: Examples of extcon-gpio node as listed below:
> +
> + usb_cable: extcon-gpio-0 {
> + compatible = "extcon-gpio";
> + extcon-id = <EXTCON_USB>;
> + extcon-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
> + }
> +
> + ta_cable: extcon-gpio-1 {
> + compatible = "extcon-gpio";
> + extcon-id = <EXTCON_CHG_USB_DCP>;
> + extcon-gpio = <&gpio3 2 GPIO_ACTIVE_LOW>;
> + debounce-ms = <50>; /* 50 millisecond */
> + wakeup-source;
> + }
> +
> + &dwc3_usb {
> + extcon = <&usb_cable>;
> + };
> +
> + &charger {
> + extcon = <&ta_cable>;
> + };
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index d023789..592f395 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -1,11 +1,9 @@
> /*
> * extcon_gpio.c - Single-state GPIO extcon driver based on extcon class
> *
> - * Copyright (C) 2008 Google, Inc.
> - * Author: Mike Lockwood <lockwood@android.com>
> - *
> - * Modified by MyungJoo Ham <myungjoo.ham@samsung.com> to support extcon
> - * (originally switch class is supported)
> + * Copyright (C) 2016 Chanwoo Choi <cw00.choi@samsung.com>, Samsung Electronics
> + * Copyright (C) 2012 MyungJoo Ham <myungjoo.ham@samsung.com>, Samsung Electronics
> + * Copyright (C) 2008 Mike Lockwood <lockwood@android.com>, Google, Inc.
> *
> * This software is licensed under the terms of the GNU General Public
> * License version 2, as published by the Free Software Foundation, and
> @@ -25,13 +23,17 @@
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
> #include <linux/workqueue.h>
>
> struct gpio_extcon_data {
> struct extcon_dev *edev;
> int irq;
> + bool irq_wakeup;
> struct delayed_work work;
> unsigned long debounce_jiffies;
>
> @@ -49,7 +51,7 @@ static void gpio_extcon_work(struct work_struct *work)
> state = gpiod_get_value_cansleep(data->id_gpiod);
> if (data->pdata->gpio_active_low)
> state = !state;
> - extcon_set_state(data->edev, state);
> + extcon_set_cable_state_(data->edev, data->pdata->extcon_id, state);
> }
>
> static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
> @@ -61,6 +63,42 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static int gpio_extcon_parse_of(struct platform_device *pdev,
> + struct gpio_extcon_data *data)
> +{
> + struct gpio_extcon_pdata *pdata;
> + struct device_node *np = pdev->dev.of_node;
> + enum of_gpio_flags flags;
> + int ret;
> +
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + ret = device_property_read_u32(&pdev->dev, "extcon-id",
> + &pdata->extcon_id);
> + if (ret < 0)
> + return -EINVAL;
> +
> + pdata->gpio = of_get_named_gpio_flags(np, "gpios", 0, &flags);
> + if (pdata->gpio < 0)
> + return -EINVAL;
> +
> + if (flags & OF_GPIO_ACTIVE_LOW)
> + pdata->gpio_active_low = true;
> +
> + data->irq_wakeup = device_property_read_bool(&pdev->dev,
> + "wakeup-source");
> +
> + device_property_read_u32(&pdev->dev, "debounce-ms", &pdata->debounce);
> +
> + pdata->irq_flags = (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
> + | IRQF_ONESHOT);
> +
> + data->pdata = pdata;
> + return 0;
> +}
> +
> static int gpio_extcon_init(struct device *dev, struct gpio_extcon_data *data)
> {
> struct gpio_extcon_pdata *pdata = data->pdata;
> @@ -96,16 +134,20 @@ static int gpio_extcon_probe(struct platform_device *pdev)
> struct gpio_extcon_data *data;
> int ret;
>
> - if (!pdata)
> - return -EBUSY;
> - if (!pdata->irq_flags || pdata->extcon_id > EXTCON_NONE)
> - return -EINVAL;
> -
> - data = devm_kzalloc(&pdev->dev, sizeof(struct gpio_extcon_data),
> - GFP_KERNEL);
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
> - data->pdata = pdata;
> +
> + if (!pdata) {
> + ret = gpio_extcon_parse_of(pdev, data);
> + if (ret < 0)
> + return ret;
> + } else {
> + data->pdata = pdata;
> + }
> +
> + if (!data->pdata->irq_flags || data->pdata->extcon_id == EXTCON_NONE)
> + return -EINVAL;
>
> /* Initialize the gpio */
> ret = gpio_extcon_init(&pdev->dev, data);
> @@ -113,7 +155,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
> return ret;
>
> /* Allocate the memory of extcon devie and register extcon device */
> - data->edev = devm_extcon_dev_allocate(&pdev->dev, &pdata->extcon_id);
> + data->edev = devm_extcon_dev_allocate(&pdev->dev,
> + &data->pdata->extcon_id);
> if (IS_ERR(data->edev)) {
> dev_err(&pdev->dev, "failed to allocate extcon device\n");
> return -ENOMEM;
> @@ -130,7 +173,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
> * is attached or detached.
> */
> ret = devm_request_any_context_irq(&pdev->dev, data->irq,
> - gpio_irq_handler, pdata->irq_flags,
> + gpio_irq_handler,
> + data->pdata->irq_flags,
> pdev->name, data);
> if (ret < 0)
> return ret;
> @@ -139,6 +183,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
> /* Perform initial detection */
> gpio_extcon_work(&data->work.work);
>
> + if (data->irq_wakeup)
> + device_init_wakeup(&pdev->dev, data->irq_wakeup);
> return 0;
> }
>
> @@ -152,11 +198,23 @@ static int gpio_extcon_remove(struct platform_device *pdev)
> }
>
> #ifdef CONFIG_PM_SLEEP
> +static int gpio_extcon_suspend(struct device *dev)
> +{
> + struct gpio_extcon_data *data = dev_get_drvdata(dev);
> +
> + if (data->irq_wakeup)
> + enable_irq_wake(data->irq);
> +
> + return 0;
> +}
> +
> static int gpio_extcon_resume(struct device *dev)
> {
> - struct gpio_extcon_data *data;
> + struct gpio_extcon_data *data = dev_get_drvdata(dev);
> +
> + if (data->irq_wakeup)
> + disable_irq_wake(data->irq);
>
> - data = dev_get_drvdata(dev);
> if (data->pdata->check_on_resume)
> queue_delayed_work(system_power_efficient_wq,
> &data->work, data->debounce_jiffies);
> @@ -165,7 +223,18 @@ static int gpio_extcon_resume(struct device *dev)
> }
> #endif
>
> -static SIMPLE_DEV_PM_OPS(gpio_extcon_pm_ops, NULL, gpio_extcon_resume);
> +#if defined(CONFIG_OF)
> +static const struct of_device_id gpio_extcon_of_match[] = {
> + { .compatible = "extcon-gpio", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, gpio_extcon_of_match);
> +#else
> +#define gpio_extcon_of_match NULL
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(gpio_extcon_pm_ops,
> + gpio_extcon_suspend, gpio_extcon_resume);
>
> static struct platform_driver gpio_extcon_driver = {
> .probe = gpio_extcon_probe,
> @@ -173,11 +242,13 @@ static struct platform_driver gpio_extcon_driver = {
> .driver = {
> .name = "extcon-gpio",
> .pm = &gpio_extcon_pm_ops,
> + .of_match_table = gpio_extcon_of_match,
> },
> };
>
> module_platform_driver(gpio_extcon_driver);
>
> +MODULE_AUTHOR("Chanwoo Choi <cw00.choi@samsung.com>");
> MODULE_AUTHOR("Mike Lockwood <lockwood@android.com>");
> MODULE_DESCRIPTION("GPIO extcon driver");
> MODULE_LICENSE("GPL");
> diff --git a/include/dt-bindings/extcon/extcon.h b/include/dt-bindings/extcon/extcon.h
> new file mode 100644
> index 0000000..dbba588
> --- /dev/null
> +++ b/include/dt-bindings/extcon/extcon.h
> @@ -0,0 +1,47 @@
> +/*
> + * This header provides constants for most extcon bindings.
> + *
> + * Most extcon bindings include the unique identification format.
> + * In most cases, the unique id format uses the standard values/macro
> + * defined in this header.
> + */
> +
> +#ifndef _DT_BINDINGS_EXTCON_EXTCON_H
> +#define _DT_BINDINGS_EXTCON_EXTCON_H
> +
> +/* USB external connector */
> +#define EXTCON_USB 1
> +#define EXTCON_USB_HOST 2
> +
> +/* Charging external connector */
> +#define EXTCON_CHG_USB_SDP 5 /* Standard Downstream Port */
> +#define EXTCON_CHG_USB_DCP 6 /* Dedicated Charging Port */
> +#define EXTCON_CHG_USB_CDP 7 /* Charging Downstream Port */
> +#define EXTCON_CHG_USB_ACA 8 /* Accessory Charger Adapter */
> +#define EXTCON_CHG_USB_FAST 9
> +#define EXTCON_CHG_USB_SLOW 10
> +
> +/* Jack external connector */
> +#define EXTCON_JACK_MICROPHONE 20
> +#define EXTCON_JACK_HEADPHONE 21
> +#define EXTCON_JACK_LINE_IN 22
> +#define EXTCON_JACK_LINE_OUT 23
> +#define EXTCON_JACK_VIDEO_IN 24
> +#define EXTCON_JACK_VIDEO_OUT 25
> +#define EXTCON_JACK_SPDIF_IN 26 /* Sony Philips Digital InterFace */
> +#define EXTCON_JACK_SPDIF_OUT 27
> +
> +/* Display external connector */
> +#define EXTCON_DISP_HDMI 40 /* High-Definition Multimedia Interface */
> +#define EXTCON_DISP_MHL 41 /* Mobile High-Definition Link */
> +#define EXTCON_DISP_DVI 42 /* Digital Visual Interface */
> +#define EXTCON_DISP_VGA 43 /* Video Graphics Array */
> +
> +/* Miscellaneous external connector */
> +#define EXTCON_DOCK 60
> +#define EXTCON_JIG 61
> +#define EXTCON_MECHANICAL 62
> +
> +#define EXTCON_NUM 63
> +
> +#endif /* _DT_BINDINGS_EXTCON_EXTCON_H */
> diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
> index 7cacafb..f7e7673 100644
> --- a/include/linux/extcon/extcon-gpio.h
> +++ b/include/linux/extcon/extcon-gpio.h
> @@ -1,8 +1,8 @@
> /*
> * Single-state GPIO extcon driver based on extcon class
> *
> - * Copyright (C) 2012 Samsung Electronics
> - * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
> + * Copyright (C) 2016 Chanwoo Choi <cw00.choi@samsung.com>, Samsung Electronics
> + * Copyright (C) 2012 MyungJoo Ham <myungjoo.ham@samsung.com>, Samsung Electronics
> *
> * based on switch class driver
> * Copyright (C) 2008 Google, Inc.
> @@ -35,10 +35,10 @@
> * while resuming from sleep.
> */
> struct gpio_extcon_pdata {
> - unsigned int extcon_id;
> + u32 extcon_id;
> unsigned gpio;
> bool gpio_active_low;
> - unsigned long debounce;
> + u32 debounce;
> unsigned long irq_flags;
>
> bool check_on_resume;
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-26 13:21 ` Chanwoo Choi
@ 2016-05-27 5:13 ` Venkat Reddy Talla
2016-05-31 7:13 ` Chanwoo Choi
0 siblings, 1 reply; 14+ messages in thread
From: Venkat Reddy Talla @ 2016-05-27 5:13 UTC (permalink / raw)
To: cw00.choi
Cc: MyungJoo Ham, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, devicetree, Kumar Gala, linux-kernel,
Laxman Dewangan
Hi Choi,
Sorry for changing author, will update author field with your name.
Regarding Rob Herring comments, You had already replied.
I felt separate compatible for each external connector is not required,
as client driver can detect the type of external cable(sdp,dcp, microphone) on receiving notification from extcon provider,
I have also added more description for wakeup-source.
Do you see any other changes required on top of v4 patch?
Regards,
Venkat
> -----Original Message-----
> From: Chanwoo Choi [mailto:cwchoi00@gmail.com]
> Sent: Thursday, May 26, 2016 6:52 PM
> To: Venkat Reddy Talla
> Cc: MyungJoo Ham; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell;
> devicetree; Kumar Gala; linux-kernel; Laxman Dewangan
> Subject: Re: [PATCH v4] extcon: gpio: Add the support for Device tree
> bindings
>
> Hi Venkat,
>
> There is some miscommunication. On previous my reply, I don't mean that
> the author of the patch[1] is changed from me to you.
>
> I'd like you to remain the original author(me) for the patch[1] without
> changing the author.
> [1] https://lkml.org/lkml/2015/10/21/8
> - [PATCH v3] extcon: gpio: Add the support for Device tree bindings
>
> You can use the patch[1] as based patch and you can add new feature on
> base patch[1]. Also, If you ok, you can modify the extccon-gpio.c driver as
> Rob comment[2].
>
> But, Rob Herring gave me the some comment[2].
> [2] https://lkml.org/lkml/2015/10/21/906
>
>
> Thanks,
> Chanwoo Choi
>
>
> On Thu, May 26, 2016 at 8:47 PM, Venkat Reddy Talla
> <vreddytalla@nvidia.com> wrote:
> > Add the support for Device tree bindings of extcon-gpio driver.
> > The extcon-gpio device tree node must include the both 'extcon-id' and
> > 'gpios' property.
> >
> > For example:
> > usb_cable: extcon-gpio-0 {
> > compatible = "extcon-gpio";
> > extcon-id = <EXTCON_USB>;
> > gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
> > }
> > ta_cable: extcon-gpio-1 {
> > compatible = "extcon-gpio";
> > extcon-id = <EXTCON_CHG_USB_DCP>;
> > gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
> > debounce-ms = <50>; /* 50 millisecond */
> > wakeup-source;
> > }
> > &dwc3_usb {
> > extcon = <&usb_cable>;
> > };
> > &charger {
> > extcon = <&ta_cable>;
> > };
> >
> > Signed-off-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
> > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >
> > ---
> > changes from v3:
> > - add description for wakeup-source in documentation
> > - change dts property extcon-gpio name to gpios
> > - use of_get_named_gpio_flags to get gpio number and flags Changes
> > from v2:
> > - Add the more description for 'extcon-id' property in documentation
> > Changes from v1:
> > - Create the include/dt-bindings/extcon/extcon.h including the
> > identification of external connector. These definitions are used in dts file.
> > - Fix error if CONFIG_OF is disabled.
> > - Add signed-off tag by Myungjoo Ham
> > ---
> > ---
> > .../devicetree/bindings/extcon/extcon-gpio.txt | 48 +++++++++
> > drivers/extcon/extcon-gpio.c | 109 +++++++++++++++++----
> > include/dt-bindings/extcon/extcon.h | 47 +++++++++
> > include/linux/extcon/extcon-gpio.h | 8 +-
> > 4 files changed, 189 insertions(+), 23 deletions(-) create mode
> > 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > create mode 100644 include/dt-bindings/extcon/extcon.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..81f7932
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > @@ -0,0 +1,48 @@
> > +GPIO Extcon device
> > +
> > +This is a virtual device used to generate the specific cable states
> > +from the GPIO pin.
> > +
> > +Required properties:
> > +- compatible: Must be "extcon-gpio".
> > +- extcon-id: The extcon support the various type of external
> > +connector to check
> > + whether connector is attached or detached. The each external
> > +connector has
> > + the unique number to identify it. So this property includes the
> > +unique number
> > + which indicates the specific external connector. When external
> > +connector is
> > + attached or detached, GPIO pin detect the changed state. See
> > +include/
> > + dt-bindings/extcon/extcon.h which defines the unique number for
> > +supported
> > + external connector from extcon framework.
> > +- gpios: GPIO pin to detect the external connector. See gpio binding.
> > +
> > +Optional properties:
> > +- debounce-ms: the debounce dealy for GPIO pin in millisecond.
> > +- wakeup-source: Boolean, extcon can wake-up the system from
> suspend.
> > + if gpio provided in extcon-gpio DT node is registered as interrupt,
> > + then extcon can wake-up the system from suspend if wakeup-source
> > +property
> > + is available in DT node, if gpio registered as interrupt but
> > +wakeup-source
> > + is not available in DT node, then system wake-up due to extcon
> > +events
> > + not supported.
> > +
> > +Example: Examples of extcon-gpio node as listed below:
> > +
> > + usb_cable: extcon-gpio-0 {
> > + compatible = "extcon-gpio";
> > + extcon-id = <EXTCON_USB>;
> > + extcon-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
> > + }
> > +
> > + ta_cable: extcon-gpio-1 {
> > + compatible = "extcon-gpio";
> > + extcon-id = <EXTCON_CHG_USB_DCP>;
> > + extcon-gpio = <&gpio3 2 GPIO_ACTIVE_LOW>;
> > + debounce-ms = <50>; /* 50 millisecond */
> > + wakeup-source;
> > + }
> > +
> > + &dwc3_usb {
> > + extcon = <&usb_cable>;
> > + };
> > +
> > + &charger {
> > + extcon = <&ta_cable>;
> > + };
> > diff --git a/drivers/extcon/extcon-gpio.c
> > b/drivers/extcon/extcon-gpio.c index d023789..592f395 100644
> > --- a/drivers/extcon/extcon-gpio.c
> > +++ b/drivers/extcon/extcon-gpio.c
> > @@ -1,11 +1,9 @@
> > /*
> > * extcon_gpio.c - Single-state GPIO extcon driver based on extcon class
> > *
> > - * Copyright (C) 2008 Google, Inc.
> > - * Author: Mike Lockwood <lockwood@android.com>
> > - *
> > - * Modified by MyungJoo Ham <myungjoo.ham@samsung.com> to
> support
> > extcon
> > - * (originally switch class is supported)
> > + * Copyright (C) 2016 Chanwoo Choi <cw00.choi@samsung.com>,
> Samsung
> > + Electronics
> > + * Copyright (C) 2012 MyungJoo Ham <myungjoo.ham@samsung.com>,
> > + Samsung Electronics
> > + * Copyright (C) 2008 Mike Lockwood <lockwood@android.com>, Google,
> Inc.
> > *
> > * This software is licensed under the terms of the GNU General Public
> > * License version 2, as published by the Free Software Foundation,
> > and @@ -25,13 +23,17 @@ #include <linux/interrupt.h> #include
> > <linux/kernel.h> #include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> > #include <linux/workqueue.h>
> >
> > struct gpio_extcon_data {
> > struct extcon_dev *edev;
> > int irq;
> > + bool irq_wakeup;
> > struct delayed_work work;
> > unsigned long debounce_jiffies;
> >
> > @@ -49,7 +51,7 @@ static void gpio_extcon_work(struct work_struct
> *work)
> > state = gpiod_get_value_cansleep(data->id_gpiod);
> > if (data->pdata->gpio_active_low)
> > state = !state;
> > - extcon_set_state(data->edev, state);
> > + extcon_set_cable_state_(data->edev, data->pdata->extcon_id,
> > + state);
> > }
> >
> > static irqreturn_t gpio_irq_handler(int irq, void *dev_id) @@ -61,6
> > +63,42 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
> > return IRQ_HANDLED;
> > }
> >
> > +static int gpio_extcon_parse_of(struct platform_device *pdev,
> > + struct gpio_extcon_data *data) {
> > + struct gpio_extcon_pdata *pdata;
> > + struct device_node *np = pdev->dev.of_node;
> > + enum of_gpio_flags flags;
> > + int ret;
> > +
> > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + ret = device_property_read_u32(&pdev->dev, "extcon-id",
> > + &pdata->extcon_id);
> > + if (ret < 0)
> > + return -EINVAL;
> > +
> > + pdata->gpio = of_get_named_gpio_flags(np, "gpios", 0, &flags);
> > + if (pdata->gpio < 0)
> > + return -EINVAL;
> > +
> > + if (flags & OF_GPIO_ACTIVE_LOW)
> > + pdata->gpio_active_low = true;
> > +
> > + data->irq_wakeup = device_property_read_bool(&pdev->dev,
> > + "wakeup-source");
> > +
> > + device_property_read_u32(&pdev->dev, "debounce-ms",
> > + &pdata->debounce);
> > +
> > + pdata->irq_flags = (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
> > + | IRQF_ONESHOT);
> > +
> > + data->pdata = pdata;
> > + return 0;
> > +}
> > +
> > static int gpio_extcon_init(struct device *dev, struct
> > gpio_extcon_data *data) {
> > struct gpio_extcon_pdata *pdata = data->pdata; @@ -96,16
> > +134,20 @@ static int gpio_extcon_probe(struct platform_device *pdev)
> > struct gpio_extcon_data *data;
> > int ret;
> >
> > - if (!pdata)
> > - return -EBUSY;
> > - if (!pdata->irq_flags || pdata->extcon_id > EXTCON_NONE)
> > - return -EINVAL;
> > -
> > - data = devm_kzalloc(&pdev->dev, sizeof(struct gpio_extcon_data),
> > - GFP_KERNEL);
> > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > if (!data)
> > return -ENOMEM;
> > - data->pdata = pdata;
> > +
> > + if (!pdata) {
> > + ret = gpio_extcon_parse_of(pdev, data);
> > + if (ret < 0)
> > + return ret;
> > + } else {
> > + data->pdata = pdata;
> > + }
> > +
> > + if (!data->pdata->irq_flags || data->pdata->extcon_id ==
> EXTCON_NONE)
> > + return -EINVAL;
> >
> > /* Initialize the gpio */
> > ret = gpio_extcon_init(&pdev->dev, data); @@ -113,7 +155,8 @@
> > static int gpio_extcon_probe(struct platform_device *pdev)
> > return ret;
> >
> > /* Allocate the memory of extcon devie and register extcon device */
> > - data->edev = devm_extcon_dev_allocate(&pdev->dev, &pdata-
> >extcon_id);
> > + data->edev = devm_extcon_dev_allocate(&pdev->dev,
> > +
> > + &data->pdata->extcon_id);
> > if (IS_ERR(data->edev)) {
> > dev_err(&pdev->dev, "failed to allocate extcon device\n");
> > return -ENOMEM;
> > @@ -130,7 +173,8 @@ static int gpio_extcon_probe(struct
> platform_device *pdev)
> > * is attached or detached.
> > */
> > ret = devm_request_any_context_irq(&pdev->dev, data->irq,
> > - gpio_irq_handler, pdata->irq_flags,
> > + gpio_irq_handler,
> > + data->pdata->irq_flags,
> > pdev->name, data);
> > if (ret < 0)
> > return ret;
> > @@ -139,6 +183,8 @@ static int gpio_extcon_probe(struct
> platform_device *pdev)
> > /* Perform initial detection */
> > gpio_extcon_work(&data->work.work);
> >
> > + if (data->irq_wakeup)
> > + device_init_wakeup(&pdev->dev, data->irq_wakeup);
> > return 0;
> > }
> >
> > @@ -152,11 +198,23 @@ static int gpio_extcon_remove(struct
> > platform_device *pdev) }
> >
> > #ifdef CONFIG_PM_SLEEP
> > +static int gpio_extcon_suspend(struct device *dev) {
> > + struct gpio_extcon_data *data = dev_get_drvdata(dev);
> > +
> > + if (data->irq_wakeup)
> > + enable_irq_wake(data->irq);
> > +
> > + return 0;
> > +}
> > +
> > static int gpio_extcon_resume(struct device *dev) {
> > - struct gpio_extcon_data *data;
> > + struct gpio_extcon_data *data = dev_get_drvdata(dev);
> > +
> > + if (data->irq_wakeup)
> > + disable_irq_wake(data->irq);
> >
> > - data = dev_get_drvdata(dev);
> > if (data->pdata->check_on_resume)
> > queue_delayed_work(system_power_efficient_wq,
> > &data->work, data->debounce_jiffies); @@
> > -165,7 +223,18 @@ static int gpio_extcon_resume(struct device *dev) }
> > #endif
> >
> > -static SIMPLE_DEV_PM_OPS(gpio_extcon_pm_ops, NULL,
> > gpio_extcon_resume);
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id gpio_extcon_of_match[] = {
> > + { .compatible = "extcon-gpio", },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, gpio_extcon_of_match); #else
> > +#define gpio_extcon_of_match NULL
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(gpio_extcon_pm_ops,
> > + gpio_extcon_suspend, gpio_extcon_resume);
> >
> > static struct platform_driver gpio_extcon_driver = {
> > .probe = gpio_extcon_probe,
> > @@ -173,11 +242,13 @@ static struct platform_driver gpio_extcon_driver =
> {
> > .driver = {
> > .name = "extcon-gpio",
> > .pm = &gpio_extcon_pm_ops,
> > + .of_match_table = gpio_extcon_of_match,
> > },
> > };
> >
> > module_platform_driver(gpio_extcon_driver);
> >
> > +MODULE_AUTHOR("Chanwoo Choi <cw00.choi@samsung.com>");
> > MODULE_AUTHOR("Mike Lockwood <lockwood@android.com>");
> > MODULE_DESCRIPTION("GPIO extcon driver"); MODULE_LICENSE("GPL");
> diff
> > --git a/include/dt-bindings/extcon/extcon.h
> > b/include/dt-bindings/extcon/extcon.h
> > new file mode 100644
> > index 0000000..dbba588
> > --- /dev/null
> > +++ b/include/dt-bindings/extcon/extcon.h
> > @@ -0,0 +1,47 @@
> > +/*
> > + * This header provides constants for most extcon bindings.
> > + *
> > + * Most extcon bindings include the unique identification format.
> > + * In most cases, the unique id format uses the standard values/macro
> > + * defined in this header.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_EXTCON_EXTCON_H
> > +#define _DT_BINDINGS_EXTCON_EXTCON_H
> > +
> > +/* USB external connector */
> > +#define EXTCON_USB 1
> > +#define EXTCON_USB_HOST 2
> > +
> > +/* Charging external connector */
> > +#define EXTCON_CHG_USB_SDP 5 /* Standard Downstream Port */
> > +#define EXTCON_CHG_USB_DCP 6 /* Dedicated Charging Port */
> > +#define EXTCON_CHG_USB_CDP 7 /* Charging Downstream Port */
> > +#define EXTCON_CHG_USB_ACA 8 /* Accessory Charger Adapter */
> > +#define EXTCON_CHG_USB_FAST 9
> > +#define EXTCON_CHG_USB_SLOW 10
> > +
> > +/* Jack external connector */
> > +#define EXTCON_JACK_MICROPHONE 20
> > +#define EXTCON_JACK_HEADPHONE 21
> > +#define EXTCON_JACK_LINE_IN 22
> > +#define EXTCON_JACK_LINE_OUT 23
> > +#define EXTCON_JACK_VIDEO_IN 24
> > +#define EXTCON_JACK_VIDEO_OUT 25
> > +#define EXTCON_JACK_SPDIF_IN 26 /* Sony Philips Digital InterFace
> */
> > +#define EXTCON_JACK_SPDIF_OUT 27
> > +
> > +/* Display external connector */
> > +#define EXTCON_DISP_HDMI 40 /* High-Definition Multimedia
> Interface */
> > +#define EXTCON_DISP_MHL 41 /* Mobile High-Definition Link
> */
> > +#define EXTCON_DISP_DVI 42 /* Digital Visual Interface */
> > +#define EXTCON_DISP_VGA 43 /* Video Graphics Array */
> > +
> > +/* Miscellaneous external connector */
> > +#define EXTCON_DOCK 60
> > +#define EXTCON_JIG 61
> > +#define EXTCON_MECHANICAL 62
> > +
> > +#define EXTCON_NUM 63
> > +
> > +#endif /* _DT_BINDINGS_EXTCON_EXTCON_H */
> > diff --git a/include/linux/extcon/extcon-gpio.h
> > b/include/linux/extcon/extcon-gpio.h
> > index 7cacafb..f7e7673 100644
> > --- a/include/linux/extcon/extcon-gpio.h
> > +++ b/include/linux/extcon/extcon-gpio.h
> > @@ -1,8 +1,8 @@
> > /*
> > * Single-state GPIO extcon driver based on extcon class
> > *
> > - * Copyright (C) 2012 Samsung Electronics
> > - * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
> > + * Copyright (C) 2016 Chanwoo Choi <cw00.choi@samsung.com>,
> Samsung
> > + Electronics
> > + * Copyright (C) 2012 MyungJoo Ham <myungjoo.ham@samsung.com>,
> > + Samsung Electronics
> > *
> > * based on switch class driver
> > * Copyright (C) 2008 Google, Inc.
> > @@ -35,10 +35,10 @@
> > * while resuming from sleep.
> > */
> > struct gpio_extcon_pdata {
> > - unsigned int extcon_id;
> > + u32 extcon_id;
> > unsigned gpio;
> > bool gpio_active_low;
> > - unsigned long debounce;
> > + u32 debounce;
> > unsigned long irq_flags;
> >
> > bool check_on_resume;
> > --
> > 2.1.4
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-26 11:47 [PATCH v4] extcon: gpio: Add the support for Device tree bindings Venkat Reddy Talla
2016-05-26 13:21 ` Chanwoo Choi
@ 2016-05-27 15:29 ` Rob Herring
2016-05-31 6:44 ` Chanwoo Choi
1 sibling, 1 reply; 14+ messages in thread
From: Rob Herring @ 2016-05-27 15:29 UTC (permalink / raw)
To: Venkat Reddy Talla
Cc: MyungJoo Ham, Chanwoo Choi, Pawel Moll, Mark Rutland,
Ian Campbell, devicetree, Kumar Gala, linux-kernel,
Laxman Dewangan
On Thu, May 26, 2016 at 05:17:45PM +0530, Venkat Reddy Talla wrote:
> Add the support for Device tree bindings of extcon-gpio driver.
> The extcon-gpio device tree node must include the both 'extcon-id' and
> 'gpios' property.
I think extcon bindings are a mess in general...
> For example:
> usb_cable: extcon-gpio-0 {
> compatible = "extcon-gpio";
> extcon-id = <EXTCON_USB>;
> gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
> }
> ta_cable: extcon-gpio-1 {
> compatible = "extcon-gpio";
> extcon-id = <EXTCON_CHG_USB_DCP>;
> gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
> debounce-ms = <50>; /* 50 millisecond */
> wakeup-source;
> }
This is all 1 logical connector, the USB connector. Why are you
describing cables? Those are not part of the h/w and are dynamic.
Describe this as a connector which is one thing (i.e. node). Use a
compatible string that reflects the type of connector
(usb-microab-connector), not the driver you want to use. Then define
GPIO lines needed to provide state information like VBus, ID, charger
modes and control lines like soft connect (D+ pullup enable), VBus
enable, etc.
> &dwc3_usb {
> extcon = <&usb_cable>;
> };
> &charger {
> extcon = <&ta_cable>;
> };
Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-27 15:29 ` Rob Herring
@ 2016-05-31 6:44 ` Chanwoo Choi
2016-05-31 7:35 ` Chanwoo Choi
0 siblings, 1 reply; 14+ messages in thread
From: Chanwoo Choi @ 2016-05-31 6:44 UTC (permalink / raw)
To: Rob Herring, Venkat Reddy Talla
Cc: MyungJoo Ham, Pawel Moll, Mark Rutland, Ian Campbell, devicetree,
Kumar Gala, linux-kernel, Laxman Dewangan
On 2016년 05월 28일 00:29, Rob Herring wrote:
> On Thu, May 26, 2016 at 05:17:45PM +0530, Venkat Reddy Talla wrote:
>> Add the support for Device tree bindings of extcon-gpio driver.
>> The extcon-gpio device tree node must include the both 'extcon-id' and
>> 'gpios' property.
>
> I think extcon bindings are a mess in general...
>
>> For example:
>> usb_cable: extcon-gpio-0 {
>> compatible = "extcon-gpio";
>> extcon-id = <EXTCON_USB>;
>> gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
>> }
>> ta_cable: extcon-gpio-1 {
>> compatible = "extcon-gpio";
>> extcon-id = <EXTCON_CHG_USB_DCP>;
>> gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
>> debounce-ms = <50>; /* 50 millisecond */
>> wakeup-source;
>> }
>
> This is all 1 logical connector, the USB connector. Why are you
> describing cables? Those are not part of the h/w and are dynamic.
> Describe this as a connector which is one thing (i.e. node). Use a
> compatible string that reflects the type of connector
> (usb-microab-connector), not the driver you want to use. Then define
> GPIO lines needed to provide state information like VBus, ID, charger
> modes and control lines like soft connect (D+ pullup enable), VBus
> enable, etc.
You're right. The extcon-gpio driver will not use the "extcon-gpio" raw compatible.
As you commented[1], the each connector will have the unique name to use the extcon-gpio.c driver.
[1] https://lkml.org/lkml/2015/10/21/906
For example,
The extcon-gpio.c driver may have the different name including the h/w information
according to the kind of external connector.
static const struct of_device_id gpio_extcon_of_match[] = {
{
.compatible = "extcon-chg-sdp", /* SDP charger connector */
.data = EXTCON_CHG_SDP_DATA,
}, {
.compatible = "extcon-chg-dcp", /* DCP charger connector */
.data = EXTCON_CHG_DCP_DATA,
}, {
.compatible = "extcon-jack-microphone", /* Microphone jack connector */
.data = EXTCON_JACK_MICROPHONE_DATA,
}, {
.compatible = "extcon-disp-hdmi", /* HDMI connector*/
.data = EXTCON_DISP_HDMI_DATA,
},
......
};
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-27 5:13 ` Venkat Reddy Talla
@ 2016-05-31 7:13 ` Chanwoo Choi
0 siblings, 0 replies; 14+ messages in thread
From: Chanwoo Choi @ 2016-05-31 7:13 UTC (permalink / raw)
To: Venkat Reddy Talla
Cc: MyungJoo Ham, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, devicetree, Kumar Gala, linux-kernel,
Laxman Dewangan
Hi Venkat,
On 2016년 05월 27일 14:13, Venkat Reddy Talla wrote:
> Hi Choi,
>
> Sorry for changing author, will update author field with your name.
>
> Regarding Rob Herring comments, You had already replied.
Rob gave some comment which the compatible string of extcon-gpio.c should
include the type of external connector. I replied about it [1].
[1] https://lkml.org/lkml/2016/5/31/109
>
> I felt separate compatible for each external connector is not required,
> as client driver can detect the type of external cable(sdp,dcp, microphone) on receiving notification from extcon provider,
You're right about the operation point of view.
But, As Rob gave some comment, The device-tree should include the device information
instead of driver information. The 'extcon-gpio' compatible mean the only driver
without h/w information.
I think that there is more discussion how to
decide the compatible name of extcon-gpio.c driver.
> I have also added more description for wakeup-source.
OK.
>
> Do you see any other changes required on top of v4 patch?
Regards,
Chanwoo Choi
>
> Regards,
> Venkat
>
>> -----Original Message-----
>> From: Chanwoo Choi [mailto:cwchoi00@gmail.com]
>> Sent: Thursday, May 26, 2016 6:52 PM
>> To: Venkat Reddy Talla
>> Cc: MyungJoo Ham; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell;
>> devicetree; Kumar Gala; linux-kernel; Laxman Dewangan
>> Subject: Re: [PATCH v4] extcon: gpio: Add the support for Device tree
>> bindings
>>
>> Hi Venkat,
>>
>> There is some miscommunication. On previous my reply, I don't mean that
>> the author of the patch[1] is changed from me to you.
>>
>> I'd like you to remain the original author(me) for the patch[1] without
>> changing the author.
>> [1] https://lkml.org/lkml/2015/10/21/8
>> - [PATCH v3] extcon: gpio: Add the support for Device tree bindings
>>
>> You can use the patch[1] as based patch and you can add new feature on
>> base patch[1]. Also, If you ok, you can modify the extccon-gpio.c driver as
>> Rob comment[2].
>>
>> But, Rob Herring gave me the some comment[2].
>> [2] https://lkml.org/lkml/2015/10/21/906
>>
>>
>> Thanks,
>> Chanwoo Choi
>>
>>
>> On Thu, May 26, 2016 at 8:47 PM, Venkat Reddy Talla
>> <vreddytalla@nvidia.com> wrote:
>>> Add the support for Device tree bindings of extcon-gpio driver.
>>> The extcon-gpio device tree node must include the both 'extcon-id' and
>>> 'gpios' property.
>>>
>>> For example:
>>> usb_cable: extcon-gpio-0 {
>>> compatible = "extcon-gpio";
>>> extcon-id = <EXTCON_USB>;
>>> gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
>>> }
>>> ta_cable: extcon-gpio-1 {
>>> compatible = "extcon-gpio";
>>> extcon-id = <EXTCON_CHG_USB_DCP>;
>>> gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
>>> debounce-ms = <50>; /* 50 millisecond */
>>> wakeup-source;
>>> }
>>> &dwc3_usb {
>>> extcon = <&usb_cable>;
>>> };
>>> &charger {
>>> extcon = <&ta_cable>;
>>> };
>>>
>>> Signed-off-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>>>
>>> ---
>>> changes from v3:
>>> - add description for wakeup-source in documentation
>>> - change dts property extcon-gpio name to gpios
>>> - use of_get_named_gpio_flags to get gpio number and flags Changes
>>> from v2:
>>> - Add the more description for 'extcon-id' property in documentation
>>> Changes from v1:
>>> - Create the include/dt-bindings/extcon/extcon.h including the
>>> identification of external connector. These definitions are used in dts file.
>>> - Fix error if CONFIG_OF is disabled.
>>> - Add signed-off tag by Myungjoo Ham
>>> ---
>>> ---
>>> .../devicetree/bindings/extcon/extcon-gpio.txt | 48 +++++++++
>>> drivers/extcon/extcon-gpio.c | 109 +++++++++++++++++----
>>> include/dt-bindings/extcon/extcon.h | 47 +++++++++
>>> include/linux/extcon/extcon-gpio.h | 8 +-
>>> 4 files changed, 189 insertions(+), 23 deletions(-) create mode
>>> 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
>>> create mode 100644 include/dt-bindings/extcon/extcon.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..81f7932
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
>>> @@ -0,0 +1,48 @@
>>> +GPIO Extcon device
>>> +
>>> +This is a virtual device used to generate the specific cable states
>>> +from the GPIO pin.
>>> +
>>> +Required properties:
>>> +- compatible: Must be "extcon-gpio".
>>> +- extcon-id: The extcon support the various type of external
>>> +connector to check
>>> + whether connector is attached or detached. The each external
>>> +connector has
>>> + the unique number to identify it. So this property includes the
>>> +unique number
>>> + which indicates the specific external connector. When external
>>> +connector is
>>> + attached or detached, GPIO pin detect the changed state. See
>>> +include/
>>> + dt-bindings/extcon/extcon.h which defines the unique number for
>>> +supported
>>> + external connector from extcon framework.
>>> +- gpios: GPIO pin to detect the external connector. See gpio binding.
>>> +
>>> +Optional properties:
>>> +- debounce-ms: the debounce dealy for GPIO pin in millisecond.
>>> +- wakeup-source: Boolean, extcon can wake-up the system from
>> suspend.
>>> + if gpio provided in extcon-gpio DT node is registered as interrupt,
>>> + then extcon can wake-up the system from suspend if wakeup-source
>>> +property
>>> + is available in DT node, if gpio registered as interrupt but
>>> +wakeup-source
>>> + is not available in DT node, then system wake-up due to extcon
>>> +events
>>> + not supported.
>>> +
>>> +Example: Examples of extcon-gpio node as listed below:
>>> +
>>> + usb_cable: extcon-gpio-0 {
>>> + compatible = "extcon-gpio";
>>> + extcon-id = <EXTCON_USB>;
>>> + extcon-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
>>> + }
>>> +
>>> + ta_cable: extcon-gpio-1 {
>>> + compatible = "extcon-gpio";
>>> + extcon-id = <EXTCON_CHG_USB_DCP>;
>>> + extcon-gpio = <&gpio3 2 GPIO_ACTIVE_LOW>;
>>> + debounce-ms = <50>; /* 50 millisecond */
>>> + wakeup-source;
>>> + }
>>> +
>>> + &dwc3_usb {
>>> + extcon = <&usb_cable>;
>>> + };
>>> +
>>> + &charger {
>>> + extcon = <&ta_cable>;
>>> + };
>>> diff --git a/drivers/extcon/extcon-gpio.c
>>> b/drivers/extcon/extcon-gpio.c index d023789..592f395 100644
>>> --- a/drivers/extcon/extcon-gpio.c
>>> +++ b/drivers/extcon/extcon-gpio.c
>>> @@ -1,11 +1,9 @@
>>> /*
>>> * extcon_gpio.c - Single-state GPIO extcon driver based on extcon class
>>> *
>>> - * Copyright (C) 2008 Google, Inc.
>>> - * Author: Mike Lockwood <lockwood@android.com>
>>> - *
>>> - * Modified by MyungJoo Ham <myungjoo.ham@samsung.com> to
>> support
>>> extcon
>>> - * (originally switch class is supported)
>>> + * Copyright (C) 2016 Chanwoo Choi <cw00.choi@samsung.com>,
>> Samsung
>>> + Electronics
>>> + * Copyright (C) 2012 MyungJoo Ham <myungjoo.ham@samsung.com>,
>>> + Samsung Electronics
>>> + * Copyright (C) 2008 Mike Lockwood <lockwood@android.com>, Google,
>> Inc.
>>> *
>>> * This software is licensed under the terms of the GNU General Public
>>> * License version 2, as published by the Free Software Foundation,
>>> and @@ -25,13 +23,17 @@ #include <linux/interrupt.h> #include
>>> <linux/kernel.h> #include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_gpio.h>
>>> #include <linux/platform_device.h>
>>> +#include <linux/property.h>
>>> #include <linux/slab.h>
>>> #include <linux/workqueue.h>
>>>
>>> struct gpio_extcon_data {
>>> struct extcon_dev *edev;
>>> int irq;
>>> + bool irq_wakeup;
>>> struct delayed_work work;
>>> unsigned long debounce_jiffies;
>>>
>>> @@ -49,7 +51,7 @@ static void gpio_extcon_work(struct work_struct
>> *work)
>>> state = gpiod_get_value_cansleep(data->id_gpiod);
>>> if (data->pdata->gpio_active_low)
>>> state = !state;
>>> - extcon_set_state(data->edev, state);
>>> + extcon_set_cable_state_(data->edev, data->pdata->extcon_id,
>>> + state);
>>> }
>>>
>>> static irqreturn_t gpio_irq_handler(int irq, void *dev_id) @@ -61,6
>>> +63,42 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
>>> return IRQ_HANDLED;
>>> }
>>>
>>> +static int gpio_extcon_parse_of(struct platform_device *pdev,
>>> + struct gpio_extcon_data *data) {
>>> + struct gpio_extcon_pdata *pdata;
>>> + struct device_node *np = pdev->dev.of_node;
>>> + enum of_gpio_flags flags;
>>> + int ret;
>>> +
>>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>> + if (!pdata)
>>> + return -ENOMEM;
>>> +
>>> + ret = device_property_read_u32(&pdev->dev, "extcon-id",
>>> + &pdata->extcon_id);
>>> + if (ret < 0)
>>> + return -EINVAL;
>>> +
>>> + pdata->gpio = of_get_named_gpio_flags(np, "gpios", 0, &flags);
>>> + if (pdata->gpio < 0)
>>> + return -EINVAL;
>>> +
>>> + if (flags & OF_GPIO_ACTIVE_LOW)
>>> + pdata->gpio_active_low = true;
>>> +
>>> + data->irq_wakeup = device_property_read_bool(&pdev->dev,
>>> + "wakeup-source");
>>> +
>>> + device_property_read_u32(&pdev->dev, "debounce-ms",
>>> + &pdata->debounce);
>>> +
>>> + pdata->irq_flags = (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
>>> + | IRQF_ONESHOT);
>>> +
>>> + data->pdata = pdata;
>>> + return 0;
>>> +}
>>> +
>>> static int gpio_extcon_init(struct device *dev, struct
>>> gpio_extcon_data *data) {
>>> struct gpio_extcon_pdata *pdata = data->pdata; @@ -96,16
>>> +134,20 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>>> struct gpio_extcon_data *data;
>>> int ret;
>>>
>>> - if (!pdata)
>>> - return -EBUSY;
>>> - if (!pdata->irq_flags || pdata->extcon_id > EXTCON_NONE)
>>> - return -EINVAL;
>>> -
>>> - data = devm_kzalloc(&pdev->dev, sizeof(struct gpio_extcon_data),
>>> - GFP_KERNEL);
>>> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>> if (!data)
>>> return -ENOMEM;
>>> - data->pdata = pdata;
>>> +
>>> + if (!pdata) {
>>> + ret = gpio_extcon_parse_of(pdev, data);
>>> + if (ret < 0)
>>> + return ret;
>>> + } else {
>>> + data->pdata = pdata;
>>> + }
>>> +
>>> + if (!data->pdata->irq_flags || data->pdata->extcon_id ==
>> EXTCON_NONE)
>>> + return -EINVAL;
>>>
>>> /* Initialize the gpio */
>>> ret = gpio_extcon_init(&pdev->dev, data); @@ -113,7 +155,8 @@
>>> static int gpio_extcon_probe(struct platform_device *pdev)
>>> return ret;
>>>
>>> /* Allocate the memory of extcon devie and register extcon device */
>>> - data->edev = devm_extcon_dev_allocate(&pdev->dev, &pdata-
>>> extcon_id);
>>> + data->edev = devm_extcon_dev_allocate(&pdev->dev,
>>> +
>>> + &data->pdata->extcon_id);
>>> if (IS_ERR(data->edev)) {
>>> dev_err(&pdev->dev, "failed to allocate extcon device\n");
>>> return -ENOMEM;
>>> @@ -130,7 +173,8 @@ static int gpio_extcon_probe(struct
>> platform_device *pdev)
>>> * is attached or detached.
>>> */
>>> ret = devm_request_any_context_irq(&pdev->dev, data->irq,
>>> - gpio_irq_handler, pdata->irq_flags,
>>> + gpio_irq_handler,
>>> + data->pdata->irq_flags,
>>> pdev->name, data);
>>> if (ret < 0)
>>> return ret;
>>> @@ -139,6 +183,8 @@ static int gpio_extcon_probe(struct
>> platform_device *pdev)
>>> /* Perform initial detection */
>>> gpio_extcon_work(&data->work.work);
>>>
>>> + if (data->irq_wakeup)
>>> + device_init_wakeup(&pdev->dev, data->irq_wakeup);
>>> return 0;
>>> }
>>>
>>> @@ -152,11 +198,23 @@ static int gpio_extcon_remove(struct
>>> platform_device *pdev) }
>>>
>>> #ifdef CONFIG_PM_SLEEP
>>> +static int gpio_extcon_suspend(struct device *dev) {
>>> + struct gpio_extcon_data *data = dev_get_drvdata(dev);
>>> +
>>> + if (data->irq_wakeup)
>>> + enable_irq_wake(data->irq);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int gpio_extcon_resume(struct device *dev) {
>>> - struct gpio_extcon_data *data;
>>> + struct gpio_extcon_data *data = dev_get_drvdata(dev);
>>> +
>>> + if (data->irq_wakeup)
>>> + disable_irq_wake(data->irq);
>>>
>>> - data = dev_get_drvdata(dev);
>>> if (data->pdata->check_on_resume)
>>> queue_delayed_work(system_power_efficient_wq,
>>> &data->work, data->debounce_jiffies); @@
>>> -165,7 +223,18 @@ static int gpio_extcon_resume(struct device *dev) }
>>> #endif
>>>
>>> -static SIMPLE_DEV_PM_OPS(gpio_extcon_pm_ops, NULL,
>>> gpio_extcon_resume);
>>> +#if defined(CONFIG_OF)
>>> +static const struct of_device_id gpio_extcon_of_match[] = {
>>> + { .compatible = "extcon-gpio", },
>>> + { /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, gpio_extcon_of_match); #else
>>> +#define gpio_extcon_of_match NULL
>>> +#endif
>>> +
>>> +static SIMPLE_DEV_PM_OPS(gpio_extcon_pm_ops,
>>> + gpio_extcon_suspend, gpio_extcon_resume);
>>>
>>> static struct platform_driver gpio_extcon_driver = {
>>> .probe = gpio_extcon_probe,
>>> @@ -173,11 +242,13 @@ static struct platform_driver gpio_extcon_driver =
>> {
>>> .driver = {
>>> .name = "extcon-gpio",
>>> .pm = &gpio_extcon_pm_ops,
>>> + .of_match_table = gpio_extcon_of_match,
>>> },
>>> };
>>>
>>> module_platform_driver(gpio_extcon_driver);
>>>
>>> +MODULE_AUTHOR("Chanwoo Choi <cw00.choi@samsung.com>");
>>> MODULE_AUTHOR("Mike Lockwood <lockwood@android.com>");
>>> MODULE_DESCRIPTION("GPIO extcon driver"); MODULE_LICENSE("GPL");
>> diff
>>> --git a/include/dt-bindings/extcon/extcon.h
>>> b/include/dt-bindings/extcon/extcon.h
>>> new file mode 100644
>>> index 0000000..dbba588
>>> --- /dev/null
>>> +++ b/include/dt-bindings/extcon/extcon.h
>>> @@ -0,0 +1,47 @@
>>> +/*
>>> + * This header provides constants for most extcon bindings.
>>> + *
>>> + * Most extcon bindings include the unique identification format.
>>> + * In most cases, the unique id format uses the standard values/macro
>>> + * defined in this header.
>>> + */
>>> +
>>> +#ifndef _DT_BINDINGS_EXTCON_EXTCON_H
>>> +#define _DT_BINDINGS_EXTCON_EXTCON_H
>>> +
>>> +/* USB external connector */
>>> +#define EXTCON_USB 1
>>> +#define EXTCON_USB_HOST 2
>>> +
>>> +/* Charging external connector */
>>> +#define EXTCON_CHG_USB_SDP 5 /* Standard Downstream Port */
>>> +#define EXTCON_CHG_USB_DCP 6 /* Dedicated Charging Port */
>>> +#define EXTCON_CHG_USB_CDP 7 /* Charging Downstream Port */
>>> +#define EXTCON_CHG_USB_ACA 8 /* Accessory Charger Adapter */
>>> +#define EXTCON_CHG_USB_FAST 9
>>> +#define EXTCON_CHG_USB_SLOW 10
>>> +
>>> +/* Jack external connector */
>>> +#define EXTCON_JACK_MICROPHONE 20
>>> +#define EXTCON_JACK_HEADPHONE 21
>>> +#define EXTCON_JACK_LINE_IN 22
>>> +#define EXTCON_JACK_LINE_OUT 23
>>> +#define EXTCON_JACK_VIDEO_IN 24
>>> +#define EXTCON_JACK_VIDEO_OUT 25
>>> +#define EXTCON_JACK_SPDIF_IN 26 /* Sony Philips Digital InterFace
>> */
>>> +#define EXTCON_JACK_SPDIF_OUT 27
>>> +
>>> +/* Display external connector */
>>> +#define EXTCON_DISP_HDMI 40 /* High-Definition Multimedia
>> Interface */
>>> +#define EXTCON_DISP_MHL 41 /* Mobile High-Definition Link
>> */
>>> +#define EXTCON_DISP_DVI 42 /* Digital Visual Interface */
>>> +#define EXTCON_DISP_VGA 43 /* Video Graphics Array */
>>> +
>>> +/* Miscellaneous external connector */
>>> +#define EXTCON_DOCK 60
>>> +#define EXTCON_JIG 61
>>> +#define EXTCON_MECHANICAL 62
>>> +
>>> +#define EXTCON_NUM 63
>>> +
>>> +#endif /* _DT_BINDINGS_EXTCON_EXTCON_H */
>>> diff --git a/include/linux/extcon/extcon-gpio.h
>>> b/include/linux/extcon/extcon-gpio.h
>>> index 7cacafb..f7e7673 100644
>>> --- a/include/linux/extcon/extcon-gpio.h
>>> +++ b/include/linux/extcon/extcon-gpio.h
>>> @@ -1,8 +1,8 @@
>>> /*
>>> * Single-state GPIO extcon driver based on extcon class
>>> *
>>> - * Copyright (C) 2012 Samsung Electronics
>>> - * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
>>> + * Copyright (C) 2016 Chanwoo Choi <cw00.choi@samsung.com>,
>> Samsung
>>> + Electronics
>>> + * Copyright (C) 2012 MyungJoo Ham <myungjoo.ham@samsung.com>,
>>> + Samsung Electronics
>>> *
>>> * based on switch class driver
>>> * Copyright (C) 2008 Google, Inc.
>>> @@ -35,10 +35,10 @@
>>> * while resuming from sleep.
>>> */
>>> struct gpio_extcon_pdata {
>>> - unsigned int extcon_id;
>>> + u32 extcon_id;
>>> unsigned gpio;
>>> bool gpio_active_low;
>>> - unsigned long debounce;
>>> + u32 debounce;
>>> unsigned long irq_flags;
>>>
>>> bool check_on_resume;
>>> --
>>> 2.1.4
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-31 6:44 ` Chanwoo Choi
@ 2016-05-31 7:35 ` Chanwoo Choi
2016-05-31 13:35 ` Rob Herring
0 siblings, 1 reply; 14+ messages in thread
From: Chanwoo Choi @ 2016-05-31 7:35 UTC (permalink / raw)
To: Rob Herring, Venkat Reddy Talla
Cc: MyungJoo Ham, Pawel Moll, Mark Rutland, Ian Campbell, devicetree,
Kumar Gala, linux-kernel, Laxman Dewangan
Hi Rob,
On 2016년 05월 31일 15:44, Chanwoo Choi wrote:
> On 2016년 05월 28일 00:29, Rob Herring wrote:
>> On Thu, May 26, 2016 at 05:17:45PM +0530, Venkat Reddy Talla wrote:
>>> Add the support for Device tree bindings of extcon-gpio driver.
>>> The extcon-gpio device tree node must include the both 'extcon-id' and
>>> 'gpios' property.
>>
>> I think extcon bindings are a mess in general...
>>
>>> For example:
>>> usb_cable: extcon-gpio-0 {
>>> compatible = "extcon-gpio";
>>> extcon-id = <EXTCON_USB>;
>>> gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
>>> }
>>> ta_cable: extcon-gpio-1 {
>>> compatible = "extcon-gpio";
>>> extcon-id = <EXTCON_CHG_USB_DCP>;
>>> gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
>>> debounce-ms = <50>; /* 50 millisecond */
>>> wakeup-source;
>>> }
>>
>> This is all 1 logical connector, the USB connector. Why are you
>> describing cables? Those are not part of the h/w and are dynamic.
>> Describe this as a connector which is one thing (i.e. node). Use a
>> compatible string that reflects the type of connector
>> (usb-microab-connector), not the driver you want to use. Then define
>> GPIO lines needed to provide state information like VBus, ID, charger
>> modes and control lines like soft connect (D+ pullup enable), VBus
>> enable, etc.
>
> You're right. The extcon-gpio driver will not use the "extcon-gpio" raw compatible.
> As you commented[1], the each connector will have the unique name to use the extcon-gpio.c driver.
> [1] https://lkml.org/lkml/2015/10/21/906
>
>
> For example,
> The extcon-gpio.c driver may have the different name including the h/w information
> according to the kind of external connector.
>
> static const struct of_device_id gpio_extcon_of_match[] = {
> {
> .compatible = "extcon-chg-sdp", /* SDP charger connector */
> .data = EXTCON_CHG_SDP_DATA,
> }, {
> .compatible = "extcon-chg-dcp", /* DCP charger connector */
> .data = EXTCON_CHG_DCP_DATA,
> }, {
> .compatible = "extcon-jack-microphone", /* Microphone jack connector */
> .data = EXTCON_JACK_MICROPHONE_DATA,
> }, {
> .compatible = "extcon-disp-hdmi", /* HDMI connector*/
> .data = EXTCON_DISP_HDMI_DATA,
> },
> ......
> };
I reply it again.
The extcon-gpio.c is very similar with existing gpio_keys.c driver[1]
[1] drivers/input/keyboard/gpio_keys.c
The gpio_keys.c driver use the following style to support the device-tree.
It use the "gpio-keys" compatible and this dt node include the specific
'key code' such as 'extcon-id = <EXTCON_CHG_USB_DCP>;'
gpio_keys {
compatible = "gpio-keys";
power_key {
gpios = <&gpx2 7 GPIO_ACTIVE_LOW>;
linux,code = <KEY_POWER>;
label = "power key";
debounce-interval = <10>;
wakeup-source;
};
};
If the extcon-gpio.c driver should have the separate compatible according to
the kind of external connector, the list of compatible name of extcon-gpio.c driver
will be increased when new external connector is attached.
The extcon-gpio.c driver can separate the kind of external connector
by using the 'extcon-id' property.
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-31 7:35 ` Chanwoo Choi
@ 2016-05-31 13:35 ` Rob Herring
2016-05-31 13:44 ` Laxman Dewangan
2016-05-31 14:34 ` Chanwoo Choi
0 siblings, 2 replies; 14+ messages in thread
From: Rob Herring @ 2016-05-31 13:35 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Venkat Reddy Talla, MyungJoo Ham, Pawel Moll, Mark Rutland,
Ian Campbell, devicetree, Kumar Gala, linux-kernel,
Laxman Dewangan
On Tue, May 31, 2016 at 2:35 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> Hi Rob,
>
> On 2016년 05월 31일 15:44, Chanwoo Choi wrote:
>> On 2016년 05월 28일 00:29, Rob Herring wrote:
>>> On Thu, May 26, 2016 at 05:17:45PM +0530, Venkat Reddy Talla wrote:
>>>> Add the support for Device tree bindings of extcon-gpio driver.
>>>> The extcon-gpio device tree node must include the both 'extcon-id' and
>>>> 'gpios' property.
>>>
>>> I think extcon bindings are a mess in general...
>>>
>>>> For example:
>>>> usb_cable: extcon-gpio-0 {
>>>> compatible = "extcon-gpio";
>>>> extcon-id = <EXTCON_USB>;
>>>> gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
>>>> }
>>>> ta_cable: extcon-gpio-1 {
>>>> compatible = "extcon-gpio";
>>>> extcon-id = <EXTCON_CHG_USB_DCP>;
>>>> gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
>>>> debounce-ms = <50>; /* 50 millisecond */
>>>> wakeup-source;
>>>> }
>>>
>>> This is all 1 logical connector, the USB connector. Why are you
>>> describing cables? Those are not part of the h/w and are dynamic.
>>> Describe this as a connector which is one thing (i.e. node). Use a
>>> compatible string that reflects the type of connector
>>> (usb-microab-connector), not the driver you want to use. Then define
>>> GPIO lines needed to provide state information like VBus, ID, charger
>>> modes and control lines like soft connect (D+ pullup enable), VBus
>>> enable, etc.
>>
>> You're right. The extcon-gpio driver will not use the "extcon-gpio" raw compatible.
>> As you commented[1], the each connector will have the unique name to use the extcon-gpio.c driver.
>> [1] https://lkml.org/lkml/2015/10/21/906
>>
>>
>> For example,
>> The extcon-gpio.c driver may have the different name including the h/w information
>> according to the kind of external connector.
>>
>> static const struct of_device_id gpio_extcon_of_match[] = {
>> {
>> .compatible = "extcon-chg-sdp", /* SDP charger connector */
>> .data = EXTCON_CHG_SDP_DATA,
>> }, {
>> .compatible = "extcon-chg-dcp", /* DCP charger connector */
>> .data = EXTCON_CHG_DCP_DATA,
>> }, {
>> .compatible = "extcon-jack-microphone", /* Microphone jack connector */
>> .data = EXTCON_JACK_MICROPHONE_DATA,
>> }, {
>> .compatible = "extcon-disp-hdmi", /* HDMI connector*/
>> .data = EXTCON_DISP_HDMI_DATA,
>> },
>> ......
>> };
>
> I reply it again.
>
> The extcon-gpio.c is very similar with existing gpio_keys.c driver[1]
> [1] drivers/input/keyboard/gpio_keys.c
There is a big difference in that each gpio-key is independent. The
only state is pressed or not. A USB connector has multiple pieces of
state information. You may be treating them independently, but I don't
think they should be.
> The gpio_keys.c driver use the following style to support the device-tree.
> It use the "gpio-keys" compatible and this dt node include the specific
> 'key code' such as 'extcon-id = <EXTCON_CHG_USB_DCP>;'
This is state information about what is currently attached. The
analogy with gpio-keys would be multiple key codes on one gpio which
would be broken...
>
> gpio_keys {
> compatible = "gpio-keys";
>
> power_key {
> gpios = <&gpx2 7 GPIO_ACTIVE_LOW>;
> linux,code = <KEY_POWER>;
> label = "power key";
> debounce-interval = <10>;
> wakeup-source;
> };
> };
>
> If the extcon-gpio.c driver should have the separate compatible according to
> the kind of external connector, the list of compatible name of extcon-gpio.c driver
> will be increased when new external connector is attached.
So? Different h/w needs different compatible strings.
But again, you are mixing describing the connector (only what is
soldered on a board) and state information (what is attached). Do not
put state information into DT (describe the gpio signals or chip that
provides the state information).
> The extcon-gpio.c driver can separate the kind of external connector
> by using the 'extcon-id' property.
This use of DT is just broken. Come up with another way.
Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-31 13:35 ` Rob Herring
@ 2016-05-31 13:44 ` Laxman Dewangan
2016-05-31 15:48 ` Rob Herring
2016-05-31 14:34 ` Chanwoo Choi
1 sibling, 1 reply; 14+ messages in thread
From: Laxman Dewangan @ 2016-05-31 13:44 UTC (permalink / raw)
To: Rob Herring, Chanwoo Choi
Cc: Venkat Reddy Talla, MyungJoo Ham, Pawel Moll, Mark Rutland,
Ian Campbell, devicetree, Kumar Gala, linux-kernel
Hi Rob,
On Tuesday 31 May 2016 07:05 PM, Rob Herring wrote:
> On Tue, May 31, 2016 at 2:35 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> The extcon-gpio.c driver can separate the kind of external connector
>> by using the 'extcon-id' property.
> This use of DT is just broken. Come up with another way.
>
>
Can we have the DT binding very similar to IIO, clock, reset etc?
Here is details for extcon-jack DT binding and its client:
The client can get the cable information through its node or extcon name
and once cable information is available, it can register for
notification when state gets changed.
The typical dt nodes are:
Extcon-driver node:
extcon: arizona-extcon {
compatible = "wlf,arizona-extcon";
#extcon-cells = <1>;
};
Driver need to specify the cable ID as
Cable ID
----------------------------
Mechanical 0
Microphone 1
Headphone 2
Line-out 3
Here #extcon-cells is must and specifies the size of extcon cells.
The client need to provide the driver specific information as argument
along with handle.
Extcon Client node:
audio-controller@b0000 {
::::
extcon-cables = <&extcon 1>, <&extcon 3>;
extcon-cable-names = "Microphone", "Line-out";
};
and client driver can register the cable by passing the cable name
as above along with its node.
struct extcon_cable {
struct extcon_dev *edev,
int cable_id;
};
edev_mic_cable = extcon_get_extcon_cable(dev, "Microphone");
extcon_register_notification(edev_mic_cable, notifier);
edev_line_out_cable = extcon_get_extcon_cable(dev, "Line-out");
extcon_register_notification(edev_line_out_cable, notifier);
Prototype:
struct extcon_cable *extcon_get_extcon_cable(struct device *dev, const
char *cable_name)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-31 13:35 ` Rob Herring
2016-05-31 13:44 ` Laxman Dewangan
@ 2016-05-31 14:34 ` Chanwoo Choi
2016-06-07 1:52 ` Chanwoo Choi
1 sibling, 1 reply; 14+ messages in thread
From: Chanwoo Choi @ 2016-05-31 14:34 UTC (permalink / raw)
To: Rob Herring
Cc: Venkat Reddy Talla, MyungJoo Ham, Pawel Moll, Mark Rutland,
Ian Campbell, devicetree, Kumar Gala, linux-kernel,
Laxman Dewangan
Hi Rob,
On Tue, May 31, 2016 at 10:35 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, May 31, 2016 at 2:35 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> Hi Rob,
>>
>> On 2016년 05월 31일 15:44, Chanwoo Choi wrote:
>>> On 2016년 05월 28일 00:29, Rob Herring wrote:
>>>> On Thu, May 26, 2016 at 05:17:45PM +0530, Venkat Reddy Talla wrote:
>>>>> Add the support for Device tree bindings of extcon-gpio driver.
>>>>> The extcon-gpio device tree node must include the both 'extcon-id' and
>>>>> 'gpios' property.
>>>>
>>>> I think extcon bindings are a mess in general...
>>>>
>>>>> For example:
>>>>> usb_cable: extcon-gpio-0 {
>>>>> compatible = "extcon-gpio";
>>>>> extcon-id = <EXTCON_USB>;
>>>>> gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
>>>>> }
>>>>> ta_cable: extcon-gpio-1 {
>>>>> compatible = "extcon-gpio";
>>>>> extcon-id = <EXTCON_CHG_USB_DCP>;
>>>>> gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
>>>>> debounce-ms = <50>; /* 50 millisecond */
>>>>> wakeup-source;
>>>>> }
>>>>
>>>> This is all 1 logical connector, the USB connector. Why are you
>>>> describing cables? Those are not part of the h/w and are dynamic.
>>>> Describe this as a connector which is one thing (i.e. node). Use a
>>>> compatible string that reflects the type of connector
>>>> (usb-microab-connector), not the driver you want to use. Then define
>>>> GPIO lines needed to provide state information like VBus, ID, charger
>>>> modes and control lines like soft connect (D+ pullup enable), VBus
>>>> enable, etc.
>>>
>>> You're right. The extcon-gpio driver will not use the "extcon-gpio" raw compatible.
>>> As you commented[1], the each connector will have the unique name to use the extcon-gpio.c driver.
>>> [1] https://lkml.org/lkml/2015/10/21/906
>>>
>>>
>>> For example,
>>> The extcon-gpio.c driver may have the different name including the h/w information
>>> according to the kind of external connector.
>>>
>>> static const struct of_device_id gpio_extcon_of_match[] = {
>>> {
>>> .compatible = "extcon-chg-sdp", /* SDP charger connector */
>>> .data = EXTCON_CHG_SDP_DATA,
>>> }, {
>>> .compatible = "extcon-chg-dcp", /* DCP charger connector */
>>> .data = EXTCON_CHG_DCP_DATA,
>>> }, {
>>> .compatible = "extcon-jack-microphone", /* Microphone jack connector */
>>> .data = EXTCON_JACK_MICROPHONE_DATA,
>>> }, {
>>> .compatible = "extcon-disp-hdmi", /* HDMI connector*/
>>> .data = EXTCON_DISP_HDMI_DATA,
>>> },
>>> ......
>>> };
>>
>> I reply it again.
>>
>> The extcon-gpio.c is very similar with existing gpio_keys.c driver[1]
>> [1] drivers/input/keyboard/gpio_keys.c
>
> There is a big difference in that each gpio-key is independent. The
> only state is pressed or not. A USB connector has multiple pieces of
> state information. You may be treating them independently, but I don't
> think they should be.
I think that is it not different because the EXTCON can only notify the whether
external connector is attached or detached such as gpio-key (pressed or not).
EXTCON don't handle the any additional state except for attached or detached.
>
>> The gpio_keys.c driver use the following style to support the device-tree.
>> It use the "gpio-keys" compatible and this dt node include the specific
>> 'key code' such as 'extcon-id = <EXTCON_CHG_USB_DCP>;'
>
> This is state information about what is currently attached. The
> analogy with gpio-keys would be multiple key codes on one gpio which
> would be broken...
I compared between 'gpis-keys' and ' extcon-gpio' driver as following:
name | gpio-keys | extcon-gpio
---------------------------------------------------------------------------------------------
gpio | gpios = <> | extcon-gpio = <>
---------------------------------------------------------------------------------------------
type | linux,code = <> | extcon-id = <>
---------------------------------------------------------------------------------------------
key & | KEY_POWER | EXTCON_USB
extcon id | KEY_VOLUME_UP | EXTCON_CHG_USB_SDP
| KEY_VOLUME_DOWN | EXTCON_JACK_MICROPHONE
| etc | etc
---------------------------------------------------------------------------------------------
state | pressed or not | attached or detached
---------------------------------------------------------------------------------------------
>
>>
>> gpio_keys {
>> compatible = "gpio-keys";
>>
>> power_key {
>> gpios = <&gpx2 7 GPIO_ACTIVE_LOW>;
>> linux,code = <KEY_POWER>;
>> label = "power key";
>> debounce-interval = <10>;
>> wakeup-source;
>> };
>> };
>>
>> If the extcon-gpio.c driver should have the separate compatible according to
>> the kind of external connector, the list of compatible name of extcon-gpio.c driver
>> will be increased when new external connector is attached.
>
> So? Different h/w needs different compatible strings.
>
> But again, you are mixing describing the connector (only what is
> soldered on a board) and state information (what is attached). Do not
> put state information into DT (describe the gpio signals or chip that
> provides the state information).
>
>> The extcon-gpio.c driver can separate the kind of external connector
>> by using the 'extcon-id' property.
>
> This use of DT is just broken. Come up with another way.
>
> Rob
Thanks,
Chanwoo Choi
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-31 13:44 ` Laxman Dewangan
@ 2016-05-31 15:48 ` Rob Herring
2016-06-01 14:49 ` Laxman Dewangan
0 siblings, 1 reply; 14+ messages in thread
From: Rob Herring @ 2016-05-31 15:48 UTC (permalink / raw)
To: Laxman Dewangan
Cc: Chanwoo Choi, Venkat Reddy Talla, MyungJoo Ham, Pawel Moll,
Mark Rutland, Ian Campbell, devicetree, Kumar Gala, linux-kernel
On Tue, May 31, 2016 at 8:44 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> Hi Rob,
>
> On Tuesday 31 May 2016 07:05 PM, Rob Herring wrote:
>>
>> On Tue, May 31, 2016 at 2:35 AM, Chanwoo Choi <cw00.choi@samsung.com>
>> wrote:
>>>
>>> The extcon-gpio.c driver can separate the kind of external connector
>>> by using the 'extcon-id' property.
>>
>> This use of DT is just broken. Come up with another way.
>>
>>
>
>
> Can we have the DT binding very similar to IIO, clock, reset etc?
In what way? I'm guessing you mean to describe what controller a
connector is associated with. Certainly, some sort of phandle
reference will be needed. However. the node it points to is what needs
a lot of work first.
>
> Here is details for extcon-jack DT binding and its client:
>
> The client can get the cable information through its node or extcon name
> and once cable information is available, it can register for notification
> when state gets changed.
>
> The typical dt nodes are:
>
> Extcon-driver node:
>
> extcon: arizona-extcon {
> compatible = "wlf,arizona-extcon";
> #extcon-cells = <1>;
> };
>
>
>
> Driver need to specify the cable ID as
Unless you have cables hardwired to a board, please stop describing
cables in DT. Connectors!
> Cable ID
> ----------------------------
> Mechanical 0
> Microphone 1
> Headphone 2
> Line-out 3
No, please don't create some made up some number space. I don't see
why you need this. You should just need the phandle to "the microphone
jack" node.
> Here #extcon-cells is must and specifies the size of extcon cells. The
> client need to provide the driver specific information as argument along
> with handle.
>
>
> Extcon Client node:
>
> audio-controller@b0000 {
> ::::
> extcon-cables = <&extcon 1>, <&extcon 3>;
> extcon-cable-names = "Microphone", "Line-out";
> };
>
> and client driver can register the cable by passing the cable name
> as above along with its node.
>
> struct extcon_cable {
> struct extcon_dev *edev,
> int cable_id;
> };
If you are showing driver details to explain the binding, something is wrong.
>
> edev_mic_cable = extcon_get_extcon_cable(dev, "Microphone");
> extcon_register_notification(edev_mic_cable, notifier);
>
> edev_line_out_cable = extcon_get_extcon_cable(dev, "Line-out");
> extcon_register_notification(edev_line_out_cable, notifier);
Just remember that "*-names" should be optional (nor am I a fan of
adding -names everywhere). This can be defined as first entry is mic
and 2nd entry is line-out.
Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-31 15:48 ` Rob Herring
@ 2016-06-01 14:49 ` Laxman Dewangan
2016-06-01 16:00 ` Rob Herring
0 siblings, 1 reply; 14+ messages in thread
From: Laxman Dewangan @ 2016-06-01 14:49 UTC (permalink / raw)
To: Rob Herring
Cc: Chanwoo Choi, Venkat Reddy Talla, MyungJoo Ham, Pawel Moll,
Mark Rutland, Ian Campbell, devicetree, Kumar Gala, linux-kernel
On Tuesday 31 May 2016 09:18 PM, Rob Herring wrote:
> On Tue, May 31, 2016 at 8:44 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> Hi Rob,
>>
>> On Tuesday 31 May 2016 07:05 PM, Rob Herring wrote:
>>> On Tue, May 31, 2016 at 2:35 AM, Chanwoo Choi <cw00.choi@samsung.com>
>>> wrote:
>>>> The extcon-gpio.c driver can separate the kind of external connector
>>>> by using the 'extcon-id' property.
>>> This use of DT is just broken. Come up with another way.
>>>
>>>
>>
>> Can we have the DT binding very similar to IIO, clock, reset etc?
> In what way? I'm guessing you mean to describe what controller a
> connector is associated with. Certainly, some sort of phandle
> reference will be needed. However. the node it points to is what needs
> a lot of work first.
Here, we just need to define that controller support how many different
cable? It does not say anything that which cable is what and all its on
platform dependent.
So for gpio-extcon, it will just say 1 cable. the name is not decided by
driver but it is by platform.
>> Here is details for extcon-jack DT binding and its client:
>>
>> The client can get the cable information through its node or extcon name
>> and once cable information is available, it can register for notification
>> when state gets changed.
>>
>> The typical dt nodes are:
>>
>> Extcon-driver node:
>>
>> extcon: arizona-extcon {
>> compatible = "wlf,arizona-extcon";
>> #extcon-cells = <1>;
>> };
>>
>>
>>
>> Driver need to specify the cable ID as
> Unless you have cables hardwired to a board, please stop describing
> cables in DT. Connectors!
If some controller only support cable with names then we need to do it.
Like palmas extcon, it detect VBUS, ID_GND, RIDA, RIDB, RIDC. So for
these cases, driver need to define cables.
Driver can specify that cable index 0 is for VBUS, 1 is for ID-GND etc.
>
>> Cable ID
>> ----------------------------
>> Mechanical 0
>> Microphone 1
>> Headphone 2
>> Line-out 3
> No, please don't create some made up some number space. I don't see
> why you need this. You should just need the phandle to "the microphone
> jack" node.
I am thinking that I will need the phandle of extcon nodes with index
for cable ID.
>> Here #extcon-cells is must and specifies the size of extcon cells. The
>> client need to provide the driver specific information as argument along
>> with handle.
>>
>>
>> Extcon Client node:
>>
>> audio-controller@b0000 {
>> ::::
>> extcon-cables = <&extcon 1>, <&extcon 3>;
>> extcon-cable-names = "Microphone", "Line-out";
>> };
>>
>> and client driver can register the cable by passing the cable name
>> as above along with its node.
>>
>> struct extcon_cable {
>> struct extcon_dev *edev,
>> int cable_id;
>> };
> If you are showing driver details to explain the binding, something is wrong.
I have USB OTG driver which need the notification when VBUS or ID
detected. For this, it can get the phandle of extcon and cable ID from
that driver for "id" and "vbus".
I have platform like follows:
I have 2 gpios which detect the ID and VBUS.
Both are independent. My USB driver get the extcon handle from the DT:
extcon_vbus: extcon@0 {
comptaible = "extcon-gpio";
gpio = <&gpio JETSON_VBUS_GPIO 0>;
#extcon-cells = <1>;
};
extcon_id: extcon@1 {
comptaible = "extcon-gpio";
gpio = <&gpio JETSON_ID_GPIO 0>;
#extcon-cells = <1>;
};
usb-otg {
::
extcon-cable = <&extcon_vbus 0 &extcon_d 0>;
extcon-cable-names = "id", "vbus";
::
};
>> edev_mic_cable = extcon_get_extcon_cable(dev, "Microphone");
>> extcon_register_notification(edev_mic_cable, notifier);
>>
>> edev_line_out_cable = extcon_get_extcon_cable(dev, "Line-out");
>> extcon_register_notification(edev_line_out_cable, notifier);
> Just remember that "*-names" should be optional (nor am I a fan of
> adding -names everywhere). This can be defined as first entry is mic
> and 2nd entry is line-out.
>
> Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-06-01 14:49 ` Laxman Dewangan
@ 2016-06-01 16:00 ` Rob Herring
0 siblings, 0 replies; 14+ messages in thread
From: Rob Herring @ 2016-06-01 16:00 UTC (permalink / raw)
To: Laxman Dewangan
Cc: Chanwoo Choi, Venkat Reddy Talla, MyungJoo Ham, Pawel Moll,
Mark Rutland, Ian Campbell, devicetree, Kumar Gala, linux-kernel
On Wed, Jun 1, 2016 at 9:49 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>
> On Tuesday 31 May 2016 09:18 PM, Rob Herring wrote:
>>
>> On Tue, May 31, 2016 at 8:44 AM, Laxman Dewangan <ldewangan@nvidia.com>
>> wrote:
>>>
>>> Hi Rob,
>>>
>>> On Tuesday 31 May 2016 07:05 PM, Rob Herring wrote:
>>>>
>>>> On Tue, May 31, 2016 at 2:35 AM, Chanwoo Choi <cw00.choi@samsung.com>
>>>> wrote:
>>>>>
>>>>> The extcon-gpio.c driver can separate the kind of external connector
>>>>> by using the 'extcon-id' property.
>>>>
>>>> This use of DT is just broken. Come up with another way.
>>>>
>>>>
>>>
>>> Can we have the DT binding very similar to IIO, clock, reset etc?
>>
>> In what way? I'm guessing you mean to describe what controller a
>> connector is associated with. Certainly, some sort of phandle
>> reference will be needed. However. the node it points to is what needs
>> a lot of work first.
>
>
> Here, we just need to define that controller support how many different
> cable? It does not say anything that which cable is what and all its on
> platform dependent.
>
> So for gpio-extcon, it will just say 1 cable. the name is not decided by
> driver but it is by platform.
Why the fascination with cables? You need to describe the connector
and circuitry between the connector and phy/controller, and its
capabilities. Maybe those properties are implied or explicit. If you
have some USB chip for Vbus control and charger detect, then it's all
going to be implied by that chip's compatible. If you have gpio lines
then the properties and capabilities are explicit.
>>> Here is details for extcon-jack DT binding and its client:
>>>
>>> The client can get the cable information through its node or extcon name
>>> and once cable information is available, it can register for notification
>>> when state gets changed.
>>>
>>> The typical dt nodes are:
>>>
>>> Extcon-driver node:
>>>
>>> extcon: arizona-extcon {
>>> compatible = "wlf,arizona-extcon";
>>> #extcon-cells = <1>;
>>> };
>>>
>>>
>>>
>>> Driver need to specify the cable ID as
>>
>> Unless you have cables hardwired to a board, please stop describing
>> cables in DT. Connectors!
>
>
> If some controller only support cable with names then we need to do it. Like
> palmas extcon, it detect VBUS, ID_GND, RIDA, RIDB, RIDC. So for these cases,
> driver need to define cables.
The only thing the DT should have is the palmas chip and any
connections to it like regulators or interrupts. What the chip can
control/detect doesn't belong in DT.
> Driver can specify that cable index 0 is for VBUS, 1 is for ID-GND etc.
You are still just making up indexes.
>>> Cable ID
>>> ----------------------------
>>> Mechanical 0
>>> Microphone 1
>>> Headphone 2
>>> Line-out 3
>>
>> No, please don't create some made up some number space. I don't see
>> why you need this. You should just need the phandle to "the microphone
>> jack" node.
>
>
> I am thinking that I will need the phandle of extcon nodes with index for
> cable ID.
>
>
>>> Here #extcon-cells is must and specifies the size of extcon cells.
>>> The
>>> client need to provide the driver specific information as argument along
>>> with handle.
>>>
>>>
>>> Extcon Client node:
>>>
>>> audio-controller@b0000 {
>>> ::::
>>> extcon-cables = <&extcon 1>, <&extcon 3>;
>>> extcon-cable-names = "Microphone", "Line-out";
>>> };
>>>
>>> and client driver can register the cable by passing the cable name
>>> as above along with its node.
>>>
>>> struct extcon_cable {
>>> struct extcon_dev *edev,
>>> int cable_id;
>>> };
>>
>> If you are showing driver details to explain the binding, something is
>> wrong.
>
>
>
> I have USB OTG driver which need the notification when VBUS or ID detected.
That's a kernel architectural detail.
> For this, it can get the phandle of extcon and cable ID from that driver for
> "id" and "vbus".
You only need to describe the connection of the connector to the
controller. These details don't need to be in DT.
> I have platform like follows:
> I have 2 gpios which detect the ID and VBUS.
> Both are independent. My USB driver get the extcon handle from the DT:
>
> extcon_vbus: extcon@0 {
> comptaible = "extcon-gpio";
> gpio = <&gpio JETSON_VBUS_GPIO 0>;
> #extcon-cells = <1>;
> };
>
>
>
> extcon_id: extcon@1 {
> comptaible = "extcon-gpio";
> gpio = <&gpio JETSON_ID_GPIO 0>;
> #extcon-cells = <1>;
> };
Sigh. These are all properties of one entity: a connector.
>
> usb-otg {
> ::
> extcon-cable = <&extcon_vbus 0 &extcon_d 0>;
> extcon-cable-names = "id", "vbus";
> ::
> };
Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] extcon: gpio: Add the support for Device tree bindings
2016-05-31 14:34 ` Chanwoo Choi
@ 2016-06-07 1:52 ` Chanwoo Choi
0 siblings, 0 replies; 14+ messages in thread
From: Chanwoo Choi @ 2016-06-07 1:52 UTC (permalink / raw)
To: Rob Herring
Cc: Venkat Reddy Talla, MyungJoo Ham, Pawel Moll, Mark Rutland,
Ian Campbell, devicetree, Kumar Gala, linux-kernel,
Laxman Dewangan
Ping.
Hi Rob,
I add some comment about gpio-keys and extcon-gpio.
I'm waiting for your reply.
On 2016년 05월 31일 23:34, Chanwoo Choi wrote:
> Hi Rob,
>
> On Tue, May 31, 2016 at 10:35 PM, Rob Herring <robh@kernel.org> wrote:
>> On Tue, May 31, 2016 at 2:35 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> Hi Rob,
>>>
>>> On 2016년 05월 31일 15:44, Chanwoo Choi wrote:
>>>> On 2016년 05월 28일 00:29, Rob Herring wrote:
>>>>> On Thu, May 26, 2016 at 05:17:45PM +0530, Venkat Reddy Talla wrote:
>>>>>> Add the support for Device tree bindings of extcon-gpio driver.
>>>>>> The extcon-gpio device tree node must include the both 'extcon-id' and
>>>>>> 'gpios' property.
>>>>>
>>>>> I think extcon bindings are a mess in general...
>>>>>
>>>>>> For example:
>>>>>> usb_cable: extcon-gpio-0 {
>>>>>> compatible = "extcon-gpio";
>>>>>> extcon-id = <EXTCON_USB>;
>>>>>> gpios = <&gpio6 1 GPIO_ACTIVE_HIGH>;
>>>>>> }
>>>>>> ta_cable: extcon-gpio-1 {
>>>>>> compatible = "extcon-gpio";
>>>>>> extcon-id = <EXTCON_CHG_USB_DCP>;
>>>>>> gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
>>>>>> debounce-ms = <50>; /* 50 millisecond */
>>>>>> wakeup-source;
>>>>>> }
>>>>>
>>>>> This is all 1 logical connector, the USB connector. Why are you
>>>>> describing cables? Those are not part of the h/w and are dynamic.
>>>>> Describe this as a connector which is one thing (i.e. node). Use a
>>>>> compatible string that reflects the type of connector
>>>>> (usb-microab-connector), not the driver you want to use. Then define
>>>>> GPIO lines needed to provide state information like VBus, ID, charger
>>>>> modes and control lines like soft connect (D+ pullup enable), VBus
>>>>> enable, etc.
>>>>
>>>> You're right. The extcon-gpio driver will not use the "extcon-gpio" raw compatible.
>>>> As you commented[1], the each connector will have the unique name to use the extcon-gpio.c driver.
>>>> [1] https://lkml.org/lkml/2015/10/21/906
>>>>
>>>>
>>>> For example,
>>>> The extcon-gpio.c driver may have the different name including the h/w information
>>>> according to the kind of external connector.
>>>>
>>>> static const struct of_device_id gpio_extcon_of_match[] = {
>>>> {
>>>> .compatible = "extcon-chg-sdp", /* SDP charger connector */
>>>> .data = EXTCON_CHG_SDP_DATA,
>>>> }, {
>>>> .compatible = "extcon-chg-dcp", /* DCP charger connector */
>>>> .data = EXTCON_CHG_DCP_DATA,
>>>> }, {
>>>> .compatible = "extcon-jack-microphone", /* Microphone jack connector */
>>>> .data = EXTCON_JACK_MICROPHONE_DATA,
>>>> }, {
>>>> .compatible = "extcon-disp-hdmi", /* HDMI connector*/
>>>> .data = EXTCON_DISP_HDMI_DATA,
>>>> },
>>>> ......
>>>> };
>>>
>>> I reply it again.
>>>
>>> The extcon-gpio.c is very similar with existing gpio_keys.c driver[1]
>>> [1] drivers/input/keyboard/gpio_keys.c
>>
>> There is a big difference in that each gpio-key is independent. The
>> only state is pressed or not. A USB connector has multiple pieces of
>> state information. You may be treating them independently, but I don't
>> think they should be.
>
> I think that is it not different because the EXTCON can only notify the whether
> external connector is attached or detached such as gpio-key (pressed or not).
>
> EXTCON don't handle the any additional state except for attached or detached.
>
>>
>>> The gpio_keys.c driver use the following style to support the device-tree.
>>> It use the "gpio-keys" compatible and this dt node include the specific
>>> 'key code' such as 'extcon-id = <EXTCON_CHG_USB_DCP>;'
>>
>> This is state information about what is currently attached. The
>> analogy with gpio-keys would be multiple key codes on one gpio which
>> would be broken...
>
> I compared between 'gpis-keys' and ' extcon-gpio' driver as following:
>
> name | gpio-keys | extcon-gpio
> ---------------------------------------------------------------------------------------------
> gpio | gpios = <> | extcon-gpio = <>
> ---------------------------------------------------------------------------------------------
> type | linux,code = <> | extcon-id = <>
> ---------------------------------------------------------------------------------------------
> key & | KEY_POWER | EXTCON_USB
> extcon id | KEY_VOLUME_UP | EXTCON_CHG_USB_SDP
> | KEY_VOLUME_DOWN | EXTCON_JACK_MICROPHONE
> | etc | etc
> ---------------------------------------------------------------------------------------------
> state | pressed or not | attached or detached
> ---------------------------------------------------------------------------------------------
>
>>
>>>
>>> gpio_keys {
>>> compatible = "gpio-keys";
>>>
>>> power_key {
>>> gpios = <&gpx2 7 GPIO_ACTIVE_LOW>;
>>> linux,code = <KEY_POWER>;
>>> label = "power key";
>>> debounce-interval = <10>;
>>> wakeup-source;
>>> };
>>> };
>>>
>>> If the extcon-gpio.c driver should have the separate compatible according to
>>> the kind of external connector, the list of compatible name of extcon-gpio.c driver
>>> will be increased when new external connector is attached.
>>
>> So? Different h/w needs different compatible strings.
>>
>> But again, you are mixing describing the connector (only what is
>> soldered on a board) and state information (what is attached). Do not
>> put state information into DT (describe the gpio signals or chip that
>> provides the state information).
>>
>>> The extcon-gpio.c driver can separate the kind of external connector
>>> by using the 'extcon-id' property.
>>
>> This use of DT is just broken. Come up with another way.
>>
>> Rob
>
> Thanks,
> Chanwoo Choi
Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-06-07 1:52 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-26 11:47 [PATCH v4] extcon: gpio: Add the support for Device tree bindings Venkat Reddy Talla
2016-05-26 13:21 ` Chanwoo Choi
2016-05-27 5:13 ` Venkat Reddy Talla
2016-05-31 7:13 ` Chanwoo Choi
2016-05-27 15:29 ` Rob Herring
2016-05-31 6:44 ` Chanwoo Choi
2016-05-31 7:35 ` Chanwoo Choi
2016-05-31 13:35 ` Rob Herring
2016-05-31 13:44 ` Laxman Dewangan
2016-05-31 15:48 ` Rob Herring
2016-06-01 14:49 ` Laxman Dewangan
2016-06-01 16:00 ` Rob Herring
2016-05-31 14:34 ` Chanwoo Choi
2016-06-07 1:52 ` Chanwoo Choi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).