[PATCH 0/8] GPIO extcon modernization

[PATCH 0/8] GPIO extcon modernization

[Linus Walleij]

Like with the GPIO mouse input, I found that this driver has no in-tree
users at all.

I *could* just propose to delete it. But the driver seems generally useful,
so I made a patch series shaping it up to get configuration from device
tree or ACPI DSDT using device properties instead.

We start off the series by defining a set of device tree bindings for it.

It would be great to have some input from the initial authors on this
rewrite. I think we would not merge any board file using the platform
data anymore, so making it an all-in device property (device tree || ACPI)
driver seems reasonable.

Linus Walleij (8):
  extcon: gpio: Add DT bindings
  extcon: gpio: Localize platform data
  extcon: gpio: Move platform data into state container
  extcon: gpio: Convert to fully use GPIO descriptor
  extcon: gpio: Request reasonable interrupts
  extcon: gpio: Get debounce setting from device property
  extcon: gpio: Get connector type from device property
  extcon: gpio: Always check state on resume

 .../devicetree/bindings/extcon/extcon-gpio.txt     |  24 ++++
 drivers/extcon/extcon-gpio.c                       | 132 ++++++++++++---------
 include/dt-bindings/extcon/connectors.h            |  38 ++++++
 include/linux/extcon/extcon-gpio.h                 |  47 --------
 4 files changed, 135 insertions(+), 106 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
 create mode 100644 include/dt-bindings/extcon/connectors.h
 delete mode 100644 include/linux/extcon/extcon-gpio.h

-- 
2.13.5

[PATCH 1/8] extcon: gpio: Add DT bindings

[Linus Walleij]

Add some reasonable device tree bindings and also add the cable defines
to the extcon include in <dt-bindings/extcon/connectors.h> since
the GPIO extcon definately need to specify which cable/connector it is
detecting.

Adding the include file makes the (as it happens) Linux numbers into an
ABI, but I do not see any better method. It is possible to define strings
for all cable types but it seems like overkill, just reusing Linux'
enumerators seems like a good idea.

The binding supports any number of GPIOs and connectors, but the driver
currently only supports one connector on one GPIO line.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/extcon/extcon-gpio.txt     | 24 ++++++++++++++
 include/dt-bindings/extcon/connectors.h            | 38 ++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
 create mode 100644 include/dt-bindings/extcon/connectors.h

diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
new file mode 100644
index 000000000000..2f5e21b94a64
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
@@ -0,0 +1,24 @@
+External Connector Using GPIO
+
+Required properties:
+- compatible: should be "extcon-gpio"
+- extcon-gpios: the GPIO lines used for the external connectors
+  See gpio/gpio.txt
+- extcon-connector-types: set to an unsigned integer value arrat representing the types
+  of this connector, matched to the corresponding GPIO lines in the previous array.
+  Those are defined with unique IDs in <dt-bindings/extcon/connectors.h>
+- input-debounce: The number of microseconds to wait for the
+  connector state to stabilize. This property is reused from pin control
+  See pinctrl/pinctrl-bindings.txt
+
+Example:
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/extcon/connectors.h>
+
+extcon {
+	compatible = "extcon-gpio";
+	extcon-gpios = <&gpio0 42 GPIO_ACTIVE_LOW>;
+	extcon-connector-types = <EXTCON_USB>;
+	input-debounce = <20000>; /* 20 ms */
+};
diff --git a/include/dt-bindings/extcon/connectors.h b/include/dt-bindings/extcon/connectors.h
new file mode 100644
index 000000000000..61bed24eaadc
--- /dev/null
+++ b/include/dt-bindings/extcon/connectors.h
@@ -0,0 +1,38 @@
+#ifndef _DT_BINDINGS_EXTCON_CONNECTORS_H
+#define _DT_BINDINGS_EXTCON_CONNECTORS_H
+
+/* USB external connector */
+#define EXTCON_USB		1
+#define EXTCON_USB_HOST		2
+#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
+#define EXTCON_CHG_WPT		11	/* Wireless Power Transfer */
+#define EXTCON_CHG_USB_PD	12	/* USB Power Delivery */
+/* 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 */
+#define EXTCON_DISP_DP		44	/* Display Port */
+#define EXTCON_DISP_HMD		45	/* Head-Mounted Display */
+/* Miscellaneous external connector */
+#define EXTCON_DOCK		60
+#define EXTCON_JIG		61
+#define EXTCON_MECHANICAL	62
+
+#define EXTCON_NUM		63
+
+#endif /* _DT_BINDINGS_EXTCON_CONNECTORS_H */
-- 
2.13.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/8] extcon: gpio: Add DT bindings

[Linus Walleij]

Add some reasonable device tree bindings and also add the cable defines
to the extcon include in <dt-bindings/extcon/connectors.h> since
the GPIO extcon definately need to specify which cable/connector it is
detecting.

Adding the include file makes the (as it happens) Linux numbers into an
ABI, but I do not see any better method. It is possible to define strings
for all cable types but it seems like overkill, just reusing Linux'
enumerators seems like a good idea.

The binding supports any number of GPIOs and connectors, but the driver
currently only supports one connector on one GPIO line.

Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../devicetree/bindings/extcon/extcon-gpio.txt     | 24 ++++++++++++++
 include/dt-bindings/extcon/connectors.h            | 38 ++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
 create mode 100644 include/dt-bindings/extcon/connectors.h

diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
new file mode 100644
index 000000000000..2f5e21b94a64
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
@@ -0,0 +1,24 @@
+External Connector Using GPIO
+
+Required properties:
+- compatible: should be "extcon-gpio"
+- extcon-gpios: the GPIO lines used for the external connectors
+  See gpio/gpio.txt
+- extcon-connector-types: set to an unsigned integer value arrat representing the types
+  of this connector, matched to the corresponding GPIO lines in the previous array.
+  Those are defined with unique IDs in <dt-bindings/extcon/connectors.h>
+- input-debounce: The number of microseconds to wait for the
+  connector state to stabilize. This property is reused from pin control
+  See pinctrl/pinctrl-bindings.txt
+
+Example:
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/extcon/connectors.h>
+
+extcon {
+	compatible = "extcon-gpio";
+	extcon-gpios = <&gpio0 42 GPIO_ACTIVE_LOW>;
+	extcon-connector-types = <EXTCON_USB>;
+	input-debounce = <20000>; /* 20 ms */
+};
diff --git a/include/dt-bindings/extcon/connectors.h b/include/dt-bindings/extcon/connectors.h
new file mode 100644
index 000000000000..61bed24eaadc
--- /dev/null
+++ b/include/dt-bindings/extcon/connectors.h
@@ -0,0 +1,38 @@
+#ifndef _DT_BINDINGS_EXTCON_CONNECTORS_H
+#define _DT_BINDINGS_EXTCON_CONNECTORS_H
+
+/* USB external connector */
+#define EXTCON_USB		1
+#define EXTCON_USB_HOST		2
+#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
+#define EXTCON_CHG_WPT		11	/* Wireless Power Transfer */
+#define EXTCON_CHG_USB_PD	12	/* USB Power Delivery */
+/* 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 */
+#define EXTCON_DISP_DP		44	/* Display Port */
+#define EXTCON_DISP_HMD		45	/* Head-Mounted Display */
+/* Miscellaneous external connector */
+#define EXTCON_DOCK		60
+#define EXTCON_JIG		61
+#define EXTCON_MECHANICAL	62
+
+#define EXTCON_NUM		63
+
+#endif /* _DT_BINDINGS_EXTCON_CONNECTORS_H */
-- 
2.13.5

[PATCH 2/8] extcon: gpio: Localize platform data

[Linus Walleij]

Nothing in the entire kernel #includes <linux/extcon/extcon-gpio.h>
so move the platform data declaration inside of the driver.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/extcon/extcon-gpio.c       | 22 +++++++++++++++++-
 include/linux/extcon/extcon-gpio.h | 47 --------------------------------------
 2 files changed, 21 insertions(+), 48 deletions(-)
 delete mode 100644 include/linux/extcon/extcon-gpio.h

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index ebed22f22d75..6abf5f70fdbf 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -18,7 +18,6 @@
  */
 
 #include <linux/extcon.h>
-#include <linux/extcon/extcon-gpio.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/init.h>
@@ -29,6 +28,27 @@
 #include <linux/slab.h>
 #include <linux/workqueue.h>
 
