All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.