linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drivers: extcon: Add support for ptn5150
@ 2019-01-21  9:09 ` Vijai Kumar K
  2019-01-22  0:05   ` kbuild test robot
  2019-01-22  6:20   ` Chanwoo Choi
  0 siblings, 2 replies; 12+ messages in thread
From: Vijai Kumar K @ 2019-01-21  9:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, myungjoo.ham, Vijai Kumar K

PTN5150A is a small thin low power CC Logic chip supporting
the USB Type-C connector application with Configurationn Channel(CC)
control logic detection and indication functions.

Add driver, Kconfig and Makefile entries.

Change-Id: I3b2da147a33b913b9ec60cd914b20acd955950c7
Signed-off-by: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
---
 .../devicetree/bindings/extcon/extcon-ptn5150.txt  |  22 ++
 drivers/extcon/Kconfig                             |   8 +
 drivers/extcon/Makefile                            |   1 +
 drivers/extcon/extcon-ptn5150.c                    | 327 +++++++++++++++++++++
 drivers/extcon/extcon-ptn5150.h                    |  51 ++++
 5 files changed, 409 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
 create mode 100644 drivers/extcon/extcon-ptn5150.c
 create mode 100644 drivers/extcon/extcon-ptn5150.h

diff --git a/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
new file mode 100644
index 0000000..8ea33bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
@@ -0,0 +1,22 @@
+
+* PTN5150 CC logic device
+PTN5150A is a small thin low power CC Logic chip supporting the USB Type-C
+connector application with Configuration Channel (CC) control logic detection and
+indication functions. It is Interfaced to the host controller using an I2C interface.
+
+Required properties:
+- compatible: Should be "nxp,ptn5150"
+- reg: Specifies the I2C slave address of the device
+- int-gpio: GPIO to which INTB is connected
+- vbus-gpio: GPIO which controls VBUS on/off
+
+Example:
+		ptn5150@1d  {
+			compatible = "nxp,ptn5150";
+			reg = <0x1d>;
+			int-gpio = <&msmgpio 78 GPIO_ACTIVE_HIGH>;
+			vbus-gpio = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&ptn5150_default>;
+			status = "okay";
+		};
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index a7bca42..6adc797 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -113,6 +113,14 @@ config EXTCON_PALMAS
 	  Say Y here to enable support for USB peripheral and USB host
 	  detection by palmas usb.
 
+config EXTCON_PTN5150
+	tristate "PTN5150 CC LOGIC USB EXTCON support"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to enable support for USB peripheral and USB host
+	  detection by ptn5150 cc logic chip.
+
 config EXTCON_QCOM_SPMI_MISC
 	tristate "Qualcomm USB extcon support"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0888fde..261ce4c 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
 obj-$(CONFIG_EXTCON_MAX77843)	+= extcon-max77843.o
 obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
 obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
+obj-$(CONFIG_EXTCON_PTN5150)	+= extcon-ptn5150.o
 obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
new file mode 100644
index 0000000..bc00874
--- /dev/null
+++ b/drivers/extcon/extcon-ptn5150.c
@@ -0,0 +1,327 @@
+/*
+ * extcon-ptn5150.c - PTN5150 CC logic extcon driver to support USB detection
+ *
+ * Copyright (c) 2018-2019 by Vijai Kumar K
+ * Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
+ *
+ * Based on extcon-sm5502.c driver
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/extcon.h>
+#include <linux/gpio.h>
+
+#include "extcon-ptn5150.h"
+
+struct ptn5150_info {
+	struct device *dev;
+	struct extcon_dev *edev;
+	struct i2c_client *i2c;
+	struct regmap *regmap;
+	struct gpio_desc *int_gpiod;
+	struct gpio_desc *vbus_gpiod;
+	int irq;
+	struct work_struct irq_work;
+	struct mutex mutex;
+};
+
+/* List of detectable cables */
+static const unsigned int ptn5150_extcon_cable[] = {
+	EXTCON_USB,
+	EXTCON_USB_HOST,
+	EXTCON_NONE,
+};
+
+static const struct regmap_config ptn5150_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= PTN5150_REG_END,
+};
+
+static void ptn5150_irq_work(struct work_struct *work)
+{
+	struct ptn5150_info *info = container_of(work,
+			struct ptn5150_info, irq_work);
+	int ret = 0;
+	unsigned int reg_data;
+	unsigned int port_status;
+	unsigned int vbus;
+	unsigned int cable_attach;
+	unsigned int int_status;
+
+	if (!info->edev)
+		return;
+
+	mutex_lock(&info->mutex);
+
+	ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read CC STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+	/* Clear interrupt. Read would clear the register */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read INT STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	if (int_status) {
+		cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
+		if (cable_attach) {
+			port_status = ((reg_data &
+					PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
+					PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
+
+			switch (port_status) {
+			case PTN5150_DFP_ATTACHED:
+				extcon_set_state_sync(info->edev,
+						      EXTCON_USB_HOST, false);
+				gpiod_set_value(info->vbus_gpiod, 0);
+				extcon_set_state_sync(info->edev, EXTCON_USB,
+						      true);
+				break;
+			case PTN5150_UFP_ATTACHED:
+				extcon_set_state_sync(info->edev, EXTCON_USB,
+						      false);
+				vbus = ((reg_data &
+					PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
+					PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
+				if (vbus)
+					gpiod_set_value(info->vbus_gpiod, 0);
+				else
+					gpiod_set_value(info->vbus_gpiod, 1);
+
+				extcon_set_state_sync(info->edev,
+						      EXTCON_USB_HOST, true);
+				break;
+			default:
+				dev_err(info->dev,
+					"Unknown Port status : %x\n",
+					port_status);
+				break;
+			}
+		} else {
+			extcon_set_state_sync(info->edev,
+					      EXTCON_USB_HOST, false);
+			extcon_set_state_sync(info->edev,
+					      EXTCON_USB, false);
+			gpiod_set_value(info->vbus_gpiod, 0);
+		}
+	}
+
+	/* Clear interrupt. Read would clear the register */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
+				&int_status);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read INT REG STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+
+	mutex_unlock(&info->mutex);
+}
+
+
+static irqreturn_t ptn5150_irq_handler(int irq, void *data)
+{
+	struct ptn5150_info *info = data;
+
+	schedule_work(&info->irq_work);
+
+	return IRQ_HANDLED;
+}
+
+static int ptn5150_init_dev_type(struct ptn5150_info *info)
+{
+	unsigned int reg_data, vendor_id, version_id;
+	int ret;
+
+	ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
+	if (ret) {
+		dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
+		return -EINVAL;
+	}
+
+	vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
+				PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
+	version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
+				PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
+
+	dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
+			    version_id, vendor_id);
+
+	/* Clear any existing interrupts */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read PTN5150_REG_INT_STATUS %d\n",
+			ret);
+		return -EINVAL;
+	}
+
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ptn5150_i2c_probe(struct i2c_client *i2c,
+				 const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct device_node *np = i2c->dev.of_node;
+	struct ptn5150_info *info;
+	int ret;
+
+	if (!np)
+		return -EINVAL;
+
+	info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+	i2c_set_clientdata(i2c, info);
+
+	info->dev = &i2c->dev;
+	info->i2c = i2c;
+	info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
+	if (!info->int_gpiod) {
+		dev_err(dev, "failed to get INT GPIO\n");
+		return -EINVAL;
+	}
+	info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
+	if (!info->vbus_gpiod) {
+		dev_err(dev, "failed to get VBUS GPIO\n");
+		return -EINVAL;
+	}
+	ret = gpiod_direction_output(info->vbus_gpiod, 0);
+	if (ret) {
+		dev_err(dev, "failed to set VBUS GPIO direction\n");
+		return -EINVAL;
+	}
+
+	mutex_init(&info->mutex);
+
+	INIT_WORK(&info->irq_work, ptn5150_irq_work);
+
+	info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
+	if (IS_ERR(info->regmap)) {
+		ret = PTR_ERR(info->regmap);
+		dev_err(info->dev, "failed to allocate register map: %d\n",
+				   ret);
+		return ret;
+	}
+
+	if (info->int_gpiod) {
+		info->irq = gpiod_to_irq(info->int_gpiod);
+		if (info->irq < 0) {
+			dev_err(dev, "failed to get INTB IRQ\n");
+			return info->irq;
+		}
+
+		ret = devm_request_threaded_irq(dev, info->irq, NULL,
+						ptn5150_irq_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						i2c->name, info);
+		if (ret < 0) {
+			dev_err(dev, "failed to request handler for INTB IRQ\n");
+			return ret;
+		}
+	}
+
+	/* Allocate extcon device */
+	info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
+	if (IS_ERR(info->edev)) {
+		dev_err(info->dev, "failed to allocate memory for extcon\n");
+		return -ENOMEM;
+	}
+
+	/* Register extcon device */
+	ret = devm_extcon_dev_register(info->dev, info->edev);
+	if (ret) {
+		dev_err(info->dev, "failed to register extcon device\n");
+		return ret;
+	}
+
+	/* Initialize PTN5150 device and print vendor id and version id */
+	ret = ptn5150_init_dev_type(info);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ptn5150_i2c_remove(struct i2c_client *i2c)
+{
+	return 0;
+}
+
+static const struct of_device_id ptn5150_dt_match[] = {
+	{ .compatible = "nxp,ptn5150" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ptn5150_dt_match);
+
+#ifdef CONFIG_PM_SLEEP
+static int ptn5150_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int ptn5150_resume(struct device *dev)
+{
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(ptn5150_pm_ops,
+			 ptn5150_suspend, ptn5150_resume);
+
+static const struct i2c_device_id ptn5150_i2c_id[] = {
+	{ "ptn5150", TYPE_PTN5150A },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ptn5150_i2c_id);
+
+static struct i2c_driver ptn5150_i2c_driver = {
+	.driver		= {
+		.name	= "ptn5150",
+		.pm	= &ptn5150_pm_ops,
+		.of_match_table = ptn5150_dt_match,
+	},
+	.probe	= ptn5150_i2c_probe,
+	.remove	= ptn5150_i2c_remove,
+	.id_table = ptn5150_i2c_id,
+};
+
+static int __init ptn5150_i2c_init(void)
+{
+	return i2c_add_driver(&ptn5150_i2c_driver);
+}
+subsys_initcall(ptn5150_i2c_init);
+
+MODULE_DESCRIPTION("NXP PTN5150 CC logic Extcon driver");
+MODULE_AUTHOR("Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/extcon/extcon-ptn5150.h b/drivers/extcon/extcon-ptn5150.h
new file mode 100644
index 0000000..217dab0
--- /dev/null
+++ b/drivers/extcon/extcon-ptn5150.h
@@ -0,0 +1,51 @@
+/*
+ * extcon-ptn5150.h
+ *
+ * Copyright (c) 2018-2019 by Vijai Kumar K
+ * Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#ifndef __LINUX_EXTCON_PTN5150_H
+#define __LINUX_EXTCON_PTN5150_H
+
+enum ptn5150_types {
+	TYPE_PTN5150A,
+};
+
+/* PTN5150 registers */
+enum ptn5150_reg {
+	PTN5150_REG_DEVICE_ID = 0x01,
+	PTN5150_REG_CONTROL,
+	PTN5150_REG_INT_STATUS,
+	PTN5150_REG_CC_STATUS,
+	PTN5150_REG_CON_DET = 0x09,
+	PTN5150_REG_VCONN_STATUS,
+	PTN5150_REG_RESET,
+	PTN5150_REG_INT_MASK = 0x18,
+	PTN5150_REG_INT_REG_STATUS,
+	PTN5150_REG_END,
+};
+
+#define PTN5150_DFP_ATTACHED			0x1
+#define PTN5150_UFP_ATTACHED			0x2
+
+/* Define PTN5150 MASK/SHIFT constant */
+#define PTN5150_REG_DEVICE_ID_VENDOR_SHIFT	0
+#define PTN5150_REG_DEVICE_ID_VERSION_SHIFT	3
+#define PTN5150_REG_DEVICE_ID_VENDOR_MASK	(0x3 << PTN5150_REG_DEVICE_ID_VENDOR_SHIFT)
+#define PTN5150_REG_DEVICE_ID_VERSION_MASK	(0x1f << PTN5150_REG_DEVICE_ID_VERSION_SHIFT)
+
+#define PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT	2
+#define PTN5150_REG_CC_PORT_ATTACHMENT_MASK	(0x7 << PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT)
+#define PTN5150_REG_CC_VBUS_DETECTION_SHIFT	7
+#define PTN5150_REG_CC_VBUS_DETECTION_MASK	(0x1 << PTN5150_REG_CC_VBUS_DETECTION_SHIFT)
+#define PTN5150_REG_INT_CABLE_ATTACH_SHIFT	0
+#define PTN5150_REG_INT_CABLE_ATTACH_MASK	(0x1 << PTN5150_REG_INT_CABLE_ATTACH_SHIFT)
+#define PTN5150_REG_INT_CABLE_DETACH_SHIFT	1
+#define PTN5150_REG_INT_CABLE_DETACH_MASK	(0x1 << PTN5150_REG_CC_CABLE_DETACH_SHIFT)
+#endif /*  __LINUX_EXTCON_PTN5150_H */
-- 
2.7.4


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

* Re: [PATCH] drivers: extcon: Add support for ptn5150
  2019-01-21  9:09 ` [PATCH] drivers: extcon: Add support for ptn5150 Vijai Kumar K
@ 2019-01-22  0:05   ` kbuild test robot
  2019-01-22  4:42     ` Vijai Kumar K
       [not found]     ` <CGME20190122044306epcas5p30f875260e568d5fb0e4909035060f8ff@epcms1p8>
  2019-01-22  6:20   ` Chanwoo Choi
  1 sibling, 2 replies; 12+ messages in thread
From: kbuild test robot @ 2019-01-22  0:05 UTC (permalink / raw)
  To: Vijai Kumar K
  Cc: kbuild-all, linux-kernel, cw00.choi, myungjoo.ham, Vijai Kumar K

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

