All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-26  5:56 ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-26  5:56 UTC (permalink / raw)
  To: Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm, Roger Quadros

Some Qualcomm PMICs have a misc device that performs USB id pin
detection via an interrupt. When the interrupt triggers, we
should read the interrupt line to see if it has gone high or low.
If the interrupt is low then the ID pin is grounded, and if the
interrupt is high then the ID pin is being held high.

Cc: Roger Quadros <rogerq@ti.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
 .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
 drivers/extcon/Kconfig                             |   6 +
 drivers/extcon/Makefile                            |   1 +
 drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
 4 files changed, 218 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
 create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c

diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
new file mode 100644
index 000000000000..35383adb10f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
@@ -0,0 +1,41 @@
+Qualcomm's PM8941 USB ID Extcon device
+
+Some Qualcomm PMICs have a "misc" module that can be used to detect when
+the USB ID pin has been pulled low or high.
+
+PROPERTIES
+
+- compatible:
+    Usage: required
+    Value type: <string>
+    Definition: Should contain "qcom,pm8941-misc";
+
+- reg:
+    Usage: required
+    Value type: <u32>
+    Definition: Should contain the offset to the misc address space
+
+- interrupts:
+    Usage: required
+    Value type: <prop-encoded-array>
+    Definition: Should contain the usb id interrupt
+
+- interrupt-names:
+    Usage: required
+    Value type: <stringlist>
+    Definition: Should contain the string "usb_id" for the usb id interrupt
+
+Example:
+
+	pmic {
+		usb_id: misc@900 {
+			compatible = "qcom,pm8941-misc";
+			reg = <0x900>;
+			interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
+			interrupt-names = "usb_id";
+		};
+	}
+
+	usb-controller {
+		extcon = <&usb_id>;
+	};
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 3d89e60a3e71..b2ee47cb10ca 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -119,6 +119,12 @@ config EXTCON_SM5502
 	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
 	  detector and switch.
 
+config EXTCON_QCOM_SPMI_MISC
+	tristate "Qualcomm USB extcon support"
+	help
+	  Say Y here to enable SPMI PMIC based USB cable detection
+	  support on Qualcomm PMICs such as PM8941.
+
 config EXTCON_USB_GPIO
 	tristate "USB GPIO extcon support"
 	depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 2a0e4f45d5b2..8cf6eb068d34 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
 obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
+obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
 obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