+/**
+ * struct gpio_extcon_pdata - A simple GPIO-controlled extcon device.
+ * @extcon_id:		The unique id of specific external connector.
+ * @gpio:		Corresponding GPIO.
+ * @gpio_active_low:	Boolean describing whether gpio active state is 1 or 0
+ *			If true, low state of gpio means active.
+ *			If false, high state of gpio means active.
+ * @debounce:		Debounce time for GPIO IRQ in ms.
+ * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
+ * @check_on_resume:	Boolean describing whether to check the state of gpio
+ *			while resuming from sleep.
+ */
+struct gpio_extcon_pdata {
+	unsigned int extcon_id;
+	unsigned gpio;
+	bool gpio_active_low;
+	unsigned long debounce;
+	unsigned long irq_flags;
+	bool check_on_resume;
+};
+
 struct gpio_extcon_data {
 	struct extcon_dev *edev;
 	int irq;
diff --git a/include/linux/extcon/extcon-gpio.h b/include/linux/extcon/extcon-gpio.h
deleted file mode 100644
index 7cacafb78b09..000000000000
--- a/include/linux/extcon/extcon-gpio.h
+++ /dev/null
@@ -1,47 +0,0 @@
-/*
- * Single-state GPIO extcon driver based on extcon class
- *
- * Copyright (C) 2012 Samsung Electronics
- * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
- *
- * based on switch class driver
- * Copyright (C) 2008 Google, Inc.
- * Author: Mike Lockwood <lockwood@android.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-#ifndef __EXTCON_GPIO_H__
-#define __EXTCON_GPIO_H__ __FILE__
-
-#include <linux/extcon.h>
-
-/**
- * struct gpio_extcon_pdata - A simple GPIO-controlled extcon device.
- * @extcon_id:		The unique id of specific external connector.
- * @gpio:		Corresponding GPIO.
- * @gpio_active_low:	Boolean describing whether gpio active state is 1 or 0
- *			If true, low state of gpio means active.
- *			If false, high state of gpio means active.
- * @debounce:		Debounce time for GPIO IRQ in ms.
- * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
- * @check_on_resume:	Boolean describing whether to check the state of gpio
- *			while resuming from sleep.
- */
-struct gpio_extcon_pdata {
-	unsigned int extcon_id;
-	unsigned gpio;
-	bool gpio_active_low;
-	unsigned long debounce;
-	unsigned long irq_flags;
-
-	bool check_on_resume;
-};
-
-#endif /* __EXTCON_GPIO_H__ */
-- 
2.13.5

[PATCH 3/8] extcon: gpio: Move platform data into state container

[Linus Walleij]

This moves the platform data settings from the platform data
struct and into the state container, saving some unnecessary
references and simplifying things a bit.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/extcon/extcon-gpio.c | 56 ++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 6abf5f70fdbf..9c4094edd123 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -29,7 +29,13 @@
 #include <linux/workqueue.h>
 
 /**
- * struct gpio_extcon_pdata - A simple GPIO-controlled extcon device.
+ * struct gpio_extcon_data - A simple GPIO-controlled extcon device state container.
+ * @edev:		Extcon device.
+ * @irq:		Interrupt line for the external connector.
+ * @work:		Work fired by the interrupt.
+ * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
+ *			value.
+ * @id_gpiod:		GPIO descriptor for this external connector.
  * @extcon_id:		The unique id of specific external connector.
  * @gpio:		Corresponding GPIO.
  * @gpio_active_low:	Boolean describing whether gpio active state is 1 or 0
@@ -40,23 +46,18 @@
  * @check_on_resume:	Boolean describing whether to check the state of gpio
  *			while resuming from sleep.
  */
-struct gpio_extcon_pdata {
-	unsigned int extcon_id;
-	unsigned gpio;
-	bool gpio_active_low;
-	unsigned long debounce;
-	unsigned long irq_flags;
-	bool check_on_resume;
-};
-
 struct gpio_extcon_data {
 	struct extcon_dev *edev;
 	int irq;
 	struct delayed_work work;
 	unsigned long debounce_jiffies;
-
 	struct gpio_desc *id_gpiod;
-	struct gpio_extcon_pdata *pdata;
+	unsigned int extcon_id;
+	unsigned gpio;
+	bool gpio_active_low;
+	unsigned long debounce;
+	unsigned long irq_flags;
+	bool check_on_resume;
 };
 
 static void gpio_extcon_work(struct work_struct *work)
@@ -67,10 +68,10 @@ static void gpio_extcon_work(struct work_struct *work)
 			     work);
 
 	state = gpiod_get_value_cansleep(data->id_gpiod);
-	if (data->pdata->gpio_active_low)
+	if (data->gpio_active_low)
 		state = !state;
 
-	extcon_set_state_sync(data->edev, data->pdata->extcon_id, state);
+	extcon_set_state_sync(data->edev, data->extcon_id, state);
 }
 
 static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