Hi Vijai,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on chanwoo-extcon/extcon-next]
[also build test ERROR on v5.0-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vijai-Kumar-K/drivers-extcon-Add-support-for-ptn5150/20190122-041153
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
config: sh-allyesconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=sh 

All error/warnings (new ones prefixed by >>):

   drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_irq_work':
>> drivers//extcon/extcon-ptn5150.c:93:5: error: implicit declaration of function 'extcon_set_state_sync'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
        extcon_set_state_sync(info->edev,
        ^~~~~~~~~~~~~~~~~~~~~
        extcon_get_state
   drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_i2c_probe':
>> drivers//extcon/extcon-ptn5150.c:255:15: error: implicit declaration of function 'devm_extcon_dev_allocate'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
     info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
                  ^~~~~~~~~~~~~~~~~~~~~~~~
                  extcon_get_state
>> drivers//extcon/extcon-ptn5150.c:255:13: warning: assignment to 'struct extcon_dev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
                ^
>> drivers//extcon/extcon-ptn5150.c:262:8: error: implicit declaration of function 'devm_extcon_dev_register'; did you mean 'devm_pinctrl_register'? [-Werror=implicit-function-declaration]
     ret = devm_extcon_dev_register(info->dev, info->edev);
           ^~~~~~~~~~~~~~~~~~~~~~~~
           devm_pinctrl_register
   cc1: some warnings being treated as errors

vim +93 drivers//extcon/extcon-ptn5150.c

    51	
    52	static void ptn5150_irq_work(struct work_struct *work)
    53	{
    54		struct ptn5150_info *info = container_of(work,
    55				struct ptn5150_info, irq_work);
    56		int ret = 0;
    57		unsigned int reg_data;
    58		unsigned int port_status;
    59		unsigned int vbus;
    60		unsigned int cable_attach;
    61		unsigned int int_status;
    62	
    63		if (!info->edev)
    64			return;
    65	
    66		mutex_lock(&info->mutex);
    67	
    68		ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
    69		if (ret) {
    70			dev_err(info->dev,
    71				"failed to read CC STATUS %d\n", ret);
    72			mutex_unlock(&info->mutex);
    73			return;
    74		}
    75		/* Clear interrupt. Read would clear the register */
    76		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
    77		if (ret) {
    78			dev_err(info->dev,
    79				"failed to read INT STATUS %d\n", ret);
    80			mutex_unlock(&info->mutex);
    81			return;
    82		}
    83	
    84		if (int_status) {
    85			cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
    86			if (cable_attach) {
    87				port_status = ((reg_data &
    88						PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
    89						PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
    90	
    91				switch (port_status) {
    92				case PTN5150_DFP_ATTACHED:
  > 93					extcon_set_state_sync(info->edev,
    94							      EXTCON_USB_HOST, false);
    95					gpiod_set_value(info->vbus_gpiod, 0);
    96					extcon_set_state_sync(info->edev, EXTCON_USB,
    97							      true);
    98					break;
    99				case PTN5150_UFP_ATTACHED:
   100					extcon_set_state_sync(info->edev, EXTCON_USB,
   101							      false);
   102					vbus = ((reg_data &
   103						PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
   104						PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
   105					if (vbus)
   106						gpiod_set_value(info->vbus_gpiod, 0);
   107					else
   108						gpiod_set_value(info->vbus_gpiod, 1);
   109	
   110					extcon_set_state_sync(info->edev,
   111							      EXTCON_USB_HOST, true);
   112					break;
   113				default:
   114					dev_err(info->dev,
   115						"Unknown Port status : %x\n",
   116						port_status);
   117					break;
   118				}
   119			} else {
   120				extcon_set_state_sync(info->edev,
   121						      EXTCON_USB_HOST, false);
   122				extcon_set_state_sync(info->edev,
   123						      EXTCON_USB, false);
   124				gpiod_set_value(info->vbus_gpiod, 0);
   125			}
   126		}
   127	
   128		/* Clear interrupt. Read would clear the register */
   129		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
   130					&int_status);
   131		if (ret) {
   132			dev_err(info->dev,
   133				"failed to read INT REG STATUS %d\n", ret);
   134			mutex_unlock(&info->mutex);
   135			return;
   136		}
   137	
   138	
   139		mutex_unlock(&info->mutex);
   140	}
   141	
   142	
   143	static irqreturn_t ptn5150_irq_handler(int irq, void *data)
   144	{
   145		struct ptn5150_info *info = data;
   146	
   147		schedule_work(&info->irq_work);
   148	
   149		return IRQ_HANDLED;
   150	}
   151	
   152	static int ptn5150_init_dev_type(struct ptn5150_info *info)
   153	{
   154		unsigned int reg_data, vendor_id, version_id;
   155		int ret;
   156	
   157		ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
   158		if (ret) {
   159			dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
   160			return -EINVAL;
   161		}
   162	
   163		vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
   164					PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
   165		version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
   166					PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
   167	
   168		dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
   169				    version_id, vendor_id);
   170	
   171		/* Clear any existing interrupts */
   172		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
   173		if (ret) {
   174			dev_err(info->dev,
   175				"failed to read PTN5150_REG_INT_STATUS %d\n",
   176				ret);
   177			return -EINVAL;
   178		}
   179	
   180		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
   181		if (ret) {
   182			dev_err(info->dev,
   183				"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
   184			return -EINVAL;
   185		}
   186	
   187		return 0;
   188	}
   189	
   190	static int ptn5150_i2c_probe(struct i2c_client *i2c,
   191					 const struct i2c_device_id *id)
   192	{
   193		struct device *dev = &i2c->dev;
   194		struct device_node *np = i2c->dev.of_node;
   195		struct ptn5150_info *info;
   196		int ret;
   197	
   198		if (!np)
   199			return -EINVAL;
   200	
   201		info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
   202		if (!info)
   203			return -ENOMEM;
   204		i2c_set_clientdata(i2c, info);
   205	
   206		info->dev = &i2c->dev;
   207		info->i2c = i2c;
   208		info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
   209		if (!info->int_gpiod) {
   210			dev_err(dev, "failed to get INT GPIO\n");
   211			return -EINVAL;
   212		}
   213		info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
   214		if (!info->vbus_gpiod) {
   215			dev_err(dev, "failed to get VBUS GPIO\n");
   216			return -EINVAL;
   217		}
   218		ret = gpiod_direction_output(info->vbus_gpiod, 0);
   219		if (ret) {
   220			dev_err(dev, "failed to set VBUS GPIO direction\n");
   221			return -EINVAL;
   222		}
   223	
   224		mutex_init(&info->mutex);
   225	
   226		INIT_WORK(&info->irq_work, ptn5150_irq_work);
   227	
   228		info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
   229		if (IS_ERR(info->regmap)) {
   230			ret = PTR_ERR(info->regmap);
   231			dev_err(info->dev, "failed to allocate register map: %d\n",
   232					   ret);
   233			return ret;
   234		}
   235	
   236		if (info->int_gpiod) {
   237			info->irq = gpiod_to_irq(info->int_gpiod);
   238			if (info->irq < 0) {
   239				dev_err(dev, "failed to get INTB IRQ\n");
   240				return info->irq;
   241			}
   242	
   243			ret = devm_request_threaded_irq(dev, info->irq, NULL,
   244							ptn5150_irq_handler,
   245							IRQF_TRIGGER_FALLING |
   246							IRQF_ONESHOT,
   247							i2c->name, info);
   248			if (ret < 0) {
   249				dev_err(dev, "failed to request handler for INTB IRQ\n");
   250				return ret;
   251			}
   252		}
   253	
   254		/* Allocate extcon device */
 > 255		info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
   256		if (IS_ERR(info->edev)) {
   257			dev_err(info->dev, "failed to allocate memory for extcon\n");
   258			return -ENOMEM;
   259		}
   260	
   261		/* Register extcon device */
 > 262		ret = devm_extcon_dev_register(info->dev, info->edev);
   263		if (ret) {
   264			dev_err(info->dev, "failed to register extcon device\n");
   265			return ret;
   266		}
   267	
   268		/* Initialize PTN5150 device and print vendor id and version id */
   269		ret = ptn5150_init_dev_type(info);
   270		if (ret)
   271			return -EINVAL;
   272	
   273		return 0;
   274	}
   275	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51002 bytes --]

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

* Re: [PATCH] drivers: extcon: Add support for ptn5150
  2019-01-22  0:05   ` kbuild test robot
@ 2019-01-22  4:42     ` Vijai Kumar K
  2019-01-22  5:27       ` Chanwoo Choi
       [not found]     ` <CGME20190122044306epcas5p30f875260e568d5fb0e4909035060f8ff@epcms1p8>
  1 sibling, 1 reply; 12+ messages in thread
From: Vijai Kumar K @ 2019-01-22  4:42 UTC (permalink / raw)
  To: cw00.choi; +Cc: kbuild-all, linux-kernel, myungjoo.ham

Hi Chanwoo Choi,

This is the first time I am sending a driver to LKML. I have a few doubts. Can
you please clarify them when you are free?

1. I have developed and tested this patch on 4.14.89 kernel. When trying to 
mainline the driver should I rebase and send the patch on top of current tree(v5.0-rc2)?

2. Is there any specific git on which I should test this driver on? Like below?
https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

Thanks,
Vijai Kumar K

On Tue, Jan 22, 2019 at 08:05:33AM +0800, kbuild test robot wrote:
> Hi Vijai,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on chanwoo-extcon/extcon-next]
> [also build test ERROR on v5.0-rc2]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Vijai-Kumar-K/drivers-extcon-Add-support-for-ptn5150/20190122-041153
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
> config: sh-allyesconfig (attached as .config)
> compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=8.2.0 make.cross ARCH=sh 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_irq_work':
> >> drivers//extcon/extcon-ptn5150.c:93:5: error: implicit declaration of function 'extcon_set_state_sync'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
>         extcon_set_state_sync(info->edev,
>         ^~~~~~~~~~~~~~~~~~~~~
>         extcon_get_state
>    drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_i2c_probe':
> >> drivers//extcon/extcon-ptn5150.c:255:15: error: implicit declaration of function 'devm_extcon_dev_allocate'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
>      info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>                   ^~~~~~~~~~~~~~~~~~~~~~~~
>                   extcon_get_state
> >> drivers//extcon/extcon-ptn5150.c:255:13: warning: assignment to 'struct extcon_dev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
>      info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>                 ^
> >> drivers//extcon/extcon-ptn5150.c:262:8: error: implicit declaration of function 'devm_extcon_dev_register'; did you mean 'devm_pinctrl_register'? [-Werror=implicit-function-declaration]
>      ret = devm_extcon_dev_register(info->dev, info->edev);
>            ^~~~~~~~~~~~~~~~~~~~~~~~
>            devm_pinctrl_register
>    cc1: some warnings being treated as errors
> 
> vim +93 drivers//extcon/extcon-ptn5150.c
> 
>     51	
>     52	static void ptn5150_irq_work(struct work_struct *work)
>     53	{
>     54		struct ptn5150_info *info = container_of(work,
>     55				struct ptn5150_info, irq_work);
>     56		int ret = 0;
>     57		unsigned int reg_data;
>     58		unsigned int port_status;
>     59		unsigned int vbus;
>     60		unsigned int cable_attach;
>     61		unsigned int int_status;
>     62	
>     63		if (!info->edev)
>     64			return;
>     65	
>     66		mutex_lock(&info->mutex);
>     67	
>     68		ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
>     69		if (ret) {
>     70			dev_err(info->dev,
>     71				"failed to read CC STATUS %d\n", ret);
>     72			mutex_unlock(&info->mutex);
>     73			return;
>     74		}
>     75		/* Clear interrupt. Read would clear the register */
>     76		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
>     77		if (ret) {
>     78			dev_err(info->dev,
>     79				"failed to read INT STATUS %d\n", ret);
>     80			mutex_unlock(&info->mutex);
>     81			return;
>     82		}
>     83	
>     84		if (int_status) {
>     85			cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
>     86			if (cable_attach) {
>     87				port_status = ((reg_data &
>     88						PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
>     89						PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
>     90	
>     91				switch (port_status) {
>     92				case PTN5150_DFP_ATTACHED:
>   > 93					extcon_set_state_sync(info->edev,
>     94							      EXTCON_USB_HOST, false);
>     95					gpiod_set_value(info->vbus_gpiod, 0);
>     96					extcon_set_state_sync(info->edev, EXTCON_USB,
>     97							      true);
>     98					break;
>     99				case PTN5150_UFP_ATTACHED:
>    100					extcon_set_state_sync(info->edev, EXTCON_USB,
>    101							      false);
>    102					vbus = ((reg_data &
>    103						PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
>    104						PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
>    105					if (vbus)
>    106						gpiod_set_value(info->vbus_gpiod, 0);
>    107					else
>    108						gpiod_set_value(info->vbus_gpiod, 1);
>    109	
>    110					extcon_set_state_sync(info->edev,
>    111							      EXTCON_USB_HOST, true);
>    112					break;
>    113				default:
>    114					dev_err(info->dev,
>    115						"Unknown Port status : %x\n",
>    116						port_status);
>    117					break;
>    118				}
>    119			} else {
>    120				extcon_set_state_sync(info->edev,
>    121						      EXTCON_USB_HOST, false);
>    122				extcon_set_state_sync(info->edev,
>    123						      EXTCON_USB, false);
>    124				gpiod_set_value(info->vbus_gpiod, 0);
>    125			}
>    126		}
>    127	
>    128		/* Clear interrupt. Read would clear the register */
>    129		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
>    130					&int_status);
>    131		if (ret) {
>    132			dev_err(info->dev,
>    133				"failed to read INT REG STATUS %d\n", ret);
>    134			mutex_unlock(&info->mutex);
>    135			return;
>    136		}
>    137	
>    138	
>    139		mutex_unlock(&info->mutex);
>    140	}
>    141	
>    142	
>    143	static irqreturn_t ptn5150_irq_handler(int irq, void *data)
>    144	{
>    145		struct ptn5150_info *info = data;
>    146	
>    147		schedule_work(&info->irq_work);
>    148	
>    149		return IRQ_HANDLED;
>    150	}
>    151	
>    152	static int ptn5150_init_dev_type(struct ptn5150_info *info)
>    153	{
>    154		unsigned int reg_data, vendor_id, version_id;
>    155		int ret;
>    156	
>    157		ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
>    158		if (ret) {
>    159			dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
>    160			return -EINVAL;
>    161		}
>    162	
>    163		vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
>    164					PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
>    165		version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
>    166					PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
>    167	
>    168		dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
>    169				    version_id, vendor_id);
>    170	
>    171		/* Clear any existing interrupts */
>    172		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
>    173		if (ret) {
>    174			dev_err(info->dev,
>    175				"failed to read PTN5150_REG_INT_STATUS %d\n",
>    176				ret);
>    177			return -EINVAL;
>    178		}
>    179	
>    180		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
>    181		if (ret) {
>    182			dev_err(info->dev,
>    183				"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
>    184			return -EINVAL;
>    185		}
>    186	
>    187		return 0;
>    188	}
>    189	
>    190	static int ptn5150_i2c_probe(struct i2c_client *i2c,
>    191					 const struct i2c_device_id *id)
>    192	{
>    193		struct device *dev = &i2c->dev;
>    194		struct device_node *np = i2c->dev.of_node;
>    195		struct ptn5150_info *info;
>    196		int ret;
>    197	
>    198		if (!np)
>    199			return -EINVAL;
>    200	
>    201		info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
>    202		if (!info)
>    203			return -ENOMEM;
>    204		i2c_set_clientdata(i2c, info);
>    205	
>    206		info->dev = &i2c->dev;
>    207		info->i2c = i2c;
>    208		info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
>    209		if (!info->int_gpiod) {
>    210			dev_err(dev, "failed to get INT GPIO\n");
>    211			return -EINVAL;
>    212		}
>    213		info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
>    214		if (!info->vbus_gpiod) {
>    215			dev_err(dev, "failed to get VBUS GPIO\n");
>    216			return -EINVAL;
>    217		}
>    218		ret = gpiod_direction_output(info->vbus_gpiod, 0);
>    219		if (ret) {
>    220			dev_err(dev, "failed to set VBUS GPIO direction\n");
>    221			return -EINVAL;
>    222		}
>    223	
>    224		mutex_init(&info->mutex);
>    225	
>    226		INIT_WORK(&info->irq_work, ptn5150_irq_work);
>    227	
>    228		info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
>    229		if (IS_ERR(info->regmap)) {
>    230			ret = PTR_ERR(info->regmap);
>    231			dev_err(info->dev, "failed to allocate register map: %d\n",
>    232					   ret);
>    233			return ret;
>    234		}
>    235	
>    236		if (info->int_gpiod) {
>    237			info->irq = gpiod_to_irq(info->int_gpiod);
>    238			if (info->irq < 0) {
>    239				dev_err(dev, "failed to get INTB IRQ\n");
>    240				return info->irq;
>    241			}
>    242	
>    243			ret = devm_request_threaded_irq(dev, info->irq, NULL,
>    244							ptn5150_irq_handler,
>    245							IRQF_TRIGGER_FALLING |
>    246							IRQF_ONESHOT,
>    247							i2c->name, info);
>    248			if (ret < 0) {
>    249				dev_err(dev, "failed to request handler for INTB IRQ\n");
>    250				return ret;
>    251			}
>    252		}
>    253	
>    254		/* Allocate extcon device */
>  > 255		info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>    256		if (IS_ERR(info->edev)) {
>    257			dev_err(info->dev, "failed to allocate memory for extcon\n");
>    258			return -ENOMEM;
>    259		}
>    260	
>    261		/* Register extcon device */
>  > 262		ret = devm_extcon_dev_register(info->dev, info->edev);
>    263		if (ret) {
>    264			dev_err(info->dev, "failed to register extcon device\n");
>    265			return ret;
>    266		}
>    267	
>    268		/* Initialize PTN5150 device and print vendor id and version id */
>    269		ret = ptn5150_init_dev_type(info);
>    270		if (ret)
>    271			return -EINVAL;
>    272	
>    273		return 0;
>    274	}
>    275	
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation



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

* RE: Re: [PATCH] drivers: extcon: Add support for ptn5150
       [not found]     ` <CGME20190122044306epcas5p30f875260e568d5fb0e4909035060f8ff@epcms1p8>
@ 2019-01-22  4:57       ` MyungJoo Ham
  2019-01-22  6:57         ` Vijai Kumar K
  0 siblings, 1 reply; 12+ messages in thread
From: MyungJoo Ham @ 2019-01-22  4:57 UTC (permalink / raw)
  To: Vijai Kumar K, Chanwoo Choi; +Cc: kbuild-all, linux-kernel

>Hi Chanwoo Choi,
>
>This is the first time I am sending a driver to LKML. I have a few doubts. Can
>you please clarify them when you are free?

Although I'm not Chanwoo, but a guy who's about 50ft away from his cubicle,
as he appears to be busy today... :)

>
>1. I have developed and tested this patch on 4.14.89 kernel. When trying to 
>mainline the driver should I rebase and send the patch on top of current tree(v5.0-rc2)?

Yes, you are supposed to be based on the most recent RC.

>
>2. Is there any specific git on which I should test this driver on? Like below?
>https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

I'd recommend to pull torvald's master with RC tag.


Cheers,
MyungJoo

>
>Thanks,
>Vijai Kumar K

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

* Re: [PATCH] drivers: extcon: Add support for ptn5150
  2019-01-22  4:42     ` Vijai Kumar K
@ 2019-01-22  5:27       ` Chanwoo Choi
  2019-01-22  7:05         ` Vijai Kumar K
  0 siblings, 1 reply; 12+ messages in thread
From: Chanwoo Choi @ 2019-01-22  5:27 UTC (permalink / raw)
  To: Vijai Kumar K; +Cc: kbuild-all, linux-kernel, myungjoo.ham

Hi Vijai,

On 19. 1. 22. 오후 1:42, Vijai Kumar K wrote:
> Hi Chanwoo Choi,
> 
> This is the first time I am sending a driver to LKML. I have a few doubts. Can
> you please clarify them when you are free?
> 
> 1. I have developed and tested this patch on 4.14.89 kernel. When trying to 
> mainline the driver should I rebase and send the patch on top of current tree(v5.0-rc2)?

You better to rebase your patch on extcon-next branch (v5.0-rc1).
because the extcon.git[1] keep the v5.0-rc1 version. But, if you use over v5.0-rc1 version,
it doesn't matter. Maybe, this patch doesn't get the any impact from the modification
of v5.0-rcX.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

> 
> 2. Is there any specific git on which I should test this driver on? Like below?
> https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

Yes.

> 
> Thanks,
> Vijai Kumar K
> 
> On Tue, Jan 22, 2019 at 08:05:33AM +0800, kbuild test robot wrote:
>> Hi Vijai,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on chanwoo-extcon/extcon-next]
>> [also build test ERROR on v5.0-rc2]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url:    https://github.com/0day-ci/linux/commits/Vijai-Kumar-K/drivers-extcon-Add-support-for-ptn5150/20190122-041153
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
>> config: sh-allyesconfig (attached as .config)
>> compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
>> reproduce:
>>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>         chmod +x ~/bin/make.cross
>>         # save the attached .config to linux build tree
>>         GCC_VERSION=8.2.0 make.cross ARCH=sh 
>>
>> All error/warnings (new ones prefixed by >>):
>>
>>    drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_irq_work':
>>>> drivers//extcon/extcon-ptn5150.c:93:5: error: implicit declaration of function 'extcon_set_state_sync'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
>>         extcon_set_state_sync(info->edev,
>>         ^~~~~~~~~~~~~~~~~~~~~
>>         extcon_get_state
>>    drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_i2c_probe':
>>>> drivers//extcon/extcon-ptn5150.c:255:15: error: implicit declaration of function 'devm_extcon_dev_allocate'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
>>      info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>>                   ^~~~~~~~~~~~~~~~~~~~~~~~
>>                   extcon_get_state
>>>> drivers//extcon/extcon-ptn5150.c:255:13: warning: assignment to 'struct extcon_dev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
>>      info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>>                 ^
>>>> drivers//extcon/extcon-ptn5150.c:262:8: error: implicit declaration of function 'devm_extcon_dev_register'; did you mean 'devm_pinctrl_register'? [-Werror=implicit-function-declaration]
>>      ret = devm_extcon_dev_register(info->dev, info->edev);
>>            ^~~~~~~~~~~~~~~~~~~~~~~~
>>            devm_pinctrl_register
>>    cc1: some warnings being treated as errors
>>
>> vim +93 drivers//extcon/extcon-ptn5150.c
>>
>>     51	
>>     52	static void ptn5150_irq_work(struct work_struct *work)
>>     53	{
>>     54		struct ptn5150_info *info = container_of(work,
>>     55				struct ptn5150_info, irq_work);
>>     56		int ret = 0;
>>     57		unsigned int reg_data;
>>     58		unsigned int port_status;
>>     59		unsigned int vbus;
>>     60		unsigned int cable_attach;
>>     61		unsigned int int_status;
>>     62	
>>     63		if (!info->edev)
>>     64			return;
>>     65	
>>     66		mutex_lock(&info->mutex);
>>     67	
>>     68		ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
>>     69		if (ret) {
>>     70			dev_err(info->dev,
>>     71				"failed to read CC STATUS %d\n", ret);
>>     72			mutex_unlock(&info->mutex);
>>     73			return;
>>     74		}
>>     75		/* Clear interrupt. Read would clear the register */
>>     76		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
>>     77		if (ret) {
>>     78			dev_err(info->dev,
>>     79				"failed to read INT STATUS %d\n", ret);
>>     80			mutex_unlock(&info->mutex);
>>     81			return;
>>     82		}
>>     83	
>>     84		if (int_status) {
>>     85			cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
>>     86			if (cable_attach) {
>>     87				port_status = ((reg_data &
>>     88						PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
>>     89						PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
>>     90	
>>     91				switch (port_status) {
>>     92				case PTN5150_DFP_ATTACHED:
>>   > 93					extcon_set_state_sync(info->edev,
>>     94							      EXTCON_USB_HOST, false);
>>     95					gpiod_set_value(info->vbus_gpiod, 0);
>>     96					extcon_set_state_sync(info->edev, EXTCON_USB,
>>     97							      true);
>>     98					break;
>>     99				case PTN5150_UFP_ATTACHED:
>>    100					extcon_set_state_sync(info->edev, EXTCON_USB,
>>    101							      false);
>>    102					vbus = ((reg_data &
>>    103						PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
>>    104						PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
>>    105					if (vbus)
>>    106						gpiod_set_value(info->vbus_gpiod, 0);
>>    107					else
>>    108						gpiod_set_value(info->vbus_gpiod, 1);
>>    109	
>>    110					extcon_set_state_sync(info->edev,
>>    111							      EXTCON_USB_HOST, true);
>>    112					break;
>>    113				default:
>>    114					dev_err(info->dev,
>>    115						"Unknown Port status : %x\n",
>>    116						port_status);
>>    117					break;
>>    118				}
>>    119			} else {
>>    120				extcon_set_state_sync(info->edev,
>>    121						      EXTCON_USB_HOST, false);
>>    122				extcon_set_state_sync(info->edev,
>>    123						      EXTCON_USB, false);
>>    124				gpiod_set_value(info->vbus_gpiod, 0);
>>    125			}
>>    126		}
>>    127	
>>    128		/* Clear interrupt. Read would clear the register */
>>    129		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
>>    130					&int_status);
>>    131		if (ret) {
>>    132			dev_err(info->dev,
>>    133				"failed to read INT REG STATUS %d\n", ret);
>>    134			mutex_unlock(&info->mutex);
>>    135			return;
>>    136		}
>>    137	
>>    138	
>>    139		mutex_unlock(&info->mutex);
>>    140	}
>>    141	
>>    142	
>>    143	static irqreturn_t ptn5150_irq_handler(int irq, void *data)
>>    144	{
>>    145		struct ptn5150_info *info = data;
>>    146	
>>    147		schedule_work(&info->irq_work);
>>    148	
>>    149		return IRQ_HANDLED;
>>    150	}
>>    151	
>>    152	static int ptn5150_init_dev_type(struct ptn5150_info *info)
>>    153	{
>>    154		unsigned int reg_data, vendor_id, version_id;
>>    155		int ret;
>>    156	
>>    157		ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
>>    158		if (ret) {
>>    159			dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
>>    160			return -EINVAL;
>>    161		}
>>    162	
>>    163		vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
>>    164					PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
>>    165		version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
>>    166					PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
>>    167	
>>    168		dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
>>    169				    version_id, vendor_id);
>>    170	
>>    171		/* Clear any existing interrupts */
>>    172		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
>>    173		if (ret) {
>>    174			dev_err(info->dev,
>>    175				"failed to read PTN5150_REG_INT_STATUS %d\n",
>>    176				ret);
>>    177			return -EINVAL;
>>    178		}
>>    179	
>>    180		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
>>    181		if (ret) {
>>    182			dev_err(info->dev,
>>    183				"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
>>    184			return -EINVAL;
>>    185		}
>>    186	
>>    187		return 0;
>>    188	}
>>    189	
>>    190	static int ptn5150_i2c_probe(struct i2c_client *i2c,
>>    191					 const struct i2c_device_id *id)
>>    192	{
>>    193		struct device *dev = &i2c->dev;
>>    194		struct device_node *np = i2c->dev.of_node;
>>    195		struct ptn5150_info *info;
>>    196		int ret;
>>    197	
>>    198		if (!np)
>>    199			return -EINVAL;
>>    200	
>>    201		info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
>>    202		if (!info)
>>    203			return -ENOMEM;
>>    204		i2c_set_clientdata(i2c, info);
>>    205	
>>    206		info->dev = &i2c->dev;
>>    207		info->i2c = i2c;
>>    208		info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
>>    209		if (!info->int_gpiod) {
>>    210			dev_err(dev, "failed to get INT GPIO\n");
>>    211			return -EINVAL;
>>    212		}
>>    213		info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
>>    214		if (!info->vbus_gpiod) {
>>    215			dev_err(dev, "failed to get VBUS GPIO\n");
>>    216			return -EINVAL;
>>    217		}
>>    218		ret = gpiod_direction_output(info->vbus_gpiod, 0);
>>    219		if (ret) {
>>    220			dev_err(dev, "failed to set VBUS GPIO direction\n");
>>    221			return -EINVAL;
>>    222		}
>>    223	
>>    224		mutex_init(&info->mutex);
>>    225	
>>    226		INIT_WORK(&info->irq_work, ptn5150_irq_work);
>>    227	
>>    228		info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
>>    229		if (IS_ERR(info->regmap)) {
>>    230			ret = PTR_ERR(info->regmap);
>>    231			dev_err(info->dev, "failed to allocate register map: %d\n",
>>    232					   ret);
>>    233			return ret;
>>    234		}
>>    235	
>>    236		if (info->int_gpiod) {
>>    237			info->irq = gpiod_to_irq(info->int_gpiod);
>>    238			if (info->irq < 0) {
>>    239				dev_err(dev, "failed to get INTB IRQ\n");
>>    240				return info->irq;
>>    241			}
>>    242	
>>    243			ret = devm_request_threaded_irq(dev, info->irq, NULL,
>>    244							ptn5150_irq_handler,
>>    245							IRQF_TRIGGER_FALLING |
>>    246							IRQF_ONESHOT,
>>    247							i2c->name, info);
>>    248			if (ret < 0) {
>>    249				dev_err(dev, "failed to request handler for INTB IRQ\n");
>>    250				return ret;
>>    251			}
>>    252		}
>>    253	
>>    254		/* Allocate extcon device */
>>  > 255		info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
>>    256		if (IS_ERR(info->edev)) {
>>    257			dev_err(info->dev, "failed to allocate memory for extcon\n");
>>    258			return -ENOMEM;
>>    259		}
>>    260	
>>    261		/* Register extcon device */
>>  > 262		ret = devm_extcon_dev_register(info->dev, info->edev);
>>    263		if (ret) {
>>    264			dev_err(info->dev, "failed to register extcon device\n");
>>    265			return ret;
>>    266		}
>>    267	
>>    268		/* Initialize PTN5150 device and print vendor id and version id */
>>    269		ret = ptn5150_init_dev_type(info);
>>    270		if (ret)
>>    271			return -EINVAL;
>>    272	
>>    273		return 0;
>>    274	}
>>    275	
>>
>> ---
>> 0-DAY kernel test infrastructure                Open Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

* Re: [PATCH] drivers: extcon: Add support for ptn5150
  2019-01-21  9:09 ` [PATCH] drivers: extcon: Add support for ptn5150 Vijai Kumar K
  2019-01-22  0:05   ` kbuild test robot
@ 2019-01-22  6:20   ` Chanwoo Choi
  2019-01-22  7:55     ` Vijai Kumar K
  1 sibling, 1 reply; 12+ messages in thread
From: Chanwoo Choi @ 2019-01-22  6:20 UTC (permalink / raw)
  To: Vijai Kumar K, linux-kernel; +Cc: myungjoo.ham

Hi Vijai,

This patch looks better. But theare are comments about code clean.
I added the comments.

On 19. 1. 21. 오후 6:09, Vijai Kumar K wrote:
> PTN5150A is a small thin low power CC Logic chip supporting
> the USB Type-C connector application with Configurationn Channel(CC)
> control logic detection and indication functions.
> 
> Add driver, Kconfig and Makefile entries.
> 
> Change-Id: I3b2da147a33b913b9ec60cd914b20acd955950c7

Remove it of gerrit system.

> Signed-off-by: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> ---
>  .../devicetree/bindings/extcon/extcon-ptn5150.txt  |  22 ++
>  drivers/extcon/Kconfig                             |   8 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-ptn5150.c                    | 327 +++++++++++++++++++++
>  drivers/extcon/extcon-ptn5150.h                    |  51 ++++
>  5 files changed, 409 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
>  create mode 100644 drivers/extcon/extcon-ptn5150.c
>  create mode 100644 drivers/extcon/extcon-ptn5150.h
> 
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
> new file mode 100644
> index 0000000..8ea33bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
> @@ -0,0 +1,22 @@
> +
> +* PTN5150 CC logic device

You better to specify the full name as following:
: PTN5150 CC (Configuration Channel) Logic device

> +PTN5150A is a small thin low power CC Logic chip supporting the USB Type-C

s/Logic/logic

> +connector application with Configuration Channel (CC) control logic detection and
> +indication functions. It is Interfaced to the host controller using an I2C interface.

s/Interfaced/interfaced

> +
> +Required properties:
> +- compatible: Should be "nxp,ptn5150"
> +- reg: Specifies the I2C slave address of the device
> +- int-gpio: GPIO to which INTB is connected

What is meaning of 'INTB'?

> +- vbus-gpio: GPIO which controls VBUS on/off
> +
> +Example:
> +		ptn5150@1d  {
> +			compatible = "nxp,ptn5150";
> +			reg = <0x1d>;
> +			int-gpio = <&msmgpio 78 GPIO_ACTIVE_HIGH>;
> +			vbus-gpio = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&ptn5150_default>;

You need to add some description why pinctrl properties are needed.
For example, some hwmon device driver[1] explained the properties
related to pinctrl. Please add the description as following.

- pinctrl-names : a pinctrl state named "default" must be defined.              
- pinctrl-0 : phandle referencing pin configuration of the PWM ports.           

[1] Documentation/devicetree/hwmon/aspeed-pwm-tacho.txt


> +			status = "okay";
> +		};

Remove the one tab in front of dt node.

	ptn5150@1d  {
		compatible = "nxp,ptn5150";
		reg = <0x1d>;
		int-gpio = <&msmgpio 78 GPIO_ACTIVE_HIGH>;
		vbus-gpio = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
		pinctrl-names = "default";
		pinctrl-0 = <&ptn5150_default>;
		status = "okay";
	};



> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index a7bca42..6adc797 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -113,6 +113,14 @@ config EXTCON_PALMAS
>  	  Say Y here to enable support for USB peripheral and USB host
>  	  detection by palmas usb.
>  
> +config EXTCON_PTN5150
> +	tristate "PTN5150 CC LOGIC USB EXTCON support"

PTN5150 CC (Configuration Channel) LOGIC USB EXTCON support

> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say Y here to enable support for USB peripheral and USB host
> +	  detection by ptn5150 cc logic chip.

cc -> CC (Configuration Channel)

> +
>  config EXTCON_QCOM_SPMI_MISC
>  	tristate "Qualcomm USB extcon support"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 0888fde..261ce4c 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>  obj-$(CONFIG_EXTCON_MAX77843)	+= extcon-max77843.o
>  obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> +obj-$(CONFIG_EXTCON_PTN5150)	+= extcon-ptn5150.o
>  obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
> new file mode 100644
> index 0000000..bc00874
> --- /dev/null
> +++ b/drivers/extcon/extcon-ptn5150.c
> @@ -0,0 +1,327 @@
> +/*
> + * extcon-ptn5150.c - PTN5150 CC logic extcon driver to support USB detection
> + *
> + * Copyright (c) 2018-2019 by Vijai Kumar K
> + * Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> + *
> + * Based on extcon-sm5502.c driver
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */

Please SPDX license format instead of old type as following according to your
license: You can find the SPDX examples in kernel code easily.

//SPDX-License-Identifier: GPL-2.0+
//
// extcon-ptn5150.c - Extcon driver to support USB detection for PTN5150 CC logic
//                                                                              
//  Copyright (C) 2018-2019 by Vijai Kumar K
//  Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>

> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/extcon.h>

This extcon driver is provider driver. You have to change is as following:
- extcon.h -> extcon-provider.h

extcon.h header file should be used on extcon consumer driver.

> +#include <linux/gpio.h>
> +
> +#include "extcon-ptn5150.h"

extcon-ptn5150.h header file is only used on drivers/extcon/extcon-ptn5150.c.
You don't need to create the separate header file. Please move all definitions
of extcon-ptn5150.h to extcon-pth5150.c and remove extcon-ptn5150.h.

> +
> +struct ptn5150_info {
> +	struct device *dev;
> +	struct extcon_dev *edev;
> +	struct i2c_client *i2c;
> +	struct regmap *regmap;
> +	struct gpio_desc *int_gpiod;
> +	struct gpio_desc *vbus_gpiod;
> +	int irq;
> +	struct work_struct irq_work;
> +	struct mutex mutex;
> +};
> +
> +/* List of detectable cables */
> +static const unsigned int ptn5150_extcon_cable[] = {
> +	EXTCON_USB,
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static const struct regmap_config ptn5150_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= PTN5150_REG_END,
> +};
> +
> +static void ptn5150_irq_work(struct work_struct *work)
> +{
> +	struct ptn5150_info *info = container_of(work,
> +			struct ptn5150_info, irq_work);
> +	int ret = 0;
> +	unsigned int reg_data;
> +	unsigned int port_status;
> +	unsigned int vbus;

port_status and vbus are not always used. You can define them
under 'if (cable_attach) {' sentence.

> +	unsigned int cable_attach;
> +	unsigned int int_status;
> +
> +	if (!info->edev)
> +		return;
> +
> +	mutex_lock(&info->mutex);
> +
> +	ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"failed to read CC STATUS %d\n", ret);

You is able to make it on one line as following: it is not over 80 line.
		dev_err(info->dev, "failed to read CC STATUS %d\n", ret);


> +		mutex_unlock(&info->mutex);
> +		return;
> +	}

Add one blank line.

> +	/* Clear interrupt. Read would clear the register */
> +	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
> +	if (ret) {> +		dev_err(info->dev,
> +			"failed to read INT STATUS %d\n", ret);

ditto.

> +		mutex_unlock(&info->mutex);
> +		return;
> +	}
> +
> +	if (int_status) {
> +		cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
> +		if (cable_attach) {

			unsigned int port_status;
			unsigned int vbus;


> +			port_status = ((reg_data &
> +					PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
> +					PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
> +
> +			switch (port_status) {
> +			case PTN5150_DFP_ATTACHED:
> +				extcon_set_state_sync(info->edev,
> +						      EXTCON_USB_HOST, false);

You have to use tab for indentation. Please remove all space for indentation.

> +				gpiod_set_value(info->vbus_gpiod, 0);
> +				extcon_set_state_sync(info->edev, EXTCON_USB,
> +						      true);

ditto.

> +				break;
> +			case PTN5150_UFP_ATTACHED:
> +				extcon_set_state_sync(info->edev, EXTCON_USB,
> +						      false);

ditto.

> +				vbus = ((reg_data &
> +					PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
> +					PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
> +				if (vbus)
> +					gpiod_set_value(info->vbus_gpiod, 0);
> +				else
> +					gpiod_set_value(info->vbus_gpiod, 1);
> +
> +				extcon_set_state_sync(info->edev,
> +						      EXTCON_USB_HOST, true);

ditto.

> +				break;
> +			default:
> +				dev_err(info->dev,
> +					"Unknown Port status : %x\n",
> +					port_status);
> +				break;
> +			}
> +		} else {
> +			extcon_set_state_sync(info->edev,
> +					      EXTCON_USB_HOST, false);

ditto.

> +			extcon_set_state_sync(info->edev,
> +					      EXTCON_USB, false);

ditto.

> +			gpiod_set_value(info->vbus_gpiod, 0);
> +		}
> +	}
> +
> +	/* Clear interrupt. Read would clear the register */
> +	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
> +				&int_status);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"failed to read INT REG STATUS %d\n", ret);
> +		mutex_unlock(&info->mutex);
> +		return;
> +	}
> +
> +

Remove redundant blank line.

> +	mutex_unlock(&info->mutex);
> +}
> +
> +

Remove redundant blank line.

> +static irqreturn_t ptn5150_irq_handler(int irq, void *data)
> +{
> +	struct ptn5150_info *info = data;
> +
> +	schedule_work(&info->irq_work);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ptn5150_init_dev_type(struct ptn5150_info *info)
> +{
> +	unsigned int reg_data, vendor_id, version_id;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
> +	if (ret) {
> +		dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
> +				PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
> +	version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
> +				PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
> +
> +	dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
> +			    version_id, vendor_id);
> +
> +	/* Clear any existing interrupts */
> +	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"failed to read PTN5150_REG_INT_STATUS %d\n",
> +			ret);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ptn5150_i2c_probe(struct i2c_client *i2c,
> +				 const struct i2c_device_id *id)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct device_node *np = i2c->dev.of_node;
> +	struct ptn5150_info *info;
> +	int ret;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +	i2c_set_clientdata(i2c, info);
> +
> +	info->dev = &i2c->dev;
> +	info->i2c = i2c;
> +	info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
> +	if (!info->int_gpiod) {
> +		dev_err(dev, "failed to get INT GPIO\n");
> +		return -EINVAL;
> +	}
> +	info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
> +	if (!info->vbus_gpiod) {
> +		dev_err(dev, "failed to get VBUS GPIO\n");
> +		return -EINVAL;
> +	}
> +	ret = gpiod_direction_output(info->vbus_gpiod, 0);
> +	if (ret) {
> +		dev_err(dev, "failed to set VBUS GPIO direction\n");
> +		return -EINVAL;
> +	}
> +
> +	mutex_init(&info->mutex);
> +
> +	INIT_WORK(&info->irq_work, ptn5150_irq_work);
> +
> +	info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
> +	if (IS_ERR(info->regmap)) {
> +		ret = PTR_ERR(info->regmap);
> +		dev_err(info->dev, "failed to allocate register map: %d\n",
> +				   ret);
> +		return ret;
> +	}
> +
> +	if (info->int_gpiod) {
> +		info->irq = gpiod_to_irq(info->int_gpiod);
> +		if (info->irq < 0) {
> +			dev_err(dev, "failed to get INTB IRQ\n");
> +			return info->irq;
> +		}
> +
> +		ret = devm_request_threaded_irq(dev, info->irq, NULL,
> +						ptn5150_irq_handler,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						i2c->name, info);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to request handler for INTB IRQ\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* Allocate extcon device */
> +	info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
> +	if (IS_ERR(info->edev)) {
> +		dev_err(info->dev, "failed to allocate memory for extcon\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Register extcon device */
> +	ret = devm_extcon_dev_register(info->dev, info->edev);
> +	if (ret) {
> +		dev_err(info->dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	/* Initialize PTN5150 device and print vendor id and version id */
> +	ret = ptn5150_init_dev_type(info);
> +	if (ret)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ptn5150_i2c_remove(struct i2c_client *i2c)
> +{
> +	return 0;
> +}

Please remove dummy ptn5150_i2c_remove() defintions.
If the remove function on later, you can add it. 

> +
> +static const struct of_device_id ptn5150_dt_match[] = {
> +	{ .compatible = "nxp,ptn5150" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ptn5150_dt_match);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ptn5150_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int ptn5150_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +#endif

Please remove dummy functions (_suspend, _resume). You can add it on later. 


> +
> +static SIMPLE_DEV_PM_OPS(ptn5150_pm_ops,
> +			 ptn5150_suspend, ptn5150_resume);

Remove it.

> +
> +static const struct i2c_device_id ptn5150_i2c_id[] = {
> +	{ "ptn5150", TYPE_PTN5150A },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ptn5150_i2c_id);
> +
> +static struct i2c_driver ptn5150_i2c_driver = {
> +	.driver		= {
> +		.name	= "ptn5150",
> +		.pm	= &ptn5150_pm_ops,

Remove it.

> +		.of_match_table = ptn5150_dt_match,
> +	},
> +	.probe	= ptn5150_i2c_probe,
> +	.remove	= ptn5150_i2c_remove,

Remove it.

> +	.id_table = ptn5150_i2c_id,
> +};
> +
> +static int __init ptn5150_i2c_init(void)
> +{
> +	return i2c_add_driver(&ptn5150_i2c_driver);
> +}
> +subsys_initcall(ptn5150_i2c_init);
> +
> +MODULE_DESCRIPTION("NXP PTN5150 CC logic Extcon driver");
> +MODULE_AUTHOR("Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>");
> +MODULE_LICENSE("GPL");

If you use SPDX license format, you dont' need to add module information.
Please remove MODULE_*() macro.

> diff --git a/drivers/extcon/extcon-ptn5150.h b/drivers/extcon/extcon-ptn5150.h

As I already commented, you have to move all definitiosn of extcon-ptn5150.h
to extcon-ptn5150.c and remove extcon-ptn5150.h.

> new file mode 100644
> index 0000000..217dab0
> --- /dev/null
> +++ b/drivers/extcon/extcon-ptn5150.h
> @@ -0,0 +1,51 @@
> +/*
> + * extcon-ptn5150.h
> + *
> + * Copyright (c) 2018-2019 by Vijai Kumar K
> + * Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#ifndef __LINUX_EXTCON_PTN5150_H
> +#define __LINUX_EXTCON_PTN5150_H
> +
> +enum ptn5150_types {
> +	TYPE_PTN5150A,

Do you have additional plan to suppor other type for extcon-ptn5150.c driver?
If extcon-ptn5150.c supports only one driver, you don't need to this enumeration.

> +};
> +
> +/* PTN5150 registers */
> +enum ptn5150_reg {
> +	PTN5150_REG_DEVICE_ID = 0x01,
> +	PTN5150_REG_CONTROL,
> +	PTN5150_REG_INT_STATUS,
> +	PTN5150_REG_CC_STATUS,
> +	PTN5150_REG_CON_DET = 0x09,
> +	PTN5150_REG_VCONN_STATUS,
> +	PTN5150_REG_RESET,
> +	PTN5150_REG_INT_MASK = 0x18,
> +	PTN5150_REG_INT_REG_STATUS,
> +	PTN5150_REG_END,
> +};
> +
> +#define PTN5150_DFP_ATTACHED			0x1
> +#define PTN5150_UFP_ATTACHED			0x2
> +
> +/* Define PTN5150 MASK/SHIFT constant */
> +#define PTN5150_REG_DEVICE_ID_VENDOR_SHIFT	0
> +#define PTN5150_REG_DEVICE_ID_VERSION_SHIFT	3
> +#define PTN5150_REG_DEVICE_ID_VENDOR_MASK	(0x3 << PTN5150_REG_DEVICE_ID_VENDOR_SHIFT)
> +#define PTN5150_REG_DEVICE_ID_VERSION_MASK	(0x1f << PTN5150_REG_DEVICE_ID_VERSION_SHIFT)
> +
> +#define PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT	2
> +#define PTN5150_REG_CC_PORT_ATTACHMENT_MASK	(0x7 << PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT)
> +#define PTN5150_REG_CC_VBUS_DETECTION_SHIFT	7
> +#define PTN5150_REG_CC_VBUS_DETECTION_MASK	(0x1 << PTN5150_REG_CC_VBUS_DETECTION_SHIFT)
> +#define PTN5150_REG_INT_CABLE_ATTACH_SHIFT	0
> +#define PTN5150_REG_INT_CABLE_ATTACH_MASK	(0x1 << PTN5150_REG_INT_CABLE_ATTACH_SHIFT)
> +#define PTN5150_REG_INT_CABLE_DETACH_SHIFT	1
> +#define PTN5150_REG_INT_CABLE_DETACH_MASK	(0x1 << PTN5150_REG_CC_CABLE_DETACH_SHIFT)
> +#endif /*  __LINUX_EXTCON_PTN5150_H */
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

* Re: Re: [PATCH] drivers: extcon: Add support for ptn5150
  2019-01-22  4:57       ` MyungJoo Ham
@ 2019-01-22  6:57         ` Vijai Kumar K
  0 siblings, 0 replies; 12+ messages in thread
From: Vijai Kumar K @ 2019-01-22  6:57 UTC (permalink / raw)
  To: MyungJoo Ham; +Cc: Chanwoo Choi, kbuild-all, linux-kernel

On Tue, Jan 22, 2019 at 01:57:46PM +0900, MyungJoo Ham wrote:
> >Hi Chanwoo Choi,
> >
> >This is the first time I am sending a driver to LKML. I have a few doubts. Can
> >you please clarify them when you are free?
> 
> Although I'm not Chanwoo, but a guy who's about 50ft away from his cubicle,
> as he appears to be busy today... :)
:)
> 
> >
> >1. I have developed and tested this patch on 4.14.89 kernel. When trying to 
> >mainline the driver should I rebase and send the patch on top of current tree(v5.0-rc2)?
> 
> Yes, you are supposed to be based on the most recent RC.
Sure. Will do that and push an updated patch
> 
> >
> >2. Is there any specific git on which I should test this driver on? Like below?
> >https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git
> 
> I'd recommend to pull torvald's master with RC tag.
Thanks. Will took into that.
> 
> 
> Cheers,
> MyungJoo
> 
> >
> >Thanks,
> >Vijai Kumar K

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

* Re: [PATCH] drivers: extcon: Add support for ptn5150
  2019-01-22  5:27       ` Chanwoo Choi
@ 2019-01-22  7:05         ` Vijai Kumar K
  0 siblings, 0 replies; 12+ messages in thread
From: Vijai Kumar K @ 2019-01-22  7:05 UTC (permalink / raw)
  To: Chanwoo Choi; +Cc: kbuild-all, linux-kernel, myungjoo.ham

On Tue, Jan 22, 2019 at 02:27:51PM +0900, Chanwoo Choi wrote:
> Hi Vijai,
> 
> On 19. 1. 22. 오후 1:42, Vijai Kumar K wrote:
> > Hi Chanwoo Choi,
> > 
> > This is the first time I am sending a driver to LKML. I have a few doubts. Can
> > you please clarify them when you are free?
> > 
> > 1. I have developed and tested this patch on 4.14.89 kernel. When trying to 
> > mainline the driver should I rebase and send the patch on top of current tree(v5.0-rc2)?
> 
> You better to rebase your patch on extcon-next branch (v5.0-rc1).
> because the extcon.git[1] keep the v5.0-rc1 version. But, if you use over v5.0-rc1 version,
> it doesn't matter. Maybe, this patch doesn't get the any impact from the modification
> of v5.0-rcX.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git
Thanks, I will rebase and share an updated patchset.
> 
> > 
> > 2. Is there any specific git on which I should test this driver on? Like below?
> > https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git
> 
> Yes.
> 
> > 
> > Thanks,
> > Vijai Kumar K
> > 
> > On Tue, Jan 22, 2019 at 08:05:33AM +0800, kbuild test robot wrote:
> >> Hi Vijai,
> >>
> >> Thank you for the patch! Yet something to improve:
> >>
> >> [auto build test ERROR on chanwoo-extcon/extcon-next]
> >> [also build test ERROR on v5.0-rc2]
> >> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >>
> >> url:    https://github.com/0day-ci/linux/commits/Vijai-Kumar-K/drivers-extcon-Add-support-for-ptn5150/20190122-041153
> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
> >> config: sh-allyesconfig (attached as .config)
> >> compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
> >> reproduce:
> >>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >>         chmod +x ~/bin/make.cross
> >>         # save the attached .config to linux build tree
> >>         GCC_VERSION=8.2.0 make.cross ARCH=sh 
> >>
> >> All error/warnings (new ones prefixed by >>):
> >>
> >>    drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_irq_work':
> >>>> drivers//extcon/extcon-ptn5150.c:93:5: error: implicit declaration of function 'extcon_set_state_sync'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
> >>         extcon_set_state_sync(info->edev,
> >>         ^~~~~~~~~~~~~~~~~~~~~
> >>         extcon_get_state
> >>    drivers//extcon/extcon-ptn5150.c: In function 'ptn5150_i2c_probe':
> >>>> drivers//extcon/extcon-ptn5150.c:255:15: error: implicit declaration of function 'devm_extcon_dev_allocate'; did you mean 'extcon_get_state'? [-Werror=implicit-function-declaration]
> >>      info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
> >>                   ^~~~~~~~~~~~~~~~~~~~~~~~
> >>                   extcon_get_state
> >>>> drivers//extcon/extcon-ptn5150.c:255:13: warning: assignment to 'struct extcon_dev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
> >>      info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
> >>                 ^
> >>>> drivers//extcon/extcon-ptn5150.c:262:8: error: implicit declaration of function 'devm_extcon_dev_register'; did you mean 'devm_pinctrl_register'? [-Werror=implicit-function-declaration]
> >>      ret = devm_extcon_dev_register(info->dev, info->edev);
> >>            ^~~~~~~~~~~~~~~~~~~~~~~~
> >>            devm_pinctrl_register
> >>    cc1: some warnings being treated as errors
> >>
> >> vim +93 drivers//extcon/extcon-ptn5150.c
> >>
> >>     51	
> >>     52	static void ptn5150_irq_work(struct work_struct *work)
> >>     53	{
> >>     54		struct ptn5150_info *info = container_of(work,
> >>     55				struct ptn5150_info, irq_work);
> >>     56		int ret = 0;
> >>     57		unsigned int reg_data;
> >>     58		unsigned int port_status;
> >>     59		unsigned int vbus;
> >>     60		unsigned int cable_attach;
> >>     61		unsigned int int_status;
> >>     62	
> >>     63		if (!info->edev)
> >>     64			return;
> >>     65	
> >>     66		mutex_lock(&info->mutex);
> >>     67	
> >>     68		ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
> >>     69		if (ret) {
> >>     70			dev_err(info->dev,
> >>     71				"failed to read CC STATUS %d\n", ret);
> >>     72			mutex_unlock(&info->mutex);
> >>     73			return;
> >>     74		}
> >>     75		/* Clear interrupt. Read would clear the register */
> >>     76		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
> >>     77		if (ret) {
> >>     78			dev_err(info->dev,
> >>     79				"failed to read INT STATUS %d\n", ret);
> >>     80			mutex_unlock(&info->mutex);
> >>     81			return;
> >>     82		}
> >>     83	
> >>     84		if (int_status) {
> >>     85			cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
> >>     86			if (cable_attach) {
> >>     87				port_status = ((reg_data &
> >>     88						PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
> >>     89						PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
> >>     90	
> >>     91				switch (port_status) {
> >>     92				case PTN5150_DFP_ATTACHED:
> >>   > 93					extcon_set_state_sync(info->edev,
> >>     94							      EXTCON_USB_HOST, false);
> >>     95					gpiod_set_value(info->vbus_gpiod, 0);
> >>     96					extcon_set_state_sync(info->edev, EXTCON_USB,
> >>     97							      true);
> >>     98					break;
> >>     99				case PTN5150_UFP_ATTACHED:
> >>    100					extcon_set_state_sync(info->edev, EXTCON_USB,
> >>    101							      false);
> >>    102					vbus = ((reg_data &
> >>    103						PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
> >>    104						PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
> >>    105					if (vbus)
> >>    106						gpiod_set_value(info->vbus_gpiod, 0);
> >>    107					else
> >>    108						gpiod_set_value(info->vbus_gpiod, 1);
> >>    109	
> >>    110					extcon_set_state_sync(info->edev,
> >>    111							      EXTCON_USB_HOST, true);
> >>    112					break;
> >>    113				default:
> >>    114					dev_err(info->dev,
> >>    115						"Unknown Port status : %x\n",
> >>    116						port_status);
> >>    117					break;
> >>    118				}
> >>    119			} else {
> >>    120				extcon_set_state_sync(info->edev,
> >>    121						      EXTCON_USB_HOST, false);
> >>    122				extcon_set_state_sync(info->edev,
> >>    123						      EXTCON_USB, false);
> >>    124				gpiod_set_value(info->vbus_gpiod, 0);
> >>    125			}
> >>    126		}
> >>    127	
> >>    128		/* Clear interrupt. Read would clear the register */
> >>    129		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
> >>    130					&int_status);
> >>    131		if (ret) {
> >>    132			dev_err(info->dev,
> >>    133				"failed to read INT REG STATUS %d\n", ret);
> >>    134			mutex_unlock(&info->mutex);
> >>    135			return;
> >>    136		}
> >>    137	
> >>    138	
> >>    139		mutex_unlock(&info->mutex);
> >>    140	}
> >>    141	
> >>    142	
> >>    143	static irqreturn_t ptn5150_irq_handler(int irq, void *data)
> >>    144	{
> >>    145		struct ptn5150_info *info = data;
> >>    146	
> >>    147		schedule_work(&info->irq_work);
> >>    148	
> >>    149		return IRQ_HANDLED;
> >>    150	}
> >>    151	
> >>    152	static int ptn5150_init_dev_type(struct ptn5150_info *info)
> >>    153	{
> >>    154		unsigned int reg_data, vendor_id, version_id;
> >>    155		int ret;
> >>    156	
> >>    157		ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
> >>    158		if (ret) {
> >>    159			dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
> >>    160			return -EINVAL;
> >>    161		}
> >>    162	
> >>    163		vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
> >>    164					PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
> >>    165		version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
> >>    166					PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
> >>    167	
> >>    168		dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
> >>    169				    version_id, vendor_id);
> >>    170	
> >>    171		/* Clear any existing interrupts */
> >>    172		ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
> >>    173		if (ret) {
> >>    174			dev_err(info->dev,
> >>    175				"failed to read PTN5150_REG_INT_STATUS %d\n",
> >>    176				ret);
> >>    177			return -EINVAL;
> >>    178		}
> >>    179	
> >>    180		ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
> >>    181		if (ret) {
> >>    182			dev_err(info->dev,
> >>    183				"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
> >>    184			return -EINVAL;
> >>    185		}
> >>    186	
> >>    187		return 0;
> >>    188	}
> >>    189	
> >>    190	static int ptn5150_i2c_probe(struct i2c_client *i2c,
> >>    191					 const struct i2c_device_id *id)
> >>    192	{
> >>    193		struct device *dev = &i2c->dev;
> >>    194		struct device_node *np = i2c->dev.of_node;
> >>    195		struct ptn5150_info *info;
> >>    196		int ret;
> >>    197	
> >>    198		if (!np)
> >>    199			return -EINVAL;
> >>    200	
> >>    201		info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
> >>    202		if (!info)
> >>    203			return -ENOMEM;
> >>    204		i2c_set_clientdata(i2c, info);
> >>    205	
> >>    206		info->dev = &i2c->dev;
> >>    207		info->i2c = i2c;
> >>    208		info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
> >>    209		if (!info->int_gpiod) {
> >>    210			dev_err(dev, "failed to get INT GPIO\n");
> >>    211			return -EINVAL;
> >>    212		}
> >>    213		info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
> >>    214		if (!info->vbus_gpiod) {
> >>    215			dev_err(dev, "failed to get VBUS GPIO\n");
> >>    216			return -EINVAL;
> >>    217		}
> >>    218		ret = gpiod_direction_output(info->vbus_gpiod, 0);
> >>    219		if (ret) {
> >>    220			dev_err(dev, "failed to set VBUS GPIO direction\n");
> >>    221			return -EINVAL;
> >>    222		}
> >>    223	
> >>    224		mutex_init(&info->mutex);
> >>    225	
> >>    226		INIT_WORK(&info->irq_work, ptn5150_irq_work);
> >>    227	
> >>    228		info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
> >>    229		if (IS_ERR(info->regmap)) {
> >>    230			ret = PTR_ERR(info->regmap);
> >>    231			dev_err(info->dev, "failed to allocate register map: %d\n",
> >>    232					   ret);
> >>    233			return ret;
> >>    234		}
> >>    235	
> >>    236		if (info->int_gpiod) {
> >>    237			info->irq = gpiod_to_irq(info->int_gpiod);
> >>    238			if (info->irq < 0) {
> >>    239				dev_err(dev, "failed to get INTB IRQ\n");
> >>    240				return info->irq;
> >>    241			}
> >>    242	
> >>    243			ret = devm_request_threaded_irq(dev, info->irq, NULL,
> >>    244							ptn5150_irq_handler,
> >>    245							IRQF_TRIGGER_FALLING |
> >>    246							IRQF_ONESHOT,
> >>    247							i2c->name, info);
> >>    248			if (ret < 0) {
> >>    249				dev_err(dev, "failed to request handler for INTB IRQ\n");
> >>    250				return ret;
> >>    251			}
> >>    252		}
> >>    253	
> >>    254		/* Allocate extcon device */
> >>  > 255		info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
> >>    256		if (IS_ERR(info->edev)) {
> >>    257			dev_err(info->dev, "failed to allocate memory for extcon\n");
> >>    258			return -ENOMEM;
> >>    259		}
> >>    260	
> >>    261		/* Register extcon device */
> >>  > 262		ret = devm_extcon_dev_register(info->dev, info->edev);
> >>    263		if (ret) {
> >>    264			dev_err(info->dev, "failed to register extcon device\n");
> >>    265			return ret;
> >>    266		}
> >>    267	
> >>    268		/* Initialize PTN5150 device and print vendor id and version id */
> >>    269		ret = ptn5150_init_dev_type(info);
> >>    270		if (ret)
> >>    271			return -EINVAL;
> >>    272	
> >>    273		return 0;
> >>    274	}
> >>    275	
> >>
> >> ---
> >> 0-DAY kernel test infrastructure                Open Source Technology Center
> >> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> > 
> > 
> > 
> > 
> 
> 
> -- 
> Best Regards,
> Chanwoo Choi
> Samsung Electronics

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

* Re: [PATCH] drivers: extcon: Add support for ptn5150
  2019-01-22  6:20   ` Chanwoo Choi
@ 2019-01-22  7:55     ` Vijai Kumar K
  2019-01-23 12:46       ` [PATCH V2] " Vijai Kumar K
  0 siblings, 1 reply; 12+ messages in thread
From: Vijai Kumar K @ 2019-01-22  7:55 UTC (permalink / raw)
  To: Chanwoo Choi; +Cc: linux-kernel, myungjoo.ham

On Tue, Jan 22, 2019 at 03:20:09PM +0900, Chanwoo Choi wrote:
> Hi Vijai,
> 
> This patch looks better. But theare are comments about code clean.
> I added the comments.
> 
Thanks. And Thank you for reviewing it Chanwoo. Please find my inline comments.

I will rebase, implement the review comments and will push an updated
patch.
> On 19. 1. 21. 오후 6:09, Vijai Kumar K wrote:
> > PTN5150A is a small thin low power CC Logic chip supporting
> > the USB Type-C connector application with Configurationn Channel(CC)
> > control logic detection and indication functions.
> > 
> > Add driver, Kconfig and Makefile entries.
> > 
> > Change-Id: I3b2da147a33b913b9ec60cd914b20acd955950c7
> 
> Remove it of gerrit system.
> 
Sure.

> > Signed-off-by: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> > ---
> >  .../devicetree/bindings/extcon/extcon-ptn5150.txt  |  22 ++
> >  drivers/extcon/Kconfig                             |   8 +
> >  drivers/extcon/Makefile                            |   1 +
> >  drivers/extcon/extcon-ptn5150.c                    | 327 +++++++++++++++++++++
> >  drivers/extcon/extcon-ptn5150.h                    |  51 ++++
> >  5 files changed, 409 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
> >  create mode 100644 drivers/extcon/extcon-ptn5150.c
> >  create mode 100644 drivers/extcon/extcon-ptn5150.h
> > 
> > diff --git a/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
> > new file mode 100644
> > index 0000000..8ea33bb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
> > @@ -0,0 +1,22 @@
> > +
> > +* PTN5150 CC logic device
> 
> You better to specify the full name as following:
> : PTN5150 CC (Configuration Channel) Logic device
> 
> > +PTN5150A is a small thin low power CC Logic chip supporting the USB Type-C
> 
> s/Logic/logic
> 
noted.

> > +connector application with Configuration Channel (CC) control logic detection and
> > +indication functions. It is Interfaced to the host controller using an I2C interface.
> 
> s/Interfaced/interfaced
> 
noted.

> > +
> > +Required properties:
> > +- compatible: Should be "nxp,ptn5150"
> > +- reg: Specifies the I2C slave address of the device
> > +- int-gpio: GPIO to which INTB is connected
> 
> What is meaning of 'INTB'?
> 
INTB is the interrupt out pin of PTN5150.

> > +- vbus-gpio: GPIO which controls VBUS on/off
> > +
> > +Example:
> > +		ptn5150@1d  {
> > +			compatible = "nxp,ptn5150";
> > +			reg = <0x1d>;
> > +			int-gpio = <&msmgpio 78 GPIO_ACTIVE_HIGH>;
> > +			vbus-gpio = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
> > +			pinctrl-names = "default";
> > +			pinctrl-0 = <&ptn5150_default>;
> 
> You need to add some description why pinctrl properties are needed.
> For example, some hwmon device driver[1] explained the properties
> related to pinctrl. Please add the description as following.
> 
> - pinctrl-names : a pinctrl state named "default" must be defined.              
> - pinctrl-0 : phandle referencing pin configuration of the PWM ports.           
> 
> [1] Documentation/devicetree/hwmon/aspeed-pwm-tacho.txt
> 
Thanks for the pointer. Will look into it and add appropiate description.

> 
> > +			status = "okay";
> > +		};
> 
> Remove the one tab in front of dt node.
noted.

> 
> 	ptn5150@1d  {
> 		compatible = "nxp,ptn5150";
> 		reg = <0x1d>;
> 		int-gpio = <&msmgpio 78 GPIO_ACTIVE_HIGH>;
> 		vbus-gpio = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&ptn5150_default>;
> 		status = "okay";
> 	};
> 
> 
> 
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> > index a7bca42..6adc797 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -113,6 +113,14 @@ config EXTCON_PALMAS
> >  	  Say Y here to enable support for USB peripheral and USB host
> >  	  detection by palmas usb.
> >  
> > +config EXTCON_PTN5150
> > +	tristate "PTN5150 CC LOGIC USB EXTCON support"
> 
> PTN5150 CC (Configuration Channel) LOGIC USB EXTCON support
> 
noted.

> > +	depends on I2C
> > +	select REGMAP_I2C
> > +	help
> > +	  Say Y here to enable support for USB peripheral and USB host
> > +	  detection by ptn5150 cc logic chip.
> 
> cc -> CC (Configuration Channel)
> 
noted.

> > +
> >  config EXTCON_QCOM_SPMI_MISC
> >  	tristate "Qualcomm USB extcon support"
> >  	depends on ARCH_QCOM || COMPILE_TEST
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > index 0888fde..261ce4c 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -17,6 +17,7 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
> >  obj-$(CONFIG_EXTCON_MAX77843)	+= extcon-max77843.o
> >  obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> > +obj-$(CONFIG_EXTCON_PTN5150)	+= extcon-ptn5150.o
> >  obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
> >  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> > diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
> > new file mode 100644
> > index 0000000..bc00874
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-ptn5150.c
> > @@ -0,0 +1,327 @@
> > +/*
> > + * extcon-ptn5150.c - PTN5150 CC logic extcon driver to support USB detection
> > + *
> > + * Copyright (c) 2018-2019 by Vijai Kumar K
> > + * Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> > + *
> > + * Based on extcon-sm5502.c driver
> > + *
> > + * This program is free software; you can redistribute  it and/or modify it
> > + * under  the terms of  the GNU General  Public License as published by the
> > + * Free Software Foundation;  either version 2 of the  License, or (at your
> > + * option) any later version.
> > + */
> 
> Please SPDX license format instead of old type as following according to your
> license: You can find the SPDX examples in kernel code easily.
> 
> //SPDX-License-Identifier: GPL-2.0+
> //
> // extcon-ptn5150.c - Extcon driver to support USB detection for PTN5150 CC logic
> //                                                                              
> //  Copyright (C) 2018-2019 by Vijai Kumar K
> //  Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> 
Noted. Will update them.

> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/extcon.h>
> 
> This extcon driver is provider driver. You have to change is as following:
> - extcon.h -> extcon-provider.h
> 
> extcon.h header file should be used on extcon consumer driver.
> 
noted.

> > +#include <linux/gpio.h>
> > +
> > +#include "extcon-ptn5150.h"
> 
> extcon-ptn5150.h header file is only used on drivers/extcon/extcon-ptn5150.c.
> You don't need to create the separate header file. Please move all definitions
> of extcon-ptn5150.h to extcon-pth5150.c and remove extcon-ptn5150.h.
> 
Sure, I will move them to extcon-ptn5150.c file.

> > +
> > +struct ptn5150_info {
> > +	struct device *dev;
> > +	struct extcon_dev *edev;
> > +	struct i2c_client *i2c;
> > +	struct regmap *regmap;
> > +	struct gpio_desc *int_gpiod;
> > +	struct gpio_desc *vbus_gpiod;
> > +	int irq;
> > +	struct work_struct irq_work;
> > +	struct mutex mutex;
> > +};
> > +
> > +/* List of detectable cables */
> > +static const unsigned int ptn5150_extcon_cable[] = {
> > +	EXTCON_USB,
> > +	EXTCON_USB_HOST,
> > +	EXTCON_NONE,
> > +};
> > +
> > +static const struct regmap_config ptn5150_regmap_config = {
> > +	.reg_bits	= 8,
> > +	.val_bits	= 8,
> > +	.max_register	= PTN5150_REG_END,
> > +};
> > +
> > +static void ptn5150_irq_work(struct work_struct *work)
> > +{
> > +	struct ptn5150_info *info = container_of(work,
> > +			struct ptn5150_info, irq_work);
> > +	int ret = 0;
> > +	unsigned int reg_data;
> > +	unsigned int port_status;
> > +	unsigned int vbus;
> 
> port_status and vbus are not always used. You can define them
> under 'if (cable_attach) {' sentence.
> 
It is not always used. Will move it under 'if (cable_attach) {' sentence.

> > +	unsigned int cable_attach;
> > +	unsigned int int_status;
> > +
> > +	if (!info->edev)
> > +		return;
> > +
> > +	mutex_lock(&info->mutex);
> > +
> > +	ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
> > +	if (ret) {
> > +		dev_err(info->dev,
> > +			"failed to read CC STATUS %d\n", ret);
> 
> You is able to make it on one line as following: it is not over 80 line.
> 		dev_err(info->dev, "failed to read CC STATUS %d\n", ret);
> 
> 
> > +		mutex_unlock(&info->mutex);
> > +		return;
> > +	}
> 
> Add one blank line.
>
noted. 

> > +	/* Clear interrupt. Read would clear the register */
> > +	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
> > +	if (ret) {> +		dev_err(info->dev,
> > +			"failed to read INT STATUS %d\n", ret);
> 
> ditto.
> 
noted.

> > +		mutex_unlock(&info->mutex);
> > +		return;
> > +	}
> > +
> > +	if (int_status) {
> > +		cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
> > +		if (cable_attach) {
> 
> 			unsigned int port_status;
> 			unsigned int vbus;
> 
> 
> > +			port_status = ((reg_data &
> > +					PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
> > +					PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
> > +
> > +			switch (port_status) {
> > +			case PTN5150_DFP_ATTACHED:
> > +				extcon_set_state_sync(info->edev,
> > +						      EXTCON_USB_HOST, false);
> 
> You have to use tab for indentation. Please remove all space for indentation.
> 
oops. noted.

> > +				gpiod_set_value(info->vbus_gpiod, 0);
> > +				extcon_set_state_sync(info->edev, EXTCON_USB,
> > +						      true);
> 
> ditto.
> 
noted.

> > +				break;
> > +			case PTN5150_UFP_ATTACHED:
> > +				extcon_set_state_sync(info->edev, EXTCON_USB,
> > +						      false);
> 
> ditto.
> 
noted.

> > +				vbus = ((reg_data &
> > +					PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
> > +					PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
> > +				if (vbus)
> > +					gpiod_set_value(info->vbus_gpiod, 0);
> > +				else
> > +					gpiod_set_value(info->vbus_gpiod, 1);
> > +
> > +				extcon_set_state_sync(info->edev,
> > +						      EXTCON_USB_HOST, true);
> 
> ditto.
> 
noted.

> > +				break;
> > +			default:
> > +				dev_err(info->dev,
> > +					"Unknown Port status : %x\n",
> > +					port_status);
> > +				break;
> > +			}
> > +		} else {
> > +			extcon_set_state_sync(info->edev,
> > +					      EXTCON_USB_HOST, false);
> 
> ditto.
> 
noted.

> > +			extcon_set_state_sync(info->edev,
> > +					      EXTCON_USB, false);
> 
> ditto.
> 
noted.

> > +			gpiod_set_value(info->vbus_gpiod, 0);
> > +		}
> > +	}
> > +
> > +	/* Clear interrupt. Read would clear the register */
> > +	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
> > +				&int_status);
> > +	if (ret) {
> > +		dev_err(info->dev,
> > +			"failed to read INT REG STATUS %d\n", ret);
> > +		mutex_unlock(&info->mutex);
> > +		return;
> > +	}
> > +
> > +
> 
> Remove redundant blank line.
> 
ok.

> > +	mutex_unlock(&info->mutex);
> > +}
> > +
> > +
> 
> Remove redundant blank line.
> 
ok.

> > +static irqreturn_t ptn5150_irq_handler(int irq, void *data)
> > +{
> > +	struct ptn5150_info *info = data;
> > +
> > +	schedule_work(&info->irq_work);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int ptn5150_init_dev_type(struct ptn5150_info *info)
> > +{
> > +	unsigned int reg_data, vendor_id, version_id;
> > +	int ret;
> > +
> > +	ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
> > +	if (ret) {
> > +		dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
> > +				PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
> > +	version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
> > +				PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
> > +
> > +	dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
> > +			    version_id, vendor_id);
> > +
> > +	/* Clear any existing interrupts */
> > +	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
> > +	if (ret) {
> > +		dev_err(info->dev,
> > +			"failed to read PTN5150_REG_INT_STATUS %d\n",
> > +			ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
> > +	if (ret) {
> > +		dev_err(info->dev,
> > +			"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ptn5150_i2c_probe(struct i2c_client *i2c,
> > +				 const struct i2c_device_id *id)
> > +{
> > +	struct device *dev = &i2c->dev;
> > +	struct device_node *np = i2c->dev.of_node;
> > +	struct ptn5150_info *info;
> > +	int ret;
> > +
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
> > +	if (!info)
> > +		return -ENOMEM;
> > +	i2c_set_clientdata(i2c, info);
> > +
> > +	info->dev = &i2c->dev;
> > +	info->i2c = i2c;
> > +	info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
> > +	if (!info->int_gpiod) {
> > +		dev_err(dev, "failed to get INT GPIO\n");
> > +		return -EINVAL;
> > +	}
> > +	info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
> > +	if (!info->vbus_gpiod) {
> > +		dev_err(dev, "failed to get VBUS GPIO\n");
> > +		return -EINVAL;
> > +	}
> > +	ret = gpiod_direction_output(info->vbus_gpiod, 0);
> > +	if (ret) {
> > +		dev_err(dev, "failed to set VBUS GPIO direction\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	mutex_init(&info->mutex);
> > +
> > +	INIT_WORK(&info->irq_work, ptn5150_irq_work);
> > +
> > +	info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
> > +	if (IS_ERR(info->regmap)) {
> > +		ret = PTR_ERR(info->regmap);
> > +		dev_err(info->dev, "failed to allocate register map: %d\n",
> > +				   ret);
> > +		return ret;
> > +	}
> > +
> > +	if (info->int_gpiod) {
> > +		info->irq = gpiod_to_irq(info->int_gpiod);
> > +		if (info->irq < 0) {
> > +			dev_err(dev, "failed to get INTB IRQ\n");
> > +			return info->irq;
> > +		}
> > +
> > +		ret = devm_request_threaded_irq(dev, info->irq, NULL,
> > +						ptn5150_irq_handler,
> > +						IRQF_TRIGGER_FALLING |
> > +						IRQF_ONESHOT,
> > +						i2c->name, info);
> > +		if (ret < 0) {
> > +			dev_err(dev, "failed to request handler for INTB IRQ\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* Allocate extcon device */
> > +	info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
> > +	if (IS_ERR(info->edev)) {
> > +		dev_err(info->dev, "failed to allocate memory for extcon\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Register extcon device */
> > +	ret = devm_extcon_dev_register(info->dev, info->edev);
> > +	if (ret) {
> > +		dev_err(info->dev, "failed to register extcon device\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Initialize PTN5150 device and print vendor id and version id */
> > +	ret = ptn5150_init_dev_type(info);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ptn5150_i2c_remove(struct i2c_client *i2c)
> > +{
> > +	return 0;
> > +}
> 
> Please remove dummy ptn5150_i2c_remove() defintions.
> If the remove function on later, you can add it. 
> 
ok.

> > +
> > +static const struct of_device_id ptn5150_dt_match[] = {
> > +	{ .compatible = "nxp,ptn5150" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ptn5150_dt_match);
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int ptn5150_suspend(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int ptn5150_resume(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +#endif
> 
> Please remove dummy functions (_suspend, _resume). You can add it on later. 
> 
> 
ok.

> > +
> > +static SIMPLE_DEV_PM_OPS(ptn5150_pm_ops,
> > +			 ptn5150_suspend, ptn5150_resume);
> 
> Remove it.
> 
ok.

> > +
> > +static const struct i2c_device_id ptn5150_i2c_id[] = {
> > +	{ "ptn5150", TYPE_PTN5150A },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ptn5150_i2c_id);
> > +
> > +static struct i2c_driver ptn5150_i2c_driver = {
> > +	.driver		= {
> > +		.name	= "ptn5150",
> > +		.pm	= &ptn5150_pm_ops,
> 
> Remove it.
> 
ok.

> > +		.of_match_table = ptn5150_dt_match,
> > +	},
> > +	.probe	= ptn5150_i2c_probe,
> > +	.remove	= ptn5150_i2c_remove,
> 
> Remove it.
> 
ok.

> > +	.id_table = ptn5150_i2c_id,
> > +};
> > +
> > +static int __init ptn5150_i2c_init(void)
> > +{
> > +	return i2c_add_driver(&ptn5150_i2c_driver);
> > +}
> > +subsys_initcall(ptn5150_i2c_init);
> > +
> > +MODULE_DESCRIPTION("NXP PTN5150 CC logic Extcon driver");
> > +MODULE_AUTHOR("Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>");
> > +MODULE_LICENSE("GPL");
> 
> If you use SPDX license format, you dont' need to add module information.
> Please remove MODULE_*() macro.
> 
ok. Will update to SPDX license format and will remove these.

> > diff --git a/drivers/extcon/extcon-ptn5150.h b/drivers/extcon/extcon-ptn5150.h
> 
> As I already commented, you have to move all definitiosn of extcon-ptn5150.h
> to extcon-ptn5150.c and remove extcon-ptn5150.h.
> 
sure.

> > new file mode 100644
> > index 0000000..217dab0
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-ptn5150.h
> > @@ -0,0 +1,51 @@
> > +/*
> > + * extcon-ptn5150.h
> > + *
> > + * Copyright (c) 2018-2019 by Vijai Kumar K
> > + * Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> > + *
> > + * This program is free software; you can redistribute  it and/or modify it
> > + * under  the terms of  the GNU General  Public License as published by the
> > + * Free Software Foundation;  either version 2 of the  License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#ifndef __LINUX_EXTCON_PTN5150_H
> > +#define __LINUX_EXTCON_PTN5150_H
> > +
> > +enum ptn5150_types {
> > +	TYPE_PTN5150A,
> 
> Do you have additional plan to suppor other type for extcon-ptn5150.c driver?
> If extcon-ptn5150.c supports only one driver, you don't need to this enumeration.
> 
Right now, I donot have plans, as I dont have means to test them. I will remove this.
BTW, This driver is for ptn5150A.

> > +};
> > +
> > +/* PTN5150 registers */
> > +enum ptn5150_reg {
> > +	PTN5150_REG_DEVICE_ID = 0x01,
> > +	PTN5150_REG_CONTROL,
> > +	PTN5150_REG_INT_STATUS,
> > +	PTN5150_REG_CC_STATUS,
> > +	PTN5150_REG_CON_DET = 0x09,
> > +	PTN5150_REG_VCONN_STATUS,
> > +	PTN5150_REG_RESET,
> > +	PTN5150_REG_INT_MASK = 0x18,
> > +	PTN5150_REG_INT_REG_STATUS,
> > +	PTN5150_REG_END,
> > +};
> > +
> > +#define PTN5150_DFP_ATTACHED			0x1
> > +#define PTN5150_UFP_ATTACHED			0x2
> > +
> > +/* Define PTN5150 MASK/SHIFT constant */
> > +#define PTN5150_REG_DEVICE_ID_VENDOR_SHIFT	0
> > +#define PTN5150_REG_DEVICE_ID_VERSION_SHIFT	3
> > +#define PTN5150_REG_DEVICE_ID_VENDOR_MASK	(0x3 << PTN5150_REG_DEVICE_ID_VENDOR_SHIFT)
> > +#define PTN5150_REG_DEVICE_ID_VERSION_MASK	(0x1f << PTN5150_REG_DEVICE_ID_VERSION_SHIFT)
> > +
> > +#define PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT	2
> > +#define PTN5150_REG_CC_PORT_ATTACHMENT_MASK	(0x7 << PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT)
> > +#define PTN5150_REG_CC_VBUS_DETECTION_SHIFT	7
> > +#define PTN5150_REG_CC_VBUS_DETECTION_MASK	(0x1 << PTN5150_REG_CC_VBUS_DETECTION_SHIFT)
> > +#define PTN5150_REG_INT_CABLE_ATTACH_SHIFT	0
> > +#define PTN5150_REG_INT_CABLE_ATTACH_MASK	(0x1 << PTN5150_REG_INT_CABLE_ATTACH_SHIFT)
> > +#define PTN5150_REG_INT_CABLE_DETACH_SHIFT	1
> > +#define PTN5150_REG_INT_CABLE_DETACH_MASK	(0x1 << PTN5150_REG_CC_CABLE_DETACH_SHIFT)
> > +#endif /*  __LINUX_EXTCON_PTN5150_H */
> > 
> 
> -- 
> Best Regards,
> Chanwoo Choi
> Samsung Electronics
Thanks,
Vijai Kumar K

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

* [PATCH V2] drivers: extcon: Add support for ptn5150
  2019-01-22  7:55     ` Vijai Kumar K
@ 2019-01-23 12:46       ` Vijai Kumar K
  2019-01-24  2:03         ` Chanwoo Choi
       [not found]         ` <CGME20190124060419epcas1p4aed00219fc0e0281c2d5435ce6ec2e48@epcas1p4.samsung.com>
  0 siblings, 2 replies; 12+ messages in thread
From: Vijai Kumar K @ 2019-01-23 12:46 UTC (permalink / raw)
  To: cw00.choi; +Cc: linux-kernel, myungjoo.ham, Vijai Kumar K

PTN5150A is a small thin low power CC Logic chip supporting
the USB Type-C connector application with Configurationn Channel(CC)
control logic detection and indication functions.

Add driver, Kconfig and Makefile entries.

Signed-off-by: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
---
Hi Chanwoo,

I have implemented the review comments and below is the updated patchset.
Can you please review it?

Highlights:
 - extcon.h -> extcon-provider.h
 - Remove dummy implementations for .remove, _suspend/_resume
 - Change license format to SPDX
 - remove extcon-ptn5150.h and collapse definitions to extcon-ptn5150.c
 - Replace tabs with spaces
 - Update Documentation
 - Fix coding style issues 

Thanks
Vijai Kumar K

 .../devicetree/bindings/extcon/extcon-ptn5150.txt  |  27 ++
 drivers/extcon/Kconfig                             |   8 +
 drivers/extcon/Makefile                            |   1 +
 drivers/extcon/extcon-ptn5150.c                    | 335 +++++++++++++++++++++
 4 files changed, 371 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
 create mode 100644 drivers/extcon/extcon-ptn5150.c

diff --git a/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
new file mode 100644
index 0000000..e772d42
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
@@ -0,0 +1,27 @@
+
+* PTN5150 CC (Configuration Channel) Logic device
+PTN5150A is a small thin low power CC logic chip supporting the USB Type-C
+connector application with Configuration Channel (CC) control logic detection and
+indication functions. It is interfaced to the host controller using an I2C interface.
+
+Required properties:
+- compatible: Should be "nxp,ptn5150"
+- reg: Specifies the I2C slave address of the device
+- int-gpio: should contain a phandle and GPIO specifier for the GPIO pin
+	    connected to the PTN5150's INTB pin.
+- vbus-gpio: should contain a phandle and GPIO specifier for the GPIO pin which
+	     is used to control VBUS.
+- pinctrl-names : a pinctrl state named "default" must be defined.
+- pinctrl-0 : phandle referencing pin configuration of interrupt and vbus control.
+
+Example:
+
+	ptn5150@1d  {
+		compatible = "nxp,ptn5150";
+		reg = <0x1d>;
+		int-gpio = <&msmgpio 78 GPIO_ACTIVE_HIGH>;
+		vbus-gpio = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&ptn5150_default>;
+		status = "okay";
+	};
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index de15bf5..405cd76 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -114,6 +114,14 @@ config EXTCON_PALMAS
 	  Say Y here to enable support for USB peripheral and USB host
 	  detection by palmas usb.
 
+config EXTCON_PTN5150
+	tristate "PTN5150 CC (Configuration Channel) LOGIC USB EXTCON support"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to enable support for USB peripheral and USB host
+	  detection by ptn5150 CC (Configuration Channel) logic chip.
+
 config EXTCON_QCOM_SPMI_MISC
 	tristate "Qualcomm USB extcon support"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0888fde..261ce4c 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
 obj-$(CONFIG_EXTCON_MAX77843)	+= extcon-max77843.o
 obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
 obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
+obj-$(CONFIG_EXTCON_PTN5150)	+= extcon-ptn5150.o
 obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
new file mode 100644
index 0000000..155620b
--- /dev/null
+++ b/drivers/extcon/extcon-ptn5150.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// extcon-ptn5150.c - PTN5150 CC logic extcon driver to support USB detection
+//
+// Based on extcon-sm5502.c driver
+// Copyright (c) 2018-2019 by Vijai Kumar K
+// Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/extcon-provider.h>
+#include <linux/gpio.h>
+
+/* PTN5150 registers */
+enum ptn5150_reg {
+	PTN5150_REG_DEVICE_ID = 0x01,
+	PTN5150_REG_CONTROL,
+	PTN5150_REG_INT_STATUS,
+	PTN5150_REG_CC_STATUS,
+	PTN5150_REG_CON_DET = 0x09,
+	PTN5150_REG_VCONN_STATUS,
+	PTN5150_REG_RESET,
+	PTN5150_REG_INT_MASK = 0x18,
+	PTN5150_REG_INT_REG_STATUS,
+	PTN5150_REG_END,
+};
+
+#define PTN5150_DFP_ATTACHED			0x1
+#define PTN5150_UFP_ATTACHED			0x2
+
+/* Define PTN5150 MASK/SHIFT constant */
+#define PTN5150_REG_DEVICE_ID_VENDOR_SHIFT	0
+#define PTN5150_REG_DEVICE_ID_VENDOR_MASK	\
+	(0x3 << PTN5150_REG_DEVICE_ID_VENDOR_SHIFT)
+
+#define PTN5150_REG_DEVICE_ID_VERSION_SHIFT	3
+#define PTN5150_REG_DEVICE_ID_VERSION_MASK	\
+	(0x1f << PTN5150_REG_DEVICE_ID_VERSION_SHIFT)
+
+#define PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT	2
+#define PTN5150_REG_CC_PORT_ATTACHMENT_MASK	\
+	(0x7 << PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT)
+
+#define PTN5150_REG_CC_VBUS_DETECTION_SHIFT	7
+#define PTN5150_REG_CC_VBUS_DETECTION_MASK	\
+	(0x1 << PTN5150_REG_CC_VBUS_DETECTION_SHIFT)
+
+#define PTN5150_REG_INT_CABLE_ATTACH_SHIFT	0
+#define PTN5150_REG_INT_CABLE_ATTACH_MASK	\
+	(0x1 << PTN5150_REG_INT_CABLE_ATTACH_SHIFT)
+
+#define PTN5150_REG_INT_CABLE_DETACH_SHIFT	1
+#define PTN5150_REG_INT_CABLE_DETACH_MASK	\
+	(0x1 << PTN5150_REG_CC_CABLE_DETACH_SHIFT)
+
+struct ptn5150_info {
+	struct device *dev;
+	struct extcon_dev *edev;
+	struct i2c_client *i2c;
+	struct regmap *regmap;
+	struct gpio_desc *int_gpiod;
+	struct gpio_desc *vbus_gpiod;
+	int irq;
+	struct work_struct irq_work;
+	struct mutex mutex;
+};
+
+/* List of detectable cables */
+static const unsigned int ptn5150_extcon_cable[] = {
+	EXTCON_USB,
+	EXTCON_USB_HOST,
+	EXTCON_NONE,
+};
+
+static const struct regmap_config ptn5150_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= PTN5150_REG_END,
+};
+
+static void ptn5150_irq_work(struct work_struct *work)
+{
+	struct ptn5150_info *info = container_of(work,
+			struct ptn5150_info, irq_work);
+	int ret = 0;
+	unsigned int reg_data;
+	unsigned int int_status;
+
+	if (!info->edev)
+		return;
+
+	mutex_lock(&info->mutex);
+
+	ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev, "failed to read CC STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	/* Clear interrupt. Read would clear the register */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
+	if (ret) {
+		dev_err(info->dev, "failed to read INT STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	if (int_status) {
+		unsigned int cable_attach;
+
+		cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
+		if (cable_attach) {
+			unsigned int port_status;
+			unsigned int vbus;
+
+			port_status = ((reg_data &
+					PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
+					PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
+
+			switch (port_status) {
+			case PTN5150_DFP_ATTACHED:
+				extcon_set_state_sync(info->edev,
+						EXTCON_USB_HOST, false);
+				gpiod_set_value(info->vbus_gpiod, 0);
+				extcon_set_state_sync(info->edev, EXTCON_USB,
+						true);
+				break;
+			case PTN5150_UFP_ATTACHED:
+				extcon_set_state_sync(info->edev, EXTCON_USB,
+						false);
+				vbus = ((reg_data &
+					PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
+					PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
+				if (vbus)
+					gpiod_set_value(info->vbus_gpiod, 0);
+				else
+					gpiod_set_value(info->vbus_gpiod, 1);
+
+				extcon_set_state_sync(info->edev,
+						EXTCON_USB_HOST, true);
+				break;
+			default:
+				dev_err(info->dev,
+					"Unknown Port status : %x\n",
+					port_status);
+				break;
+			}
+		} else {
+			extcon_set_state_sync(info->edev,
+					EXTCON_USB_HOST, false);
+			extcon_set_state_sync(info->edev,
+					EXTCON_USB, false);
+			gpiod_set_value(info->vbus_gpiod, 0);
+		}
+	}
+
+	/* Clear interrupt. Read would clear the register */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
+			&int_status);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read INT REG STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	mutex_unlock(&info->mutex);
+}
+
+
+static irqreturn_t ptn5150_irq_handler(int irq, void *data)
+{
+	struct ptn5150_info *info = data;
+
+	schedule_work(&info->irq_work);
+
+	return IRQ_HANDLED;
+}
+
+static int ptn5150_init_dev_type(struct ptn5150_info *info)
+{
+	unsigned int reg_data, vendor_id, version_id;
+	int ret;
+
+	ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
+	if (ret) {
+		dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
+		return -EINVAL;
+	}
+
+	vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
+				PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
+	version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
+				PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
+
+	dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
+			    version_id, vendor_id);
+
+	/* Clear any existing interrupts */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read PTN5150_REG_INT_STATUS %d\n",
+			ret);
+		return -EINVAL;
+	}
+
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ptn5150_i2c_probe(struct i2c_client *i2c,
+				 const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct device_node *np = i2c->dev.of_node;
+	struct ptn5150_info *info;
+	int ret;
+
+	if (!np)
+		return -EINVAL;
+
+	info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+	i2c_set_clientdata(i2c, info);
+
+	info->dev = &i2c->dev;
+	info->i2c = i2c;
+	info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
+	if (!info->int_gpiod) {
+		dev_err(dev, "failed to get INT GPIO\n");
+		return -EINVAL;
+	}
+	info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
+	if (!info->vbus_gpiod) {
+		dev_err(dev, "failed to get VBUS GPIO\n");
+		return -EINVAL;
+	}
+	ret = gpiod_direction_output(info->vbus_gpiod, 0);
+	if (ret) {
+		dev_err(dev, "failed to set VBUS GPIO direction\n");
+		return -EINVAL;
+	}
+
+	mutex_init(&info->mutex);
+
+	INIT_WORK(&info->irq_work, ptn5150_irq_work);
+
+	info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
+	if (IS_ERR(info->regmap)) {
+		ret = PTR_ERR(info->regmap);
+		dev_err(info->dev, "failed to allocate register map: %d\n",
+				   ret);
+		return ret;
+	}
+
+	if (info->int_gpiod) {
+		info->irq = gpiod_to_irq(info->int_gpiod);
+		if (info->irq < 0) {
+			dev_err(dev, "failed to get INTB IRQ\n");
+			return info->irq;
+		}
+
+		ret = devm_request_threaded_irq(dev, info->irq, NULL,
+						ptn5150_irq_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						i2c->name, info);
+		if (ret < 0) {
+			dev_err(dev, "failed to request handler for INTB IRQ\n");
+			return ret;
+		}
+	}
+
+	/* Allocate extcon device */
+	info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
+	if (IS_ERR(info->edev)) {
+		dev_err(info->dev, "failed to allocate memory for extcon\n");
+		return -ENOMEM;
+	}
+
+	/* Register extcon device */
+	ret = devm_extcon_dev_register(info->dev, info->edev);
+	if (ret) {
+		dev_err(info->dev, "failed to register extcon device\n");
+		return ret;
+	}
+
+	/* Initialize PTN5150 device and print vendor id and version id */
+	ret = ptn5150_init_dev_type(info);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct of_device_id ptn5150_dt_match[] = {
+	{ .compatible = "nxp,ptn5150" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ptn5150_dt_match);
+
+static const struct i2c_device_id ptn5150_i2c_id[] = {
+	{ "ptn5150", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ptn5150_i2c_id);
+
+static struct i2c_driver ptn5150_i2c_driver = {
+	.driver		= {
+		.name	= "ptn5150",
+		.of_match_table = ptn5150_dt_match,
+	},
+	.probe	= ptn5150_i2c_probe,
+	.id_table = ptn5150_i2c_id,
+};
+
+static int __init ptn5150_i2c_init(void)
+{
+	return i2c_add_driver(&ptn5150_i2c_driver);
+}
+subsys_initcall(ptn5150_i2c_init);
-- 
2.7.4


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

* Re: [PATCH V2] drivers: extcon: Add support for ptn5150
  2019-01-23 12:46       ` [PATCH V2] " Vijai Kumar K
@ 2019-01-24  2:03         ` Chanwoo Choi
       [not found]         ` <CGME20190124060419epcas1p4aed00219fc0e0281c2d5435ce6ec2e48@epcas1p4.samsung.com>
  1 sibling, 0 replies; 12+ messages in thread
From: Chanwoo Choi @ 2019-01-24  2:03 UTC (permalink / raw)
  To: Vijai Kumar K; +Cc: linux-kernel, myungjoo.ham

Hi Vijai,

Looks good to me. But, there are minor modification by myself as following:
After fixed them, applied it to extcon-next. Thanks.

- PTN5150A -> PTN5150 because this patch usually used the 'ptn5150' word without 'A'.
- Modify the patch subject to keep the same format of extcon drivers
	- before : drivers: extcon: Add support for ptn5150
	- after  : extcon: Add support for ptn5150 extcon driver
- Remove the space in extcon-ptn5150.txt and then use tab for indentation
- Limit the under 80 char on one line


[Modified version]
    extcon: Add support for ptn5150 extcon driver

    PTN5150 is a small thin low power CC (Configurationn Channel)
    Logic chip supporting the USB Type-C connector application with
    CC control logic detection and indication functions.

    Signed-off-by: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>


Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt

+* PTN5150 CC (Configuration Channel) Logic device
+
+PTN5150 is a small thin low power CC logic chip supporting the USB Type-C
+connector application with CC control logic detection and indication functions.
+It is interfaced to the host controller using an I2C interface.
+
+Required properties:
+- compatible: should be "nxp,ptn5150"
+- reg: specifies the I2C slave address of the device
+- int-gpio: should contain a phandle and GPIO specifier for the GPIO pin
+       connected to the PTN5150's INTB pin.
+- vbus-gpio: should contain a phandle and GPIO specifier for the GPIO pin which
+       is used to control VBUS.
+- pinctrl-names : a pinctrl state named "default" must be defined.
+- pinctrl-0 : phandle referencing pin configuration of interrupt and vbus
+       control.
+

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

On 19. 1. 23. 오후 9:46, Vijai Kumar K wrote:
> PTN5150A is a small thin low power CC Logic chip supporting
> the USB Type-C connector application with Configurationn Channel(CC)
> control logic detection and indication functions.
> 
> Add driver, Kconfig and Makefile entries.
> 
> Signed-off-by: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> ---
> Hi Chanwoo,
> 
> I have implemented the review comments and below is the updated patchset.
> Can you please review it?
> 
> Highlights:
>  - extcon.h -> extcon-provider.h
>  - Remove dummy implementations for .remove, _suspend/_resume
>  - Change license format to SPDX
>  - remove extcon-ptn5150.h and collapse definitions to extcon-ptn5150.c
>  - Replace tabs with spaces
>  - Update Documentation
>  - Fix coding style issues 
> 
> Thanks
> Vijai Kumar K
> 
>  .../devicetree/bindings/extcon/extcon-ptn5150.txt  |  27 ++
>  drivers/extcon/Kconfig                             |   8 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-ptn5150.c                    | 335 +++++++++++++++++++++
>  4 files changed, 371 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
>  create mode 100644 drivers/extcon/extcon-ptn5150.c
> 
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
> new file mode 100644
> index 0000000..e772d42
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
> @@ -0,0 +1,27 @@
> +
> +* PTN5150 CC (Configuration Channel) Logic device
> +PTN5150A is a small thin low power CC logic chip supporting the USB Type-C
> +connector application with Configuration Channel (CC) control logic detection and
> +indication functions. It is interfaced to the host controller using an I2C interface.
> +
> +Required properties:
> +- compatible: Should be "nxp,ptn5150"
> +- reg: Specifies the I2C slave address of the device
> +- int-gpio: should contain a phandle and GPIO specifier for the GPIO pin
> +	    connected to the PTN5150's INTB pin.
> +- vbus-gpio: should contain a phandle and GPIO specifier for the GPIO pin which
> +	     is used to control VBUS.
> +- pinctrl-names : a pinctrl state named "default" must be defined.
> +- pinctrl-0 : phandle referencing pin configuration of interrupt and vbus control.
> +
> +Example:
> +
> +	ptn5150@1d  {
> +		compatible = "nxp,ptn5150";
> +		reg = <0x1d>;
> +		int-gpio = <&msmgpio 78 GPIO_ACTIVE_HIGH>;
> +		vbus-gpio = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&ptn5150_default>;
> +		status = "okay";
> +	};
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index de15bf5..405cd76 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -114,6 +114,14 @@ config EXTCON_PALMAS
>  	  Say Y here to enable support for USB peripheral and USB host
>  	  detection by palmas usb.
>  
> +config EXTCON_PTN5150
> +	tristate "PTN5150 CC (Configuration Channel) LOGIC USB EXTCON support"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say Y here to enable support for USB peripheral and USB host
> +	  detection by ptn5150 CC (Configuration Channel) logic chip.
> +
>  config EXTCON_QCOM_SPMI_MISC
>  	tristate "Qualcomm USB extcon support"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 0888fde..261ce4c 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>  obj-$(CONFIG_EXTCON_MAX77843)	+= extcon-max77843.o
>  obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> +obj-$(CONFIG_EXTCON_PTN5150)	+= extcon-ptn5150.o
>  obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
> new file mode 100644
> index 0000000..155620b
> --- /dev/null
> +++ b/drivers/extcon/extcon-ptn5150.c
> @@ -0,0 +1,335 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// extcon-ptn5150.c - PTN5150 CC logic extcon driver to support USB detection
> +//
> +// Based on extcon-sm5502.c driver
> +// Copyright (c) 2018-2019 by Vijai Kumar K
> +// Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/extcon-provider.h>
> +#include <linux/gpio.h>
> +
> +/* PTN5150 registers */
> +enum ptn5150_reg {
> +	PTN5150_REG_DEVICE_ID = 0x01,
> +	PTN5150_REG_CONTROL,
> +	PTN5150_REG_INT_STATUS,
> +	PTN5150_REG_CC_STATUS,
> +	PTN5150_REG_CON_DET = 0x09,
> +	PTN5150_REG_VCONN_STATUS,
> +	PTN5150_REG_RESET,
> +	PTN5150_REG_INT_MASK = 0x18,
> +	PTN5150_REG_INT_REG_STATUS,
> +	PTN5150_REG_END,
> +};
> +
> +#define PTN5150_DFP_ATTACHED			0x1
> +#define PTN5150_UFP_ATTACHED			0x2
> +
> +/* Define PTN5150 MASK/SHIFT constant */
> +#define PTN5150_REG_DEVICE_ID_VENDOR_SHIFT	0
> +#define PTN5150_REG_DEVICE_ID_VENDOR_MASK	\
> +	(0x3 << PTN5150_REG_DEVICE_ID_VENDOR_SHIFT)
> +
> +#define PTN5150_REG_DEVICE_ID_VERSION_SHIFT	3
> +#define PTN5150_REG_DEVICE_ID_VERSION_MASK	\
> +	(0x1f << PTN5150_REG_DEVICE_ID_VERSION_SHIFT)
> +
> +#define PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT	2
> +#define PTN5150_REG_CC_PORT_ATTACHMENT_MASK	\
> +	(0x7 << PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT)
> +
> +#define PTN5150_REG_CC_VBUS_DETECTION_SHIFT	7
> +#define PTN5150_REG_CC_VBUS_DETECTION_MASK	\
> +	(0x1 << PTN5150_REG_CC_VBUS_DETECTION_SHIFT)
> +
> +#define PTN5150_REG_INT_CABLE_ATTACH_SHIFT	0
> +#define PTN5150_REG_INT_CABLE_ATTACH_MASK	\
> +	(0x1 << PTN5150_REG_INT_CABLE_ATTACH_SHIFT)
> +
> +#define PTN5150_REG_INT_CABLE_DETACH_SHIFT	1
> +#define PTN5150_REG_INT_CABLE_DETACH_MASK	\
> +	(0x1 << PTN5150_REG_CC_CABLE_DETACH_SHIFT)
> +
> +struct ptn5150_info {
> +	struct device *dev;
> +	struct extcon_dev *edev;
> +	struct i2c_client *i2c;
> +	struct regmap *regmap;
> +	struct gpio_desc *int_gpiod;
> +	struct gpio_desc *vbus_gpiod;
> +	int irq;
> +	struct work_struct irq_work;
> +	struct mutex mutex;
> +};
> +
> +/* List of detectable cables */
> +static const unsigned int ptn5150_extcon_cable[] = {
> +	EXTCON_USB,
> +	EXTCON_USB_HOST,
> +	EXTCON_NONE,
> +};
> +
> +static const struct regmap_config ptn5150_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= PTN5150_REG_END,
> +};
> +
> +static void ptn5150_irq_work(struct work_struct *work)
> +{
> +	struct ptn5150_info *info = container_of(work,
> +			struct ptn5150_info, irq_work);
> +	int ret = 0;
> +	unsigned int reg_data;
> +	unsigned int int_status;
> +
> +	if (!info->edev)
> +		return;
> +
> +	mutex_lock(&info->mutex);
> +
> +	ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
> +	if (ret) {
> +		dev_err(info->dev, "failed to read CC STATUS %d\n", ret);
> +		mutex_unlock(&info->mutex);
> +		return;
> +	}
> +
> +	/* Clear interrupt. Read would clear the register */
> +	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
> +	if (ret) {
> +		dev_err(info->dev, "failed to read INT STATUS %d\n", ret);
> +		mutex_unlock(&info->mutex);
> +		return;
> +	}
> +
> +	if (int_status) {
> +		unsigned int cable_attach;
> +
> +		cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
> +		if (cable_attach) {
> +			unsigned int port_status;
> +			unsigned int vbus;
> +
> +			port_status = ((reg_data &
> +					PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
> +					PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
> +
> +			switch (port_status) {
> +			case PTN5150_DFP_ATTACHED:
> +				extcon_set_state_sync(info->edev,
> +						EXTCON_USB_HOST, false);
> +				gpiod_set_value(info->vbus_gpiod, 0);
> +				extcon_set_state_sync(info->edev, EXTCON_USB,
> +						true);
> +				break;
> +			case PTN5150_UFP_ATTACHED:
> +				extcon_set_state_sync(info->edev, EXTCON_USB,
> +						false);
> +				vbus = ((reg_data &
> +					PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
> +					PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
> +				if (vbus)
> +					gpiod_set_value(info->vbus_gpiod, 0);
> +				else
> +					gpiod_set_value(info->vbus_gpiod, 1);
> +
> +				extcon_set_state_sync(info->edev,
> +						EXTCON_USB_HOST, true);
> +				break;
> +			default:
> +				dev_err(info->dev,
> +					"Unknown Port status : %x\n",
> +					port_status);
> +				break;
> +			}
> +		} else {
> +			extcon_set_state_sync(info->edev,
> +					EXTCON_USB_HOST, false);
> +			extcon_set_state_sync(info->edev,
> +					EXTCON_USB, false);
> +			gpiod_set_value(info->vbus_gpiod, 0);
> +		}
> +	}
> +
> +	/* Clear interrupt. Read would clear the register */
> +	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
> +			&int_status);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"failed to read INT REG STATUS %d\n", ret);
> +		mutex_unlock(&info->mutex);
> +		return;
> +	}
> +
> +	mutex_unlock(&info->mutex);
> +}
> +
> +
> +static irqreturn_t ptn5150_irq_handler(int irq, void *data)
> +{
> +	struct ptn5150_info *info = data;
> +
> +	schedule_work(&info->irq_work);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ptn5150_init_dev_type(struct ptn5150_info *info)
> +{
> +	unsigned int reg_data, vendor_id, version_id;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
> +	if (ret) {
> +		dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
> +				PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
> +	version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
> +				PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
> +
> +	dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
> +			    version_id, vendor_id);
> +
> +	/* Clear any existing interrupts */
> +	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"failed to read PTN5150_REG_INT_STATUS %d\n",
> +			ret);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ptn5150_i2c_probe(struct i2c_client *i2c,
> +				 const struct i2c_device_id *id)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct device_node *np = i2c->dev.of_node;
> +	struct ptn5150_info *info;
> +	int ret;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +	i2c_set_clientdata(i2c, info);
> +
> +	info->dev = &i2c->dev;
> +	info->i2c = i2c;
> +	info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
> +	if (!info->int_gpiod) {
> +		dev_err(dev, "failed to get INT GPIO\n");
> +		return -EINVAL;
> +	}
> +	info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
> +	if (!info->vbus_gpiod) {
> +		dev_err(dev, "failed to get VBUS GPIO\n");
> +		return -EINVAL;
> +	}
> +	ret = gpiod_direction_output(info->vbus_gpiod, 0);
> +	if (ret) {
> +		dev_err(dev, "failed to set VBUS GPIO direction\n");
> +		return -EINVAL;
> +	}
> +
> +	mutex_init(&info->mutex);
> +
> +	INIT_WORK(&info->irq_work, ptn5150_irq_work);
> +
> +	info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
> +	if (IS_ERR(info->regmap)) {
> +		ret = PTR_ERR(info->regmap);
> +		dev_err(info->dev, "failed to allocate register map: %d\n",
> +				   ret);
> +		return ret;
> +	}
> +
> +	if (info->int_gpiod) {
> +		info->irq = gpiod_to_irq(info->int_gpiod);
> +		if (info->irq < 0) {
> +			dev_err(dev, "failed to get INTB IRQ\n");
> +			return info->irq;
> +		}
> +
> +		ret = devm_request_threaded_irq(dev, info->irq, NULL,
> +						ptn5150_irq_handler,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						i2c->name, info);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to request handler for INTB IRQ\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* Allocate extcon device */
> +	info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
> +	if (IS_ERR(info->edev)) {
> +		dev_err(info->dev, "failed to allocate memory for extcon\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Register extcon device */
> +	ret = devm_extcon_dev_register(info->dev, info->edev);
> +	if (ret) {
> +		dev_err(info->dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	/* Initialize PTN5150 device and print vendor id and version id */
> +	ret = ptn5150_init_dev_type(info);
> +	if (ret)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ptn5150_dt_match[] = {
> +	{ .compatible = "nxp,ptn5150" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ptn5150_dt_match);
> +
> +static const struct i2c_device_id ptn5150_i2c_id[] = {
> +	{ "ptn5150", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ptn5150_i2c_id);
> +
> +static struct i2c_driver ptn5150_i2c_driver = {
> +	.driver		= {
> +		.name	= "ptn5150",
> +		.of_match_table = ptn5150_dt_match,
> +	},
> +	.probe	= ptn5150_i2c_probe,
> +	.id_table = ptn5150_i2c_id,
> +};
> +
> +static int __init ptn5150_i2c_init(void)
> +{
> +	return i2c_add_driver(&ptn5150_i2c_driver);
> +}
> +subsys_initcall(ptn5150_i2c_init);
> 



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

* [PATCH v3] extcon: Add support for ptn5150 extcon driver
       [not found]         ` <CGME20190124060419epcas1p4aed00219fc0e0281c2d5435ce6ec2e48@epcas1p4.samsung.com>
@ 2019-01-24  6:04           ` Chanwoo Choi
  0 siblings, 0 replies; 12+ messages in thread
From: Chanwoo Choi @ 2019-01-24  6:04 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, chanwoo, vijaikumar.kanagarajan, myungjoo.ham

From: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>

PTN5150 is a small thin low power CC (Configurationn Channel)
Logic chip supporting the USB Type-C connector application with
CC control logic detection and indication functions.

Signed-off-by: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
[cw00.choi: fix indentation of binding document and change patch subject]
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 .../devicetree/bindings/extcon/extcon-ptn5150.txt  |  27 ++
 drivers/extcon/Kconfig                             |   8 +
 drivers/extcon/Makefile                            |   1 +
 drivers/extcon/extcon-ptn5150.c                    | 339 +++++++++++++++++++++
 4 files changed, 375 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
 create mode 100644 drivers/extcon/extcon-ptn5150.c

diff --git a/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
new file mode 100644
index 000000000000..936fbdf12815
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.txt
@@ -0,0 +1,27 @@
+* PTN5150 CC (Configuration Channel) Logic device
+
+PTN5150 is a small thin low power CC logic chip supporting the USB Type-C
+connector application with CC control logic detection and indication functions.
+It is interfaced to the host controller using an I2C interface.
+
+Required properties:
+- compatible: should be "nxp,ptn5150"
+- reg: specifies the I2C slave address of the device
+- int-gpio: should contain a phandle and GPIO specifier for the GPIO pin
+	connected to the PTN5150's INTB pin.
+- vbus-gpio: should contain a phandle and GPIO specifier for the GPIO pin which
+	is used to control VBUS.
+- pinctrl-names : a pinctrl state named "default" must be defined.
+- pinctrl-0 : phandle referencing pin configuration of interrupt and vbus
+	control.
+
+Example:
+	ptn5150@1d {
+		compatible = "nxp,ptn5150";
+		reg = <0x1d>;
+		int-gpio = <&msmgpio 78 GPIO_ACTIVE_HIGH>;
+		vbus-gpio = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&ptn5150_default>;
+		status = "okay";
+	};
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index de15bf55895b..b9cc027e89ab 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -114,6 +114,14 @@ config EXTCON_PALMAS
 	  Say Y here to enable support for USB peripheral and USB host
 	  detection by palmas usb.

+config EXTCON_PTN5150
+	tristate "NXP PTN5150 CC LOGIC USB EXTCON support"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to enable support for USB peripheral and USB host
+	  detection by NXP PTN5150 CC (Configuration Channel) logic chip.
+
 config EXTCON_QCOM_SPMI_MISC
 	tristate "Qualcomm USB extcon support"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0888fdeded72..261ce4cfe209 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
 obj-$(CONFIG_EXTCON_MAX77843)	+= extcon-max77843.o
 obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
 obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
+obj-$(CONFIG_EXTCON_PTN5150)	+= extcon-ptn5150.o
 obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
new file mode 100644
index 000000000000..b6d0557030d3
--- /dev/null
+++ b/drivers/extcon/extcon-ptn5150.c
@@ -0,0 +1,339 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// extcon-ptn5150.c - PTN5150 CC logic extcon driver to support USB detection
+//
+// Based on extcon-sm5502.c driver
+// Copyright (c) 2018-2019 by Vijai Kumar K
+// Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/extcon-provider.h>
+#include <linux/gpio.h>
+
+/* PTN5150 registers */
+enum ptn5150_reg {
+	PTN5150_REG_DEVICE_ID = 0x01,
+	PTN5150_REG_CONTROL,
+	PTN5150_REG_INT_STATUS,
+	PTN5150_REG_CC_STATUS,
+	PTN5150_REG_CON_DET = 0x09,
+	PTN5150_REG_VCONN_STATUS,
+	PTN5150_REG_RESET,
+	PTN5150_REG_INT_MASK = 0x18,
+	PTN5150_REG_INT_REG_STATUS,
+	PTN5150_REG_END,
+};
+
+#define PTN5150_DFP_ATTACHED			0x1
+#define PTN5150_UFP_ATTACHED			0x2
+
+/* Define PTN5150 MASK/SHIFT constant */
+#define PTN5150_REG_DEVICE_ID_VENDOR_SHIFT	0
+#define PTN5150_REG_DEVICE_ID_VENDOR_MASK	\
+	(0x3 << PTN5150_REG_DEVICE_ID_VENDOR_SHIFT)
+
+#define PTN5150_REG_DEVICE_ID_VERSION_SHIFT	3
+#define PTN5150_REG_DEVICE_ID_VERSION_MASK	\
+	(0x1f << PTN5150_REG_DEVICE_ID_VERSION_SHIFT)
+
+#define PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT	2
+#define PTN5150_REG_CC_PORT_ATTACHMENT_MASK	\
+	(0x7 << PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT)
+
+#define PTN5150_REG_CC_VBUS_DETECTION_SHIFT	7
+#define PTN5150_REG_CC_VBUS_DETECTION_MASK	\
+	(0x1 << PTN5150_REG_CC_VBUS_DETECTION_SHIFT)
+
+#define PTN5150_REG_INT_CABLE_ATTACH_SHIFT	0
+#define PTN5150_REG_INT_CABLE_ATTACH_MASK	\
+	(0x1 << PTN5150_REG_INT_CABLE_ATTACH_SHIFT)
+
+#define PTN5150_REG_INT_CABLE_DETACH_SHIFT	1
+#define PTN5150_REG_INT_CABLE_DETACH_MASK	\
+	(0x1 << PTN5150_REG_CC_CABLE_DETACH_SHIFT)
+
+struct ptn5150_info {
+	struct device *dev;
+	struct extcon_dev *edev;
+	struct i2c_client *i2c;
+	struct regmap *regmap;
+	struct gpio_desc *int_gpiod;
+	struct gpio_desc *vbus_gpiod;
+	int irq;
+	struct work_struct irq_work;
+	struct mutex mutex;
+};
+
+/* List of detectable cables */
+static const unsigned int ptn5150_extcon_cable[] = {
+	EXTCON_USB,
+	EXTCON_USB_HOST,
+	EXTCON_NONE,
+};
+
+static const struct regmap_config ptn5150_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= PTN5150_REG_END,
+};
+
+static void ptn5150_irq_work(struct work_struct *work)
+{
+	struct ptn5150_info *info = container_of(work,
+			struct ptn5150_info, irq_work);
+	int ret = 0;
+	unsigned int reg_data;
+	unsigned int int_status;
+
+	if (!info->edev)
+		return;
+
+	mutex_lock(&info->mutex);
+
+	ret = regmap_read(info->regmap, PTN5150_REG_CC_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev, "failed to read CC STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	/* Clear interrupt. Read would clear the register */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &int_status);
+	if (ret) {
+		dev_err(info->dev, "failed to read INT STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	if (int_status) {
+		unsigned int cable_attach;
+
+		cable_attach = int_status & PTN5150_REG_INT_CABLE_ATTACH_MASK;
+		if (cable_attach) {
+			unsigned int port_status;
+			unsigned int vbus;
+
+			port_status = ((reg_data &
+					PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
+					PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
+
+			switch (port_status) {
+			case PTN5150_DFP_ATTACHED:
+				extcon_set_state_sync(info->edev,
+						EXTCON_USB_HOST, false);
+				gpiod_set_value(info->vbus_gpiod, 0);
+				extcon_set_state_sync(info->edev, EXTCON_USB,
+						true);
+				break;
+			case PTN5150_UFP_ATTACHED:
+				extcon_set_state_sync(info->edev, EXTCON_USB,
+						false);
+				vbus = ((reg_data &
+					PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
+					PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
+				if (vbus)
+					gpiod_set_value(info->vbus_gpiod, 0);
+				else
+					gpiod_set_value(info->vbus_gpiod, 1);
+
+				extcon_set_state_sync(info->edev,
+						EXTCON_USB_HOST, true);
+				break;
+			default:
+				dev_err(info->dev,
+					"Unknown Port status : %x\n",
+					port_status);
+				break;
+			}
+		} else {
+			extcon_set_state_sync(info->edev,
+					EXTCON_USB_HOST, false);
+			extcon_set_state_sync(info->edev,
+					EXTCON_USB, false);
+			gpiod_set_value(info->vbus_gpiod, 0);
+		}
+	}
+
+	/* Clear interrupt. Read would clear the register */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS,
+			&int_status);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read INT REG STATUS %d\n", ret);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	mutex_unlock(&info->mutex);
+}
+
+
+static irqreturn_t ptn5150_irq_handler(int irq, void *data)
+{
+	struct ptn5150_info *info = data;
+
+	schedule_work(&info->irq_work);
+
+	return IRQ_HANDLED;
+}
+
+static int ptn5150_init_dev_type(struct ptn5150_info *info)
+{
+	unsigned int reg_data, vendor_id, version_id;
+	int ret;
+
+	ret = regmap_read(info->regmap, PTN5150_REG_DEVICE_ID, &reg_data);
+	if (ret) {
+		dev_err(info->dev, "failed to read DEVICE_ID %d\n", ret);
+		return -EINVAL;
+	}
+
+	vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
+				PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
+	version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
+				PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
+
+	dev_info(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
+			    version_id, vendor_id);
+
+	/* Clear any existing interrupts */
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read PTN5150_REG_INT_STATUS %d\n",
+			ret);
+		return -EINVAL;
+	}
+
+	ret = regmap_read(info->regmap, PTN5150_REG_INT_REG_STATUS, &reg_data);
+	if (ret) {
+		dev_err(info->dev,
+			"failed to read PTN5150_REG_INT_REG_STATUS %d\n", ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ptn5150_i2c_probe(struct i2c_client *i2c,
+				 const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct device_node *np = i2c->dev.of_node;
+	struct ptn5150_info *info;
+	int ret;
+
+	if (!np)
+		return -EINVAL;
+
+	info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+	i2c_set_clientdata(i2c, info);
+
+	info->dev = &i2c->dev;
+	info->i2c = i2c;
+	info->int_gpiod = devm_gpiod_get(&i2c->dev, "int", GPIOD_IN);
+	if (!info->int_gpiod) {
+		dev_err(dev, "failed to get INT GPIO\n");
+		return -EINVAL;
+	}
+	info->vbus_gpiod = devm_gpiod_get(&i2c->dev, "vbus", GPIOD_IN);
+	if (!info->vbus_gpiod) {
+		dev_err(dev, "failed to get VBUS GPIO\n");
+		return -EINVAL;
+	}
+	ret = gpiod_direction_output(info->vbus_gpiod, 0);
+	if (ret) {
+		dev_err(dev, "failed to set VBUS GPIO direction\n");
+		return -EINVAL;
+	}
+
+	mutex_init(&info->mutex);
+
+	INIT_WORK(&info->irq_work, ptn5150_irq_work);
+
+	info->regmap = devm_regmap_init_i2c(i2c, &ptn5150_regmap_config);
+	if (IS_ERR(info->regmap)) {
+		ret = PTR_ERR(info->regmap);
+		dev_err(info->dev, "failed to allocate register map: %d\n",
+				   ret);
+		return ret;
+	}
+
+	if (info->int_gpiod) {
+		info->irq = gpiod_to_irq(info->int_gpiod);
+		if (info->irq < 0) {
+			dev_err(dev, "failed to get INTB IRQ\n");
+			return info->irq;
+		}
+
+		ret = devm_request_threaded_irq(dev, info->irq, NULL,
+						ptn5150_irq_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						i2c->name, info);
+		if (ret < 0) {
+			dev_err(dev, "failed to request handler for INTB IRQ\n");
+			return ret;
+		}
+	}
+
+	/* Allocate extcon device */
+	info->edev = devm_extcon_dev_allocate(info->dev, ptn5150_extcon_cable);
+	if (IS_ERR(info->edev)) {
+		dev_err(info->dev, "failed to allocate memory for extcon\n");
+		return -ENOMEM;
+	}
+
+	/* Register extcon device */
+	ret = devm_extcon_dev_register(info->dev, info->edev);
+	if (ret) {
+		dev_err(info->dev, "failed to register extcon device\n");
+		return ret;
+	}
+
+	/* Initialize PTN5150 device and print vendor id and version id */
+	ret = ptn5150_init_dev_type(info);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct of_device_id ptn5150_dt_match[] = {
+	{ .compatible = "nxp,ptn5150" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ptn5150_dt_match);
+
+static const struct i2c_device_id ptn5150_i2c_id[] = {
+	{ "ptn5150", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ptn5150_i2c_id);
+
+static struct i2c_driver ptn5150_i2c_driver = {
+	.driver		= {
+		.name	= "ptn5150",
+		.of_match_table = ptn5150_dt_match,
+	},
+	.probe	= ptn5150_i2c_probe,
+	.id_table = ptn5150_i2c_id,
+};
+
+static int __init ptn5150_i2c_init(void)
+{
+	return i2c_add_driver(&ptn5150_i2c_driver);
+}
+subsys_initcall(ptn5150_i2c_init);
+
+MODULE_DESCRIPTION("NXP PTN5150 CC logic Extcon driver");
+MODULE_AUTHOR("Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>");
+MODULE_LICENSE("GPL v2");
--
2.7.4


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

end of thread, other threads:[~2019-01-24  6:08 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20190121091105epcas2p4bad949be391e855358295cf3eae8d773@epcas2p4.samsung.com>
2019-01-21  9:09 ` [PATCH] drivers: extcon: Add support for ptn5150 Vijai Kumar K
2019-01-22  0:05   ` kbuild test robot
2019-01-22  4:42     ` Vijai Kumar K
2019-01-22  5:27       ` Chanwoo Choi
2019-01-22  7:05         ` Vijai Kumar K
     [not found]     ` <CGME20190122044306epcas5p30f875260e568d5fb0e4909035060f8ff@epcms1p8>
2019-01-22  4:57       ` MyungJoo Ham
2019-01-22  6:57         ` Vijai Kumar K
2019-01-22  6:20   ` Chanwoo Choi
2019-01-22  7:55     ` Vijai Kumar K
2019-01-23 12:46       ` [PATCH V2] " Vijai Kumar K
2019-01-24  2:03         ` Chanwoo Choi
     [not found]         ` <CGME20190124060419epcas1p4aed00219fc0e0281c2d5435ce6ec2e48@epcas1p4.samsung.com>
2019-01-24  6:04           ` [PATCH v3] extcon: Add support for ptn5150 extcon driver Chanwoo Choi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).