new file mode 100644
index 000000000000..f0ec6f1541e1
--- /dev/null
+++ b/drivers/extcon/extcon-qcom-spmi-misc.c
@@ -0,0 +1,170 @@
+/**
+ * Based on extcon-usb-gpio.c
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
+ * Author: Roger Quadros <rogerq@ti.com>
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/extcon.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#define USB_ID_DEBOUNCE_MS	5	/* ms */
+
+struct qcom_usb_extcon_info {
+	struct extcon_dev *edev;
+	int irq;
+	struct delayed_work wq_detcable;
+	unsigned long debounce_jiffies;
+};
+
+static const unsigned int qcom_usb_extcon_cable[] = {
+	EXTCON_USB_HOST,
+	EXTCON_NONE,
+};
+
+static void qcom_usb_extcon_detect_cable(struct work_struct *work)
+{
+	bool id;
+	int ret;
+	struct qcom_usb_extcon_info *info = container_of(to_delayed_work(work),
+						    struct qcom_usb_extcon_info,
+						    wq_detcable);
+
+	/* check ID and update cable state */
+	ret = irq_get_irqchip_state(info->irq, IRQCHIP_STATE_LINE_LEVEL, &id);
+	if (ret)
+		return;
+
+	extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, !id);
+}
+
+static irqreturn_t qcom_usb_irq_handler(int irq, void *dev_id)
+{
+	struct qcom_usb_extcon_info *info = dev_id;
+
+	queue_delayed_work(system_power_efficient_wq, &info->wq_detcable,
+			   info->debounce_jiffies);
+
+	return IRQ_HANDLED;
+}
+
+static int qcom_usb_extcon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_usb_extcon_info *info;
+	int ret;
+
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->edev = devm_extcon_dev_allocate(dev, qcom_usb_extcon_cable);
+	if (IS_ERR(info->edev)) {
+		dev_err(dev, "failed to allocate extcon device\n");
+		return -ENOMEM;
+	}
+
+	ret = devm_extcon_dev_register(dev, info->edev);
+	if (ret < 0) {
+		dev_err(dev, "failed to register extcon device\n");
+		return ret;
+	}
+
+	info->debounce_jiffies = msecs_to_jiffies(USB_ID_DEBOUNCE_MS);
+	INIT_DELAYED_WORK(&info->wq_detcable, qcom_usb_extcon_detect_cable);
+
+	info->irq = platform_get_irq_byname(pdev, "usb_id");
+	if (info->irq < 0)
+		return info->irq;
+
+	ret = devm_request_threaded_irq(dev, info->irq, NULL,
+					qcom_usb_irq_handler,
+					IRQF_TRIGGER_RISING |
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					pdev->name, info);
+	if (ret < 0) {
+		dev_err(dev, "failed to request handler for ID IRQ\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, info);
+	device_init_wakeup(dev, 1);
+
+	/* Perform initial detection */
+	qcom_usb_extcon_detect_cable(&info->wq_detcable.work);
+
+	return 0;
+}
+
+static int qcom_usb_extcon_remove(struct platform_device *pdev)
+{
+	struct qcom_usb_extcon_info *info = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&info->wq_detcable);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int qcom_usb_extcon_suspend(struct device *dev)
+{
+	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (device_may_wakeup(dev))
+		ret = enable_irq_wake(info->irq);
+
+	return ret;
+}
+
+static int qcom_usb_extcon_resume(struct device *dev)
+{
+	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (device_may_wakeup(dev))
+		ret = disable_irq_wake(info->irq);
+
+	return ret;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(qcom_usb_extcon_pm_ops,
+			 qcom_usb_extcon_suspend, qcom_usb_extcon_resume);
+
+static const struct of_device_id qcom_usb_extcon_dt_match[] = {
+	{ .compatible = "qcom,pm8941-misc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, qcom_usb_extcon_dt_match);
+
+static struct platform_driver qcom_usb_extcon_driver = {
+	.probe		= qcom_usb_extcon_probe,
+	.remove		= qcom_usb_extcon_remove,
+	.driver		= {
+		.name	= "extcon-pm8941-misc",
+		.pm	= &qcom_usb_extcon_pm_ops,
+		.of_match_table = qcom_usb_extcon_dt_match,
+	},
+};
+module_platform_driver(qcom_usb_extcon_driver);
+
+MODULE_DESCRIPTION("QCOM USB ID extcon driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.0.rc2.8.ga28705d

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-26  5:56 ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-26  5:56 UTC (permalink / raw)
  To: linux-arm-kernel

Some Qualcomm PMICs have a misc device that performs USB id pin
detection via an interrupt. When the interrupt triggers, we
should read the interrupt line to see if it has gone high or low.
If the interrupt is low then the ID pin is grounded, and if the
interrupt is high then the ID pin is being held high.

Cc: Roger Quadros <rogerq@ti.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
 .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
 drivers/extcon/Kconfig                             |   6 +
 drivers/extcon/Makefile                            |   1 +
 drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
 4 files changed, 218 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
 create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c

diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
new file mode 100644
index 000000000000..35383adb10f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
@@ -0,0 +1,41 @@
+Qualcomm's PM8941 USB ID Extcon device
+
+Some Qualcomm PMICs have a "misc" module that can be used to detect when
+the USB ID pin has been pulled low or high.
+
+PROPERTIES
+
+- compatible:
+    Usage: required
+    Value type: <string>
+    Definition: Should contain "qcom,pm8941-misc";
+
+- reg:
+    Usage: required
+    Value type: <u32>
+    Definition: Should contain the offset to the misc address space
+
+- interrupts:
+    Usage: required
+    Value type: <prop-encoded-array>
+    Definition: Should contain the usb id interrupt
+
+- interrupt-names:
+    Usage: required
+    Value type: <stringlist>
+    Definition: Should contain the string "usb_id" for the usb id interrupt
+
+Example:
+
+	pmic {
+		usb_id: misc at 900 {
+			compatible = "qcom,pm8941-misc";
+			reg = <0x900>;
+			interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
+			interrupt-names = "usb_id";
+		};
+	}
+
+	usb-controller {
+		extcon = <&usb_id>;
+	};
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 3d89e60a3e71..b2ee47cb10ca 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -119,6 +119,12 @@ config EXTCON_SM5502
 	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
 	  detector and switch.
 
+config EXTCON_QCOM_SPMI_MISC
+	tristate "Qualcomm USB extcon support"
+	help
+	  Say Y here to enable SPMI PMIC based USB cable detection
+	  support on Qualcomm PMICs such as PM8941.
+
 config EXTCON_USB_GPIO
 	tristate "USB GPIO extcon support"
 	depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 2a0e4f45d5b2..8cf6eb068d34 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
 obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
+obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
 obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
new file mode 100644
index 000000000000..f0ec6f1541e1
--- /dev/null
+++ b/drivers/extcon/extcon-qcom-spmi-misc.c
@@ -0,0 +1,170 @@
+/**
+ * Based on extcon-usb-gpio.c
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
+ * Author: Roger Quadros <rogerq@ti.com>
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/extcon.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#define USB_ID_DEBOUNCE_MS	5	/* ms */
+
+struct qcom_usb_extcon_info {
+	struct extcon_dev *edev;
+	int irq;
+	struct delayed_work wq_detcable;
+	unsigned long debounce_jiffies;
+};
+
+static const unsigned int qcom_usb_extcon_cable[] = {
+	EXTCON_USB_HOST,
+	EXTCON_NONE,
+};
+
+static void qcom_usb_extcon_detect_cable(struct work_struct *work)
+{
+	bool id;
+	int ret;
+	struct qcom_usb_extcon_info *info = container_of(to_delayed_work(work),
+						    struct qcom_usb_extcon_info,
+						    wq_detcable);
+
+	/* check ID and update cable state */
+	ret = irq_get_irqchip_state(info->irq, IRQCHIP_STATE_LINE_LEVEL, &id);
+	if (ret)
+		return;
+
+	extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, !id);
+}
+
+static irqreturn_t qcom_usb_irq_handler(int irq, void *dev_id)
+{
+	struct qcom_usb_extcon_info *info = dev_id;
+
+	queue_delayed_work(system_power_efficient_wq, &info->wq_detcable,
+			   info->debounce_jiffies);
+
+	return IRQ_HANDLED;
+}
+
+static int qcom_usb_extcon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_usb_extcon_info *info;
+	int ret;
+
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->edev = devm_extcon_dev_allocate(dev, qcom_usb_extcon_cable);
+	if (IS_ERR(info->edev)) {
+		dev_err(dev, "failed to allocate extcon device\n");
+		return -ENOMEM;
+	}
+
+	ret = devm_extcon_dev_register(dev, info->edev);
+	if (ret < 0) {
+		dev_err(dev, "failed to register extcon device\n");
+		return ret;
+	}
+
+	info->debounce_jiffies = msecs_to_jiffies(USB_ID_DEBOUNCE_MS);
+	INIT_DELAYED_WORK(&info->wq_detcable, qcom_usb_extcon_detect_cable);
+
+	info->irq = platform_get_irq_byname(pdev, "usb_id");
+	if (info->irq < 0)
+		return info->irq;
+
+	ret = devm_request_threaded_irq(dev, info->irq, NULL,
+					qcom_usb_irq_handler,
+					IRQF_TRIGGER_RISING |
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					pdev->name, info);
+	if (ret < 0) {
+		dev_err(dev, "failed to request handler for ID IRQ\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, info);
+	device_init_wakeup(dev, 1);
+
+	/* Perform initial detection */
+	qcom_usb_extcon_detect_cable(&info->wq_detcable.work);
+
+	return 0;
+}
+
+static int qcom_usb_extcon_remove(struct platform_device *pdev)
+{
+	struct qcom_usb_extcon_info *info = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&info->wq_detcable);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int qcom_usb_extcon_suspend(struct device *dev)
+{
+	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (device_may_wakeup(dev))
+		ret = enable_irq_wake(info->irq);
+
+	return ret;
+}
+
+static int qcom_usb_extcon_resume(struct device *dev)
+{
+	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (device_may_wakeup(dev))
+		ret = disable_irq_wake(info->irq);
+
+	return ret;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(qcom_usb_extcon_pm_ops,
+			 qcom_usb_extcon_suspend, qcom_usb_extcon_resume);
+
+static const struct of_device_id qcom_usb_extcon_dt_match[] = {
+	{ .compatible = "qcom,pm8941-misc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, qcom_usb_extcon_dt_match);
+
+static struct platform_driver qcom_usb_extcon_driver = {
+	.probe		= qcom_usb_extcon_probe,
+	.remove		= qcom_usb_extcon_remove,
+	.driver		= {
+		.name	= "extcon-pm8941-misc",
+		.pm	= &qcom_usb_extcon_pm_ops,
+		.of_match_table = qcom_usb_extcon_dt_match,
+	},
+};
+module_platform_driver(qcom_usb_extcon_driver);
+
+MODULE_DESCRIPTION("QCOM USB ID extcon driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.0.rc2.8.ga28705d

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-26  5:56 ` Stephen Boyd
@ 2016-06-26 11:20   ` Chanwoo Choi
  -1 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-26 11:20 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm, Roger Quadros

Hi,

This patch looks good to me.
But, there is some comment.

2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> Some Qualcomm PMICs have a misc device that performs USB id pin
> detection via an interrupt. When the interrupt triggers, we
> should read the interrupt line to see if it has gone high or low.
> If the interrupt is low then the ID pin is grounded, and if the
> interrupt is high then the ID pin is being held high.
>
> Cc: Roger Quadros <rogerq@ti.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>  drivers/extcon/Kconfig                             |   6 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
>  4 files changed, 218 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>  create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c
>
> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> new file mode 100644
> index 000000000000..35383adb10f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> @@ -0,0 +1,41 @@
> +Qualcomm's PM8941 USB ID Extcon device
> +
> +Some Qualcomm PMICs have a "misc" module that can be used to detect when
> +the USB ID pin has been pulled low or high.
> +
> +PROPERTIES
> +
> +- compatible:
> +    Usage: required
> +    Value type: <string>
> +    Definition: Should contain "qcom,pm8941-misc";
> +
> +- reg:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: Should contain the offset to the misc address space

'reg' property is used on extcon-qcom-spmi-misc.c?
I think that you don't need to include this property.

> +
> +- interrupts:
> +    Usage: required
> +    Value type: <prop-encoded-array>
> +    Definition: Should contain the usb id interrupt
> +
> +- interrupt-names:
> +    Usage: required
> +    Value type: <stringlist>
> +    Definition: Should contain the string "usb_id" for the usb id interrupt
> +
> +Example:
> +
> +       pmic {
> +               usb_id: misc@900 {
> +                       compatible = "qcom,pm8941-misc";
> +                       reg = <0x900>;
> +                       interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
> +                       interrupt-names = "usb_id";
> +               };
> +       }
> +
> +       usb-controller {
> +               extcon = <&usb_id>;
> +       };
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 3d89e60a3e71..b2ee47cb10ca 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -119,6 +119,12 @@ config EXTCON_SM5502
>           Silicon Mitus SM5502. The SM5502 is a USB port accessory
>           detector and switch.
>
> +config EXTCON_QCOM_SPMI_MISC
> +       tristate "Qualcomm USB extcon support"
> +       help
> +         Say Y here to enable SPMI PMIC based USB cable detection
> +         support on Qualcomm PMICs such as PM8941.

You need to reorder the entry alphabetically. You better to move
it below

> +
>  config EXTCON_USB_GPIO
>         tristate "USB GPIO extcon support"
>         depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 2a0e4f45d5b2..8cf6eb068d34 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)  += extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)    += extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)    += extcon-sm5502.o
> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)  += extcon-usb-gpio.o
> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> new file mode 100644
> index 000000000000..f0ec6f1541e1
> --- /dev/null
> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> @@ -0,0 +1,170 @@
> +/**
> + * Based on extcon-usb-gpio.c
> + *
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Roger Quadros <rogerq@ti.com>
> + *
> + * Copyright (C) 2016 Linaro Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/extcon.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +#define USB_ID_DEBOUNCE_MS     5       /* ms */
> +
> +struct qcom_usb_extcon_info {
> +       struct extcon_dev *edev;
> +       int irq;
> +       struct delayed_work wq_detcable;
> +       unsigned long debounce_jiffies;
> +};
> +
> +static const unsigned int qcom_usb_extcon_cable[] = {
> +       EXTCON_USB_HOST,
> +       EXTCON_NONE,
> +};
> +
> +static void qcom_usb_extcon_detect_cable(struct work_struct *work)
> +{
> +       bool id;
> +       int ret;
> +       struct qcom_usb_extcon_info *info = container_of(to_delayed_work(work),
> +                                                   struct qcom_usb_extcon_info,
> +                                                   wq_detcable);
> +
> +       /* check ID and update cable state */
> +       ret = irq_get_irqchip_state(info->irq, IRQCHIP_STATE_LINE_LEVEL, &id);
> +       if (ret)
> +               return;
> +
> +       extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, !id);
> +}
> +
> +static irqreturn_t qcom_usb_irq_handler(int irq, void *dev_id)
> +{
> +       struct qcom_usb_extcon_info *info = dev_id;
> +
> +       queue_delayed_work(system_power_efficient_wq, &info->wq_detcable,
> +                          info->debounce_jiffies);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int qcom_usb_extcon_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct qcom_usb_extcon_info *info;
> +       int ret;
> +
> +       info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +       if (!info)
> +               return -ENOMEM;
> +
> +       info->edev = devm_extcon_dev_allocate(dev, qcom_usb_extcon_cable);
> +       if (IS_ERR(info->edev)) {
> +               dev_err(dev, "failed to allocate extcon device\n");
> +               return -ENOMEM;
> +       }
> +
> +       ret = devm_extcon_dev_register(dev, info->edev);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to register extcon device\n");
> +               return ret;
> +       }
> +
> +       info->debounce_jiffies = msecs_to_jiffies(USB_ID_DEBOUNCE_MS);
> +       INIT_DELAYED_WORK(&info->wq_detcable, qcom_usb_extcon_detect_cable);
> +
> +       info->irq = platform_get_irq_byname(pdev, "usb_id");
> +       if (info->irq < 0)
> +               return info->irq;
> +
> +       ret = devm_request_threaded_irq(dev, info->irq, NULL,
> +                                       qcom_usb_irq_handler,
> +                                       IRQF_TRIGGER_RISING |
> +                                       IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +                                       pdev->name, info);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to request handler for ID IRQ\n");
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, info);
> +       device_init_wakeup(dev, 1);
> +
> +       /* Perform initial detection */
> +       qcom_usb_extcon_detect_cable(&info->wq_detcable.work);
> +
> +       return 0;
> +}
> +
> +static int qcom_usb_extcon_remove(struct platform_device *pdev)
> +{
> +       struct qcom_usb_extcon_info *info = platform_get_drvdata(pdev);
> +
> +       cancel_delayed_work_sync(&info->wq_detcable);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int qcom_usb_extcon_suspend(struct device *dev)
> +{
> +       struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +       int ret = 0;
> +
> +       if (device_may_wakeup(dev))
> +               ret = enable_irq_wake(info->irq);
> +
> +       return ret;
> +}
> +
> +static int qcom_usb_extcon_resume(struct device *dev)
> +{
> +       struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +       int ret = 0;
> +
> +       if (device_may_wakeup(dev))
> +               ret = disable_irq_wake(info->irq);
> +
> +       return ret;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(qcom_usb_extcon_pm_ops,
> +                        qcom_usb_extcon_suspend, qcom_usb_extcon_resume);
> +
> +static const struct of_device_id qcom_usb_extcon_dt_match[] = {
> +       { .compatible = "qcom,pm8941-misc", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_usb_extcon_dt_match);
> +
> +static struct platform_driver qcom_usb_extcon_driver = {
> +       .probe          = qcom_usb_extcon_probe,
> +       .remove         = qcom_usb_extcon_remove,
> +       .driver         = {
> +               .name   = "extcon-pm8941-misc",
> +               .pm     = &qcom_usb_extcon_pm_ops,
> +               .of_match_table = qcom_usb_extcon_dt_match,
> +       },
> +};
> +module_platform_driver(qcom_usb_extcon_driver);
> +
> +MODULE_DESCRIPTION("QCOM USB ID extcon driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.9.0.rc2.8.ga28705d
>

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-26 11:20   ` Chanwoo Choi
  0 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-26 11:20 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

This patch looks good to me.
But, there is some comment.

2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> Some Qualcomm PMICs have a misc device that performs USB id pin
> detection via an interrupt. When the interrupt triggers, we
> should read the interrupt line to see if it has gone high or low.
> If the interrupt is low then the ID pin is grounded, and if the
> interrupt is high then the ID pin is being held high.
>
> Cc: Roger Quadros <rogerq@ti.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>  drivers/extcon/Kconfig                             |   6 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
>  4 files changed, 218 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>  create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c
>
> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> new file mode 100644
> index 000000000000..35383adb10f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> @@ -0,0 +1,41 @@
> +Qualcomm's PM8941 USB ID Extcon device
> +
> +Some Qualcomm PMICs have a "misc" module that can be used to detect when
> +the USB ID pin has been pulled low or high.
> +
> +PROPERTIES
> +
> +- compatible:
> +    Usage: required
> +    Value type: <string>
> +    Definition: Should contain "qcom,pm8941-misc";
> +
> +- reg:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: Should contain the offset to the misc address space

'reg' property is used on extcon-qcom-spmi-misc.c?
I think that you don't need to include this property.

> +
> +- interrupts:
> +    Usage: required
> +    Value type: <prop-encoded-array>
> +    Definition: Should contain the usb id interrupt
> +
> +- interrupt-names:
> +    Usage: required
> +    Value type: <stringlist>
> +    Definition: Should contain the string "usb_id" for the usb id interrupt
> +
> +Example:
> +
> +       pmic {
> +               usb_id: misc at 900 {
> +                       compatible = "qcom,pm8941-misc";
> +                       reg = <0x900>;
> +                       interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
> +                       interrupt-names = "usb_id";
> +               };
> +       }
> +
> +       usb-controller {
> +               extcon = <&usb_id>;
> +       };
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 3d89e60a3e71..b2ee47cb10ca 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -119,6 +119,12 @@ config EXTCON_SM5502
>           Silicon Mitus SM5502. The SM5502 is a USB port accessory
>           detector and switch.
>
> +config EXTCON_QCOM_SPMI_MISC
> +       tristate "Qualcomm USB extcon support"
> +       help
> +         Say Y here to enable SPMI PMIC based USB cable detection
> +         support on Qualcomm PMICs such as PM8941.

You need to reorder the entry alphabetically. You better to move
it below

> +
>  config EXTCON_USB_GPIO
>         tristate "USB GPIO extcon support"
>         depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 2a0e4f45d5b2..8cf6eb068d34 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)  += extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)    += extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)    += extcon-sm5502.o
> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)  += extcon-usb-gpio.o
> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> new file mode 100644
> index 000000000000..f0ec6f1541e1
> --- /dev/null
> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> @@ -0,0 +1,170 @@
> +/**
> + * Based on extcon-usb-gpio.c
> + *
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Roger Quadros <rogerq@ti.com>
> + *
> + * Copyright (C) 2016 Linaro Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/extcon.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +#define USB_ID_DEBOUNCE_MS     5       /* ms */
> +
> +struct qcom_usb_extcon_info {
> +       struct extcon_dev *edev;
> +       int irq;
> +       struct delayed_work wq_detcable;
> +       unsigned long debounce_jiffies;
> +};
> +
> +static const unsigned int qcom_usb_extcon_cable[] = {
> +       EXTCON_USB_HOST,
> +       EXTCON_NONE,
> +};
> +
> +static void qcom_usb_extcon_detect_cable(struct work_struct *work)
> +{
> +       bool id;
> +       int ret;
> +       struct qcom_usb_extcon_info *info = container_of(to_delayed_work(work),
> +                                                   struct qcom_usb_extcon_info,
> +                                                   wq_detcable);
> +
> +       /* check ID and update cable state */
> +       ret = irq_get_irqchip_state(info->irq, IRQCHIP_STATE_LINE_LEVEL, &id);
> +       if (ret)
> +               return;
> +
> +       extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, !id);
> +}
> +
> +static irqreturn_t qcom_usb_irq_handler(int irq, void *dev_id)
> +{
> +       struct qcom_usb_extcon_info *info = dev_id;
> +
> +       queue_delayed_work(system_power_efficient_wq, &info->wq_detcable,
> +                          info->debounce_jiffies);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int qcom_usb_extcon_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct qcom_usb_extcon_info *info;
> +       int ret;
> +
> +       info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +       if (!info)
> +               return -ENOMEM;
> +
> +       info->edev = devm_extcon_dev_allocate(dev, qcom_usb_extcon_cable);
> +       if (IS_ERR(info->edev)) {
> +               dev_err(dev, "failed to allocate extcon device\n");
> +               return -ENOMEM;
> +       }
> +
> +       ret = devm_extcon_dev_register(dev, info->edev);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to register extcon device\n");
> +               return ret;
> +       }
> +
> +       info->debounce_jiffies = msecs_to_jiffies(USB_ID_DEBOUNCE_MS);
> +       INIT_DELAYED_WORK(&info->wq_detcable, qcom_usb_extcon_detect_cable);
> +
> +       info->irq = platform_get_irq_byname(pdev, "usb_id");
> +       if (info->irq < 0)
> +               return info->irq;
> +
> +       ret = devm_request_threaded_irq(dev, info->irq, NULL,
> +                                       qcom_usb_irq_handler,
> +                                       IRQF_TRIGGER_RISING |
> +                                       IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +                                       pdev->name, info);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to request handler for ID IRQ\n");
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, info);
> +       device_init_wakeup(dev, 1);
> +
> +       /* Perform initial detection */
> +       qcom_usb_extcon_detect_cable(&info->wq_detcable.work);
> +
> +       return 0;
> +}
> +
> +static int qcom_usb_extcon_remove(struct platform_device *pdev)
> +{
> +       struct qcom_usb_extcon_info *info = platform_get_drvdata(pdev);
> +
> +       cancel_delayed_work_sync(&info->wq_detcable);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int qcom_usb_extcon_suspend(struct device *dev)
> +{
> +       struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +       int ret = 0;
> +
> +       if (device_may_wakeup(dev))
> +               ret = enable_irq_wake(info->irq);
> +
> +       return ret;
> +}
> +
> +static int qcom_usb_extcon_resume(struct device *dev)
> +{
> +       struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +       int ret = 0;
> +
> +       if (device_may_wakeup(dev))
> +               ret = disable_irq_wake(info->irq);
> +
> +       return ret;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(qcom_usb_extcon_pm_ops,
> +                        qcom_usb_extcon_suspend, qcom_usb_extcon_resume);
> +
> +static const struct of_device_id qcom_usb_extcon_dt_match[] = {
> +       { .compatible = "qcom,pm8941-misc", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_usb_extcon_dt_match);
> +
> +static struct platform_driver qcom_usb_extcon_driver = {
> +       .probe          = qcom_usb_extcon_probe,
> +       .remove         = qcom_usb_extcon_remove,
> +       .driver         = {
> +               .name   = "extcon-pm8941-misc",
> +               .pm     = &qcom_usb_extcon_pm_ops,
> +               .of_match_table = qcom_usb_extcon_dt_match,
> +       },
> +};
> +module_platform_driver(qcom_usb_extcon_driver);
> +
> +MODULE_DESCRIPTION("QCOM USB ID extcon driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.9.0.rc2.8.ga28705d
>

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-26 11:20   ` Chanwoo Choi
  (?)
@ 2016-06-26 11:22     ` Chanwoo Choi
  -1 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-26 11:22 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: linux-arm-msm, Stephen Boyd, linux-kernel, linux-arm-kernel,
	Roger Quadros

Hi,

2016-06-26 20:20 GMT+09:00 Chanwoo Choi <cwchoi00@gmail.com>:
> Hi,
>
> This patch looks good to me.
> But, there is some comment.
>
> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>> Some Qualcomm PMICs have a misc device that performs USB id pin
>> detection via an interrupt. When the interrupt triggers, we
>> should read the interrupt line to see if it has gone high or low.
>> If the interrupt is low then the ID pin is grounded, and if the
>> interrupt is high then the ID pin is being held high.
>>
>> Cc: Roger Quadros <rogerq@ti.com>
>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
>> ---
>>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>>  drivers/extcon/Kconfig                             |   6 +
>>  drivers/extcon/Makefile                            |   1 +
>>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
>>  4 files changed, 218 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>>  create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c
>>
>> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>> new file mode 100644
>> index 000000000000..35383adb10f1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>> @@ -0,0 +1,41 @@
>> +Qualcomm's PM8941 USB ID Extcon device
>> +
>> +Some Qualcomm PMICs have a "misc" module that can be used to detect when
>> +the USB ID pin has been pulled low or high.
>> +
>> +PROPERTIES
>> +
>> +- compatible:
>> +    Usage: required
>> +    Value type: <string>
>> +    Definition: Should contain "qcom,pm8941-misc";
>> +
>> +- reg:
>> +    Usage: required
>> +    Value type: <u32>
>> +    Definition: Should contain the offset to the misc address space
>
> 'reg' property is used on extcon-qcom-spmi-misc.c?
> I think that you don't need to include this property.
>
>> +
>> +- interrupts:
>> +    Usage: required
>> +    Value type: <prop-encoded-array>
>> +    Definition: Should contain the usb id interrupt
>> +
>> +- interrupt-names:
>> +    Usage: required
>> +    Value type: <stringlist>
>> +    Definition: Should contain the string "usb_id" for the usb id interrupt
>> +
>> +Example:
>> +
>> +       pmic {
>> +               usb_id: misc@900 {
>> +                       compatible = "qcom,pm8941-misc";
>> +                       reg = <0x900>;
>> +                       interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
>> +                       interrupt-names = "usb_id";
>> +               };
>> +       }
>> +
>> +       usb-controller {
>> +               extcon = <&usb_id>;
>> +       };
>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>> index 3d89e60a3e71..b2ee47cb10ca 100644
>> --- a/drivers/extcon/Kconfig
>> +++ b/drivers/extcon/Kconfig
>> @@ -119,6 +119,12 @@ config EXTCON_SM5502
>>           Silicon Mitus SM5502. The SM5502 is a USB port accessory
>>           detector and switch.
>>
>> +config EXTCON_QCOM_SPMI_MISC
>> +       tristate "Qualcomm USB extcon support"
>> +       help
>> +         Say Y here to enable SPMI PMIC based USB cable detection
>> +         support on Qualcomm PMICs such as PM8941.
>
> You need to reorder the entry alphabetically. You better to move
> it below

Sorry. Before completing the reply, I send the email.
You better to move the entry on below EXTCON_PALMAS.

>
>> +
>>  config EXTCON_USB_GPIO
>>         tristate "USB GPIO extcon support"
>>         depends on GPIOLIB || COMPILE_TEST
>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> index 2a0e4f45d5b2..8cf6eb068d34 100644
>> --- a/drivers/extcon/Makefile
>> +++ b/drivers/extcon/Makefile
>> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)  += extcon-max8997.o
>>  obj-$(CONFIG_EXTCON_PALMAS)    += extcon-palmas.o
>>  obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
>>  obj-$(CONFIG_EXTCON_SM5502)    += extcon-sm5502.o
>> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o

ditto.


Thanks,
Chanwoo Choi

[snip]

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-26 11:22     ` Chanwoo Choi
  0 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-26 11:22 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Stephen Boyd, linux-arm-kernel, linux-kernel, linux-arm-msm,
	Roger Quadros

Hi,

2016-06-26 20:20 GMT+09:00 Chanwoo Choi <cwchoi00@gmail.com>:
> Hi,
>
> This patch looks good to me.
> But, there is some comment.
>
> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>> Some Qualcomm PMICs have a misc device that performs USB id pin
>> detection via an interrupt. When the interrupt triggers, we
>> should read the interrupt line to see if it has gone high or low.
>> If the interrupt is low then the ID pin is grounded, and if the
>> interrupt is high then the ID pin is being held high.
>>
>> Cc: Roger Quadros <rogerq@ti.com>
>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
>> ---
>>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>>  drivers/extcon/Kconfig                             |   6 +
>>  drivers/extcon/Makefile                            |   1 +
>>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
>>  4 files changed, 218 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>>  create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c
>>
>> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>> new file mode 100644
>> index 000000000000..35383adb10f1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>> @@ -0,0 +1,41 @@
>> +Qualcomm's PM8941 USB ID Extcon device
>> +
>> +Some Qualcomm PMICs have a "misc" module that can be used to detect when
>> +the USB ID pin has been pulled low or high.
>> +
>> +PROPERTIES
>> +
>> +- compatible:
>> +    Usage: required
>> +    Value type: <string>
>> +    Definition: Should contain "qcom,pm8941-misc";
>> +
>> +- reg:
>> +    Usage: required
>> +    Value type: <u32>
>> +    Definition: Should contain the offset to the misc address space
>
> 'reg' property is used on extcon-qcom-spmi-misc.c?
> I think that you don't need to include this property.
>
>> +
>> +- interrupts:
>> +    Usage: required
>> +    Value type: <prop-encoded-array>
>> +    Definition: Should contain the usb id interrupt
>> +
>> +- interrupt-names:
>> +    Usage: required
>> +    Value type: <stringlist>
>> +    Definition: Should contain the string "usb_id" for the usb id interrupt
>> +
>> +Example:
>> +
>> +       pmic {
>> +               usb_id: misc@900 {
>> +                       compatible = "qcom,pm8941-misc";
>> +                       reg = <0x900>;
>> +                       interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
>> +                       interrupt-names = "usb_id";
>> +               };
>> +       }
>> +
>> +       usb-controller {
>> +               extcon = <&usb_id>;
>> +       };
>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>> index 3d89e60a3e71..b2ee47cb10ca 100644
>> --- a/drivers/extcon/Kconfig
>> +++ b/drivers/extcon/Kconfig
>> @@ -119,6 +119,12 @@ config EXTCON_SM5502
>>           Silicon Mitus SM5502. The SM5502 is a USB port accessory
>>           detector and switch.
>>
>> +config EXTCON_QCOM_SPMI_MISC
>> +       tristate "Qualcomm USB extcon support"
>> +       help
>> +         Say Y here to enable SPMI PMIC based USB cable detection
>> +         support on Qualcomm PMICs such as PM8941.
>
> You need to reorder the entry alphabetically. You better to move
> it below

Sorry. Before completing the reply, I send the email.
You better to move the entry on below EXTCON_PALMAS.

>
>> +
>>  config EXTCON_USB_GPIO
>>         tristate "USB GPIO extcon support"
>>         depends on GPIOLIB || COMPILE_TEST
>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> index 2a0e4f45d5b2..8cf6eb068d34 100644
>> --- a/drivers/extcon/Makefile
>> +++ b/drivers/extcon/Makefile
>> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)  += extcon-max8997.o
>>  obj-$(CONFIG_EXTCON_PALMAS)    += extcon-palmas.o
>>  obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
>>  obj-$(CONFIG_EXTCON_SM5502)    += extcon-sm5502.o
>> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o

ditto.


Thanks,
Chanwoo Choi

[snip]

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-26 11:22     ` Chanwoo Choi
  0 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-26 11:22 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

2016-06-26 20:20 GMT+09:00 Chanwoo Choi <cwchoi00@gmail.com>:
> Hi,
>
> This patch looks good to me.
> But, there is some comment.
>
> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>> Some Qualcomm PMICs have a misc device that performs USB id pin
>> detection via an interrupt. When the interrupt triggers, we
>> should read the interrupt line to see if it has gone high or low.
>> If the interrupt is low then the ID pin is grounded, and if the
>> interrupt is high then the ID pin is being held high.
>>
>> Cc: Roger Quadros <rogerq@ti.com>
>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
>> ---
>>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>>  drivers/extcon/Kconfig                             |   6 +
>>  drivers/extcon/Makefile                            |   1 +
>>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
>>  4 files changed, 218 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>>  create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c
>>
>> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>> new file mode 100644
>> index 000000000000..35383adb10f1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>> @@ -0,0 +1,41 @@
>> +Qualcomm's PM8941 USB ID Extcon device
>> +
>> +Some Qualcomm PMICs have a "misc" module that can be used to detect when
>> +the USB ID pin has been pulled low or high.
>> +
>> +PROPERTIES
>> +
>> +- compatible:
>> +    Usage: required
>> +    Value type: <string>
>> +    Definition: Should contain "qcom,pm8941-misc";
>> +
>> +- reg:
>> +    Usage: required
>> +    Value type: <u32>
>> +    Definition: Should contain the offset to the misc address space
>
> 'reg' property is used on extcon-qcom-spmi-misc.c?
> I think that you don't need to include this property.
>
>> +
>> +- interrupts:
>> +    Usage: required
>> +    Value type: <prop-encoded-array>
>> +    Definition: Should contain the usb id interrupt
>> +
>> +- interrupt-names:
>> +    Usage: required
>> +    Value type: <stringlist>
>> +    Definition: Should contain the string "usb_id" for the usb id interrupt
>> +
>> +Example:
>> +
>> +       pmic {
>> +               usb_id: misc at 900 {
>> +                       compatible = "qcom,pm8941-misc";
>> +                       reg = <0x900>;
>> +                       interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
>> +                       interrupt-names = "usb_id";
>> +               };
>> +       }
>> +
>> +       usb-controller {
>> +               extcon = <&usb_id>;
>> +       };
>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>> index 3d89e60a3e71..b2ee47cb10ca 100644
>> --- a/drivers/extcon/Kconfig
>> +++ b/drivers/extcon/Kconfig
>> @@ -119,6 +119,12 @@ config EXTCON_SM5502
>>           Silicon Mitus SM5502. The SM5502 is a USB port accessory
>>           detector and switch.
>>
>> +config EXTCON_QCOM_SPMI_MISC
>> +       tristate "Qualcomm USB extcon support"
>> +       help
>> +         Say Y here to enable SPMI PMIC based USB cable detection
>> +         support on Qualcomm PMICs such as PM8941.
>
> You need to reorder the entry alphabetically. You better to move
> it below

Sorry. Before completing the reply, I send the email.
You better to move the entry on below EXTCON_PALMAS.

>
>> +
>>  config EXTCON_USB_GPIO
>>         tristate "USB GPIO extcon support"
>>         depends on GPIOLIB || COMPILE_TEST
>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> index 2a0e4f45d5b2..8cf6eb068d34 100644
>> --- a/drivers/extcon/Makefile
>> +++ b/drivers/extcon/Makefile
>> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)  += extcon-max8997.o
>>  obj-$(CONFIG_EXTCON_PALMAS)    += extcon-palmas.o
>>  obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
>>  obj-$(CONFIG_EXTCON_SM5502)    += extcon-sm5502.o
>> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o

ditto.


Thanks,
Chanwoo Choi

[snip]

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-26  5:56 ` Stephen Boyd
  (?)
@ 2016-06-27  7:39   ` Roger Quadros
  -1 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-27  7:39 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

Hi Stephen,

On 26/06/16 08:56, Stephen Boyd wrote:
> Some Qualcomm PMICs have a misc device that performs USB id pin
> detection via an interrupt. When the interrupt triggers, we
> should read the interrupt line to see if it has gone high or low.
> If the interrupt is low then the ID pin is grounded, and if the
> interrupt is high then the ID pin is being held high.

Does this depend on any other drivers to configure the USB ID
interrupt or it works automatically once the interrupt is enabled?

> 
> Cc: Roger Quadros <rogerq@ti.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>  drivers/extcon/Kconfig                             |   6 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++

Should we make this driver more generic so that it can support
any other platforms as well that can give USB ID over interrupt.

What about USB_VBUS? How is that delivered?

>  4 files changed, 218 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>  create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c
> 
> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> new file mode 100644
> index 000000000000..35383adb10f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> @@ -0,0 +1,41 @@
> +Qualcomm's PM8941 USB ID Extcon device
> +
> +Some Qualcomm PMICs have a "misc" module that can be used to detect when
> +the USB ID pin has been pulled low or high.
> +
> +PROPERTIES
> +
> +- compatible:
> +    Usage: required
> +    Value type: <string>
> +    Definition: Should contain "qcom,pm8941-misc";
> +
> +- reg:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: Should contain the offset to the misc address space
> +
> +- interrupts:
> +    Usage: required
> +    Value type: <prop-encoded-array>
> +    Definition: Should contain the usb id interrupt
> +
> +- interrupt-names:
> +    Usage: required
> +    Value type: <stringlist>
> +    Definition: Should contain the string "usb_id" for the usb id interrupt
> +
> +Example:
> +
> +	pmic {
> +		usb_id: misc@900 {
> +			compatible = "qcom,pm8941-misc";
> +			reg = <0x900>;
> +			interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
> +			interrupt-names = "usb_id";
> +		};
> +	}
> +
> +	usb-controller {
> +		extcon = <&usb_id>;
> +	};
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 3d89e60a3e71..b2ee47cb10ca 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -119,6 +119,12 @@ config EXTCON_SM5502
>  	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
>  	  detector and switch.
>  
> +config EXTCON_QCOM_SPMI_MISC
> +	tristate "Qualcomm USB extcon support"
> +	help
> +	  Say Y here to enable SPMI PMIC based USB cable detection
> +	  support on Qualcomm PMICs such as PM8941.
> +
>  config EXTCON_USB_GPIO
>  	tristate "USB GPIO extcon support"
>  	depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 2a0e4f45d5b2..8cf6eb068d34 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> new file mode 100644
> index 000000000000..f0ec6f1541e1
> --- /dev/null
> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> @@ -0,0 +1,170 @@
> +/**
> + * Based on extcon-usb-gpio.c
> + *
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Roger Quadros <rogerq@ti.com>

You don't need to carry the original (C) here.

> + *
> + * Copyright (C) 2016 Linaro Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/extcon.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +#define USB_ID_DEBOUNCE_MS	5	/* ms */
> +
> +struct qcom_usb_extcon_info {
> +	struct extcon_dev *edev;
> +	int irq;
> +	struct delayed_work wq_detcable;
> +	unsigned long debounce_jiffies;
> +};
> +
> +static const unsigned int qcom_usb_extcon_cable[] = {
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static void qcom_usb_extcon_detect_cable(struct work_struct *work)
> +{
> +	bool id;
> +	int ret;
> +	struct qcom_usb_extcon_info *info = container_of(to_delayed_work(work),
> +						    struct qcom_usb_extcon_info,
> +						    wq_detcable);
> +
> +	/* check ID and update cable state */
> +	ret = irq_get_irqchip_state(info->irq, IRQCHIP_STATE_LINE_LEVEL, &id);
> +	if (ret)
> +		return;
> +
> +	extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, !id);
> +}
> +
> +static irqreturn_t qcom_usb_irq_handler(int irq, void *dev_id)
> +{
> +	struct qcom_usb_extcon_info *info = dev_id;
> +
> +	queue_delayed_work(system_power_efficient_wq, &info->wq_detcable,
> +			   info->debounce_jiffies);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_usb_extcon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_usb_extcon_info *info;
> +	int ret;
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->edev = devm_extcon_dev_allocate(dev, qcom_usb_extcon_cable);
> +	if (IS_ERR(info->edev)) {
> +		dev_err(dev, "failed to allocate extcon device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = devm_extcon_dev_register(dev, info->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	info->debounce_jiffies = msecs_to_jiffies(USB_ID_DEBOUNCE_MS);
> +	INIT_DELAYED_WORK(&info->wq_detcable, qcom_usb_extcon_detect_cable);
> +
> +	info->irq = platform_get_irq_byname(pdev, "usb_id");
> +	if (info->irq < 0)
> +		return info->irq;
> +
> +	ret = devm_request_threaded_irq(dev, info->irq, NULL,
> +					qcom_usb_irq_handler,
> +					IRQF_TRIGGER_RISING |
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					pdev->name, info);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to request handler for ID IRQ\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, info);
> +	device_init_wakeup(dev, 1);
> +
> +	/* Perform initial detection */
> +	qcom_usb_extcon_detect_cable(&info->wq_detcable.work);
> +
> +	return 0;
> +}
> +
> +static int qcom_usb_extcon_remove(struct platform_device *pdev)
> +{
> +	struct qcom_usb_extcon_info *info = platform_get_drvdata(pdev);
> +
> +	cancel_delayed_work_sync(&info->wq_detcable);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int qcom_usb_extcon_suspend(struct device *dev)
> +{
> +	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (device_may_wakeup(dev))
> +		ret = enable_irq_wake(info->irq);
> +
> +	return ret;
> +}
> +
> +static int qcom_usb_extcon_resume(struct device *dev)
> +{
> +	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (device_may_wakeup(dev))
> +		ret = disable_irq_wake(info->irq);
> +
> +	return ret;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(qcom_usb_extcon_pm_ops,
> +			 qcom_usb_extcon_suspend, qcom_usb_extcon_resume);
> +
> +static const struct of_device_id qcom_usb_extcon_dt_match[] = {
> +	{ .compatible = "qcom,pm8941-misc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_usb_extcon_dt_match);
> +
> +static struct platform_driver qcom_usb_extcon_driver = {
> +	.probe		= qcom_usb_extcon_probe,
> +	.remove		= qcom_usb_extcon_remove,
> +	.driver		= {
> +		.name	= "extcon-pm8941-misc",
> +		.pm	= &qcom_usb_extcon_pm_ops,
> +		.of_match_table = qcom_usb_extcon_dt_match,
> +	},
> +};
> +module_platform_driver(qcom_usb_extcon_driver);
> +
> +MODULE_DESCRIPTION("QCOM USB ID extcon driver");
> +MODULE_LICENSE("GPL v2");
> 

--
cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-27  7:39   ` Roger Quadros
  0 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-27  7:39 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

Hi Stephen,

On 26/06/16 08:56, Stephen Boyd wrote:
> Some Qualcomm PMICs have a misc device that performs USB id pin
> detection via an interrupt. When the interrupt triggers, we
> should read the interrupt line to see if it has gone high or low.
> If the interrupt is low then the ID pin is grounded, and if the
> interrupt is high then the ID pin is being held high.

Does this depend on any other drivers to configure the USB ID
interrupt or it works automatically once the interrupt is enabled?

> 
> Cc: Roger Quadros <rogerq@ti.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>  drivers/extcon/Kconfig                             |   6 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++

Should we make this driver more generic so that it can support
any other platforms as well that can give USB ID over interrupt.

What about USB_VBUS? How is that delivered?

>  4 files changed, 218 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>  create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c
> 
> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> new file mode 100644
> index 000000000000..35383adb10f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> @@ -0,0 +1,41 @@
> +Qualcomm's PM8941 USB ID Extcon device
> +
> +Some Qualcomm PMICs have a "misc" module that can be used to detect when
> +the USB ID pin has been pulled low or high.
> +
> +PROPERTIES
> +
> +- compatible:
> +    Usage: required
> +    Value type: <string>
> +    Definition: Should contain "qcom,pm8941-misc";
> +
> +- reg:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: Should contain the offset to the misc address space
> +
> +- interrupts:
> +    Usage: required
> +    Value type: <prop-encoded-array>
> +    Definition: Should contain the usb id interrupt
> +
> +- interrupt-names:
> +    Usage: required
> +    Value type: <stringlist>
> +    Definition: Should contain the string "usb_id" for the usb id interrupt
> +
> +Example:
> +
> +	pmic {
> +		usb_id: misc@900 {
> +			compatible = "qcom,pm8941-misc";
> +			reg = <0x900>;
> +			interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
> +			interrupt-names = "usb_id";
> +		};
> +	}
> +
> +	usb-controller {
> +		extcon = <&usb_id>;
> +	};
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 3d89e60a3e71..b2ee47cb10ca 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -119,6 +119,12 @@ config EXTCON_SM5502
>  	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
>  	  detector and switch.
>  
> +config EXTCON_QCOM_SPMI_MISC
> +	tristate "Qualcomm USB extcon support"
> +	help
> +	  Say Y here to enable SPMI PMIC based USB cable detection
> +	  support on Qualcomm PMICs such as PM8941.
> +
>  config EXTCON_USB_GPIO
>  	tristate "USB GPIO extcon support"
>  	depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 2a0e4f45d5b2..8cf6eb068d34 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> new file mode 100644
> index 000000000000..f0ec6f1541e1
> --- /dev/null
> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> @@ -0,0 +1,170 @@
> +/**
> + * Based on extcon-usb-gpio.c
> + *
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Roger Quadros <rogerq@ti.com>

You don't need to carry the original (C) here.

> + *
> + * Copyright (C) 2016 Linaro Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/extcon.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +#define USB_ID_DEBOUNCE_MS	5	/* ms */
> +
> +struct qcom_usb_extcon_info {
> +	struct extcon_dev *edev;
> +	int irq;
> +	struct delayed_work wq_detcable;
> +	unsigned long debounce_jiffies;
> +};
> +
> +static const unsigned int qcom_usb_extcon_cable[] = {
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static void qcom_usb_extcon_detect_cable(struct work_struct *work)
> +{
> +	bool id;
> +	int ret;
> +	struct qcom_usb_extcon_info *info = container_of(to_delayed_work(work),
> +						    struct qcom_usb_extcon_info,
> +						    wq_detcable);
> +
> +	/* check ID and update cable state */
> +	ret = irq_get_irqchip_state(info->irq, IRQCHIP_STATE_LINE_LEVEL, &id);
> +	if (ret)
> +		return;
> +
> +	extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, !id);
> +}
> +
> +static irqreturn_t qcom_usb_irq_handler(int irq, void *dev_id)
> +{
> +	struct qcom_usb_extcon_info *info = dev_id;
> +
> +	queue_delayed_work(system_power_efficient_wq, &info->wq_detcable,
> +			   info->debounce_jiffies);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_usb_extcon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_usb_extcon_info *info;
> +	int ret;
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->edev = devm_extcon_dev_allocate(dev, qcom_usb_extcon_cable);
> +	if (IS_ERR(info->edev)) {
> +		dev_err(dev, "failed to allocate extcon device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = devm_extcon_dev_register(dev, info->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	info->debounce_jiffies = msecs_to_jiffies(USB_ID_DEBOUNCE_MS);
> +	INIT_DELAYED_WORK(&info->wq_detcable, qcom_usb_extcon_detect_cable);
> +
> +	info->irq = platform_get_irq_byname(pdev, "usb_id");
> +	if (info->irq < 0)
> +		return info->irq;
> +
> +	ret = devm_request_threaded_irq(dev, info->irq, NULL,
> +					qcom_usb_irq_handler,
> +					IRQF_TRIGGER_RISING |
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					pdev->name, info);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to request handler for ID IRQ\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, info);
> +	device_init_wakeup(dev, 1);
> +
> +	/* Perform initial detection */
> +	qcom_usb_extcon_detect_cable(&info->wq_detcable.work);
> +
> +	return 0;
> +}
> +
> +static int qcom_usb_extcon_remove(struct platform_device *pdev)
> +{
> +	struct qcom_usb_extcon_info *info = platform_get_drvdata(pdev);
> +
> +	cancel_delayed_work_sync(&info->wq_detcable);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int qcom_usb_extcon_suspend(struct device *dev)
> +{
> +	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (device_may_wakeup(dev))
> +		ret = enable_irq_wake(info->irq);
> +
> +	return ret;
> +}
> +
> +static int qcom_usb_extcon_resume(struct device *dev)
> +{
> +	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (device_may_wakeup(dev))
> +		ret = disable_irq_wake(info->irq);
> +
> +	return ret;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(qcom_usb_extcon_pm_ops,
> +			 qcom_usb_extcon_suspend, qcom_usb_extcon_resume);
> +
> +static const struct of_device_id qcom_usb_extcon_dt_match[] = {
> +	{ .compatible = "qcom,pm8941-misc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_usb_extcon_dt_match);
> +
> +static struct platform_driver qcom_usb_extcon_driver = {
> +	.probe		= qcom_usb_extcon_probe,
> +	.remove		= qcom_usb_extcon_remove,
> +	.driver		= {
> +		.name	= "extcon-pm8941-misc",
> +		.pm	= &qcom_usb_extcon_pm_ops,
> +		.of_match_table = qcom_usb_extcon_dt_match,
> +	},
> +};
> +module_platform_driver(qcom_usb_extcon_driver);
> +
> +MODULE_DESCRIPTION("QCOM USB ID extcon driver");
> +MODULE_LICENSE("GPL v2");
> 

--
cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-27  7:39   ` Roger Quadros
  0 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-27  7:39 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Stephen,

On 26/06/16 08:56, Stephen Boyd wrote:
> Some Qualcomm PMICs have a misc device that performs USB id pin
> detection via an interrupt. When the interrupt triggers, we
> should read the interrupt line to see if it has gone high or low.
> If the interrupt is low then the ID pin is grounded, and if the
> interrupt is high then the ID pin is being held high.

Does this depend on any other drivers to configure the USB ID
interrupt or it works automatically once the interrupt is enabled?

> 
> Cc: Roger Quadros <rogerq@ti.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>  drivers/extcon/Kconfig                             |   6 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++

Should we make this driver more generic so that it can support
any other platforms as well that can give USB ID over interrupt.

What about USB_VBUS? How is that delivered?

>  4 files changed, 218 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
>  create mode 100644 drivers/extcon/extcon-qcom-spmi-misc.c
> 
> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> new file mode 100644
> index 000000000000..35383adb10f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.txt
> @@ -0,0 +1,41 @@
> +Qualcomm's PM8941 USB ID Extcon device
> +
> +Some Qualcomm PMICs have a "misc" module that can be used to detect when
> +the USB ID pin has been pulled low or high.
> +
> +PROPERTIES
> +
> +- compatible:
> +    Usage: required
> +    Value type: <string>
> +    Definition: Should contain "qcom,pm8941-misc";
> +
> +- reg:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: Should contain the offset to the misc address space
> +
> +- interrupts:
> +    Usage: required
> +    Value type: <prop-encoded-array>
> +    Definition: Should contain the usb id interrupt
> +
> +- interrupt-names:
> +    Usage: required
> +    Value type: <stringlist>
> +    Definition: Should contain the string "usb_id" for the usb id interrupt
> +
> +Example:
> +
> +	pmic {
> +		usb_id: misc at 900 {
> +			compatible = "qcom,pm8941-misc";
> +			reg = <0x900>;
> +			interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
> +			interrupt-names = "usb_id";
> +		};
> +	}
> +
> +	usb-controller {
> +		extcon = <&usb_id>;
> +	};
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 3d89e60a3e71..b2ee47cb10ca 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -119,6 +119,12 @@ config EXTCON_SM5502
>  	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
>  	  detector and switch.
>  
> +config EXTCON_QCOM_SPMI_MISC
> +	tristate "Qualcomm USB extcon support"
> +	help
> +	  Say Y here to enable SPMI PMIC based USB cable detection
> +	  support on Qualcomm PMICs such as PM8941.
> +
>  config EXTCON_USB_GPIO
>  	tristate "USB GPIO extcon support"
>  	depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 2a0e4f45d5b2..8cf6eb068d34 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> new file mode 100644
> index 000000000000..f0ec6f1541e1
> --- /dev/null
> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> @@ -0,0 +1,170 @@
> +/**
> + * Based on extcon-usb-gpio.c
> + *
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Roger Quadros <rogerq@ti.com>

You don't need to carry the original (C) here.

> + *
> + * Copyright (C) 2016 Linaro Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/extcon.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +#define USB_ID_DEBOUNCE_MS	5	/* ms */
> +
> +struct qcom_usb_extcon_info {
> +	struct extcon_dev *edev;
> +	int irq;
> +	struct delayed_work wq_detcable;
> +	unsigned long debounce_jiffies;
> +};
> +
> +static const unsigned int qcom_usb_extcon_cable[] = {
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static void qcom_usb_extcon_detect_cable(struct work_struct *work)
> +{
> +	bool id;
> +	int ret;
> +	struct qcom_usb_extcon_info *info = container_of(to_delayed_work(work),
> +						    struct qcom_usb_extcon_info,
> +						    wq_detcable);
> +
> +	/* check ID and update cable state */
> +	ret = irq_get_irqchip_state(info->irq, IRQCHIP_STATE_LINE_LEVEL, &id);
> +	if (ret)
> +		return;
> +
> +	extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, !id);
> +}
> +
> +static irqreturn_t qcom_usb_irq_handler(int irq, void *dev_id)
> +{
> +	struct qcom_usb_extcon_info *info = dev_id;
> +
> +	queue_delayed_work(system_power_efficient_wq, &info->wq_detcable,
> +			   info->debounce_jiffies);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_usb_extcon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_usb_extcon_info *info;
> +	int ret;
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->edev = devm_extcon_dev_allocate(dev, qcom_usb_extcon_cable);
> +	if (IS_ERR(info->edev)) {
> +		dev_err(dev, "failed to allocate extcon device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = devm_extcon_dev_register(dev, info->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	info->debounce_jiffies = msecs_to_jiffies(USB_ID_DEBOUNCE_MS);
> +	INIT_DELAYED_WORK(&info->wq_detcable, qcom_usb_extcon_detect_cable);
> +
> +	info->irq = platform_get_irq_byname(pdev, "usb_id");
> +	if (info->irq < 0)
> +		return info->irq;
> +
> +	ret = devm_request_threaded_irq(dev, info->irq, NULL,
> +					qcom_usb_irq_handler,
> +					IRQF_TRIGGER_RISING |
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					pdev->name, info);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to request handler for ID IRQ\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, info);
> +	device_init_wakeup(dev, 1);
> +
> +	/* Perform initial detection */
> +	qcom_usb_extcon_detect_cable(&info->wq_detcable.work);
> +
> +	return 0;
> +}
> +
> +static int qcom_usb_extcon_remove(struct platform_device *pdev)
> +{
> +	struct qcom_usb_extcon_info *info = platform_get_drvdata(pdev);
> +
> +	cancel_delayed_work_sync(&info->wq_detcable);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int qcom_usb_extcon_suspend(struct device *dev)
> +{
> +	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (device_may_wakeup(dev))
> +		ret = enable_irq_wake(info->irq);
> +
> +	return ret;
> +}
> +
> +static int qcom_usb_extcon_resume(struct device *dev)
> +{
> +	struct qcom_usb_extcon_info *info = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (device_may_wakeup(dev))
> +		ret = disable_irq_wake(info->irq);
> +
> +	return ret;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(qcom_usb_extcon_pm_ops,
> +			 qcom_usb_extcon_suspend, qcom_usb_extcon_resume);
> +
> +static const struct of_device_id qcom_usb_extcon_dt_match[] = {
> +	{ .compatible = "qcom,pm8941-misc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_usb_extcon_dt_match);
> +
> +static struct platform_driver qcom_usb_extcon_driver = {
> +	.probe		= qcom_usb_extcon_probe,
> +	.remove		= qcom_usb_extcon_remove,
> +	.driver		= {
> +		.name	= "extcon-pm8941-misc",
> +		.pm	= &qcom_usb_extcon_pm_ops,
> +		.of_match_table = qcom_usb_extcon_dt_match,
> +	},
> +};
> +module_platform_driver(qcom_usb_extcon_driver);
> +
> +MODULE_DESCRIPTION("QCOM USB ID extcon driver");
> +MODULE_LICENSE("GPL v2");
> 

--
cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-26 11:20   ` Chanwoo Choi
@ 2016-06-27 19:11     ` Stephen Boyd
  -1 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-27 19:11 UTC (permalink / raw)
  To: cw00.choi, Chanwoo Choi
  Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, Roger Quadros

Quoting Chanwoo Choi (2016-06-26 04:20:43)
> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> > +PROPERTIES
> > +
> > +- compatible:
> > +    Usage: required
> > +    Value type: <string>
> > +    Definition: Should contain "qcom,pm8941-misc";
> > +
> > +- reg:
> > +    Usage: required
> > +    Value type: <u32>
> > +    Definition: Should contain the offset to the misc address space
> 
> 'reg' property is used on extcon-qcom-spmi-misc.c?
> I think that you don't need to include this property.

No it isn't used in the driver right now, but there is a register offset
for this module and there are registers that can be read/written in this
module. I'd like to keep it as required so we can easily read the
registers in the future if needed.

> 
> > +
> > +- interrupts:
> > +    Usage: required
> > +    Value type: <prop-encoded-array>
> > +    Definition: Should contain the usb id interrupt
> > +
> > +- interrupt-names:
> > +    Usage: required
> > +    Value type: <stringlist>
> > +    Definition: Should contain the string "usb_id" for the usb id interrupt
> > +
> > +Example:
> > +
> > +       pmic {
> > +               usb_id: misc@900 {
> > +                       compatible = "qcom,pm8941-misc";
> > +                       reg = <0x900>;
> > +                       interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
> > +                       interrupt-names = "usb_id";
> > +               };
> > +       }
> > +
> > +       usb-controller {
> > +               extcon = <&usb_id>;
> > +       };
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> > index 3d89e60a3e71..b2ee47cb10ca 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -119,6 +119,12 @@ config EXTCON_SM5502
> >           Silicon Mitus SM5502. The SM5502 is a USB port accessory
> >           detector and switch.
> >
> > +config EXTCON_QCOM_SPMI_MISC
> > +       tristate "Qualcomm USB extcon support"
> > +       help
> > +         Say Y here to enable SPMI PMIC based USB cable detection
> > +         support on Qualcomm PMICs such as PM8941.
> 
> You need to reorder the entry alphabetically. You better to move
> it below

Ok.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-27 19:11     ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-27 19:11 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Chanwoo Choi (2016-06-26 04:20:43)
> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> > +PROPERTIES
> > +
> > +- compatible:
> > +    Usage: required
> > +    Value type: <string>
> > +    Definition: Should contain "qcom,pm8941-misc";
> > +
> > +- reg:
> > +    Usage: required
> > +    Value type: <u32>
> > +    Definition: Should contain the offset to the misc address space
> 
> 'reg' property is used on extcon-qcom-spmi-misc.c?
> I think that you don't need to include this property.

No it isn't used in the driver right now, but there is a register offset
for this module and there are registers that can be read/written in this
module. I'd like to keep it as required so we can easily read the
registers in the future if needed.

> 
> > +
> > +- interrupts:
> > +    Usage: required
> > +    Value type: <prop-encoded-array>
> > +    Definition: Should contain the usb id interrupt
> > +
> > +- interrupt-names:
> > +    Usage: required
> > +    Value type: <stringlist>
> > +    Definition: Should contain the string "usb_id" for the usb id interrupt
> > +
> > +Example:
> > +
> > +       pmic {
> > +               usb_id: misc at 900 {
> > +                       compatible = "qcom,pm8941-misc";
> > +                       reg = <0x900>;
> > +                       interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
> > +                       interrupt-names = "usb_id";
> > +               };
> > +       }
> > +
> > +       usb-controller {
> > +               extcon = <&usb_id>;
> > +       };
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> > index 3d89e60a3e71..b2ee47cb10ca 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -119,6 +119,12 @@ config EXTCON_SM5502
> >           Silicon Mitus SM5502. The SM5502 is a USB port accessory
> >           detector and switch.
> >
> > +config EXTCON_QCOM_SPMI_MISC
> > +       tristate "Qualcomm USB extcon support"
> > +       help
> > +         Say Y here to enable SPMI PMIC based USB cable detection
> > +         support on Qualcomm PMICs such as PM8941.
> 
> You need to reorder the entry alphabetically. You better to move
> it below

Ok.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-27  7:39   ` Roger Quadros
  (?)
@ 2016-06-27 19:30     ` Stephen Boyd
  -1 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-27 19:30 UTC (permalink / raw)
  To: Roger Quadros, Chanwoo Choi; +Cc: linux-arm-msm, linux-kernel, linux-arm-kernel

Quoting Roger Quadros (2016-06-27 00:39:51)
> Hi Stephen,
> 
> On 26/06/16 08:56, Stephen Boyd wrote:
> > Some Qualcomm PMICs have a misc device that performs USB id pin
> > detection via an interrupt. When the interrupt triggers, we
> > should read the interrupt line to see if it has gone high or low.
> > If the interrupt is low then the ID pin is grounded, and if the
> > interrupt is high then the ID pin is being held high.
> 
> Does this depend on any other drivers to configure the USB ID
> interrupt or it works automatically once the interrupt is enabled?

No other configuration is required as far as I know.

> 
> > 
> > Cc: Roger Quadros <rogerq@ti.com>
> > Cc: Chanwoo Choi <cw00.choi@samsung.com>
> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> > ---
> >  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
> >  drivers/extcon/Kconfig                             |   6 +
> >  drivers/extcon/Makefile                            |   1 +
> >  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
> 
> Should we make this driver more generic so that it can support
> any other platforms as well that can give USB ID over interrupt.

I don't see a problem with that, but can that wait until we gain another
user? I'd rather not make something generic when we only have one user.

> 
> What about USB_VBUS? How is that delivered?

The VBUS notification is done through another piece of hardware. In this
case it's done by the charger module. I've sent a patch for that[1].

> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > index 2a0e4f45d5b2..8cf6eb068d34 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)        += extcon-max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)  += extcon-palmas.o
> >  obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)  += extcon-sm5502.o
> > +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
> >  obj-$(CONFIG_EXTCON_USB_GPIO)        += extcon-usb-gpio.o
> > diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> > new file mode 100644
> > index 000000000000..f0ec6f1541e1
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> > @@ -0,0 +1,170 @@
> > +/**
> > + * Based on extcon-usb-gpio.c
> > + *
> > + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> > + * Author: Roger Quadros <rogerq@ti.com>
> 
> You don't need to carry the original (C) here.

Ok I'll drop those two lines. Thanks.

[1] http://lkml.kernel.org/g/20160626055437.18516-1-stephen.boyd@linaro.org

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-27 19:30     ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-27 19:30 UTC (permalink / raw)
  To: Roger Quadros, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

Quoting Roger Quadros (2016-06-27 00:39:51)
> Hi Stephen,
> 
> On 26/06/16 08:56, Stephen Boyd wrote:
> > Some Qualcomm PMICs have a misc device that performs USB id pin
> > detection via an interrupt. When the interrupt triggers, we
> > should read the interrupt line to see if it has gone high or low.
> > If the interrupt is low then the ID pin is grounded, and if the
> > interrupt is high then the ID pin is being held high.
> 
> Does this depend on any other drivers to configure the USB ID
> interrupt or it works automatically once the interrupt is enabled?

No other configuration is required as far as I know.

> 
> > 
> > Cc: Roger Quadros <rogerq@ti.com>
> > Cc: Chanwoo Choi <cw00.choi@samsung.com>
> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> > ---
> >  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
> >  drivers/extcon/Kconfig                             |   6 +
> >  drivers/extcon/Makefile                            |   1 +
> >  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
> 
> Should we make this driver more generic so that it can support
> any other platforms as well that can give USB ID over interrupt.

I don't see a problem with that, but can that wait until we gain another
user? I'd rather not make something generic when we only have one user.

> 
> What about USB_VBUS? How is that delivered?

The VBUS notification is done through another piece of hardware. In this
case it's done by the charger module. I've sent a patch for that[1].

> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > index 2a0e4f45d5b2..8cf6eb068d34 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)        += extcon-max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)  += extcon-palmas.o
> >  obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)  += extcon-sm5502.o
> > +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
> >  obj-$(CONFIG_EXTCON_USB_GPIO)        += extcon-usb-gpio.o
> > diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> > new file mode 100644
> > index 000000000000..f0ec6f1541e1
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> > @@ -0,0 +1,170 @@
> > +/**
> > + * Based on extcon-usb-gpio.c
> > + *
> > + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> > + * Author: Roger Quadros <rogerq@ti.com>
> 
> You don't need to carry the original (C) here.

Ok I'll drop those two lines. Thanks.

[1] http://lkml.kernel.org/g/20160626055437.18516-1-stephen.boyd@linaro.org

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-27 19:30     ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-27 19:30 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Roger Quadros (2016-06-27 00:39:51)
> Hi Stephen,
> 
> On 26/06/16 08:56, Stephen Boyd wrote:
> > Some Qualcomm PMICs have a misc device that performs USB id pin
> > detection via an interrupt. When the interrupt triggers, we
> > should read the interrupt line to see if it has gone high or low.
> > If the interrupt is low then the ID pin is grounded, and if the
> > interrupt is high then the ID pin is being held high.
> 
> Does this depend on any other drivers to configure the USB ID
> interrupt or it works automatically once the interrupt is enabled?

No other configuration is required as far as I know.

> 
> > 
> > Cc: Roger Quadros <rogerq@ti.com>
> > Cc: Chanwoo Choi <cw00.choi@samsung.com>
> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> > ---
> >  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
> >  drivers/extcon/Kconfig                             |   6 +
> >  drivers/extcon/Makefile                            |   1 +
> >  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
> 
> Should we make this driver more generic so that it can support
> any other platforms as well that can give USB ID over interrupt.

I don't see a problem with that, but can that wait until we gain another
user? I'd rather not make something generic when we only have one user.

> 
> What about USB_VBUS? How is that delivered?

The VBUS notification is done through another piece of hardware. In this
case it's done by the charger module. I've sent a patch for that[1].

> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > index 2a0e4f45d5b2..8cf6eb068d34 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)        += extcon-max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)  += extcon-palmas.o
> >  obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)  += extcon-sm5502.o
> > +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
> >  obj-$(CONFIG_EXTCON_USB_GPIO)        += extcon-usb-gpio.o
> > diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> > new file mode 100644
> > index 000000000000..f0ec6f1541e1
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> > @@ -0,0 +1,170 @@
> > +/**
> > + * Based on extcon-usb-gpio.c
> > + *
> > + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
> > + * Author: Roger Quadros <rogerq@ti.com>
> 
> You don't need to carry the original (C) here.

Ok I'll drop those two lines. Thanks.

[1] http://lkml.kernel.org/g/20160626055437.18516-1-stephen.boyd at linaro.org

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-27 19:30     ` Stephen Boyd
  (?)
@ 2016-06-28  6:36       ` Roger Quadros
  -1 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-28  6:36 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi; +Cc: linux-arm-msm, linux-kernel, linux-arm-kernel

On 27/06/16 22:30, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-27 00:39:51)
>> Hi Stephen,
>>
>> On 26/06/16 08:56, Stephen Boyd wrote:
>>> Some Qualcomm PMICs have a misc device that performs USB id pin
>>> detection via an interrupt. When the interrupt triggers, we
>>> should read the interrupt line to see if it has gone high or low.
>>> If the interrupt is low then the ID pin is grounded, and if the
>>> interrupt is high then the ID pin is being held high.
>>
>> Does this depend on any other drivers to configure the USB ID
>> interrupt or it works automatically once the interrupt is enabled?
> 
> No other configuration is required as far as I know.
> 
>>
>>>
>>> Cc: Roger Quadros <rogerq@ti.com>
>>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>>> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
>>> ---
>>>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>>>  drivers/extcon/Kconfig                             |   6 +
>>>  drivers/extcon/Makefile                            |   1 +
>>>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
>>
>> Should we make this driver more generic so that it can support
>> any other platforms as well that can give USB ID over interrupt.
> 
> I don't see a problem with that, but can that wait until we gain another
> user? I'd rather not make something generic when we only have one user.

OK.
> 
>>
>> What about USB_VBUS? How is that delivered?
> 
> The VBUS notification is done through another piece of hardware. In this
> case it's done by the charger module. I've sent a patch for that[1].

Isn't it better if ID event is handled as well in that PMIC driver instead of
creating a separate one here?

Why do you need ID to be handled outside of the PMIC driver?
You mentioned earlier that some Qualcomm PMICs have ID detection.

> 
>>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>>> index 2a0e4f45d5b2..8cf6eb068d34 100644
>>> --- a/drivers/extcon/Makefile
>>> +++ b/drivers/extcon/Makefile
>>> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)        += extcon-max8997.o
>>>  obj-$(CONFIG_EXTCON_PALMAS)  += extcon-palmas.o
>>>  obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
>>>  obj-$(CONFIG_EXTCON_SM5502)  += extcon-sm5502.o
>>> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>>>  obj-$(CONFIG_EXTCON_USB_GPIO)        += extcon-usb-gpio.o
>>> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
>>> new file mode 100644
>>> index 000000000000..f0ec6f1541e1
>>> --- /dev/null
>>> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
>>> @@ -0,0 +1,170 @@
>>> +/**
>>> + * Based on extcon-usb-gpio.c
>>> + *
>>> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
>>> + * Author: Roger Quadros <rogerq@ti.com>
>>
>> You don't need to carry the original (C) here.
> 
> Ok I'll drop those two lines. Thanks.
> 
> [1] http://lkml.kernel.org/g/20160626055437.18516-1-stephen.boyd@linaro.org
> 

--
cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-28  6:36       ` Roger Quadros
  0 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-28  6:36 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

On 27/06/16 22:30, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-27 00:39:51)
>> Hi Stephen,
>>
>> On 26/06/16 08:56, Stephen Boyd wrote:
>>> Some Qualcomm PMICs have a misc device that performs USB id pin
>>> detection via an interrupt. When the interrupt triggers, we
>>> should read the interrupt line to see if it has gone high or low.
>>> If the interrupt is low then the ID pin is grounded, and if the
>>> interrupt is high then the ID pin is being held high.
>>
>> Does this depend on any other drivers to configure the USB ID
>> interrupt or it works automatically once the interrupt is enabled?
> 
> No other configuration is required as far as I know.
> 
>>
>>>
>>> Cc: Roger Quadros <rogerq@ti.com>
>>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>>> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
>>> ---
>>>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>>>  drivers/extcon/Kconfig                             |   6 +
>>>  drivers/extcon/Makefile                            |   1 +
>>>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
>>
>> Should we make this driver more generic so that it can support
>> any other platforms as well that can give USB ID over interrupt.
> 
> I don't see a problem with that, but can that wait until we gain another
> user? I'd rather not make something generic when we only have one user.

OK.
> 
>>
>> What about USB_VBUS? How is that delivered?
> 
> The VBUS notification is done through another piece of hardware. In this
> case it's done by the charger module. I've sent a patch for that[1].

Isn't it better if ID event is handled as well in that PMIC driver instead of
creating a separate one here?

Why do you need ID to be handled outside of the PMIC driver?
You mentioned earlier that some Qualcomm PMICs have ID detection.

> 
>>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>>> index 2a0e4f45d5b2..8cf6eb068d34 100644
>>> --- a/drivers/extcon/Makefile
>>> +++ b/drivers/extcon/Makefile
>>> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)        += extcon-max8997.o
>>>  obj-$(CONFIG_EXTCON_PALMAS)  += extcon-palmas.o
>>>  obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
>>>  obj-$(CONFIG_EXTCON_SM5502)  += extcon-sm5502.o
>>> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>>>  obj-$(CONFIG_EXTCON_USB_GPIO)        += extcon-usb-gpio.o
>>> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
>>> new file mode 100644
>>> index 000000000000..f0ec6f1541e1
>>> --- /dev/null
>>> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
>>> @@ -0,0 +1,170 @@
>>> +/**
>>> + * Based on extcon-usb-gpio.c
>>> + *
>>> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
>>> + * Author: Roger Quadros <rogerq@ti.com>
>>
>> You don't need to carry the original (C) here.
> 
> Ok I'll drop those two lines. Thanks.
> 
> [1] http://lkml.kernel.org/g/20160626055437.18516-1-stephen.boyd@linaro.org
> 

--
cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-28  6:36       ` Roger Quadros
  0 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-28  6:36 UTC (permalink / raw)
  To: linux-arm-kernel

On 27/06/16 22:30, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-27 00:39:51)
>> Hi Stephen,
>>
>> On 26/06/16 08:56, Stephen Boyd wrote:
>>> Some Qualcomm PMICs have a misc device that performs USB id pin
>>> detection via an interrupt. When the interrupt triggers, we
>>> should read the interrupt line to see if it has gone high or low.
>>> If the interrupt is low then the ID pin is grounded, and if the
>>> interrupt is high then the ID pin is being held high.
>>
>> Does this depend on any other drivers to configure the USB ID
>> interrupt or it works automatically once the interrupt is enabled?
> 
> No other configuration is required as far as I know.
> 
>>
>>>
>>> Cc: Roger Quadros <rogerq@ti.com>
>>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>>> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
>>> ---
>>>  .../bindings/extcon/qcom,pm8941-misc.txt           |  41 +++++
>>>  drivers/extcon/Kconfig                             |   6 +
>>>  drivers/extcon/Makefile                            |   1 +
>>>  drivers/extcon/extcon-qcom-spmi-misc.c             | 170 +++++++++++++++++++++
>>
>> Should we make this driver more generic so that it can support
>> any other platforms as well that can give USB ID over interrupt.
> 
> I don't see a problem with that, but can that wait until we gain another
> user? I'd rather not make something generic when we only have one user.

OK.
> 
>>
>> What about USB_VBUS? How is that delivered?
> 
> The VBUS notification is done through another piece of hardware. In this
> case it's done by the charger module. I've sent a patch for that[1].

Isn't it better if ID event is handled as well in that PMIC driver instead of
creating a separate one here?

Why do you need ID to be handled outside of the PMIC driver?
You mentioned earlier that some Qualcomm PMICs have ID detection.

> 
>>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>>> index 2a0e4f45d5b2..8cf6eb068d34 100644
>>> --- a/drivers/extcon/Makefile
>>> +++ b/drivers/extcon/Makefile
>>> @@ -15,4 +15,5 @@ obj-$(CONFIG_EXTCON_MAX8997)        += extcon-max8997.o
>>>  obj-$(CONFIG_EXTCON_PALMAS)  += extcon-palmas.o
>>>  obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
>>>  obj-$(CONFIG_EXTCON_SM5502)  += extcon-sm5502.o
>>> +obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>>>  obj-$(CONFIG_EXTCON_USB_GPIO)        += extcon-usb-gpio.o
>>> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
>>> new file mode 100644
>>> index 000000000000..f0ec6f1541e1
>>> --- /dev/null
>>> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
>>> @@ -0,0 +1,170 @@
>>> +/**
>>> + * Based on extcon-usb-gpio.c
>>> + *
>>> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com
>>> + * Author: Roger Quadros <rogerq@ti.com>
>>
>> You don't need to carry the original (C) here.
> 
> Ok I'll drop those two lines. Thanks.
> 
> [1] http://lkml.kernel.org/g/20160626055437.18516-1-stephen.boyd at linaro.org
> 

--
cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-28  6:36       ` Roger Quadros
@ 2016-06-28  8:47         ` Stephen Boyd
  -1 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-28  8:47 UTC (permalink / raw)
  To: Roger Quadros, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

Quoting Roger Quadros (2016-06-27 23:36:26)
> On 27/06/16 22:30, Stephen Boyd wrote:
> > 
> > The VBUS notification is done through another piece of hardware. In this
> > case it's done by the charger module. I've sent a patch for that[1].
> 
> Isn't it better if ID event is handled as well in that PMIC driver instead of
> creating a separate one here?
> 
> Why do you need ID to be handled outside of the PMIC driver?
> You mentioned earlier that some Qualcomm PMICs have ID detection.

Sorry I must have confused you. There are two modules in the PMIC that
are doing detection here. The charger module is detecting the VBUS event
and this misc module is detecting the ID pin. I'm not sure why they're
two different blocks, but it is what it is in the hardware.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-28  8:47         ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-28  8:47 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Roger Quadros (2016-06-27 23:36:26)
> On 27/06/16 22:30, Stephen Boyd wrote:
> > 
> > The VBUS notification is done through another piece of hardware. In this
> > case it's done by the charger module. I've sent a patch for that[1].
> 
> Isn't it better if ID event is handled as well in that PMIC driver instead of
> creating a separate one here?
> 
> Why do you need ID to be handled outside of the PMIC driver?
> You mentioned earlier that some Qualcomm PMICs have ID detection.

Sorry I must have confused you. There are two modules in the PMIC that
are doing detection here. The charger module is detecting the VBUS event
and this misc module is detecting the ID pin. I'm not sure why they're
two different blocks, but it is what it is in the hardware.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-28  8:47         ` Stephen Boyd
  (?)
@ 2016-06-28  9:13           ` Roger Quadros
  -1 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-28  9:13 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

On 28/06/16 11:47, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-27 23:36:26)
>> On 27/06/16 22:30, Stephen Boyd wrote:
>>>
>>> The VBUS notification is done through another piece of hardware. In this
>>> case it's done by the charger module. I've sent a patch for that[1].
>>
>> Isn't it better if ID event is handled as well in that PMIC driver instead of
>> creating a separate one here?
>>
>> Why do you need ID to be handled outside of the PMIC driver?
>> You mentioned earlier that some Qualcomm PMICs have ID detection.
>
> Sorry I must have confused you. There are two modules in the PMIC that
> are doing detection here. The charger module is detecting the VBUS event
> and this misc module is detecting the ID pin. I'm not sure why they're
> two different blocks, but it is what it is in the hardware.
> 
OK. Can the MISC block do anything else other than USB ID?
Does the USB ID interrupt come on a different line than the charger interrupt?

It would be more like MISC block interrupt than USB ID interrupt right?

cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-28  9:13           ` Roger Quadros
  0 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-28  9:13 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

On 28/06/16 11:47, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-27 23:36:26)
>> On 27/06/16 22:30, Stephen Boyd wrote:
>>>
>>> The VBUS notification is done through another piece of hardware. In this
>>> case it's done by the charger module. I've sent a patch for that[1].
>>
>> Isn't it better if ID event is handled as well in that PMIC driver instead of
>> creating a separate one here?
>>
>> Why do you need ID to be handled outside of the PMIC driver?
>> You mentioned earlier that some Qualcomm PMICs have ID detection.
>
> Sorry I must have confused you. There are two modules in the PMIC that
> are doing detection here. The charger module is detecting the VBUS event
> and this misc module is detecting the ID pin. I'm not sure why they're
> two different blocks, but it is what it is in the hardware.
> 
OK. Can the MISC block do anything else other than USB ID?
Does the USB ID interrupt come on a different line than the charger interrupt?

It would be more like MISC block interrupt than USB ID interrupt right?

cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-28  9:13           ` Roger Quadros
  0 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-28  9:13 UTC (permalink / raw)
  To: linux-arm-kernel

On 28/06/16 11:47, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-27 23:36:26)
>> On 27/06/16 22:30, Stephen Boyd wrote:
>>>
>>> The VBUS notification is done through another piece of hardware. In this
>>> case it's done by the charger module. I've sent a patch for that[1].
>>
>> Isn't it better if ID event is handled as well in that PMIC driver instead of
>> creating a separate one here?
>>
>> Why do you need ID to be handled outside of the PMIC driver?
>> You mentioned earlier that some Qualcomm PMICs have ID detection.
>
> Sorry I must have confused you. There are two modules in the PMIC that
> are doing detection here. The charger module is detecting the VBUS event
> and this misc module is detecting the ID pin. I'm not sure why they're
> two different blocks, but it is what it is in the hardware.
> 
OK. Can the MISC block do anything else other than USB ID?
Does the USB ID interrupt come on a different line than the charger interrupt?

It would be more like MISC block interrupt than USB ID interrupt right?

cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-27 19:11     ` Stephen Boyd
@ 2016-06-28 12:06       ` Chanwoo Choi
  -1 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-28 12:06 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, Roger Quadros

2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> Quoting Chanwoo Choi (2016-06-26 04:20:43)
>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>> > +PROPERTIES
>> > +
>> > +- compatible:
>> > +    Usage: required
>> > +    Value type: <string>
>> > +    Definition: Should contain "qcom,pm8941-misc";
>> > +
>> > +- reg:
>> > +    Usage: required
>> > +    Value type: <u32>
>> > +    Definition: Should contain the offset to the misc address space
>>
>> 'reg' property is used on extcon-qcom-spmi-misc.c?
>> I think that you don't need to include this property.
>
> No it isn't used in the driver right now, but there is a register offset
> for this module and there are registers that can be read/written in this
> module. I'd like to keep it as required so we can easily read the
> registers in the future if needed.

OK.
But, If you want to remain the reg property, you should add the code to get
the register offset by using OF functions. This patch don't include the OF
function to handle it.

[snip]

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-28 12:06       ` Chanwoo Choi
  0 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-28 12:06 UTC (permalink / raw)
  To: linux-arm-kernel

2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> Quoting Chanwoo Choi (2016-06-26 04:20:43)
>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>> > +PROPERTIES
>> > +
>> > +- compatible:
>> > +    Usage: required
>> > +    Value type: <string>
>> > +    Definition: Should contain "qcom,pm8941-misc";
>> > +
>> > +- reg:
>> > +    Usage: required
>> > +    Value type: <u32>
>> > +    Definition: Should contain the offset to the misc address space
>>
>> 'reg' property is used on extcon-qcom-spmi-misc.c?
>> I think that you don't need to include this property.
>
> No it isn't used in the driver right now, but there is a register offset
> for this module and there are registers that can be read/written in this
> module. I'd like to keep it as required so we can easily read the
> registers in the future if needed.

OK.
But, If you want to remain the reg property, you should add the code to get
the register offset by using OF functions. This patch don't include the OF
function to handle it.

[snip]

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-28 12:06       ` Chanwoo Choi
@ 2016-06-28 21:59         ` Stephen Boyd
  -1 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-28 21:59 UTC (permalink / raw)
  To: cw00.choi, Chanwoo Choi
  Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, Roger Quadros

Quoting Chanwoo Choi (2016-06-28 05:06:48)
> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> > Quoting Chanwoo Choi (2016-06-26 04:20:43)
> >> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> >> > +PROPERTIES
> >> > +
> >> > +- compatible:
> >> > +    Usage: required
> >> > +    Value type: <string>
> >> > +    Definition: Should contain "qcom,pm8941-misc";
> >> > +
> >> > +- reg:
> >> > +    Usage: required
> >> > +    Value type: <u32>
> >> > +    Definition: Should contain the offset to the misc address space
> >>
> >> 'reg' property is used on extcon-qcom-spmi-misc.c?
> >> I think that you don't need to include this property.
> >
> > No it isn't used in the driver right now, but there is a register offset
> > for this module and there are registers that can be read/written in this
> > module. I'd like to keep it as required so we can easily read the
> > registers in the future if needed.
> 
> OK.
> But, If you want to remain the reg property, you should add the code to get
> the register offset by using OF functions. This patch don't include the OF
> function to handle it.
> 

Sorry I don't follow the argument. I've put the reg property here for
future proofing so that the binding doesn't have to change in backwards
incompatible ways in the future if we do need to get the property later.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-28 21:59         ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-28 21:59 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Chanwoo Choi (2016-06-28 05:06:48)
> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> > Quoting Chanwoo Choi (2016-06-26 04:20:43)
> >> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> >> > +PROPERTIES
> >> > +
> >> > +- compatible:
> >> > +    Usage: required
> >> > +    Value type: <string>
> >> > +    Definition: Should contain "qcom,pm8941-misc";
> >> > +
> >> > +- reg:
> >> > +    Usage: required
> >> > +    Value type: <u32>
> >> > +    Definition: Should contain the offset to the misc address space
> >>
> >> 'reg' property is used on extcon-qcom-spmi-misc.c?
> >> I think that you don't need to include this property.
> >
> > No it isn't used in the driver right now, but there is a register offset
> > for this module and there are registers that can be read/written in this
> > module. I'd like to keep it as required so we can easily read the
> > registers in the future if needed.
> 
> OK.
> But, If you want to remain the reg property, you should add the code to get
> the register offset by using OF functions. This patch don't include the OF
> function to handle it.
> 

Sorry I don't follow the argument. I've put the reg property here for
future proofing so that the binding doesn't have to change in backwards
incompatible ways in the future if we do need to get the property later.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-28  9:13           ` Roger Quadros
@ 2016-06-28 22:01             ` Stephen Boyd
  -1 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-28 22:01 UTC (permalink / raw)
  To: Roger Quadros, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

Quoting Roger Quadros (2016-06-28 02:13:57)
> On 28/06/16 11:47, Stephen Boyd wrote:
> >
> > Sorry I must have confused you. There are two modules in the PMIC that
> > are doing detection here. The charger module is detecting the VBUS event
> > and this misc module is detecting the ID pin. I'm not sure why they're
> > two different blocks, but it is what it is in the hardware.
> > 
> OK. Can the MISC block do anything else other than USB ID?

Yes.

> Does the USB ID interrupt come on a different line than the charger interrupt?

Yes.

> 
> It would be more like MISC block interrupt than USB ID interrupt right?

There are two interrupts going to two different hw blocks inside the
PMIC.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-28 22:01             ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-28 22:01 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Roger Quadros (2016-06-28 02:13:57)
> On 28/06/16 11:47, Stephen Boyd wrote:
> >
> > Sorry I must have confused you. There are two modules in the PMIC that
> > are doing detection here. The charger module is detecting the VBUS event
> > and this misc module is detecting the ID pin. I'm not sure why they're
> > two different blocks, but it is what it is in the hardware.
> > 
> OK. Can the MISC block do anything else other than USB ID?

Yes.

> Does the USB ID interrupt come on a different line than the charger interrupt?

Yes.

> 
> It would be more like MISC block interrupt than USB ID interrupt right?

There are two interrupts going to two different hw blocks inside the
PMIC.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-28 22:01             ` Stephen Boyd
  (?)
@ 2016-06-29  6:10               ` Roger Quadros
  -1 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-29  6:10 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi; +Cc: linux-arm-msm, linux-kernel, linux-arm-kernel

On 29/06/16 01:01, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-28 02:13:57)
>> On 28/06/16 11:47, Stephen Boyd wrote:
>>>
>>> Sorry I must have confused you. There are two modules in the PMIC that
>>> are doing detection here. The charger module is detecting the VBUS event
>>> and this misc module is detecting the ID pin. I'm not sure why they're
>>> two different blocks, but it is what it is in the hardware.
>>>
>> OK. Can the MISC block do anything else other than USB ID?
> 
> Yes.
> 
>> Does the USB ID interrupt come on a different line than the charger interrupt?
> 
> Yes.
> 

OK. Is it better to have this driver somewhere in drivers/mfd or drivers/misc if the
other function doesn't need/use extcon?

>>
>> It would be more like MISC block interrupt than USB ID interrupt right?
> 
> There are two interrupts going to two different hw blocks inside the
> PMIC.
> 

cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-29  6:10               ` Roger Quadros
  0 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-29  6:10 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

On 29/06/16 01:01, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-28 02:13:57)
>> On 28/06/16 11:47, Stephen Boyd wrote:
>>>
>>> Sorry I must have confused you. There are two modules in the PMIC that
>>> are doing detection here. The charger module is detecting the VBUS event
>>> and this misc module is detecting the ID pin. I'm not sure why they're
>>> two different blocks, but it is what it is in the hardware.
>>>
>> OK. Can the MISC block do anything else other than USB ID?
> 
> Yes.
> 
>> Does the USB ID interrupt come on a different line than the charger interrupt?
> 
> Yes.
> 

OK. Is it better to have this driver somewhere in drivers/mfd or drivers/misc if the
other function doesn't need/use extcon?

>>
>> It would be more like MISC block interrupt than USB ID interrupt right?
> 
> There are two interrupts going to two different hw blocks inside the
> PMIC.
> 

cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-29  6:10               ` Roger Quadros
  0 siblings, 0 replies; 43+ messages in thread
From: Roger Quadros @ 2016-06-29  6:10 UTC (permalink / raw)
  To: linux-arm-kernel

On 29/06/16 01:01, Stephen Boyd wrote:
> Quoting Roger Quadros (2016-06-28 02:13:57)
>> On 28/06/16 11:47, Stephen Boyd wrote:
>>>
>>> Sorry I must have confused you. There are two modules in the PMIC that
>>> are doing detection here. The charger module is detecting the VBUS event
>>> and this misc module is detecting the ID pin. I'm not sure why they're
>>> two different blocks, but it is what it is in the hardware.
>>>
>> OK. Can the MISC block do anything else other than USB ID?
> 
> Yes.
> 
>> Does the USB ID interrupt come on a different line than the charger interrupt?
> 
> Yes.
> 

OK. Is it better to have this driver somewhere in drivers/mfd or drivers/misc if the
other function doesn't need/use extcon?

>>
>> It would be more like MISC block interrupt than USB ID interrupt right?
> 
> There are two interrupts going to two different hw blocks inside the
> PMIC.
> 

cheers,
-roger

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-28 21:59         ` Stephen Boyd
@ 2016-06-29  6:25           ` Chanwoo Choi
  -1 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-29  6:25 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi
  Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, Roger Quadros

On 2016년 06월 29일 06:59, Stephen Boyd wrote:
> Quoting Chanwoo Choi (2016-06-28 05:06:48)
>> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
>>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>> +PROPERTIES
>>>>> +
>>>>> +- compatible:
>>>>> +    Usage: required
>>>>> +    Value type: <string>
>>>>> +    Definition: Should contain "qcom,pm8941-misc";
>>>>> +
>>>>> +- reg:
>>>>> +    Usage: required
>>>>> +    Value type: <u32>
>>>>> +    Definition: Should contain the offset to the misc address space
>>>>
>>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
>>>> I think that you don't need to include this property.
>>>
>>> No it isn't used in the driver right now, but there is a register offset
>>> for this module and there are registers that can be read/written in this
>>> module. I'd like to keep it as required so we can easily read the
>>> registers in the future if needed.
>>
>> OK.
>> But, If you want to remain the reg property, you should add the code to get
>> the register offset by using OF functions. This patch don't include the OF
>> function to handle it.
>>
> 
> Sorry I don't follow the argument. I've put the reg property here for
> future proofing so that the binding doesn't have to change in backwards
> incompatible ways in the future if we do need to get the property later.

I don't mention that 'reg' property should be removed.
Just if you want to remain it, you should add some codes as following:
For exmaple,
- of_get_address() to get the address information from device-tree.

If documentation include the some properties, you should add the handling code
in device driver. When you add the code to get the offset from device-tree,
it doesn't influence the some behavior in the future.

Thanks,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-29  6:25           ` Chanwoo Choi
  0 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-29  6:25 UTC (permalink / raw)
  To: linux-arm-kernel

On 2016? 06? 29? 06:59, Stephen Boyd wrote:
> Quoting Chanwoo Choi (2016-06-28 05:06:48)
>> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
>>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>> +PROPERTIES
>>>>> +
>>>>> +- compatible:
>>>>> +    Usage: required
>>>>> +    Value type: <string>
>>>>> +    Definition: Should contain "qcom,pm8941-misc";
>>>>> +
>>>>> +- reg:
>>>>> +    Usage: required
>>>>> +    Value type: <u32>
>>>>> +    Definition: Should contain the offset to the misc address space
>>>>
>>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
>>>> I think that you don't need to include this property.
>>>
>>> No it isn't used in the driver right now, but there is a register offset
>>> for this module and there are registers that can be read/written in this
>>> module. I'd like to keep it as required so we can easily read the
>>> registers in the future if needed.
>>
>> OK.
>> But, If you want to remain the reg property, you should add the code to get
>> the register offset by using OF functions. This patch don't include the OF
>> function to handle it.
>>
> 
> Sorry I don't follow the argument. I've put the reg property here for
> future proofing so that the binding doesn't have to change in backwards
> incompatible ways in the future if we do need to get the property later.

I don't mention that 'reg' property should be removed.
Just if you want to remain it, you should add some codes as following:
For exmaple,
- of_get_address() to get the address information from device-tree.

If documentation include the some properties, you should add the handling code
in device driver. When you add the code to get the offset from device-tree,
it doesn't influence the some behavior in the future.

Thanks,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-29  6:25           ` Chanwoo Choi
  (?)
@ 2016-06-29 18:48             ` Stephen Boyd
  -1 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-29 18:48 UTC (permalink / raw)
  To: Chanwoo Choi, Chanwoo Choi
  Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, Roger Quadros

Quoting Chanwoo Choi (2016-06-28 23:25:57)
> On 2016년 06월 29일 06:59, Stephen Boyd wrote:
> > Quoting Chanwoo Choi (2016-06-28 05:06:48)
> >> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> >>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
> >>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> >>>>> +PROPERTIES
> >>>>> +
> >>>>> +- compatible:
> >>>>> +    Usage: required
> >>>>> +    Value type: <string>
> >>>>> +    Definition: Should contain "qcom,pm8941-misc";
> >>>>> +
> >>>>> +- reg:
> >>>>> +    Usage: required
> >>>>> +    Value type: <u32>
> >>>>> +    Definition: Should contain the offset to the misc address space
> >>>>
> >>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
> >>>> I think that you don't need to include this property.
> >>>
> >>> No it isn't used in the driver right now, but there is a register offset
> >>> for this module and there are registers that can be read/written in this
> >>> module. I'd like to keep it as required so we can easily read the
> >>> registers in the future if needed.
> >>
> >> OK.
> >> But, If you want to remain the reg property, you should add the code to get
> >> the register offset by using OF functions. This patch don't include the OF
> >> function to handle it.
> >>
> > 
> > Sorry I don't follow the argument. I've put the reg property here for
> > future proofing so that the binding doesn't have to change in backwards
> > incompatible ways in the future if we do need to get the property later.
> 
> I don't mention that 'reg' property should be removed.

Ok good. We need to keep reg property as this device is on a bus that
uses reg property for addressing.

> Just if you want to remain it, you should add some codes as following:
> For exmaple,
> - of_get_address() to get the address information from device-tree.
> 
> If documentation include the some properties, you should add the handling code
> in device driver. When you add the code to get the offset from device-tree,
> it doesn't influence the some behavior in the future.

Sorry I don't understand that argument. We can put properties into
bindings and not use them in drivers if there isn't any immediate need
to use them.

From what I can tell you're suggesting we call of_get_address() in the
driver and then do nothing with the value of the property? Is that just
to check that the node is compliant with the binding and actually has a
reg property? We don't add code in the kernel to check dts compliance,
so I'm not inclined to do anything more here.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-29 18:48             ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-29 18:48 UTC (permalink / raw)
  To: Chanwoo Choi, Chanwoo Choi
  Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, Roger Quadros

Quoting Chanwoo Choi (2016-06-28 23:25:57)
> On 2016년 06월 29일 06:59, Stephen Boyd wrote:
> > Quoting Chanwoo Choi (2016-06-28 05:06:48)
> >> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> >>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
> >>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> >>>>> +PROPERTIES
> >>>>> +
> >>>>> +- compatible:
> >>>>> +    Usage: required
> >>>>> +    Value type: <string>
> >>>>> +    Definition: Should contain "qcom,pm8941-misc";
> >>>>> +
> >>>>> +- reg:
> >>>>> +    Usage: required
> >>>>> +    Value type: <u32>
> >>>>> +    Definition: Should contain the offset to the misc address space
> >>>>
> >>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
> >>>> I think that you don't need to include this property.
> >>>
> >>> No it isn't used in the driver right now, but there is a register offset
> >>> for this module and there are registers that can be read/written in this
> >>> module. I'd like to keep it as required so we can easily read the
> >>> registers in the future if needed.
> >>
> >> OK.
> >> But, If you want to remain the reg property, you should add the code to get
> >> the register offset by using OF functions. This patch don't include the OF
> >> function to handle it.
> >>
> > 
> > Sorry I don't follow the argument. I've put the reg property here for
> > future proofing so that the binding doesn't have to change in backwards
> > incompatible ways in the future if we do need to get the property later.
> 
> I don't mention that 'reg' property should be removed.

Ok good. We need to keep reg property as this device is on a bus that
uses reg property for addressing.

> Just if you want to remain it, you should add some codes as following:
> For exmaple,
> - of_get_address() to get the address information from device-tree.
> 
> If documentation include the some properties, you should add the handling code
> in device driver. When you add the code to get the offset from device-tree,
> it doesn't influence the some behavior in the future.

Sorry I don't understand that argument. We can put properties into
bindings and not use them in drivers if there isn't any immediate need
to use them.

>From what I can tell you're suggesting we call of_get_address() in the
driver and then do nothing with the value of the property? Is that just
to check that the node is compliant with the binding and actually has a
reg property? We don't add code in the kernel to check dts compliance,
so I'm not inclined to do anything more here.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-29 18:48             ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-29 18:48 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Chanwoo Choi (2016-06-28 23:25:57)
> On 2016? 06? 29? 06:59, Stephen Boyd wrote:
> > Quoting Chanwoo Choi (2016-06-28 05:06:48)
> >> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> >>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
> >>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
> >>>>> +PROPERTIES
> >>>>> +
> >>>>> +- compatible:
> >>>>> +    Usage: required
> >>>>> +    Value type: <string>
> >>>>> +    Definition: Should contain "qcom,pm8941-misc";
> >>>>> +
> >>>>> +- reg:
> >>>>> +    Usage: required
> >>>>> +    Value type: <u32>
> >>>>> +    Definition: Should contain the offset to the misc address space
> >>>>
> >>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
> >>>> I think that you don't need to include this property.
> >>>
> >>> No it isn't used in the driver right now, but there is a register offset
> >>> for this module and there are registers that can be read/written in this
> >>> module. I'd like to keep it as required so we can easily read the
> >>> registers in the future if needed.
> >>
> >> OK.
> >> But, If you want to remain the reg property, you should add the code to get
> >> the register offset by using OF functions. This patch don't include the OF
> >> function to handle it.
> >>
> > 
> > Sorry I don't follow the argument. I've put the reg property here for
> > future proofing so that the binding doesn't have to change in backwards
> > incompatible ways in the future if we do need to get the property later.
> 
> I don't mention that 'reg' property should be removed.

Ok good. We need to keep reg property as this device is on a bus that
uses reg property for addressing.

> Just if you want to remain it, you should add some codes as following:
> For exmaple,
> - of_get_address() to get the address information from device-tree.
> 
> If documentation include the some properties, you should add the handling code
> in device driver. When you add the code to get the offset from device-tree,
> it doesn't influence the some behavior in the future.

Sorry I don't understand that argument. We can put properties into
bindings and not use them in drivers if there isn't any immediate need
to use them.

>From what I can tell you're suggesting we call of_get_address() in the
driver and then do nothing with the value of the property? Is that just
to check that the node is compliant with the binding and actually has a
reg property? We don't add code in the kernel to check dts compliance,
so I'm not inclined to do anything more here.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-29  6:10               ` Roger Quadros
@ 2016-06-29 18:51                 ` Stephen Boyd
  -1 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-29 18:51 UTC (permalink / raw)
  To: Roger Quadros, Chanwoo Choi; +Cc: linux-arm-kernel, linux-kernel, linux-arm-msm

Quoting Roger Quadros (2016-06-28 23:10:57)
> On 29/06/16 01:01, Stephen Boyd wrote:
> > Quoting Roger Quadros (2016-06-28 02:13:57)
> >> On 28/06/16 11:47, Stephen Boyd wrote:
> >>>
> >>> Sorry I must have confused you. There are two modules in the PMIC that
> >>> are doing detection here. The charger module is detecting the VBUS event
> >>> and this misc module is detecting the ID pin. I'm not sure why they're
> >>> two different blocks, but it is what it is in the hardware.
> >>>
> >> OK. Can the MISC block do anything else other than USB ID?
> > 
> > Yes.
> > 
> >> Does the USB ID interrupt come on a different line than the charger interrupt?
> > 
> > Yes.
> > 
> 
> OK. Is it better to have this driver somewhere in drivers/mfd or drivers/misc if the
> other function doesn't need/use extcon?
> 

If/when the other functionality is used by the kernel we can consider
moving this driver somewhere else. As there currently isn't any other
usage of this device node I implemented it in the extcon directory to be
explicit about the usage.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-29 18:51                 ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-29 18:51 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Roger Quadros (2016-06-28 23:10:57)
> On 29/06/16 01:01, Stephen Boyd wrote:
> > Quoting Roger Quadros (2016-06-28 02:13:57)
> >> On 28/06/16 11:47, Stephen Boyd wrote:
> >>>
> >>> Sorry I must have confused you. There are two modules in the PMIC that
> >>> are doing detection here. The charger module is detecting the VBUS event
> >>> and this misc module is detecting the ID pin. I'm not sure why they're
> >>> two different blocks, but it is what it is in the hardware.
> >>>
> >> OK. Can the MISC block do anything else other than USB ID?
> > 
> > Yes.
> > 
> >> Does the USB ID interrupt come on a different line than the charger interrupt?
> > 
> > Yes.
> > 
> 
> OK. Is it better to have this driver somewhere in drivers/mfd or drivers/misc if the
> other function doesn't need/use extcon?
> 

If/when the other functionality is used by the kernel we can consider
moving this driver somewhere else. As there currently isn't any other
usage of this device node I implemented it in the extcon directory to be
explicit about the usage.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-29 18:48             ` Stephen Boyd
@ 2016-06-30  0:35               ` Chanwoo Choi
  -1 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-30  0:35 UTC (permalink / raw)
  To: Stephen Boyd, Chanwoo Choi
  Cc: linux-arm-msm, linux-kernel, linux-arm-kernel, Roger Quadros

On 2016년 06월 30일 03:48, Stephen Boyd wrote:
> Quoting Chanwoo Choi (2016-06-28 23:25:57)
>> On 2016년 06월 29일 06:59, Stephen Boyd wrote:
>>> Quoting Chanwoo Choi (2016-06-28 05:06:48)
>>>> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
>>>>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>>>> +PROPERTIES
>>>>>>> +
>>>>>>> +- compatible:
>>>>>>> +    Usage: required
>>>>>>> +    Value type: <string>
>>>>>>> +    Definition: Should contain "qcom,pm8941-misc";
>>>>>>> +
>>>>>>> +- reg:
>>>>>>> +    Usage: required
>>>>>>> +    Value type: <u32>
>>>>>>> +    Definition: Should contain the offset to the misc address space
>>>>>>
>>>>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
>>>>>> I think that you don't need to include this property.
>>>>>
>>>>> No it isn't used in the driver right now, but there is a register offset
>>>>> for this module and there are registers that can be read/written in this
>>>>> module. I'd like to keep it as required so we can easily read the
>>>>> registers in the future if needed.
>>>>
>>>> OK.
>>>> But, If you want to remain the reg property, you should add the code to get
>>>> the register offset by using OF functions. This patch don't include the OF
>>>> function to handle it.
>>>>
>>>
>>> Sorry I don't follow the argument. I've put the reg property here for
>>> future proofing so that the binding doesn't have to change in backwards
>>> incompatible ways in the future if we do need to get the property later.
>>
>> I don't mention that 'reg' property should be removed.
> 
> Ok good. We need to keep reg property as this device is on a bus that
> uses reg property for addressing.
> 
>> Just if you want to remain it, you should add some codes as following:
>> For exmaple,
>> - of_get_address() to get the address information from device-tree.
>>
>> If documentation include the some properties, you should add the handling code
>> in device driver. When you add the code to get the offset from device-tree,
>> it doesn't influence the some behavior in the future.
> 
> Sorry I don't understand that argument. We can put properties into
> bindings and not use them in drivers if there isn't any immediate need
> to use them.
> 
> From what I can tell you're suggesting we call of_get_address() in the
> driver and then do nothing with the value of the property? Is that just
> to check that the node is compliant with the binding and actually has a
> reg property? We don't add code in the kernel to check dts compliance,
> so I'm not inclined to do anything more here.

I don't agree.

When he DT binding document include the 'reg' property.
But, the device driver don't include any code to handle the 'reg' property
(just to get the offset). It is obviously wrong.

It is just basic principle to write the Device-tree binding document.

Other developer who don' know the history about 'reg' property
would be embarrassed. Why don't extcon driver include the code to
handle the 'reg' property? There is no method to explain it.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-30  0:35               ` Chanwoo Choi
  0 siblings, 0 replies; 43+ messages in thread
From: Chanwoo Choi @ 2016-06-30  0:35 UTC (permalink / raw)
  To: linux-arm-kernel

On 2016? 06? 30? 03:48, Stephen Boyd wrote:
> Quoting Chanwoo Choi (2016-06-28 23:25:57)
>> On 2016? 06? 29? 06:59, Stephen Boyd wrote:
>>> Quoting Chanwoo Choi (2016-06-28 05:06:48)
>>>> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
>>>>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>>>> +PROPERTIES
>>>>>>> +
>>>>>>> +- compatible:
>>>>>>> +    Usage: required
>>>>>>> +    Value type: <string>
>>>>>>> +    Definition: Should contain "qcom,pm8941-misc";
>>>>>>> +
>>>>>>> +- reg:
>>>>>>> +    Usage: required
>>>>>>> +    Value type: <u32>
>>>>>>> +    Definition: Should contain the offset to the misc address space
>>>>>>
>>>>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
>>>>>> I think that you don't need to include this property.
>>>>>
>>>>> No it isn't used in the driver right now, but there is a register offset
>>>>> for this module and there are registers that can be read/written in this
>>>>> module. I'd like to keep it as required so we can easily read the
>>>>> registers in the future if needed.
>>>>
>>>> OK.
>>>> But, If you want to remain the reg property, you should add the code to get
>>>> the register offset by using OF functions. This patch don't include the OF
>>>> function to handle it.
>>>>
>>>
>>> Sorry I don't follow the argument. I've put the reg property here for
>>> future proofing so that the binding doesn't have to change in backwards
>>> incompatible ways in the future if we do need to get the property later.
>>
>> I don't mention that 'reg' property should be removed.
> 
> Ok good. We need to keep reg property as this device is on a bus that
> uses reg property for addressing.
> 
>> Just if you want to remain it, you should add some codes as following:
>> For exmaple,
>> - of_get_address() to get the address information from device-tree.
>>
>> If documentation include the some properties, you should add the handling code
>> in device driver. When you add the code to get the offset from device-tree,
>> it doesn't influence the some behavior in the future.
> 
> Sorry I don't understand that argument. We can put properties into
> bindings and not use them in drivers if there isn't any immediate need
> to use them.
> 
> From what I can tell you're suggesting we call of_get_address() in the
> driver and then do nothing with the value of the property? Is that just
> to check that the node is compliant with the binding and actually has a
> reg property? We don't add code in the kernel to check dts compliance,
> so I'm not inclined to do anything more here.

I don't agree.

When he DT binding document include the 'reg' property.
But, the device driver don't include any code to handle the 'reg' property
(just to get the offset). It is obviously wrong.

It is just basic principle to write the Device-tree binding document.

Other developer who don' know the history about 'reg' property
would be embarrassed. Why don't extcon driver include the code to
handle the 'reg' property? There is no method to explain it.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
  2016-06-30  0:35               ` Chanwoo Choi
@ 2016-06-30  1:04                 ` Stephen Boyd
  -1 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-30  1:04 UTC (permalink / raw)
  To: Chanwoo Choi, Rob Herring
  Cc: Chanwoo Choi, linux-arm-msm, linux-kernel, linux-arm-kernel,
	Roger Quadros

On 29 June 2016 at 17:35, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 2016년 06월 30일 03:48, Stephen Boyd wrote:
>> Quoting Chanwoo Choi (2016-06-28 23:25:57)
>>> On 2016년 06월 29일 06:59, Stephen Boyd wrote:
>>>> Quoting Chanwoo Choi (2016-06-28 05:06:48)
>>>>> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
>>>>>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>>>>> +PROPERTIES
>>>>>>>> +
>>>>>>>> +- compatible:
>>>>>>>> +    Usage: required
>>>>>>>> +    Value type: <string>
>>>>>>>> +    Definition: Should contain "qcom,pm8941-misc";
>>>>>>>> +
>>>>>>>> +- reg:
>>>>>>>> +    Usage: required
>>>>>>>> +    Value type: <u32>
>>>>>>>> +    Definition: Should contain the offset to the misc address space
>>>>>>>
>>>>>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
>>>>>>> I think that you don't need to include this property.
>>>>>>
>>>>>> No it isn't used in the driver right now, but there is a register offset
>>>>>> for this module and there are registers that can be read/written in this
>>>>>> module. I'd like to keep it as required so we can easily read the
>>>>>> registers in the future if needed.
>>>>>
>>>>> OK.
>>>>> But, If you want to remain the reg property, you should add the code to get
>>>>> the register offset by using OF functions. This patch don't include the OF
>>>>> function to handle it.
>>>>>
>>>>
>>>> Sorry I don't follow the argument. I've put the reg property here for
>>>> future proofing so that the binding doesn't have to change in backwards
>>>> incompatible ways in the future if we do need to get the property later.
>>>
>>> I don't mention that 'reg' property should be removed.
>>
>> Ok good. We need to keep reg property as this device is on a bus that
>> uses reg property for addressing.
>>
>>> Just if you want to remain it, you should add some codes as following:
>>> For exmaple,
>>> - of_get_address() to get the address information from device-tree.
>>>
>>> If documentation include the some properties, you should add the handling code
>>> in device driver. When you add the code to get the offset from device-tree,
>>> it doesn't influence the some behavior in the future.
>>
>> Sorry I don't understand that argument. We can put properties into
>> bindings and not use them in drivers if there isn't any immediate need
>> to use them.
>>
>> From what I can tell you're suggesting we call of_get_address() in the
>> driver and then do nothing with the value of the property? Is that just
>> to check that the node is compliant with the binding and actually has a
>> reg property? We don't add code in the kernel to check dts compliance,
>> so I'm not inclined to do anything more here.
>
> I don't agree.
>
> When he DT binding document include the 'reg' property.
> But, the device driver don't include any code to handle the 'reg' property
> (just to get the offset). It is obviously wrong.
>
> It is just basic principle to write the Device-tree binding document.
>
> Other developer who don' know the history about 'reg' property
> would be embarrassed. Why don't extcon driver include the code to
> handle the 'reg' property? There is no method to explain it.
>

Perhaps Rob can explain why having a reg property is required for a
bus that has #address-cells=<1> even though the device driver isn't
using the reg property.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware
@ 2016-06-30  1:04                 ` Stephen Boyd
  0 siblings, 0 replies; 43+ messages in thread
From: Stephen Boyd @ 2016-06-30  1:04 UTC (permalink / raw)
  To: linux-arm-kernel

On 29 June 2016 at 17:35, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 2016? 06? 30? 03:48, Stephen Boyd wrote:
>> Quoting Chanwoo Choi (2016-06-28 23:25:57)
>>> On 2016? 06? 29? 06:59, Stephen Boyd wrote:
>>>> Quoting Chanwoo Choi (2016-06-28 05:06:48)
>>>>> 2016-06-28 4:11 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>>> Quoting Chanwoo Choi (2016-06-26 04:20:43)
>>>>>>> 2016-06-26 14:56 GMT+09:00 Stephen Boyd <stephen.boyd@linaro.org>:
>>>>>>>> +PROPERTIES
>>>>>>>> +
>>>>>>>> +- compatible:
>>>>>>>> +    Usage: required
>>>>>>>> +    Value type: <string>
>>>>>>>> +    Definition: Should contain "qcom,pm8941-misc";
>>>>>>>> +
>>>>>>>> +- reg:
>>>>>>>> +    Usage: required
>>>>>>>> +    Value type: <u32>
>>>>>>>> +    Definition: Should contain the offset to the misc address space
>>>>>>>
>>>>>>> 'reg' property is used on extcon-qcom-spmi-misc.c?
>>>>>>> I think that you don't need to include this property.
>>>>>>
>>>>>> No it isn't used in the driver right now, but there is a register offset
>>>>>> for this module and there are registers that can be read/written in this
>>>>>> module. I'd like to keep it as required so we can easily read the
>>>>>> registers in the future if needed.
>>>>>
>>>>> OK.
>>>>> But, If you want to remain the reg property, you should add the code to get
>>>>> the register offset by using OF functions. This patch don't include the OF
>>>>> function to handle it.
>>>>>
>>>>
>>>> Sorry I don't follow the argument. I've put the reg property here for
>>>> future proofing so that the binding doesn't have to change in backwards
>>>> incompatible ways in the future if we do need to get the property later.
>>>
>>> I don't mention that 'reg' property should be removed.
>>
>> Ok good. We need to keep reg property as this device is on a bus that
>> uses reg property for addressing.
>>
>>> Just if you want to remain it, you should add some codes as following:
>>> For exmaple,
>>> - of_get_address() to get the address information from device-tree.
>>>
>>> If documentation include the some properties, you should add the handling code
>>> in device driver. When you add the code to get the offset from device-tree,
>>> it doesn't influence the some behavior in the future.
>>
>> Sorry I don't understand that argument. We can put properties into
>> bindings and not use them in drivers if there isn't any immediate need
>> to use them.
>>
>> From what I can tell you're suggesting we call of_get_address() in the
>> driver and then do nothing with the value of the property? Is that just
>> to check that the node is compliant with the binding and actually has a
>> reg property? We don't add code in the kernel to check dts compliance,
>> so I'm not inclined to do anything more here.
>
> I don't agree.
>
> When he DT binding document include the 'reg' property.
> But, the device driver don't include any code to handle the 'reg' property
> (just to get the offset). It is obviously wrong.
>
> It is just basic principle to write the Device-tree binding document.
>
> Other developer who don' know the history about 'reg' property
> would be embarrassed. Why don't extcon driver include the code to
> handle the 'reg' property? There is no method to explain it.
>

Perhaps Rob can explain why having a reg property is required for a
bus that has #address-cells=<1> even though the device driver isn't
using the reg property.

^ permalink raw reply	[flat|nested] 43+ messages in thread

end of thread, other threads:[~2016-06-30  1:04 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-26  5:56 [PATCH] extcon: Add support for qcom SPMI PMIC USB id detection hardware Stephen Boyd
2016-06-26  5:56 ` Stephen Boyd
2016-06-26 11:20 ` Chanwoo Choi
2016-06-26 11:20   ` Chanwoo Choi
2016-06-26 11:22   ` Chanwoo Choi
2016-06-26 11:22     ` Chanwoo Choi
2016-06-26 11:22     ` Chanwoo Choi
2016-06-27 19:11   ` Stephen Boyd
2016-06-27 19:11     ` Stephen Boyd
2016-06-28 12:06     ` Chanwoo Choi
2016-06-28 12:06       ` Chanwoo Choi
2016-06-28 21:59       ` Stephen Boyd
2016-06-28 21:59         ` Stephen Boyd
2016-06-29  6:25         ` Chanwoo Choi
2016-06-29  6:25           ` Chanwoo Choi
2016-06-29 18:48           ` Stephen Boyd
2016-06-29 18:48             ` Stephen Boyd
2016-06-29 18:48             ` Stephen Boyd
2016-06-30  0:35             ` Chanwoo Choi
2016-06-30  0:35               ` Chanwoo Choi
2016-06-30  1:04               ` Stephen Boyd
2016-06-30  1:04                 ` Stephen Boyd
2016-06-27  7:39 ` Roger Quadros
2016-06-27  7:39   ` Roger Quadros
2016-06-27  7:39   ` Roger Quadros
2016-06-27 19:30   ` Stephen Boyd
2016-06-27 19:30     ` Stephen Boyd
2016-06-27 19:30     ` Stephen Boyd
2016-06-28  6:36     ` Roger Quadros
2016-06-28  6:36       ` Roger Quadros
2016-06-28  6:36       ` Roger Quadros
2016-06-28  8:47       ` Stephen Boyd
2016-06-28  8:47         ` Stephen Boyd
2016-06-28  9:13         ` Roger Quadros
2016-06-28  9:13           ` Roger Quadros
2016-06-28  9:13           ` Roger Quadros
2016-06-28 22:01           ` Stephen Boyd
2016-06-28 22:01             ` Stephen Boyd
2016-06-29  6:10             ` Roger Quadros
2016-06-29  6:10               ` Roger Quadros
2016-06-29  6:10               ` Roger Quadros
2016-06-29 18:51               ` Stephen Boyd
2016-06-29 18:51                 ` Stephen Boyd

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.