@@ -84,24 +85,23 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
 
 static int gpio_extcon_init(struct device *dev, struct gpio_extcon_data *data)
 {
-	struct gpio_extcon_pdata *pdata = data->pdata;
 	int ret;
 
-	ret = devm_gpio_request_one(dev, pdata->gpio, GPIOF_DIR_IN,
+	ret = devm_gpio_request_one(dev, data->gpio, GPIOF_DIR_IN,
 				dev_name(dev));
 	if (ret < 0)
 		return ret;
 
-	data->id_gpiod = gpio_to_desc(pdata->gpio);
+	data->id_gpiod = gpio_to_desc(data->gpio);
 	if (!data->id_gpiod)
 		return -EINVAL;
 
-	if (pdata->debounce) {
+	if (data->debounce) {
 		ret = gpiod_set_debounce(data->id_gpiod,
-					pdata->debounce * 1000);
+					 data->debounce * 1000);
 		if (ret < 0)
 			data->debounce_jiffies =
-				msecs_to_jiffies(pdata->debounce);
+				msecs_to_jiffies(data->debounce);
 	}
 
 	data->irq = gpiod_to_irq(data->id_gpiod);
@@ -113,20 +113,16 @@ static int gpio_extcon_init(struct device *dev, struct gpio_extcon_data *data)
 
 static int gpio_extcon_probe(struct platform_device *pdev)
 {
-	struct gpio_extcon_pdata *pdata = dev_get_platdata(&pdev->dev);
 	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);
 	if (!data)
 		return -ENOMEM;
-	data->pdata = pdata;
+
+	if (!data->irq_flags || data->extcon_id > EXTCON_NONE)
+		return -EINVAL;
 
 	/* Initialize the gpio */
 	ret = gpio_extcon_init(&pdev->dev, data);
@@ -134,7 +130,7 @@ 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->extcon_id);
 	if (IS_ERR(data->edev)) {
 		dev_err(&pdev->dev, "failed to allocate extcon device\n");
 		return -ENOMEM;
@@ -151,7 +147,7 @@ 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->irq_flags,
 					pdev->name, data);
 	if (ret < 0)
 		return ret;
@@ -178,7 +174,7 @@ static int gpio_extcon_resume(struct device *dev)
 	struct gpio_extcon_data *data;
 
 	data = dev_get_drvdata(dev);
-	if (data->pdata->check_on_resume)
+	if (data->check_on_resume)
 		queue_delayed_work(system_power_efficient_wq,
 			&data->work, data->debounce_jiffies);
 
-- 
2.13.5

[PATCH 4/8] extcon: gpio: Convert to fully use GPIO descriptor

[Linus Walleij]

Since we are not getting the GPIO from any platform data and global
GPIO numberspace, we simply get the named "extcon" GPIO directly from
the device. Cut away "active low" since GPIO descriptors already know
if the line is active high or low. Simplify a bit with a
struct device *dev helper variable in probe() and cut the complex
init() function.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/extcon/extcon-gpio.c | 66 ++++++++++----------------------------------
 1 file changed, 15 insertions(+), 51 deletions(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 9c4094edd123..86f3ec6d6014 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -18,7 +18,6 @@
  */
 
 #include <linux/extcon.h>
-#include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -35,12 +34,8 @@
  * @work:		Work fired by the interrupt.
  * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
  *			value.
- * @id_gpiod:		GPIO descriptor for this external connector.
+ * @gpiod:		GPIO descriptor for this external connector.
  * @extcon_id:		The unique id of specific external connector.
- * @gpio:		Corresponding GPIO.
- * @gpio_active_low:	Boolean describing whether gpio active state is 1 or 0
- *			If true, low state of gpio means active.
- *			If false, high state of gpio means active.
  * @debounce:		Debounce time for GPIO IRQ in ms.
  * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
  * @check_on_resume:	Boolean describing whether to check the state of gpio
@@ -51,10 +46,8 @@ struct gpio_extcon_data {
 	int irq;
 	struct delayed_work work;
 	unsigned long debounce_jiffies;
-	struct gpio_desc *id_gpiod;
+	struct gpio_desc *gpiod;
 	unsigned int extcon_id;
-	unsigned gpio;
-	bool gpio_active_low;
 	unsigned long debounce;
 	unsigned long irq_flags;
 	bool check_on_resume;
@@ -67,10 +60,7 @@ static void gpio_extcon_work(struct work_struct *work)
 		container_of(to_delayed_work(work), struct gpio_extcon_data,
 			     work);
 
-	state = gpiod_get_value_cansleep(data->id_gpiod);
-	if (data->gpio_active_low)
-		state = !state;
-
+	state = gpiod_get_value_cansleep(data->gpiod);
 	extcon_set_state_sync(data->edev, data->extcon_id, state);
 }
 
@@ -83,60 +73,34 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int gpio_extcon_init(struct device *dev, struct gpio_extcon_data *data)
-{
-	int ret;
-
-	ret = devm_gpio_request_one(dev, data->gpio, GPIOF_DIR_IN,
-				dev_name(dev));
-	if (ret < 0)
-		return ret;
-
-	data->id_gpiod = gpio_to_desc(data->gpio);
-	if (!data->id_gpiod)
-		return -EINVAL;
-
-	if (data->debounce) {
-		ret = gpiod_set_debounce(data->id_gpiod,
-					 data->debounce * 1000);
-		if (ret < 0)
-			data->debounce_jiffies =
-				msecs_to_jiffies(data->debounce);
-	}
-
-	data->irq = gpiod_to_irq(data->id_gpiod);
-	if (data->irq < 0)
-		return data->irq;
-
-	return 0;
-}
-
 static int gpio_extcon_probe(struct platform_device *pdev)
 {
 	struct gpio_extcon_data *data;
+	struct device *dev = &pdev->dev;
 	int ret;
 
-	data = devm_kzalloc(&pdev->dev, sizeof(struct gpio_extcon_data),
-				   GFP_KERNEL);
+	data = devm_kzalloc(dev, sizeof(struct gpio_extcon_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
 	if (!data->irq_flags || data->extcon_id > EXTCON_NONE)
 		return -EINVAL;
 
-	/* Initialize the gpio */
-	ret = gpio_extcon_init(&pdev->dev, data);
-	if (ret < 0)
-		return ret;
+	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
+	if (IS_ERR(data->gpiod))
+		return PTR_ERR(data->gpiod);
+	data->irq = gpiod_to_irq(data->gpiod);
+	if (data->irq <= 0)
+		return data->irq;
 
 	/* Allocate the memory of extcon devie and register extcon device */
-	data->edev = devm_extcon_dev_allocate(&pdev->dev, &data->extcon_id);
+	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
 	if (IS_ERR(data->edev)) {
-		dev_err(&pdev->dev, "failed to allocate extcon device\n");
+		dev_err(dev, "failed to allocate extcon device\n");
 		return -ENOMEM;
 	}
 
-	ret = devm_extcon_dev_register(&pdev->dev, data->edev);
+	ret = devm_extcon_dev_register(dev, data->edev);
 	if (ret < 0)
 		return ret;
 
@@ -146,7 +110,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	 * Request the interrupt of gpio to detect whether external connector
 	 * is attached or detached.
 	 */
-	ret = devm_request_any_context_irq(&pdev->dev, data->irq,
+	ret = devm_request_any_context_irq(dev, data->irq,
 					gpio_irq_handler, data->irq_flags,
 					pdev->name, data);
 	if (ret < 0)
-- 
2.13.5

[PATCH 5/8] extcon: gpio: Request reasonable interrupts

[Linus Walleij]

The only thing that makes sense is to request a falling edge interrupt
if the line is active low and a rising edge interrupt if the line is
active high, so just do that and get rid of the assignment from
platform data. The GPIO descriptor knows if the line is active high
or low.

Also make irq a local variable in probe(), it's not used anywhere else.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/extcon/extcon-gpio.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 86f3ec6d6014..6d9cb4ed11c2 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -30,26 +30,22 @@
 /**
  * struct gpio_extcon_data - A simple GPIO-controlled extcon device state container.
  * @edev:		Extcon device.
- * @irq:		Interrupt line for the external connector.
  * @work:		Work fired by the interrupt.
  * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
  *			value.
  * @gpiod:		GPIO descriptor for this external connector.
  * @extcon_id:		The unique id of specific external connector.
  * @debounce:		Debounce time for GPIO IRQ in ms.
- * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
  * @check_on_resume:	Boolean describing whether to check the state of gpio
  *			while resuming from sleep.
  */
 struct gpio_extcon_data {
 	struct extcon_dev *edev;
-	int irq;
 	struct delayed_work work;
 	unsigned long debounce_jiffies;
 	struct gpio_desc *gpiod;
 	unsigned int extcon_id;
 	unsigned long debounce;
-	unsigned long irq_flags;
 	bool check_on_resume;
 };
 
@@ -77,21 +73,34 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 {
 	struct gpio_extcon_data *data;
 	struct device *dev = &pdev->dev;
+	unsigned long irq_flags;
+	int irq;
 	int ret;
 
 	data = devm_kzalloc(dev, sizeof(struct gpio_extcon_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	if (!data->irq_flags || data->extcon_id > EXTCON_NONE)
+	if (data->extcon_id > EXTCON_NONE)
 		return -EINVAL;
 
 	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
 	if (IS_ERR(data->gpiod))
 		return PTR_ERR(data->gpiod);
-	data->irq = gpiod_to_irq(data->gpiod);
-	if (data->irq <= 0)
-		return data->irq;
+	irq = gpiod_to_irq(data->gpiod);
+	if (irq <= 0)
+		return irq;
+
+	/*
+	 * It is unlikely that this is an acknowledged interrupt that goes
+	 * away after handling, what we are looking for are falling edges
+	 * if the signal is active low, and rising edges if the signal is
+	 * active high.
+	 */
+	if (gpiod_is_active_low(data->gpiod))
+		irq_flags = IRQF_TRIGGER_FALLING;
+	else
+		irq_flags = IRQF_TRIGGER_RISING;
 
 	/* Allocate the memory of extcon devie and register extcon device */
 	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
@@ -110,8 +119,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	 * Request the interrupt of gpio to detect whether external connector
 	 * is attached or detached.
 	 */
-	ret = devm_request_any_context_irq(dev, data->irq,
-					gpio_irq_handler, data->irq_flags,
+	ret = devm_request_any_context_irq(dev, irq,
+					gpio_irq_handler, irq_flags,
 					pdev->name, data);
 	if (ret < 0)
 		return ret;
-- 
2.13.5

[PATCH 6/8] extcon: gpio: Get debounce setting from device property

[Linus Walleij]

Look up the debouncing value using the device property that will
pick it from device tree or ACPI DSDT or whatever is available.

Reintroduce the debounce handling previously deleted in the
series, setting the delayed worker to delay 0 ms if the GPIO
driver supports debouncing for us, else just delay the reading
of the value delayed by jiffies.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/extcon/extcon-gpio.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 6d9cb4ed11c2..8fc52631c8a2 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
 
@@ -35,7 +36,6 @@
  *			value.
  * @gpiod:		GPIO descriptor for this external connector.
  * @extcon_id:		The unique id of specific external connector.
- * @debounce:		Debounce time for GPIO IRQ in ms.
  * @check_on_resume:	Boolean describing whether to check the state of gpio
  *			while resuming from sleep.
  */
@@ -45,7 +45,6 @@ struct gpio_extcon_data {
 	unsigned long debounce_jiffies;
 	struct gpio_desc *gpiod;
 	unsigned int extcon_id;
-	unsigned long debounce;
 	bool check_on_resume;
 };
 
@@ -74,6 +73,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	struct gpio_extcon_data *data;
 	struct device *dev = &pdev->dev;
 	unsigned long irq_flags;
+	u32 debounce_usecs;
 	int irq;
 	int ret;
 
@@ -109,6 +109,15 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	ret = device_property_read_u32(dev, "input-debounce", &debounce_usecs);
+	if (ret || !debounce_usecs) {
+		dev_err(dev, "illegal debounce value, set to 20 ms\n");
+		debounce_usecs = 20000;
+	}
+	ret = gpiod_set_debounce(data->gpiod, debounce_usecs);
+	if (ret)
+		data->debounce_jiffies = msecs_to_jiffies(debounce_usecs * 1000);
+
 	ret = devm_extcon_dev_register(dev, data->edev);
 	if (ret < 0)
 		return ret;
-- 
2.13.5

[PATCH 7/8] extcon: gpio: Get connector type from device property

[Linus Walleij]

We do not use the "EXTCON_NONE" type to report this as before, use
the connector type defined in the device property, from device tree or
ACPI DSDT.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/extcon/extcon-gpio.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 8fc52631c8a2..b7353f5018b5 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -35,7 +35,8 @@
  * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
  *			value.
  * @gpiod:		GPIO descriptor for this external connector.
- * @extcon_id:		The unique id of specific external connector.
+ * @connector_type:	The connector type we're detecting on this extcon, terminated with EXTCON_NONE
+ *			One GPIO is one cable, so one type only.
  * @check_on_resume:	Boolean describing whether to check the state of gpio
  *			while resuming from sleep.
  */
@@ -44,7 +45,7 @@ struct gpio_extcon_data {
 	struct delayed_work work;
 	unsigned long debounce_jiffies;
 	struct gpio_desc *gpiod;
-	unsigned int extcon_id;
+	unsigned int connector_type[2];
 	bool check_on_resume;
 };
 
@@ -56,7 +57,7 @@ static void gpio_extcon_work(struct work_struct *work)
 			     work);
 
 	state = gpiod_get_value_cansleep(data->gpiod);
-	extcon_set_state_sync(data->edev, data->extcon_id, state);
+	extcon_set_state_sync(data->edev, data->connector_type[0], state);
 }
 
 static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
@@ -74,6 +75,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	unsigned long irq_flags;
 	u32 debounce_usecs;
+	u32 connector_type;
 	int irq;
 	int ret;
 
@@ -81,9 +83,6 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
-	if (data->extcon_id > EXTCON_NONE)
-		return -EINVAL;
-
 	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
 	if (IS_ERR(data->gpiod))
 		return PTR_ERR(data->gpiod);
@@ -102,8 +101,16 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	else
 		irq_flags = IRQF_TRIGGER_RISING;
 
+	ret = device_property_read_u32(dev, "extcon-connector-types", &connector_type);
+	if (ret || !connector_type) {
+		dev_err(dev, "illegal cable type or undefined cable type\n");
+		return -EINVAL;
+	}
+	data->connector_type[0] = connector_type;
+	data->connector_type[1] = EXTCON_NONE;
+
 	/* Allocate the memory of extcon devie and register extcon device */
-	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
+	data->edev = devm_extcon_dev_allocate(dev, data->connector_type);
 	if (IS_ERR(data->edev)) {
 		dev_err(dev, "failed to allocate extcon device\n");
 		return -ENOMEM;
-- 
2.13.5

[PATCH 8/8] extcon: gpio: Always check state on resume

[Linus Walleij]

It makes most sense to always check the state of the GPIO external
connector at system resume so just do this by default. Add a TODO
if people turn out to desire to parameterize this.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/extcon/extcon-gpio.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index b7353f5018b5..4f0ad5ad2722 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -83,6 +83,15 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	/*
+	 * Always checking connector state on resume makes most sense so do this
+	 * by default.
+	 *
+	 * TODO: if parameterization is needed, augment this to use proper device
+	 * properties or set it up from PM core.
+	 */
+	data->check_on_resume = true;
+
 	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
 	if (IS_ERR(data->gpiod))
 		return PTR_ERR(data->gpiod);
-- 
2.13.5

Re: [PATCH 1/8] extcon: gpio: Add DT bindings

[Rob Herring]

On Sun, Sep 24, 2017 at 9:56 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Add some reasonable device tree bindings and also add the cable defines
> to the extcon include in <dt-bindings/extcon/connectors.h> since
> the GPIO extcon definately need to specify which cable/connector it is
> detecting.
>
> Adding the include file makes the (as it happens) Linux numbers into an
> ABI, but I do not see any better method. It is possible to define strings
> for all cable types but it seems like overkill, just reusing Linux'
> enumerators seems like a good idea.
>
> The binding supports any number of GPIOs and connectors, but the driver
> currently only supports one connector on one GPIO line.

My view of extcon binding is it is fundamentally broken. I've
expressed this multiple times before.

TL;DR: Anything extending the existing extcon binding will be NAKed.

>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../devicetree/bindings/extcon/extcon-gpio.txt     | 24 ++++++++++++++
>  include/dt-bindings/extcon/connectors.h            | 38 ++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
>  create mode 100644 include/dt-bindings/extcon/connectors.h
>
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> new file mode 100644
> index 000000000000..2f5e21b94a64
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> @@ -0,0 +1,24 @@
> +External Connector Using GPIO

What kind of connector? All connectors external to something... And
GPIO is not a kind of connector on its own, but an implementation.

> +
> +Required properties:
> +- compatible: should be "extcon-gpio"
> +- extcon-gpios: the GPIO lines used for the external connectors

This doesn't tell me what the GPIOs functions are and should. For
example we have hpd-gpios for HDMI hotplug detect in HDMI connector
binding.

> +  See gpio/gpio.txt
> +- extcon-connector-types: set to an unsigned integer value arrat representing the types
> +  of this connector, matched to the corresponding GPIO lines in the previous array.

This should be determined by the compatible string.

> +  Those are defined with unique IDs in <dt-bindings/extcon/connectors.h>
> +- input-debounce: The number of microseconds to wait for the
> +  connector state to stabilize. This property is reused from pin control
> +  See pinctrl/pinctrl-bindings.txt
> +
> +Example:
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/extcon/connectors.h>
> +
> +extcon {
> +       compatible = "extcon-gpio";
> +       extcon-gpios = <&gpio0 42 GPIO_ACTIVE_LOW>;
> +       extcon-connector-types = <EXTCON_USB>;
> +       input-debounce = <20000>; /* 20 ms */
> +};
> diff --git a/include/dt-bindings/extcon/connectors.h b/include/dt-bindings/extcon/connectors.h
> new file mode 100644
> index 000000000000..61bed24eaadc
> --- /dev/null
> +++ b/include/dt-bindings/extcon/connectors.h
> @@ -0,0 +1,38 @@
> +#ifndef _DT_BINDINGS_EXTCON_CONNECTORS_H
> +#define _DT_BINDINGS_EXTCON_CONNECTORS_H
> +
> +/* USB external connector */
> +#define EXTCON_USB             1
> +#define EXTCON_USB_HOST                2
> +#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
> +#define EXTCON_CHG_WPT         11      /* Wireless Power Transfer */
> +#define EXTCON_CHG_USB_PD      12      /* USB Power Delivery */

These don't all look to be mutually exclusive.

For USB PD, isn't that discoverable?

> +/* 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 */
> +#define EXTCON_DISP_DP         44      /* Display Port */
> +#define EXTCON_DISP_HMD                45      /* Head-Mounted Display */

We already have connector bindings for most of these. Use those as a
model for whatever you want to do.

> +/* Miscellaneous external connector */
> +#define EXTCON_DOCK            60
> +#define EXTCON_JIG             61
> +#define EXTCON_MECHANICAL      62
> +
> +#define EXTCON_NUM             63
> +
> +#endif /* _DT_BINDINGS_EXTCON_CONNECTORS_H */
> --
> 2.13.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] extcon: gpio: Add DT bindings

[Linus Walleij]

On Sun, Sep 24, 2017 at 9:56 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Sun, Sep 24, 2017 at 9:56 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> Add some reasonable device tree bindings and also add the cable defines
>> to the extcon include in <dt-bindings/extcon/connectors.h> since
>> the GPIO extcon definately need to specify which cable/connector it is
>> detecting.
>>
>> Adding the include file makes the (as it happens) Linux numbers into an
>> ABI, but I do not see any better method. It is possible to define strings
>> for all cable types but it seems like overkill, just reusing Linux'
>> enumerators seems like a good idea.
>>
>> The binding supports any number of GPIOs and connectors, but the driver
>> currently only supports one connector on one GPIO line.
>
> My view of extcon binding is it is fundamentally broken. I've
> expressed this multiple times before.

Sorry, I'm a newcomer in this area, so I was not aware of this.

Since this is a new binding consumer, is it something we can use
as role model to fix it?

If I understand correctly from reading up on the mailing list the root of the
problem is something like this:

"extcon" is a linuxism and ambiguous.

This driver should probe to "gpio-connector" or "gpio-switch" or something
like that if it should be generic. Or use very domain-specific compatible
strings (as you describe below), all supported maybe by the same driver
in the end.

The reason it is its own subsystem and not part of input (IIUC) is that
other drivers need to subscribe to events from these connectors,
they are not intended for userspace input such as keyboard or mice
or similar.

In the DTS file you will find stuff using extcons as resources with
 = <&extcon>; phandles so they can look them up and subscribe
to events.

Input has a whole slew of "events" that correspond to some of these
but a different usecase, but that usecase is just a linuxism in turn, there
is nothing saying another operating system could have a more versatile
defintion of "input".

Maybe from a hardware description PoV these should all move over
to devicetree/bindings/input - they all provice an input signal to the
system. The fact that Linux split a subset of these into "extcon"
is of no concern to the DT bindings.

Analogous with that some of the stuff in input/ should likely be
moved to a new output/ directory. Such as pwm-beeper,
pwm-vibrator etc. The fact that these are in the "input" subsystem
in Linux is just another linuxism.

> TL;DR: Anything extending the existing extcon binding will be NAKed.

That can be a reasonable stance.

I'm just trying to get this into a state where the code does not stand in
the way of kernel clean-up. (Especially I want to get rid of the call
to gpio_to_desc())

As stated in the cover letter the alternative will be to simply delete
the driver. But it's better if I can fix the situation, we can't have it
like this.

>> +External Connector Using GPIO
>
> What kind of connector? All connectors external to something... And
> GPIO is not a kind of connector on its own, but an implementation.

Yeah that is a problem with the whole subsystem I guess. Should
I add a paragraph describing the usecases?

The whole thing is a bit
of Androidism and is named like this because Android named it like
this in their kernel tree.

>> +Required properties:
>> +- compatible: should be "extcon-gpio"
>> +- extcon-gpios: the GPIO lines used for the external connectors
>
> This doesn't tell me what the GPIOs functions are and should. For
> example we have hpd-gpios for HDMI hotplug detect in HDMI connector
> binding.

The idea is that this array corresponds to the extcon-connector-types
right below, I'll clarify that if you think the overall idea is OK.

>> +  See gpio/gpio.txt
>> +- extcon-connector-types: set to an unsigned integer value arrat representing the types
>> +  of this connector, matched to the corresponding GPIO lines in the previous array.
>
> This should be determined by the compatible string.

So a GPIO connector is very versatile. It is "general purpose" by definition,
so it needs to be subclassed.

One possibility is to allow just one GPIO and have one comptible-string per
use case.
compatible = "gpio-usb-connector";
compatible = "gpio-charger-connector";
compatible = "gpio-headphone-connector";
etc etc

These bindings on the other hand, assume that the driver will be able
to handle an array of gpios with an associated array of connector types,
which reflects how many of the existing extcon-type driver hardware works:
a single PMIC providing some 5 misc extcons for example.

For this reason there is a generic compatible string and then the device
is subclassed per-gpio with the per-gpio connector type.

>> +/* USB external connector */
>> +#define EXTCON_USB             1
>> +#define EXTCON_USB_HOST                2
>> +#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
>> +#define EXTCON_CHG_WPT         11      /* Wireless Power Transfer */
>> +#define EXTCON_CHG_USB_PD      12      /* USB Power Delivery */
>
> These don't all look to be mutually exclusive.
>
> For USB PD, isn't that discoverable?

Someone else would have to answer, this is based on the current
connector types supported by the Linux kernel.

>> +/* 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 */
>> +#define EXTCON_DISP_DP         44      /* Display Port */
>> +#define EXTCON_DISP_HMD                45      /* Head-Mounted Display */
>
> We already have connector bindings for most of these. Use those as a
> model for whatever you want to do.

I guess they are not in Documentation/devicetree/bindings/extcon/*

Please point me in the right direction and I'll check it out.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] extcon: gpio: Add DT bindings

[Linus Walleij]

On Sun, Sep 24, 2017 at 9:56 PM, Rob Herring <robh@kernel.org> wrote:
> On Sun, Sep 24, 2017 at 9:56 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Add some reasonable device tree bindings and also add the cable defines
>> to the extcon include in <dt-bindings/extcon/connectors.h> since
>> the GPIO extcon definately need to specify which cable/connector it is
>> detecting.
>>
>> Adding the include file makes the (as it happens) Linux numbers into an
>> ABI, but I do not see any better method. It is possible to define strings
>> for all cable types but it seems like overkill, just reusing Linux'
>> enumerators seems like a good idea.
>>
>> The binding supports any number of GPIOs and connectors, but the driver
>> currently only supports one connector on one GPIO line.
>
> My view of extcon binding is it is fundamentally broken. I've
> expressed this multiple times before.

Sorry, I'm a newcomer in this area, so I was not aware of this.

Since this is a new binding consumer, is it something we can use
as role model to fix it?

If I understand correctly from reading up on the mailing list the root of the
problem is something like this:

"extcon" is a linuxism and ambiguous.

This driver should probe to "gpio-connector" or "gpio-switch" or something
like that if it should be generic. Or use very domain-specific compatible
strings (as you describe below), all supported maybe by the same driver
in the end.

The reason it is its own subsystem and not part of input (IIUC) is that
other drivers need to subscribe to events from these connectors,
they are not intended for userspace input such as keyboard or mice
or similar.

In the DTS file you will find stuff using extcons as resources with
 = <&extcon>; phandles so they can look them up and subscribe
to events.

Input has a whole slew of "events" that correspond to some of these
but a different usecase, but that usecase is just a linuxism in turn, there
is nothing saying another operating system could have a more versatile
defintion of "input".

Maybe from a hardware description PoV these should all move over
to devicetree/bindings/input - they all provice an input signal to the
system. The fact that Linux split a subset of these into "extcon"
is of no concern to the DT bindings.

Analogous with that some of the stuff in input/ should likely be
moved to a new output/ directory. Such as pwm-beeper,
pwm-vibrator etc. The fact that these are in the "input" subsystem
in Linux is just another linuxism.

> TL;DR: Anything extending the existing extcon binding will be NAKed.

That can be a reasonable stance.

I'm just trying to get this into a state where the code does not stand in
the way of kernel clean-up. (Especially I want to get rid of the call
to gpio_to_desc())

As stated in the cover letter the alternative will be to simply delete
the driver. But it's better if I can fix the situation, we can't have it
like this.

>> +External Connector Using GPIO
>
> What kind of connector? All connectors external to something... And
> GPIO is not a kind of connector on its own, but an implementation.

Yeah that is a problem with the whole subsystem I guess. Should
I add a paragraph describing the usecases?

The whole thing is a bit
of Androidism and is named like this because Android named it like
this in their kernel tree.

>> +Required properties:
>> +- compatible: should be "extcon-gpio"
>> +- extcon-gpios: the GPIO lines used for the external connectors
>
> This doesn't tell me what the GPIOs functions are and should. For
> example we have hpd-gpios for HDMI hotplug detect in HDMI connector
> binding.

The idea is that this array corresponds to the extcon-connector-types
right below, I'll clarify that if you think the overall idea is OK.

>> +  See gpio/gpio.txt
>> +- extcon-connector-types: set to an unsigned integer value arrat representing the types
>> +  of this connector, matched to the corresponding GPIO lines in the previous array.
>
> This should be determined by the compatible string.

So a GPIO connector is very versatile. It is "general purpose" by definition,
so it needs to be subclassed.

One possibility is to allow just one GPIO and have one comptible-string per
use case.
compatible = "gpio-usb-connector";
compatible = "gpio-charger-connector";
compatible = "gpio-headphone-connector";
etc etc

These bindings on the other hand, assume that the driver will be able
to handle an array of gpios with an associated array of connector types,
which reflects how many of the existing extcon-type driver hardware works:
a single PMIC providing some 5 misc extcons for example.

For this reason there is a generic compatible string and then the device
is subclassed per-gpio with the per-gpio connector type.

>> +/* USB external connector */
>> +#define EXTCON_USB             1
>> +#define EXTCON_USB_HOST                2
>> +#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
>> +#define EXTCON_CHG_WPT         11      /* Wireless Power Transfer */
>> +#define EXTCON_CHG_USB_PD      12      /* USB Power Delivery */
>
> These don't all look to be mutually exclusive.
>
> For USB PD, isn't that discoverable?

Someone else would have to answer, this is based on the current
connector types supported by the Linux kernel.

>> +/* 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 */
>> +#define EXTCON_DISP_DP         44      /* Display Port */
>> +#define EXTCON_DISP_HMD                45      /* Head-Mounted Display */
>
> We already have connector bindings for most of these. Use those as a
> model for whatever you want to do.

I guess they are not in Documentation/devicetree/bindings/extcon/*

Please point me in the right direction and I'll check it out.

Yours,
Linus Walleij

Re: [PATCH 1/8] extcon: gpio: Add DT bindings

[Chanwoo Choi]

Hi Rob,

On 2017년 09월 26일 09:39, Linus Walleij wrote:
> On Sun, Sep 24, 2017 at 9:56 PM, Rob Herring <robh@kernel.org> wrote:
>> On Sun, Sep 24, 2017 at 9:56 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> Add some reasonable device tree bindings and also add the cable defines
>>> to the extcon include in <dt-bindings/extcon/connectors.h> since
>>> the GPIO extcon definately need to specify which cable/connector it is
>>> detecting.
>>>
>>> Adding the include file makes the (as it happens) Linux numbers into an
>>> ABI, but I do not see any better method. It is possible to define strings
>>> for all cable types but it seems like overkill, just reusing Linux'
>>> enumerators seems like a good idea.
>>>
>>> The binding supports any number of GPIOs and connectors, but the driver
>>> currently only supports one connector on one GPIO line.
>>
>> My view of extcon binding is it is fundamentally broken. I've
>> expressed this multiple times before.
> 
> Sorry, I'm a newcomer in this area, so I was not aware of this.
> 
> Since this is a new binding consumer, is it something we can use
> as role model to fix it?
> 
> If I understand correctly from reading up on the mailing list the root of the
> problem is something like this:
> 
> "extcon" is a linuxism and ambiguous.
> 
> This driver should probe to "gpio-connector" or "gpio-switch" or something
> like that if it should be generic. Or use very domain-specific compatible
> strings (as you describe below), all supported maybe by the same driver
> in the end.
> 
> The reason it is its own subsystem and not part of input (IIUC) is that
> other drivers need to subscribe to events from these connectors,
> they are not intended for userspace input such as keyboard or mice
> or similar.
> 
> In the DTS file you will find stuff using extcons as resources with
>  = <&extcon>; phandles so they can look them up and subscribe
> to events.
> 
> Input has a whole slew of "events" that correspond to some of these
> but a different usecase, but that usecase is just a linuxism in turn, there
> is nothing saying another operating system could have a more versatile
> defintion of "input".
> 
> Maybe from a hardware description PoV these should all move over
> to devicetree/bindings/input - they all provice an input signal to the
> system. The fact that Linux split a subset of these into "extcon"
> is of no concern to the DT bindings.
> 
> Analogous with that some of the stuff in input/ should likely be
> moved to a new output/ directory. Such as pwm-beeper,
> pwm-vibrator etc. The fact that these are in the "input" subsystem
> in Linux is just another linuxism.
> 
>> TL;DR: Anything extending the existing extcon binding will be NAKed.
> 
> That can be a reasonable stance.
> 
> I'm just trying to get this into a state where the code does not stand in
> the way of kernel clean-up. (Especially I want to get rid of the call
> to gpio_to_desc())
> 
> As stated in the cover letter the alternative will be to simply delete
> the driver. But it's better if I can fix the situation, we can't have it
> like this.
> 
>>> +External Connector Using GPIO
>>
>> What kind of connector? All connectors external to something... And
>> GPIO is not a kind of connector on its own, but an implementation.
> 
> Yeah that is a problem with the whole subsystem I guess. Should
> I add a paragraph describing the usecases?
> 
> The whole thing is a bit
> of Androidism and is named like this because Android named it like
> this in their kernel tree.
> 
>>> +Required properties:
>>> +- compatible: should be "extcon-gpio"
>>> +- extcon-gpios: the GPIO lines used for the external connectors
>>
>> This doesn't tell me what the GPIOs functions are and should. For
>> example we have hpd-gpios for HDMI hotplug detect in HDMI connector
>> binding.
> 
> The idea is that this array corresponds to the extcon-connector-types
> right below, I'll clarify that if you think the overall idea is OK.
> 
>>> +  See gpio/gpio.txt
>>> +- extcon-connector-types: set to an unsigned integer value arrat representing the types
>>> +  of this connector, matched to the corresponding GPIO lines in the previous array.
>>
>> This should be determined by the compatible string.
> 
> So a GPIO connector is very versatile. It is "general purpose" by definition,
> so it needs to be subclassed.
> 
> One possibility is to allow just one GPIO and have one comptible-string per
> use case.
> compatible = "gpio-usb-connector";
> compatible = "gpio-charger-connector";
> compatible = "gpio-headphone-connector";
> etc etc
> 
> These bindings on the other hand, assume that the driver will be able
> to handle an array of gpios with an associated array of connector types,
> which reflects how many of the existing extcon-type driver hardware works:
> a single PMIC providing some 5 misc extcons for example.
> 
> For this reason there is a generic compatible string and then the device
> is subclassed per-gpio with the per-gpio connector type.
> 
>>> +/* USB external connector */
>>> +#define EXTCON_USB             1
>>> +#define EXTCON_USB_HOST                2
>>> +#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
>>> +#define EXTCON_CHG_WPT         11      /* Wireless Power Transfer */
>>> +#define EXTCON_CHG_USB_PD      12      /* USB Power Delivery */
>>
>> These don't all look to be mutually exclusive.
>>
>> For USB PD, isn't that discoverable?

Currently, EXTCON_CHG_USB_PD is not used on any extcon provider drivers.
I think that EXTCON_CHG_USB_PD is discoverable according to the H/W design
such as using ADC.

Also, The charger connectors of extcon are related to power_supply subsystem
because power_supply is in charge of behavior when the connector is attached/detached.
So, the extcon defines the EXTCON_CHG_USB_PD in order to detect this type.

> 
> Someone else would have to answer, this is based on the current
> connector types supported by the Linux kernel.
> 
>>> +/* 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 */
>>> +#define EXTCON_DISP_DP         44      /* Display Port */
>>> +#define EXTCON_DISP_HMD                45      /* Head-Mounted Display */
>>
>> We already have connector bindings for most of these. Use those as a
>> model for whatever you want to do.
> 
> I guess they are not in Documentation/devicetree/bindings/extcon/*
> 
> Please point me in the right direction and I'll check it out.
> 
> Yours,
> Linus Walleij
> 
> 
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/8] extcon: gpio: Localize platform data

[Chanwoo Choi]

Hi Linus,

On 2017년 09월 24일 23:56, Linus Walleij wrote:
> Nothing in the entire kernel #includes <linux/extcon/extcon-gpio.h>
> so move the platform data declaration inside of the driver.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/extcon/extcon-gpio.c       | 22 +++++++++++++++++-
>  include/linux/extcon/extcon-gpio.h | 47 --------------------------------------
>  2 files changed, 21 insertions(+), 48 deletions(-)
>  delete mode 100644 include/linux/extcon/extcon-gpio.h
> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index ebed22f22d75..6abf5f70fdbf 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -18,7 +18,6 @@

Actually, extcon-gpio.h defines the 'struct gpio_extcon_pdata'
in order to get the gpio and interrupt data from platform_data
before device-tree binding method. But, as you mentioned,
it is not used on kernel with platform_data method.

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

[snip]

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 3/8] extcon: gpio: Move platform data into state container

[Chanwoo Choi]

Hi Linus,

On 2017년 09월 24일 23:56, Linus Walleij wrote:
> This moves the platform data settings from the platform data
> struct and into the state container, saving some unnecessary
> references and simplifying things a bit.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/extcon/extcon-gpio.c | 56 ++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 30 deletions(-)

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 6abf5f70fdbf..9c4094edd123 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -29,7 +29,13 @@
>  #include <linux/workqueue.h>
>  
>  /**
> - * struct gpio_extcon_pdata - A simple GPIO-controlled extcon device.
> + * struct gpio_extcon_data - A simple GPIO-controlled extcon device state container.
> + * @edev:		Extcon device.
> + * @irq:		Interrupt line for the external connector.
> + * @work:		Work fired by the interrupt.
> + * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
> + *			value.
> + * @id_gpiod:		GPIO descriptor for this external connector.
>   * @extcon_id:		The unique id of specific external connector.
>   * @gpio:		Corresponding GPIO.
>   * @gpio_active_low:	Boolean describing whether gpio active state is 1 or 0
> @@ -40,23 +46,18 @@
>   * @check_on_resume:	Boolean describing whether to check the state of gpio
>   *			while resuming from sleep.
>   */
> -struct gpio_extcon_pdata {
> -	unsigned int extcon_id;
> -	unsigned gpio;
> -	bool gpio_active_low;
> -	unsigned long debounce;
> -	unsigned long irq_flags;
> -	bool check_on_resume;
> -};
> -
>  struct gpio_extcon_data {
>  	struct extcon_dev *edev;
>  	int irq;
>  	struct delayed_work work;
>  	unsigned long debounce_jiffies;
> -
>  	struct gpio_desc *id_gpiod;
> -	struct gpio_extcon_pdata *pdata;
> +	unsigned int extcon_id;
> +	unsigned gpio;
> +	bool gpio_active_low;
> +	unsigned long debounce;
> +	unsigned long irq_flags;
> +	bool check_on_resume;
>  };
>  
>  static void gpio_extcon_work(struct work_struct *work)
> @@ -67,10 +68,10 @@ static void gpio_extcon_work(struct work_struct *work)
>  			     work);
>  
>  	state = gpiod_get_value_cansleep(data->id_gpiod);
> -	if (data->pdata->gpio_active_low)
> +	if (data->gpio_active_low)
>  		state = !state;
>  
> -	extcon_set_state_sync(data->edev, data->pdata->extcon_id, state);
> +	extcon_set_state_sync(data->edev, data->extcon_id, state);
>  }
>  
>  static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
> @@ -84,24 +85,23 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
>  
>  static int gpio_extcon_init(struct device *dev, struct gpio_extcon_data *data)
>  {
> -	struct gpio_extcon_pdata *pdata = data->pdata;
>  	int ret;
>  
> -	ret = devm_gpio_request_one(dev, pdata->gpio, GPIOF_DIR_IN,
> +	ret = devm_gpio_request_one(dev, data->gpio, GPIOF_DIR_IN,
>  				dev_name(dev));
>  	if (ret < 0)
>  		return ret;
>  
> -	data->id_gpiod = gpio_to_desc(pdata->gpio);
> +	data->id_gpiod = gpio_to_desc(data->gpio);
>  	if (!data->id_gpiod)
>  		return -EINVAL;
>  
> -	if (pdata->debounce) {
> +	if (data->debounce) {
>  		ret = gpiod_set_debounce(data->id_gpiod,
> -					pdata->debounce * 1000);
> +					 data->debounce * 1000);
>  		if (ret < 0)
>  			data->debounce_jiffies =
> -				msecs_to_jiffies(pdata->debounce);
> +				msecs_to_jiffies(data->debounce);
>  	}
>  
>  	data->irq = gpiod_to_irq(data->id_gpiod);
> @@ -113,20 +113,16 @@ static int gpio_extcon_init(struct device *dev, struct gpio_extcon_data *data)
>  
>  static int gpio_extcon_probe(struct platform_device *pdev)
>  {
> -	struct gpio_extcon_pdata *pdata = dev_get_platdata(&pdev->dev);
>  	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);
>  	if (!data)
>  		return -ENOMEM;
> -	data->pdata = pdata;
> +
> +	if (!data->irq_flags || data->extcon_id > EXTCON_NONE)
> +		return -EINVAL;
>  
>  	/* Initialize the gpio */
>  	ret = gpio_extcon_init(&pdev->dev, data);
> @@ -134,7 +130,7 @@ 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->extcon_id);
>  	if (IS_ERR(data->edev)) {
>  		dev_err(&pdev->dev, "failed to allocate extcon device\n");
>  		return -ENOMEM;
> @@ -151,7 +147,7 @@ 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->irq_flags,
>  					pdev->name, data);
>  	if (ret < 0)
>  		return ret;
> @@ -178,7 +174,7 @@ static int gpio_extcon_resume(struct device *dev)
>  	struct gpio_extcon_data *data;
>  
>  	data = dev_get_drvdata(dev);
> -	if (data->pdata->check_on_resume)
> +	if (data->check_on_resume)
>  		queue_delayed_work(system_power_efficient_wq,
>  			&data->work, data->debounce_jiffies);
>  
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 4/8] extcon: gpio: Convert to fully use GPIO descriptor

[Chanwoo Choi]

Hi Linus,

Looks good to me. But, there is one comment
of gpiod_to_irq()'s return value.

If you modify it, feel free to add my tag:
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

On 2017년 09월 24일 23:56, Linus Walleij wrote:
> Since we are not getting the GPIO from any platform data and global
> GPIO numberspace, we simply get the named "extcon" GPIO directly from
> the device. Cut away "active low" since GPIO descriptors already know
> if the line is active high or low. Simplify a bit with a
> struct device *dev helper variable in probe() and cut the complex
> init() function.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/extcon/extcon-gpio.c | 66 ++++++++++----------------------------------
>  1 file changed, 15 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 9c4094edd123..86f3ec6d6014 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -18,7 +18,6 @@
>   */
>  
>  #include <linux/extcon.h>
> -#include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> @@ -35,12 +34,8 @@
>   * @work:		Work fired by the interrupt.
>   * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
>   *			value.
> - * @id_gpiod:		GPIO descriptor for this external connector.
> + * @gpiod:		GPIO descriptor for this external connector.
>   * @extcon_id:		The unique id of specific external connector.
> - * @gpio:		Corresponding GPIO.
> - * @gpio_active_low:	Boolean describing whether gpio active state is 1 or 0
> - *			If true, low state of gpio means active.
> - *			If false, high state of gpio means active.
>   * @debounce:		Debounce time for GPIO IRQ in ms.
>   * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
>   * @check_on_resume:	Boolean describing whether to check the state of gpio
> @@ -51,10 +46,8 @@ struct gpio_extcon_data {
>  	int irq;
>  	struct delayed_work work;
>  	unsigned long debounce_jiffies;
> -	struct gpio_desc *id_gpiod;
> +	struct gpio_desc *gpiod;
>  	unsigned int extcon_id;
> -	unsigned gpio;
> -	bool gpio_active_low;
>  	unsigned long debounce;
>  	unsigned long irq_flags;
>  	bool check_on_resume;
> @@ -67,10 +60,7 @@ static void gpio_extcon_work(struct work_struct *work)
>  		container_of(to_delayed_work(work), struct gpio_extcon_data,
>  			     work);
>  
> -	state = gpiod_get_value_cansleep(data->id_gpiod);
> -	if (data->gpio_active_low)
> -		state = !state;
> -
> +	state = gpiod_get_value_cansleep(data->gpiod);
>  	extcon_set_state_sync(data->edev, data->extcon_id, state);
>  }
>  
> @@ -83,60 +73,34 @@ static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static int gpio_extcon_init(struct device *dev, struct gpio_extcon_data *data)
> -{
> -	int ret;
> -
> -	ret = devm_gpio_request_one(dev, data->gpio, GPIOF_DIR_IN,
> -				dev_name(dev));
> -	if (ret < 0)
> -		return ret;
> -
> -	data->id_gpiod = gpio_to_desc(data->gpio);
> -	if (!data->id_gpiod)
> -		return -EINVAL;
> -
> -	if (data->debounce) {
> -		ret = gpiod_set_debounce(data->id_gpiod,
> -					 data->debounce * 1000);
> -		if (ret < 0)
> -			data->debounce_jiffies =
> -				msecs_to_jiffies(data->debounce);
> -	}
> -
> -	data->irq = gpiod_to_irq(data->id_gpiod);
> -	if (data->irq < 0)
> -		return data->irq;
> -
> -	return 0;
> -}
> -
>  static int gpio_extcon_probe(struct platform_device *pdev)
>  {
>  	struct gpio_extcon_data *data;
> +	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	data = devm_kzalloc(&pdev->dev, sizeof(struct gpio_extcon_data),
> -				   GFP_KERNEL);
> +	data = devm_kzalloc(dev, sizeof(struct gpio_extcon_data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
>  	if (!data->irq_flags || data->extcon_id > EXTCON_NONE)
>  		return -EINVAL;
>  
> -	/* Initialize the gpio */
> -	ret = gpio_extcon_init(&pdev->dev, data);
> -	if (ret < 0)
> -		return ret;
> +	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
> +	if (IS_ERR(data->gpiod))
> +		return PTR_ERR(data->gpiod);
> +	data->irq = gpiod_to_irq(data->gpiod);
> +	if (data->irq <= 0)

"if (data->irq < 0)" is enough. If irq is zero, gpiod_to_irq()
returns the -ENXIO.

> +		return data->irq;
>  
>  	/* Allocate the memory of extcon devie and register extcon device */
> -	data->edev = devm_extcon_dev_allocate(&pdev->dev, &data->extcon_id);
> +	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
>  	if (IS_ERR(data->edev)) {
> -		dev_err(&pdev->dev, "failed to allocate extcon device\n");
> +		dev_err(dev, "failed to allocate extcon device\n");
>  		return -ENOMEM;
>  	}
>  
> -	ret = devm_extcon_dev_register(&pdev->dev, data->edev);
> +	ret = devm_extcon_dev_register(dev, data->edev);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -146,7 +110,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  	 * Request the interrupt of gpio to detect whether external connector
>  	 * is attached or detached.
>  	 */
> -	ret = devm_request_any_context_irq(&pdev->dev, data->irq,
> +	ret = devm_request_any_context_irq(dev, data->irq,
>  					gpio_irq_handler, data->irq_flags,
>  					pdev->name, data);
>  	if (ret < 0)
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 5/8] extcon: gpio: Request reasonable interrupts

[Chanwoo Choi]

Hi Linus,

On 2017년 09월 24일 23:56, Linus Walleij wrote:
> The only thing that makes sense is to request a falling edge interrupt
> if the line is active low and a rising edge interrupt if the line is
> active high, so just do that and get rid of the assignment from
> platform data. The GPIO descriptor knows if the line is active high
> or low.
> 
> Also make irq a local variable in probe(), it's not used anywhere else.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/extcon/extcon-gpio.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 86f3ec6d6014..6d9cb4ed11c2 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -30,26 +30,22 @@
>  /**
>   * struct gpio_extcon_data - A simple GPIO-controlled extcon device state container.
>   * @edev:		Extcon device.
> - * @irq:		Interrupt line for the external connector.
>   * @work:		Work fired by the interrupt.
>   * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
>   *			value.
>   * @gpiod:		GPIO descriptor for this external connector.
>   * @extcon_id:		The unique id of specific external connector.
>   * @debounce:		Debounce time for GPIO IRQ in ms.
> - * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
>   * @check_on_resume:	Boolean describing whether to check the state of gpio
>   *			while resuming from sleep.
>   */
>  struct gpio_extcon_data {
>  	struct extcon_dev *edev;
> -	int irq;
>  	struct delayed_work work;
>  	unsigned long debounce_jiffies;
>  	struct gpio_desc *gpiod;
>  	unsigned int extcon_id;
>  	unsigned long debounce;
> -	unsigned long irq_flags;
>  	bool check_on_resume;
>  };
>  
> @@ -77,21 +73,34 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  {
>  	struct gpio_extcon_data *data;
>  	struct device *dev = &pdev->dev;
> +	unsigned long irq_flags;
> +	int irq;
>  	int ret;
>  
>  	data = devm_kzalloc(dev, sizeof(struct gpio_extcon_data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> -	if (!data->irq_flags || data->extcon_id > EXTCON_NONE)
> +	if (data->extcon_id > EXTCON_NONE)
>  		return -EINVAL;
>  
>  	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
>  	if (IS_ERR(data->gpiod))
>  		return PTR_ERR(data->gpiod);
> -	data->irq = gpiod_to_irq(data->gpiod);
> -	if (data->irq <= 0)
> -		return data->irq;
> +	irq = gpiod_to_irq(data->gpiod);
> +	if (irq <= 0)
> +		return irq;
> +
> +	/*
> +	 * It is unlikely that this is an acknowledged interrupt that goes
> +	 * away after handling, what we are looking for are falling edges
> +	 * if the signal is active low, and rising edges if the signal is
> +	 * active high.
> +	 */
> +	if (gpiod_is_active_low(data->gpiod))
> +		irq_flags = IRQF_TRIGGER_FALLING;
> +	else
> +		irq_flags = IRQF_TRIGGER_RISING;
>  
>  	/* Allocate the memory of extcon devie and register extcon device */
>  	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
> @@ -110,8 +119,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  	 * Request the interrupt of gpio to detect whether external connector
>  	 * is attached or detached.
>  	 */
> -	ret = devm_request_any_context_irq(dev, data->irq,
> -					gpio_irq_handler, data->irq_flags,
> +	ret = devm_request_any_context_irq(dev, irq,
> +					gpio_irq_handler, irq_flags,
>  					pdev->name, data);
>  	if (ret < 0)
>  		return ret;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 6/8] extcon: gpio: Get debounce setting from device property

[Chanwoo Choi]

Hi Linus,

On 2017년 09월 24일 23:56, Linus Walleij wrote:
> Look up the debouncing value using the device property that will
> pick it from device tree or ACPI DSDT or whatever is available.
> 
> Reintroduce the debounce handling previously deleted in the
> series, setting the delayed worker to delay 0 ms if the GPIO
> driver supports debouncing for us, else just delay the reading
> of the value delayed by jiffies.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/extcon/extcon-gpio.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

I checked the pinctrl-bindings.txt[1] in order to check
the property name of 'input-debounce'. It looks good to me.
[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 6d9cb4ed11c2..8fc52631c8a2 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -24,6 +24,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>  #include <linux/workqueue.h>
>  
> @@ -35,7 +36,6 @@
>   *			value.
>   * @gpiod:		GPIO descriptor for this external connector.
>   * @extcon_id:		The unique id of specific external connector.
> - * @debounce:		Debounce time for GPIO IRQ in ms.
>   * @check_on_resume:	Boolean describing whether to check the state of gpio
>   *			while resuming from sleep.
>   */
> @@ -45,7 +45,6 @@ struct gpio_extcon_data {
>  	unsigned long debounce_jiffies;
>  	struct gpio_desc *gpiod;
>  	unsigned int extcon_id;
> -	unsigned long debounce;
>  	bool check_on_resume;
>  };
>  
> @@ -74,6 +73,7 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  	struct gpio_extcon_data *data;
>  	struct device *dev = &pdev->dev;
>  	unsigned long irq_flags;
> +	u32 debounce_usecs;
>  	int irq;
>  	int ret;
>  
> @@ -109,6 +109,15 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	ret = device_property_read_u32(dev, "input-debounce", &debounce_usecs);
> +	if (ret || !debounce_usecs) {
> +		dev_err(dev, "illegal debounce value, set to 20 ms\n");
> +		debounce_usecs = 20000;
> +	}
> +	ret = gpiod_set_debounce(data->gpiod, debounce_usecs);
> +	if (ret)
> +		data->debounce_jiffies = msecs_to_jiffies(debounce_usecs * 1000);
> +
>  	ret = devm_extcon_dev_register(dev, data->edev);
>  	if (ret < 0)
>  		return ret;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 8/8] extcon: gpio: Always check state on resume

[Chanwoo Choi]

Hi Linus,

On 2017년 09월 24일 23:56, Linus Walleij wrote:
> It makes most sense to always check the state of the GPIO external
> connector at system resume so just do this by default. Add a TODO
> if people turn out to desire to parameterize this.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/extcon/extcon-gpio.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index b7353f5018b5..4f0ad5ad2722 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -83,6 +83,15 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Always checking connector state on resume makes most sense so do this
> +	 * by default.
> +	 *
> +	 * TODO: if parameterization is needed, augment this to use proper device
> +	 * properties or set it up from PM core.
> +	 */
> +	data->check_on_resume = true;
> +
>  	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
>  	if (IS_ERR(data->gpiod))
>  		return PTR_ERR(data->gpiod);
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 1/8] extcon: gpio: Add DT bindings

[Chanwoo Choi]

Hi Rob,

[snip]

>>
>>>> +External Connector Using GPIO
>>>
>>> What kind of connector? All connectors external to something... And
>>> GPIO is not a kind of connector on its own, but an implementation.
>>
>> Yeah that is a problem with the whole subsystem I guess. Should
>> I add a paragraph describing the usecases?
>>
>> The whole thing is a bit
>> of Androidism and is named like this because Android named it like
>> this in their kernel tree.
>>
>>>> +Required properties:
>>>> +- compatible: should be "extcon-gpio"
>>>> +- extcon-gpios: the GPIO lines used for the external connectors
>>>
>>> This doesn't tell me what the GPIOs functions are and should. For
>>> example we have hpd-gpios for HDMI hotplug detect in HDMI connector
>>> binding.
>>
>> The idea is that this array corresponds to the extcon-connector-types
>> right below, I'll clarify that if you think the overall idea is OK.
>>
>>>> +  See gpio/gpio.txt
>>>> +- extcon-connector-types: set to an unsigned integer value arrat representing the types
>>>> +  of this connector, matched to the corresponding GPIO lines in the previous array.
>>>
>>> This should be determined by the compatible string.
>>
>> So a GPIO connector is very versatile. It is "general purpose" by definition,
>> so it needs to be subclassed.
>>
>> One possibility is to allow just one GPIO and have one comptible-string per
>> use case.
>> compatible = "gpio-usb-connector";
>> compatible = "gpio-charger-connector";
>> compatible = "gpio-headphone-connector";
>> etc etc
>>
>> These bindings on the other hand, assume that the driver will be able
>> to handle an array of gpios with an associated array of connector types,
>> which reflects how many of the existing extcon-type driver hardware works:
>> a single PMIC providing some 5 misc extcons for example.
>>
>> For this reason there is a generic compatible string and then the device
>> is subclassed per-gpio with the per-gpio connector type.

The "drivers/input/keyboard/gpio_keys.c" driver used the 'linux,code' property
to get the key type as following in the device-tree file:

gpio-keys {                                       
	compatible = "gpio-keys";

	key-1 {
		gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
		linux,code = <KEY_1>;
		label = "SW2-1";
		wakeup-source;
	};

	key-2 {
		gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
		linux,code = <KEY_2>;
		label = "SW2-2";
		wakeup-source;
	};
};


IMO, extcon-gpio.c uses the 'gpio-connectors' compatible
instead of 'extcon-gpio' and then define the connector type
in the include/dt-bindings/extcon/connectors.h. How about this?

For example,
In the include/dt-bindings/extcon/connectors.h,

#define CONNECTOR_USB		1 /* EXTCON_USB */
#define CONNECTOR_USB_HOST	2 /* EXTCON_USB_HOST */
#define CONNECTOR_CHG_USB_SDP	5 /* EXTCON_CHG_USB_SDP */
#define CONNECTOR_CHG_USB_DCP	6 /* EXTCON_CHG_USB_DCP */
#define CONNECTOR_CHG_USB_CDP	7 /* EXTCON_CHG_USB_CDP */
#define CONNECTOR_CHG_USB_ACA	8 /* EXTCON_CHG_USB_ACA */
...
#define CONNECTOR_HDMI		40 /* EXTCON_DISP_HDMI */
...

In the devicetree example for 'gpio-connectors', 

	usb-connector {
		compatible = "gpio-connectors";
		gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
		linux,connector = <CONNECTOR_USB>;
		wakeup-source;
	};

	hdmi-connector {
		gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
		linux,connector = <CONNECTOR_HDMI>;
		wakeup-source;	
	};


>>
>>>> +/* USB external connector */
>>>> +#define EXTCON_USB             1
>>>> +#define EXTCON_USB_HOST                2
>>>> +#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
>>>> +#define EXTCON_CHG_WPT         11      /* Wireless Power Transfer */
>>>> +#define EXTCON_CHG_USB_PD      12      /* USB Power Delivery */
>>>
>>> These don't all look to be mutually exclusive.
>>>
>>> For USB PD, isn't that discoverable?
> 
> Currently, EXTCON_CHG_USB_PD is not used on any extcon provider drivers.
> I think that EXTCON_CHG_USB_PD is discoverable according to the H/W design
> such as using ADC.
> 
> Also, The charger connectors of extcon are related to power_supply subsystem
> because power_supply is in charge of behavior when the connector is attached/detached.
> So, the extcon defines the EXTCON_CHG_USB_PD in order to detect this type.
> 
>>
>> Someone else would have to answer, this is based on the current
>> connector types supported by the Linux kernel.
>>
>>>> +/* 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 */
>>>> +#define EXTCON_DISP_DP         44      /* Display Port */
>>>> +#define EXTCON_DISP_HMD                45      /* Head-Mounted Display */
>>>
>>> We already have connector bindings for most of these. Use those as a
>>> model for whatever you want to do.
>>
>> I guess they are not in Documentation/devicetree/bindings/extcon/*
>>
>> Please point me in the right direction and I'll check it out.
>>
>> Yours,
>> Linus Walleij
>>
>>
>>
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics