[PATCH v1 1/2] extcon: intel: Split out some definitions to a common header

[PATCH v1 1/2] extcon: intel: Split out some definitions to a common header

[Andy Shevchenko]

We are going to use some definitions in the other Intel extcon drivers,
thus, split out them to a common header file.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 21 +++++++--------------
 drivers/extcon/extcon-intel.h        | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+), 14 deletions(-)
 create mode 100644 drivers/extcon/extcon-intel.h

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 5ef215297101..110bd38a4d24 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -17,6 +17,8 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
+#include "extcon-intel.h"
+
 #define CHT_WC_PHYCTRL			0x5e07
 
 #define CHT_WC_CHGRCTRL0		0x5e16
@@ -65,15 +67,6 @@
 #define CHT_WC_VBUS_GPIO_CTLO_DRV_OD	BIT(4)
 #define CHT_WC_VBUS_GPIO_CTLO_DIR_OUT	BIT(5)
 
-enum cht_wc_usb_id {
-	USB_ID_OTG,
-	USB_ID_GND,
-	USB_ID_FLOAT,
-	USB_RID_A,
-	USB_RID_B,
-	USB_RID_C,
-};
-
 enum cht_wc_mux_select {
 	MUX_SEL_PMIC = 0,
 	MUX_SEL_SOC,
@@ -101,9 +94,9 @@ static int cht_wc_extcon_get_id(struct cht_wc_extcon_data *ext, int pwrsrc_sts)
 {
 	switch ((pwrsrc_sts & CHT_WC_PWRSRC_USBID_MASK) >> CHT_WC_PWRSRC_USBID_SHIFT) {
 	case CHT_WC_PWRSRC_RID_GND:
-		return USB_ID_GND;
+		return INTEL_USB_ID_GND;
 	case CHT_WC_PWRSRC_RID_FLOAT:
-		return USB_ID_FLOAT;
+		return INTEL_USB_ID_FLOAT;
 	case CHT_WC_PWRSRC_RID_ACA:
 	default:
 		/*
@@ -111,7 +104,7 @@ static int cht_wc_extcon_get_id(struct cht_wc_extcon_data *ext, int pwrsrc_sts)
 		 * the USBID GPADC channel here and determine ACA role
 		 * based on that.
 		 */
-		return USB_ID_FLOAT;
+		return INTEL_USB_ID_FLOAT;
 	}
 }
 
@@ -221,7 +214,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
 	}
 
 	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
-	if (id == USB_ID_GND) {
+	if (id == INTEL_USB_ID_GND) {
 		/* The 5v boost causes a false VBUS / SDP detect, skip */
 		goto charger_det_done;
 	}
@@ -248,7 +241,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
 		ext->previous_cable = cable;
 	}
 
-	ext->usb_host = ((id == USB_ID_GND) || (id == USB_RID_A));
+	ext->usb_host = ((id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A));
 	extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host);
 }
 
diff --git a/drivers/extcon/extcon-intel.h b/drivers/extcon/extcon-intel.h
new file mode 100644
index 000000000000..455986b2b004
--- /dev/null
+++ b/drivers/extcon/extcon-intel.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Intel extcon hardware
+ *
+ * Copyright (C) 2018 Intel Corporation. All rights reserved.
+ */
+
+#ifndef __EXTCON_INTEL_H__
+#define __EXTCON_INTEL_H__
+
+enum extcon_intel_usb_id {
+	INTEL_USB_ID_OTG,
+	INTEL_USB_ID_GND,
+	INTEL_USB_ID_FLOAT,
+	INTEL_USB_RID_A,
+	INTEL_USB_RID_B,
+	INTEL_USB_RID_C,
+};
+
+#endif	/* __EXTCON_INTEL_H__ */
-- 
2.20.1

[PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC

[Andy Shevchenko]

TBD

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/extcon/Kconfig              |   7 +
 drivers/extcon/Makefile             |   1 +
 drivers/extcon/extcon-intel-mrfld.c | 256 ++++++++++++++++++++++++++++
 3 files changed, 264 insertions(+)
 create mode 100644 drivers/extcon/extcon-intel-mrfld.c

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 8e17149655f0..75349c6cc89e 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -60,6 +60,13 @@ config EXTCON_INTEL_CHT_WC
 	  Say Y here to enable extcon support for charger detection / control
 	  on the Intel Cherrytrail Whiskey Cove PMIC.
 
+config EXTCON_INTEL_MRFLD
+	tristate "Intel MErrifield Basin Cove PMIC extcon driver"
+	depends on INTEL_SOC_PMIC_MRFLD
+	help
+	  Say Y here to enable extcon support for charger detection / control
+	  on the Intel Merrifiel Basin Cove PMIC.
+
 config EXTCON_MAX14577
 	tristate "Maxim MAX14577/77836 EXTCON Support"
 	depends on MFD_MAX14577
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 261ce4cfe209..d3941a735df3 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
 obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
 obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o
 obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o
+obj-$(CONFIG_EXTCON_INTEL_MRFLD) += extcon-intel-mrfld.o
 obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
 obj-$(CONFIG_EXTCON_MAX3355)	+= extcon-max3355.o
 obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
diff --git a/drivers/extcon/extcon-intel-mrfld.c b/drivers/extcon/extcon-intel-mrfld.c
new file mode 100644
index 000000000000..d45db4975b5f
--- /dev/null
+++ b/drivers/extcon/extcon-intel-mrfld.c
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Extcon driver for Basin Cove PMIC
+ *
+ * Copyright (c) 2018, Intel Corporation.
+ * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ */
+
+#include <linux/extcon-provider.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/mfd/intel_soc_pmic_mrfld.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "extcon-intel.h"
+
+#define BCOVE_USBIDCTRL			0x19
+#define BCOVE_USBIDCTRL_ID		BIT(0)
+#define BCOVE_USBIDCTRL_ACA		BIT(1)
+#define BCOVE_USBIDCTRL_ALL	(BCOVE_USBIDCTRL_ID | BCOVE_USBIDCTRL_ACA)
+
+#define BCOVE_USBIDSTS			0x1a
+#define BCOVE_USBIDSTS_GND		BIT(0)
+#define BCOVE_USBIDSTS_RARBRC_MASK	GENMASK(2, 1)
+#define BCOVE_USBIDSTS_RARBRC_SHIFT	1
+#define BCOVE_USBIDSTS_NO_ACA		0
+#define BCOVE_USBIDSTS_R_ID_A		1
+#define BCOVE_USBIDSTS_R_ID_B		2
+#define BCOVE_USBIDSTS_R_ID_C		3
+#define BCOVE_USBIDSTS_FLOAT		BIT(3)
+#define BCOVE_USBIDSTS_SHORT		BIT(4)
+
+#define BCOVE_CHGRIRQ_ALL	(BCOVE_CHGRIRQ_VBUSDET | BCOVE_CHGRIRQ_DCDET | \
+				 BCOVE_CHGRIRQ_BATTDET | BCOVE_CHGRIRQ_USBIDDET)
+
+#define BCOVE_CHGRCTRL0			0x4b
+#define BCOVE_CHGRCTRL0_CHGRRESET	BIT(0)
+#define BCOVE_CHGRCTRL0_EMRGCHREN	BIT(1)
+#define BCOVE_CHGRCTRL0_EXTCHRDIS	BIT(2)
+#define BCOVE_CHGRCTRL0_SWCONTROL	BIT(3)
+#define BCOVE_CHGRCTRL0_TTLCK		BIT(4)
+#define BCOVE_CHGRCTRL0_BIT_5		BIT(5)
+#define BCOVE_CHGRCTRL0_BIT_6		BIT(6)
+#define BCOVE_CHGRCTRL0_CHR_WDT_NOKICK	BIT(7)
+
+struct mrfld_extcon_data {
+	struct device *dev;
+	struct regmap *regmap;
+	struct extcon_dev *edev;
+	unsigned int status;
+	unsigned int id;
+};
+
+static const unsigned int mrfld_extcon_cable[] = {
+	EXTCON_USB,
+	EXTCON_USB_HOST,
+	EXTCON_CHG_USB_SDP,
+	EXTCON_CHG_USB_CDP,
+	EXTCON_CHG_USB_DCP,
+	EXTCON_CHG_USB_ACA,
+	EXTCON_NONE,
+};
+
+static int mrfld_extcon_clear(struct mrfld_extcon_data *data, unsigned int reg,
+			      unsigned int mask)
+{
+	return regmap_update_bits(data->regmap, reg, mask, 0x00);
+}
+
+static int mrfld_extcon_set(struct mrfld_extcon_data *data, unsigned int reg,
+			    unsigned int mask)
+{
+	return regmap_update_bits(data->regmap, reg, mask, 0xff);
+}
+
+static int mrfld_extcon_get_id(struct mrfld_extcon_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	unsigned int id;
+	bool ground;
+	int ret;
+
+	ret = regmap_read(regmap, BCOVE_USBIDSTS, &id);
+	if (ret)
+		return ret;
+
+	if (id & BCOVE_USBIDSTS_FLOAT)
+		return INTEL_USB_ID_FLOAT;
+
+	switch ((id & BCOVE_USBIDSTS_RARBRC_MASK) >> BCOVE_USBIDSTS_RARBRC_SHIFT) {
+	case BCOVE_USBIDSTS_R_ID_A:
+		return INTEL_USB_RID_A;
+	case BCOVE_USBIDSTS_R_ID_B:
+		return INTEL_USB_RID_B;
+	case BCOVE_USBIDSTS_R_ID_C:
+		return INTEL_USB_RID_C;
+	}
+
+	/*
+	 * PMIC A0 reports USBIDSTS_GND = 1 for ID_GND,
+	 * but PMIC B0 reports USBIDSTS_GND = 0 for ID_GND.
+	 * Thus we must check this bit at last.
+	 */
+	ground = id & BCOVE_USBIDSTS_GND;
+	switch ('A' + BCOVE_MAJOR(data->id)) {
+	case 'A':
+		return ground ? INTEL_USB_ID_GND : INTEL_USB_ID_FLOAT;
+	case 'B':
+		return ground ? INTEL_USB_ID_FLOAT : INTEL_USB_ID_GND;
+	}
+
+	/* Unknown or unsupported type */
+	return INTEL_USB_ID_FLOAT;
+}
+
+static int mrfld_extcon_role_detect(struct mrfld_extcon_data *data)
+{
+	unsigned int id;
+	bool usb_host;
+	int ret;
+
+	ret = mrfld_extcon_get_id(data);
+	if (ret < 0)
+		return ret;
+
+	id = ret;
+
+	usb_host = (id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A);
+	extcon_set_state_sync(data->edev, EXTCON_USB_HOST, usb_host);
+
+	return 0;
+}
+
+static int mrfld_extcon_cable_detect(struct mrfld_extcon_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	unsigned int status;
+	int ret;
+
+	/*
+	 * It seems SCU firmware clears the content of BCOVE_CHGRIRQ1
+	 * and makes it useless for OS. Instead we compare a previously
+	 * stored status to the current one, provided by BCOVE_SCHGRIRQ1.
+	 */
+	ret = regmap_read(regmap, BCOVE_SCHGRIRQ1, &status);
+	if (ret)
+		return ret;
+
+	if (!(status ^ data->status))
+		return -ENODATA;
+
+	if ((status ^ data->status) & BCOVE_CHGRIRQ_USBIDDET)
+		ret = mrfld_extcon_role_detect(data);
+
+	data->status = status;
+	return ret;
+}
+
+static irqreturn_t mrfld_extcon_interrupt(int irq, void *dev_id)
+{
+	struct mrfld_extcon_data *data = dev_id;
+	int ret;
+
+	ret = mrfld_extcon_cable_detect(data);
+
+	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
+
+	return ret ? IRQ_NONE: IRQ_HANDLED;
+}
+
+static int mrfld_extcon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
+	struct regmap *regmap = pmic->regmap;
+	struct mrfld_extcon_data *data;
+	unsigned int id;
+	int irq, ret;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->dev = dev;
+	data->regmap = regmap;
+
+	data->edev = devm_extcon_dev_allocate(dev, mrfld_extcon_cable);
+	if (IS_ERR(data->edev))
+		return -ENOMEM;
+
+	ret = devm_extcon_dev_register(dev, data->edev);
+	if (ret < 0) {
+		dev_err(dev, "can't register extcon device: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, mrfld_extcon_interrupt,
+					IRQF_ONESHOT | IRQF_SHARED, pdev->name,
+					data);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(regmap, BCOVE_ID, &id);
+	if (ret)
+		return ret;
+
+	data->id = id;
+
+	mrfld_extcon_set(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);
+
+	/* Get initial state */
+	mrfld_extcon_role_detect(data);
+
+	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
+	mrfld_extcon_clear(data, BCOVE_MCHGRIRQ1, BCOVE_CHGRIRQ_ALL);
+
+	mrfld_extcon_set(data, BCOVE_USBIDCTRL, BCOVE_USBIDCTRL_ALL);
+
+	platform_set_drvdata(pdev, data);
+	return 0;
+}
+
+static int mrfld_extcon_remove(struct platform_device *pdev)
+{
+	struct mrfld_extcon_data *data = platform_get_drvdata(pdev);
+
+	mrfld_extcon_clear(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);
+	return 0;
+}
+
+static const struct platform_device_id mrfld_extcon_id_table[] = {
+	{ .name = "mrfld_bcove_extcon" },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, mrfld_extcon_id_table);
+
+static struct platform_driver mrfld_extcon_driver = {
+	.driver = {
+		.name	= KBUILD_MODNAME,
+	},
+	.probe		= mrfld_extcon_probe,
+	.remove		= mrfld_extcon_remove,
+	.id_table	= mrfld_extcon_id_table,
+};
+module_platform_driver(mrfld_extcon_driver);
+
+MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
+MODULE_DESCRIPTION("Extcon driver for Basin Cove PMIC");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

Re: [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header

[Chanwoo Choi]

Hi,

Looks good to me. But just there is minor one comment as following:
- 2018 -> 2019

On 19. 3. 18. 오후 6:52, Andy Shevchenko wrote:
> We are going to use some definitions in the other Intel extcon drivers,
> thus, split out them to a common header file.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/extcon/extcon-intel-cht-wc.c | 21 +++++++--------------
>  drivers/extcon/extcon-intel.h        | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/extcon/extcon-intel.h
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 5ef215297101..110bd38a4d24 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -17,6 +17,8 @@
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  
> +#include "extcon-intel.h"
> +
>  #define CHT_WC_PHYCTRL			0x5e07
>  
>  #define CHT_WC_CHGRCTRL0		0x5e16
> @@ -65,15 +67,6 @@
>  #define CHT_WC_VBUS_GPIO_CTLO_DRV_OD	BIT(4)
>  #define CHT_WC_VBUS_GPIO_CTLO_DIR_OUT	BIT(5)
>  
> -enum cht_wc_usb_id {
> -	USB_ID_OTG,
> -	USB_ID_GND,
> -	USB_ID_FLOAT,
> -	USB_RID_A,
> -	USB_RID_B,
> -	USB_RID_C,
> -};
> -
>  enum cht_wc_mux_select {
>  	MUX_SEL_PMIC = 0,
>  	MUX_SEL_SOC,
> @@ -101,9 +94,9 @@ static int cht_wc_extcon_get_id(struct cht_wc_extcon_data *ext, int pwrsrc_sts)
>  {
>  	switch ((pwrsrc_sts & CHT_WC_PWRSRC_USBID_MASK) >> CHT_WC_PWRSRC_USBID_SHIFT) {
>  	case CHT_WC_PWRSRC_RID_GND:
> -		return USB_ID_GND;
> +		return INTEL_USB_ID_GND;
>  	case CHT_WC_PWRSRC_RID_FLOAT:
> -		return USB_ID_FLOAT;
> +		return INTEL_USB_ID_FLOAT;
>  	case CHT_WC_PWRSRC_RID_ACA:
>  	default:
>  		/*
> @@ -111,7 +104,7 @@ static int cht_wc_extcon_get_id(struct cht_wc_extcon_data *ext, int pwrsrc_sts)
>  		 * the USBID GPADC channel here and determine ACA role
>  		 * based on that.
>  		 */
> -		return USB_ID_FLOAT;
> +		return INTEL_USB_ID_FLOAT;
>  	}
>  }
>  
> @@ -221,7 +214,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
>  	}
>  
>  	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
> -	if (id == USB_ID_GND) {
> +	if (id == INTEL_USB_ID_GND) {
>  		/* The 5v boost causes a false VBUS / SDP detect, skip */
>  		goto charger_det_done;
>  	}
> @@ -248,7 +241,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
>  		ext->previous_cable = cable;
>  	}
>  
> -	ext->usb_host = ((id == USB_ID_GND) || (id == USB_RID_A));
> +	ext->usb_host = ((id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A));
>  	extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host);
>  }
>  
> diff --git a/drivers/extcon/extcon-intel.h b/drivers/extcon/extcon-intel.h
> new file mode 100644
> index 000000000000..455986b2b004
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for Intel extcon hardware
> + *
> + * Copyright (C) 2018 Intel Corporation. All rights reserved.

2018 -> 2019

> + */
> +
> +#ifndef __EXTCON_INTEL_H__
> +#define __EXTCON_INTEL_H__
> +
> +enum extcon_intel_usb_id {
> +	INTEL_USB_ID_OTG,
> +	INTEL_USB_ID_GND,
> +	INTEL_USB_ID_FLOAT,
> +	INTEL_USB_RID_A,
> +	INTEL_USB_RID_B,
> +	INTEL_USB_RID_C,
> +};
> +
> +#endif	/* __EXTCON_INTEL_H__ */
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC

[Andy Shevchenko]

On Mon, Mar 18, 2019 at 12:52:25PM +0300, Andy Shevchenko wrote:
> TBD

Oops.
I though I have written it already.

I will wait for other comments today and sent a new version with commit message
filled as follows:

On Intel Merrifield the Basin Cove PMIC provides a feature to detect
the USB connection type. This driver utilizes the feature in order to support
the USB dual role detection.

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/extcon/Kconfig              |   7 +
>  drivers/extcon/Makefile             |   1 +
>  drivers/extcon/extcon-intel-mrfld.c | 256 ++++++++++++++++++++++++++++
>  3 files changed, 264 insertions(+)
>  create mode 100644 drivers/extcon/extcon-intel-mrfld.c
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 8e17149655f0..75349c6cc89e 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -60,6 +60,13 @@ config EXTCON_INTEL_CHT_WC
>  	  Say Y here to enable extcon support for charger detection / control
>  	  on the Intel Cherrytrail Whiskey Cove PMIC.
>  
> +config EXTCON_INTEL_MRFLD

> +	tristate "Intel MErrifield Basin Cove PMIC extcon driver"

ME -> Me (will be fixed)

> +	depends on INTEL_SOC_PMIC_MRFLD
> +	help
> +	  Say Y here to enable extcon support for charger detection / control
> +	  on the Intel Merrifiel Basin Cove PMIC.
> +
>  config EXTCON_MAX14577
>  	tristate "Maxim MAX14577/77836 EXTCON Support"
>  	depends on MFD_MAX14577
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 261ce4cfe209..d3941a735df3 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
>  obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
>  obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o
>  obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o
> +obj-$(CONFIG_EXTCON_INTEL_MRFLD) += extcon-intel-mrfld.o
>  obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
>  obj-$(CONFIG_EXTCON_MAX3355)	+= extcon-max3355.o
>  obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
> diff --git a/drivers/extcon/extcon-intel-mrfld.c b/drivers/extcon/extcon-intel-mrfld.c
> new file mode 100644
> index 000000000000..d45db4975b5f
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-mrfld.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Extcon driver for Basin Cove PMIC
> + *
> + * Copyright (c) 2018, Intel Corporation.

2019 I suppose :-)

> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> + */
> +
> +#include <linux/extcon-provider.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/mfd/intel_soc_pmic_mrfld.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "extcon-intel.h"
> +
> +#define BCOVE_USBIDCTRL			0x19
> +#define BCOVE_USBIDCTRL_ID		BIT(0)
> +#define BCOVE_USBIDCTRL_ACA		BIT(1)
> +#define BCOVE_USBIDCTRL_ALL	(BCOVE_USBIDCTRL_ID | BCOVE_USBIDCTRL_ACA)
> +
> +#define BCOVE_USBIDSTS			0x1a
> +#define BCOVE_USBIDSTS_GND		BIT(0)
> +#define BCOVE_USBIDSTS_RARBRC_MASK	GENMASK(2, 1)
> +#define BCOVE_USBIDSTS_RARBRC_SHIFT	1
> +#define BCOVE_USBIDSTS_NO_ACA		0
> +#define BCOVE_USBIDSTS_R_ID_A		1
> +#define BCOVE_USBIDSTS_R_ID_B		2
> +#define BCOVE_USBIDSTS_R_ID_C		3
> +#define BCOVE_USBIDSTS_FLOAT		BIT(3)
> +#define BCOVE_USBIDSTS_SHORT		BIT(4)
> +
> +#define BCOVE_CHGRIRQ_ALL	(BCOVE_CHGRIRQ_VBUSDET | BCOVE_CHGRIRQ_DCDET | \
> +				 BCOVE_CHGRIRQ_BATTDET | BCOVE_CHGRIRQ_USBIDDET)
> +
> +#define BCOVE_CHGRCTRL0			0x4b
> +#define BCOVE_CHGRCTRL0_CHGRRESET	BIT(0)
> +#define BCOVE_CHGRCTRL0_EMRGCHREN	BIT(1)
> +#define BCOVE_CHGRCTRL0_EXTCHRDIS	BIT(2)
> +#define BCOVE_CHGRCTRL0_SWCONTROL	BIT(3)
> +#define BCOVE_CHGRCTRL0_TTLCK		BIT(4)
> +#define BCOVE_CHGRCTRL0_BIT_5		BIT(5)
> +#define BCOVE_CHGRCTRL0_BIT_6		BIT(6)
> +#define BCOVE_CHGRCTRL0_CHR_WDT_NOKICK	BIT(7)
> +
> +struct mrfld_extcon_data {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct extcon_dev *edev;
> +	unsigned int status;
> +	unsigned int id;
> +};
> +
> +static const unsigned int mrfld_extcon_cable[] = {
> +	EXTCON_USB,
> +	EXTCON_USB_HOST,
> +	EXTCON_CHG_USB_SDP,
> +	EXTCON_CHG_USB_CDP,
> +	EXTCON_CHG_USB_DCP,
> +	EXTCON_CHG_USB_ACA,
> +	EXTCON_NONE,
> +};
> +
> +static int mrfld_extcon_clear(struct mrfld_extcon_data *data, unsigned int reg,
> +			      unsigned int mask)
> +{
> +	return regmap_update_bits(data->regmap, reg, mask, 0x00);
> +}
> +
> +static int mrfld_extcon_set(struct mrfld_extcon_data *data, unsigned int reg,
> +			    unsigned int mask)
> +{
> +	return regmap_update_bits(data->regmap, reg, mask, 0xff);
> +}
> +
> +static int mrfld_extcon_get_id(struct mrfld_extcon_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	unsigned int id;
> +	bool ground;
> +	int ret;
> +
> +	ret = regmap_read(regmap, BCOVE_USBIDSTS, &id);
> +	if (ret)
> +		return ret;
> +
> +	if (id & BCOVE_USBIDSTS_FLOAT)
> +		return INTEL_USB_ID_FLOAT;
> +
> +	switch ((id & BCOVE_USBIDSTS_RARBRC_MASK) >> BCOVE_USBIDSTS_RARBRC_SHIFT) {
> +	case BCOVE_USBIDSTS_R_ID_A:
> +		return INTEL_USB_RID_A;
> +	case BCOVE_USBIDSTS_R_ID_B:
> +		return INTEL_USB_RID_B;
> +	case BCOVE_USBIDSTS_R_ID_C:
> +		return INTEL_USB_RID_C;
> +	}
> +
> +	/*
> +	 * PMIC A0 reports USBIDSTS_GND = 1 for ID_GND,
> +	 * but PMIC B0 reports USBIDSTS_GND = 0 for ID_GND.
> +	 * Thus we must check this bit at last.
> +	 */
> +	ground = id & BCOVE_USBIDSTS_GND;
> +	switch ('A' + BCOVE_MAJOR(data->id)) {
> +	case 'A':
> +		return ground ? INTEL_USB_ID_GND : INTEL_USB_ID_FLOAT;
> +	case 'B':
> +		return ground ? INTEL_USB_ID_FLOAT : INTEL_USB_ID_GND;
> +	}
> +
> +	/* Unknown or unsupported type */
> +	return INTEL_USB_ID_FLOAT;
> +}
> +
> +static int mrfld_extcon_role_detect(struct mrfld_extcon_data *data)
> +{
> +	unsigned int id;
> +	bool usb_host;
> +	int ret;
> +
> +	ret = mrfld_extcon_get_id(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	id = ret;
> +
> +	usb_host = (id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A);
> +	extcon_set_state_sync(data->edev, EXTCON_USB_HOST, usb_host);
> +
> +	return 0;
> +}
> +
> +static int mrfld_extcon_cable_detect(struct mrfld_extcon_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	unsigned int status;
> +	int ret;
> +
> +	/*
> +	 * It seems SCU firmware clears the content of BCOVE_CHGRIRQ1
> +	 * and makes it useless for OS. Instead we compare a previously
> +	 * stored status to the current one, provided by BCOVE_SCHGRIRQ1.
> +	 */
> +	ret = regmap_read(regmap, BCOVE_SCHGRIRQ1, &status);
> +	if (ret)
> +		return ret;
> +
> +	if (!(status ^ data->status))
> +		return -ENODATA;
> +
> +	if ((status ^ data->status) & BCOVE_CHGRIRQ_USBIDDET)
> +		ret = mrfld_extcon_role_detect(data);
> +
> +	data->status = status;
> +	return ret;
> +}
> +
> +static irqreturn_t mrfld_extcon_interrupt(int irq, void *dev_id)
> +{
> +	struct mrfld_extcon_data *data = dev_id;
> +	int ret;
> +
> +	ret = mrfld_extcon_cable_detect(data);
> +
> +	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
> +
> +	return ret ? IRQ_NONE: IRQ_HANDLED;
> +}
> +
> +static int mrfld_extcon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
> +	struct regmap *regmap = pmic->regmap;
> +	struct mrfld_extcon_data *data;
> +	unsigned int id;
> +	int irq, ret;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->dev = dev;
> +	data->regmap = regmap;
> +
> +	data->edev = devm_extcon_dev_allocate(dev, mrfld_extcon_cable);
> +	if (IS_ERR(data->edev))
> +		return -ENOMEM;
> +
> +	ret = devm_extcon_dev_register(dev, data->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "can't register extcon device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, mrfld_extcon_interrupt,
> +					IRQF_ONESHOT | IRQF_SHARED, pdev->name,
> +					data);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(regmap, BCOVE_ID, &id);
> +	if (ret)
> +		return ret;
> +
> +	data->id = id;
> +
> +	mrfld_extcon_set(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);
> +
> +	/* Get initial state */
> +	mrfld_extcon_role_detect(data);
> +
> +	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
> +	mrfld_extcon_clear(data, BCOVE_MCHGRIRQ1, BCOVE_CHGRIRQ_ALL);
> +
> +	mrfld_extcon_set(data, BCOVE_USBIDCTRL, BCOVE_USBIDCTRL_ALL);
> +
> +	platform_set_drvdata(pdev, data);
> +	return 0;
> +}
> +
> +static int mrfld_extcon_remove(struct platform_device *pdev)
> +{
> +	struct mrfld_extcon_data *data = platform_get_drvdata(pdev);
> +
> +	mrfld_extcon_clear(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);
> +	return 0;
> +}
> +
> +static const struct platform_device_id mrfld_extcon_id_table[] = {
> +	{ .name = "mrfld_bcove_extcon" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(platform, mrfld_extcon_id_table);
> +
> +static struct platform_driver mrfld_extcon_driver = {
> +	.driver = {
> +		.name	= KBUILD_MODNAME,
> +	},
> +	.probe		= mrfld_extcon_probe,
> +	.remove		= mrfld_extcon_remove,
> +	.id_table	= mrfld_extcon_id_table,
> +};
> +module_platform_driver(mrfld_extcon_driver);
> +
> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
> +MODULE_DESCRIPTION("Extcon driver for Basin Cove PMIC");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC

[Chanwoo Choi]

Hi Andy,

Thanks for comment. I add my comments
and then you have to rebase it on latest v5.0-rc1
because the merge conflict happen on v5.0-rc1.

On 19. 3. 18. 오후 7:11, Andy Shevchenko wrote:
> On Mon, Mar 18, 2019 at 12:52:25PM +0300, Andy Shevchenko wrote:
>> TBD
> 
> Oops.
> I though I have written it already.
> 
> I will wait for other comments today and sent a new version with commit message
> filled as follows:
> 
> On Intel Merrifield the Basin Cove PMIC provides a feature to detect
> the USB connection type. This driver utilizes the feature in order to support
> the USB dual role detection.
> 
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/extcon/Kconfig              |   7 +
>>  drivers/extcon/Makefile             |   1 +
>>  drivers/extcon/extcon-intel-mrfld.c | 256 ++++++++++++++++++++++++++++
>>  3 files changed, 264 insertions(+)
>>  create mode 100644 drivers/extcon/extcon-intel-mrfld.c
>>
>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>> index 8e17149655f0..75349c6cc89e 100644
>> --- a/drivers/extcon/Kconfig
>> +++ b/drivers/extcon/Kconfig
>> @@ -60,6 +60,13 @@ config EXTCON_INTEL_CHT_WC
>>  	  Say Y here to enable extcon support for charger detection / control
>>  	  on the Intel Cherrytrail Whiskey Cove PMIC.
>>  
>> +config EXTCON_INTEL_MRFLD
> 
>> +	tristate "Intel MErrifield Basin Cove PMIC extcon driver"
> 
> ME -> Me (will be fixed)
> 
>> +	depends on INTEL_SOC_PMIC_MRFLD

This driver uses the regmap interface. So, you better to add
following dependency?
- select REGMAP_I2C or REGMAP_SPI

But, if 'INTEL_SOC_PMIC_MRFLE' selects already REGMAP_*
configuration. It is not necessary.

>> +	help
>> +	  Say Y here to enable extcon support for charger detection / control
>> +	  on the Intel Merrifiel Basin Cove PMIC.

What is correct word?
- Merrifield? is used on above
- Merrifiel?


>> +
>>  config EXTCON_MAX14577
>>  	tristate "Maxim MAX14577/77836 EXTCON Support"
>>  	depends on MFD_MAX14577
>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> index 261ce4cfe209..d3941a735df3 100644
>> --- a/drivers/extcon/Makefile
>> +++ b/drivers/extcon/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
>>  obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
>>  obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o
>>  obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o
>> +obj-$(CONFIG_EXTCON_INTEL_MRFLD) += extcon-intel-mrfld.o
>>  obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
>>  obj-$(CONFIG_EXTCON_MAX3355)	+= extcon-max3355.o
>>  obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>> diff --git a/drivers/extcon/extcon-intel-mrfld.c b/drivers/extcon/extcon-intel-mrfld.c
>> new file mode 100644
>> index 000000000000..d45db4975b5f
>> --- /dev/null
>> +++ b/drivers/extcon/extcon-intel-mrfld.c
>> @@ -0,0 +1,256 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Extcon driver for Basin Cove PMIC
>> + *
>> + * Copyright (c) 2018, Intel Corporation.
> 
> 2019 I suppose :-)

Right.

> 
>> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> + */
>> +
>> +#include <linux/extcon-provider.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/mfd/intel_soc_pmic_mrfld.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "extcon-intel.h"
>> +
>> +#define BCOVE_USBIDCTRL			0x19
>> +#define BCOVE_USBIDCTRL_ID		BIT(0)
>> +#define BCOVE_USBIDCTRL_ACA		BIT(1)
>> +#define BCOVE_USBIDCTRL_ALL	(BCOVE_USBIDCTRL_ID | BCOVE_USBIDCTRL_ACA)
>> +
>> +#define BCOVE_USBIDSTS			0x1a
>> +#define BCOVE_USBIDSTS_GND		BIT(0)
>> +#define BCOVE_USBIDSTS_RARBRC_MASK	GENMASK(2, 1)
>> +#define BCOVE_USBIDSTS_RARBRC_SHIFT	1
>> +#define BCOVE_USBIDSTS_NO_ACA		0
>> +#define BCOVE_USBIDSTS_R_ID_A		1
>> +#define BCOVE_USBIDSTS_R_ID_B		2
>> +#define BCOVE_USBIDSTS_R_ID_C		3
>> +#define BCOVE_USBIDSTS_FLOAT		BIT(3)
>> +#define BCOVE_USBIDSTS_SHORT		BIT(4)
>> +
>> +#define BCOVE_CHGRIRQ_ALL	(BCOVE_CHGRIRQ_VBUSDET | BCOVE_CHGRIRQ_DCDET | \
>> +				 BCOVE_CHGRIRQ_BATTDET | BCOVE_CHGRIRQ_USBIDDET)
>> +
>> +#define BCOVE_CHGRCTRL0			0x4b
>> +#define BCOVE_CHGRCTRL0_CHGRRESET	BIT(0)
>> +#define BCOVE_CHGRCTRL0_EMRGCHREN	BIT(1)
>> +#define BCOVE_CHGRCTRL0_EXTCHRDIS	BIT(2)
>> +#define BCOVE_CHGRCTRL0_SWCONTROL	BIT(3)
>> +#define BCOVE_CHGRCTRL0_TTLCK		BIT(4)
>> +#define BCOVE_CHGRCTRL0_BIT_5		BIT(5)
>> +#define BCOVE_CHGRCTRL0_BIT_6		BIT(6)
>> +#define BCOVE_CHGRCTRL0_CHR_WDT_NOKICK	BIT(7)
>> +
>> +struct mrfld_extcon_data {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct extcon_dev *edev;
>> +	unsigned int status;
>> +	unsigned int id;
>> +};
>> +
>> +static const unsigned int mrfld_extcon_cable[] = {
>> +	EXTCON_USB,
>> +	EXTCON_USB_HOST,
>> +	EXTCON_CHG_USB_SDP,
>> +	EXTCON_CHG_USB_CDP,
>> +	EXTCON_CHG_USB_DCP,
>> +	EXTCON_CHG_USB_ACA,
>> +	EXTCON_NONE,
>> +};
>> +
>> +static int mrfld_extcon_clear(struct mrfld_extcon_data *data, unsigned int reg,
>> +			      unsigned int mask)
>> +{
>> +	return regmap_update_bits(data->regmap, reg, mask, 0x00);
>> +}
>> +
>> +static int mrfld_extcon_set(struct mrfld_extcon_data *data, unsigned int reg,
>> +			    unsigned int mask)
>> +{
>> +	return regmap_update_bits(data->regmap, reg, mask, 0xff);
>> +}

mrfld_extcon_clear() and mrfld_extcon_set() are just wrapper function
for regmap interface. I think that you better to define
the meaningful defintion for '0x00' and '0xff' as following:

(just example, you may make the more correct name)
#define INTEL_MRFLD_RESET	0x00
#define INTEL_MRFLD_SET		0xff

And then you better to use the 'regmap_update_bits()' function
directly instead of mrfld_extcon_clear/set'.

Also, you should handle the exception hanlding when using regmap function.

>> +
>> +static int mrfld_extcon_get_id(struct mrfld_extcon_data *data)
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	unsigned int id;
>> +	bool ground;
>> +	int ret;
>> +
>> +	ret = regmap_read(regmap, BCOVE_USBIDSTS, &id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (id & BCOVE_USBIDSTS_FLOAT)
>> +		return INTEL_USB_ID_FLOAT;
>> +
>> +	switch ((id & BCOVE_USBIDSTS_RARBRC_MASK) >> BCOVE_USBIDSTS_RARBRC_SHIFT) {
>> +	case BCOVE_USBIDSTS_R_ID_A:
>> +		return INTEL_USB_RID_A;
>> +	case BCOVE_USBIDSTS_R_ID_B:
>> +		return INTEL_USB_RID_B;
>> +	case BCOVE_USBIDSTS_R_ID_C:
>> +		return INTEL_USB_RID_C;

Please add 'default' statement for exception handling.

>> +	}
>> +
>> +	/*
>> +	 * PMIC A0 reports USBIDSTS_GND = 1 for ID_GND,
>> +	 * but PMIC B0 reports USBIDSTS_GND = 0 for ID_GND.
>> +	 * Thus we must check this bit at last.
>> +	 */
>> +	ground = id & BCOVE_USBIDSTS_GND;
>> +	switch ('A' + BCOVE_MAJOR(data->id)) {
>> +	case 'A':
>> +		return ground ? INTEL_USB_ID_GND : INTEL_USB_ID_FLOAT;
>> +	case 'B':
>> +		return ground ? INTEL_USB_ID_FLOAT : INTEL_USB_ID_GND;
>> +	}
>> +
>> +	/* Unknown or unsupported type */
>> +	return INTEL_USB_ID_FLOAT;
>> +}
>> +
>> +static int mrfld_extcon_role_detect(struct mrfld_extcon_data *data)
>> +{
>> +	unsigned int id;
>> +	bool usb_host;
>> +	int ret;>> +
>> +	ret = mrfld_extcon_get_id(data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	id = ret;
>> +
>> +	usb_host = (id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A);
>> +	extcon_set_state_sync(data->edev, EXTCON_USB_HOST, usb_host);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mrfld_extcon_cable_detect(struct mrfld_extcon_data *data)
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	unsigned int status;
>> +	int ret;
>> +
>> +	/*
>> +	 * It seems SCU firmware clears the content of BCOVE_CHGRIRQ1
>> +	 * and makes it useless for OS. Instead we compare a previously
>> +	 * stored status to the current one, provided by BCOVE_SCHGRIRQ1.
>> +	 */
>> +	ret = regmap_read(regmap, BCOVE_SCHGRIRQ1, &status);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!(status ^ data->status))
>> +		return -ENODATA;
>> +
>> +	if ((status ^ data->status) & BCOVE_CHGRIRQ_USBIDDET)
>> +		ret = mrfld_extcon_role_detect(data);
This line gets the return value from mrfld_extcon_role_detect(data)
without any error handling and then the below line just saves 'status'
to 'data->status' regardless of 'ret' value.

I think that you have to handle the error case of
'ret = mrfld_extcon_role_detect(data)'.

>> +
>> +	data->status = status;

nitpick. better to add one blank line.

>> +	return ret;
>> +}
>> +
>> +static irqreturn_t mrfld_extcon_interrupt(int irq, void *dev_id)
>> +{
>> +	struct mrfld_extcon_data *data = dev_id;
>> +	int ret;
>> +
>> +	ret = mrfld_extcon_cable_detect(data);
>> +
>> +	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
>> +
>> +	return ret ? IRQ_NONE: IRQ_HANDLED;
>> +}
>> +
>> +static int mrfld_extcon_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
>> +	struct regmap *regmap = pmic->regmap;
>> +	struct mrfld_extcon_data *data;
>> +	unsigned int id;
>> +	int irq, ret;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->dev = dev;
>> +	data->regmap = regmap;
>> +
>> +	data->edev = devm_extcon_dev_allocate(dev, mrfld_extcon_cable);
>> +	if (IS_ERR(data->edev))
>> +		return -ENOMEM;
>> +
>> +	ret = devm_extcon_dev_register(dev, data->edev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "can't register extcon device: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(dev, irq, NULL, mrfld_extcon_interrupt,
>> +					IRQF_ONESHOT | IRQF_SHARED, pdev->name,
>> +					data);
>> +	if (ret)
>> +		return ret;

You better add the error log with dev_err.

>> +
>> +	ret = regmap_read(regmap, BCOVE_ID, &id);
>> +	if (ret)
>> +		return ret;

ditto for error log.

>> +
>> +	data->id = id;
>> +
>> +	mrfld_extcon_set(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);
>> +
>> +	/* Get initial state */
>> +	mrfld_extcon_role_detect(data);

Please handle the return value for exception handling with log.

>> +
>> +	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
>> +	mrfld_extcon_clear(data, BCOVE_MCHGRIRQ1, BCOVE_CHGRIRQ_ALL);
>> +
>> +	mrfld_extcon_set(data, BCOVE_USBIDCTRL, BCOVE_USBIDCTRL_ALL);
>> +
>> +	platform_set_drvdata(pdev, data);

nitpick. better to add one blank line.

>> +	return 0;
>> +}
>> +
>> +static int mrfld_extcon_remove(struct platform_device *pdev)
>> +{
>> +	struct mrfld_extcon_data *data = platform_get_drvdata(pdev);
>> +
>> +	mrfld_extcon_clear(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);

nitpick. better to add one blank line.

>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id mrfld_extcon_id_table[] = {
>> +	{ .name = "mrfld_bcove_extcon" },

I think that it is not proper to use 'extcon' word for the compatible name
because 'extcon' word is linuxium. So, I recommend that you remove
the 'extcon' word. Instead, you better to use new word related to h/w.

>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(platform, mrfld_extcon_id_table);
>> +
>> +static struct platform_driver mrfld_extcon_driver = {
>> +	.driver = {
>> +		.name	= KBUILD_MODNAME,

Where is the definition of KBUILD_MODNAME? Are you missing?

>> +	},
>> +	.probe		= mrfld_extcon_probe,
>> +	.remove		= mrfld_extcon_remove,
>> +	.id_table	= mrfld_extcon_id_table,
>> +};
>> +module_platform_driver(mrfld_extcon_driver);
>> +
>> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
>> +MODULE_DESCRIPTION("Extcon driver for Basin Cove PMIC");

Add the 'Merrifield' word in front of 'Basin Cove PMIC' as following:
- Merrifield Basic Cove PMIC

>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.20.1
>>
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC

[Chanwoo Choi]

Hi Andy,

On 19. 3. 18. 오후 7:38, Chanwoo Choi wrote:
> Hi Andy,
> 
> Thanks for comment. I add my comments
> and then you have to rebase it on latest v5.0-rc1
> because the merge conflict happen on v5.0-rc1.

Please rebase the extcon-next branch instead of v5.0-rc1.

> 
> On 19. 3. 18. 오후 7:11, Andy Shevchenko wrote:
>> On Mon, Mar 18, 2019 at 12:52:25PM +0300, Andy Shevchenko wrote:
>>> TBD
>>
>> Oops.
>> I though I have written it already.
>>
>> I will wait for other comments today and sent a new version with commit message
>> filled as follows:
>>
>> On Intel Merrifield the Basin Cove PMIC provides a feature to detect
>> the USB connection type. This driver utilizes the feature in order to support
>> the USB dual role detection.
>>
>>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>  drivers/extcon/Kconfig              |   7 +
>>>  drivers/extcon/Makefile             |   1 +
>>>  drivers/extcon/extcon-intel-mrfld.c | 256 ++++++++++++++++++++++++++++
>>>  3 files changed, 264 insertions(+)
>>>  create mode 100644 drivers/extcon/extcon-intel-mrfld.c
>>>
>>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>>> index 8e17149655f0..75349c6cc89e 100644
>>> --- a/drivers/extcon/Kconfig
>>> +++ b/drivers/extcon/Kconfig
>>> @@ -60,6 +60,13 @@ config EXTCON_INTEL_CHT_WC
>>>  	  Say Y here to enable extcon support for charger detection / control
>>>  	  on the Intel Cherrytrail Whiskey Cove PMIC.
>>>  
>>> +config EXTCON_INTEL_MRFLD
>>
>>> +	tristate "Intel MErrifield Basin Cove PMIC extcon driver"
>>
>> ME -> Me (will be fixed)
>>
>>> +	depends on INTEL_SOC_PMIC_MRFLD
> 
> This driver uses the regmap interface. So, you better to add
> following dependency?
> - select REGMAP_I2C or REGMAP_SPI
> 
> But, if 'INTEL_SOC_PMIC_MRFLE' selects already REGMAP_*
> configuration. It is not necessary.
> 
>>> +	help
>>> +	  Say Y here to enable extcon support for charger detection / control
>>> +	  on the Intel Merrifiel Basin Cove PMIC.
> 
> What is correct word?
> - Merrifield? is used on above
> - Merrifiel?
> 
> 
>>> +
>>>  config EXTCON_MAX14577
>>>  	tristate "Maxim MAX14577/77836 EXTCON Support"
>>>  	depends on MFD_MAX14577
>>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>>> index 261ce4cfe209..d3941a735df3 100644
>>> --- a/drivers/extcon/Makefile
>>> +++ b/drivers/extcon/Makefile
>>> @@ -11,6 +11,7 @@ obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
>>>  obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
>>>  obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o
>>>  obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o
>>> +obj-$(CONFIG_EXTCON_INTEL_MRFLD) += extcon-intel-mrfld.o
>>>  obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
>>>  obj-$(CONFIG_EXTCON_MAX3355)	+= extcon-max3355.o
>>>  obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>>> diff --git a/drivers/extcon/extcon-intel-mrfld.c b/drivers/extcon/extcon-intel-mrfld.c
>>> new file mode 100644
>>> index 000000000000..d45db4975b5f
>>> --- /dev/null
>>> +++ b/drivers/extcon/extcon-intel-mrfld.c
>>> @@ -0,0 +1,256 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Extcon driver for Basin Cove PMIC
>>> + *
>>> + * Copyright (c) 2018, Intel Corporation.
>>
>> 2019 I suppose :-)
> 
> Right.
> 
>>
>>> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> + */
>>> +
>>> +#include <linux/extcon-provider.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/mfd/intel_soc_pmic.h>
>>> +#include <linux/mfd/intel_soc_pmic_mrfld.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +#include "extcon-intel.h"
>>> +
>>> +#define BCOVE_USBIDCTRL			0x19
>>> +#define BCOVE_USBIDCTRL_ID		BIT(0)
>>> +#define BCOVE_USBIDCTRL_ACA		BIT(1)
>>> +#define BCOVE_USBIDCTRL_ALL	(BCOVE_USBIDCTRL_ID | BCOVE_USBIDCTRL_ACA)
>>> +
>>> +#define BCOVE_USBIDSTS			0x1a
>>> +#define BCOVE_USBIDSTS_GND		BIT(0)
>>> +#define BCOVE_USBIDSTS_RARBRC_MASK	GENMASK(2, 1)
>>> +#define BCOVE_USBIDSTS_RARBRC_SHIFT	1
>>> +#define BCOVE_USBIDSTS_NO_ACA		0
>>> +#define BCOVE_USBIDSTS_R_ID_A		1
>>> +#define BCOVE_USBIDSTS_R_ID_B		2
>>> +#define BCOVE_USBIDSTS_R_ID_C		3
>>> +#define BCOVE_USBIDSTS_FLOAT		BIT(3)
>>> +#define BCOVE_USBIDSTS_SHORT		BIT(4)
>>> +
>>> +#define BCOVE_CHGRIRQ_ALL	(BCOVE_CHGRIRQ_VBUSDET | BCOVE_CHGRIRQ_DCDET | \
>>> +				 BCOVE_CHGRIRQ_BATTDET | BCOVE_CHGRIRQ_USBIDDET)
>>> +
>>> +#define BCOVE_CHGRCTRL0			0x4b
>>> +#define BCOVE_CHGRCTRL0_CHGRRESET	BIT(0)
>>> +#define BCOVE_CHGRCTRL0_EMRGCHREN	BIT(1)
>>> +#define BCOVE_CHGRCTRL0_EXTCHRDIS	BIT(2)
>>> +#define BCOVE_CHGRCTRL0_SWCONTROL	BIT(3)
>>> +#define BCOVE_CHGRCTRL0_TTLCK		BIT(4)
>>> +#define BCOVE_CHGRCTRL0_BIT_5		BIT(5)
>>> +#define BCOVE_CHGRCTRL0_BIT_6		BIT(6)
>>> +#define BCOVE_CHGRCTRL0_CHR_WDT_NOKICK	BIT(7)
>>> +
>>> +struct mrfld_extcon_data {
>>> +	struct device *dev;
>>> +	struct regmap *regmap;
>>> +	struct extcon_dev *edev;
>>> +	unsigned int status;
>>> +	unsigned int id;
>>> +};
>>> +
>>> +static const unsigned int mrfld_extcon_cable[] = {
>>> +	EXTCON_USB,
>>> +	EXTCON_USB_HOST,
>>> +	EXTCON_CHG_USB_SDP,
>>> +	EXTCON_CHG_USB_CDP,
>>> +	EXTCON_CHG_USB_DCP,
>>> +	EXTCON_CHG_USB_ACA,
>>> +	EXTCON_NONE,
>>> +};
>>> +
>>> +static int mrfld_extcon_clear(struct mrfld_extcon_data *data, unsigned int reg,
>>> +			      unsigned int mask)
>>> +{
>>> +	return regmap_update_bits(data->regmap, reg, mask, 0x00);
>>> +}
>>> +
>>> +static int mrfld_extcon_set(struct mrfld_extcon_data *data, unsigned int reg,
>>> +			    unsigned int mask)
>>> +{
>>> +	return regmap_update_bits(data->regmap, reg, mask, 0xff);
>>> +}
> 
> mrfld_extcon_clear() and mrfld_extcon_set() are just wrapper function
> for regmap interface. I think that you better to define
> the meaningful defintion for '0x00' and '0xff' as following:
> 
> (just example, you may make the more correct name)
> #define INTEL_MRFLD_RESET	0x00
> #define INTEL_MRFLD_SET		0xff
> 
> And then you better to use the 'regmap_update_bits()' function
> directly instead of mrfld_extcon_clear/set'.
> 
> Also, you should handle the exception hanlding when using regmap function.
> 
>>> +
>>> +static int mrfld_extcon_get_id(struct mrfld_extcon_data *data)
>>> +{
>>> +	struct regmap *regmap = data->regmap;
>>> +	unsigned int id;
>>> +	bool ground;
>>> +	int ret;
>>> +
>>> +	ret = regmap_read(regmap, BCOVE_USBIDSTS, &id);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (id & BCOVE_USBIDSTS_FLOAT)
>>> +		return INTEL_USB_ID_FLOAT;
>>> +
>>> +	switch ((id & BCOVE_USBIDSTS_RARBRC_MASK) >> BCOVE_USBIDSTS_RARBRC_SHIFT) {
>>> +	case BCOVE_USBIDSTS_R_ID_A:
>>> +		return INTEL_USB_RID_A;
>>> +	case BCOVE_USBIDSTS_R_ID_B:
>>> +		return INTEL_USB_RID_B;
>>> +	case BCOVE_USBIDSTS_R_ID_C:
>>> +		return INTEL_USB_RID_C;
> 
> Please add 'default' statement for exception handling.
> 
>>> +	}
>>> +
>>> +	/*
>>> +	 * PMIC A0 reports USBIDSTS_GND = 1 for ID_GND,
>>> +	 * but PMIC B0 reports USBIDSTS_GND = 0 for ID_GND.
>>> +	 * Thus we must check this bit at last.
>>> +	 */
>>> +	ground = id & BCOVE_USBIDSTS_GND;
>>> +	switch ('A' + BCOVE_MAJOR(data->id)) {
>>> +	case 'A':
>>> +		return ground ? INTEL_USB_ID_GND : INTEL_USB_ID_FLOAT;
>>> +	case 'B':
>>> +		return ground ? INTEL_USB_ID_FLOAT : INTEL_USB_ID_GND;
>>> +	}
>>> +
>>> +	/* Unknown or unsupported type */
>>> +	return INTEL_USB_ID_FLOAT;
>>> +}
>>> +
>>> +static int mrfld_extcon_role_detect(struct mrfld_extcon_data *data)
>>> +{
>>> +	unsigned int id;
>>> +	bool usb_host;
>>> +	int ret;>> +
>>> +	ret = mrfld_extcon_get_id(data);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	id = ret;
>>> +
>>> +	usb_host = (id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A);
>>> +	extcon_set_state_sync(data->edev, EXTCON_USB_HOST, usb_host);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int mrfld_extcon_cable_detect(struct mrfld_extcon_data *data)
>>> +{
>>> +	struct regmap *regmap = data->regmap;
>>> +	unsigned int status;
>>> +	int ret;
>>> +
>>> +	/*
>>> +	 * It seems SCU firmware clears the content of BCOVE_CHGRIRQ1
>>> +	 * and makes it useless for OS. Instead we compare a previously
>>> +	 * stored status to the current one, provided by BCOVE_SCHGRIRQ1.
>>> +	 */
>>> +	ret = regmap_read(regmap, BCOVE_SCHGRIRQ1, &status);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!(status ^ data->status))
>>> +		return -ENODATA;
>>> +
>>> +	if ((status ^ data->status) & BCOVE_CHGRIRQ_USBIDDET)
>>> +		ret = mrfld_extcon_role_detect(data);
> This line gets the return value from mrfld_extcon_role_detect(data)
> without any error handling and then the below line just saves 'status'
> to 'data->status' regardless of 'ret' value.
> 
> I think that you have to handle the error case of
> 'ret = mrfld_extcon_role_detect(data)'.
> 
>>> +
>>> +	data->status = status;
> 
> nitpick. better to add one blank line.
> 
>>> +	return ret;
>>> +}
>>> +
>>> +static irqreturn_t mrfld_extcon_interrupt(int irq, void *dev_id)
>>> +{
>>> +	struct mrfld_extcon_data *data = dev_id;
>>> +	int ret;
>>> +
>>> +	ret = mrfld_extcon_cable_detect(data);
>>> +
>>> +	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
>>> +
>>> +	return ret ? IRQ_NONE: IRQ_HANDLED;
>>> +}
>>> +
>>> +static int mrfld_extcon_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
>>> +	struct regmap *regmap = pmic->regmap;
>>> +	struct mrfld_extcon_data *data;
>>> +	unsigned int id;
>>> +	int irq, ret;
>>> +
>>> +	irq = platform_get_irq(pdev, 0);
>>> +	if (irq < 0)
>>> +		return irq;
>>> +
>>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
>>> +
>>> +	data->dev = dev;
>>> +	data->regmap = regmap;
>>> +
>>> +	data->edev = devm_extcon_dev_allocate(dev, mrfld_extcon_cable);
>>> +	if (IS_ERR(data->edev))
>>> +		return -ENOMEM;
>>> +
>>> +	ret = devm_extcon_dev_register(dev, data->edev);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "can't register extcon device: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = devm_request_threaded_irq(dev, irq, NULL, mrfld_extcon_interrupt,
>>> +					IRQF_ONESHOT | IRQF_SHARED, pdev->name,
>>> +					data);
>>> +	if (ret)
>>> +		return ret;
> 
> You better add the error log with dev_err.
> 
>>> +
>>> +	ret = regmap_read(regmap, BCOVE_ID, &id);
>>> +	if (ret)
>>> +		return ret;
> 
> ditto for error log.
> 
>>> +
>>> +	data->id = id;
>>> +
>>> +	mrfld_extcon_set(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);
>>> +
>>> +	/* Get initial state */
>>> +	mrfld_extcon_role_detect(data);
> 
> Please handle the return value for exception handling with log.
> 
>>> +
>>> +	mrfld_extcon_clear(data, BCOVE_MIRQLVL1, BCOVE_LVL1_CHGR);
>>> +	mrfld_extcon_clear(data, BCOVE_MCHGRIRQ1, BCOVE_CHGRIRQ_ALL);
>>> +
>>> +	mrfld_extcon_set(data, BCOVE_USBIDCTRL, BCOVE_USBIDCTRL_ALL);
>>> +
>>> +	platform_set_drvdata(pdev, data);
> 
> nitpick. better to add one blank line.
> 
>>> +	return 0;
>>> +}
>>> +
>>> +static int mrfld_extcon_remove(struct platform_device *pdev)
>>> +{
>>> +	struct mrfld_extcon_data *data = platform_get_drvdata(pdev);
>>> +
>>> +	mrfld_extcon_clear(data, BCOVE_CHGRCTRL0, BCOVE_CHGRCTRL0_SWCONTROL);
> 
> nitpick. better to add one blank line.
> 
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct platform_device_id mrfld_extcon_id_table[] = {
>>> +	{ .name = "mrfld_bcove_extcon" },
> 
> I think that it is not proper to use 'extcon' word for the compatible name
> because 'extcon' word is linuxium. So, I recommend that you remove
> the 'extcon' word. Instead, you better to use new word related to h/w.
> 
>>> +	{}
>>> +};
>>> +MODULE_DEVICE_TABLE(platform, mrfld_extcon_id_table);
>>> +
>>> +static struct platform_driver mrfld_extcon_driver = {
>>> +	.driver = {
>>> +		.name	= KBUILD_MODNAME,
> 
> Where is the definition of KBUILD_MODNAME? Are you missing?
> 
>>> +	},
>>> +	.probe		= mrfld_extcon_probe,
>>> +	.remove		= mrfld_extcon_remove,
>>> +	.id_table	= mrfld_extcon_id_table,
>>> +};
>>> +module_platform_driver(mrfld_extcon_driver);
>>> +
>>> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
>>> +MODULE_DESCRIPTION("Extcon driver for Basin Cove PMIC");
> 
> Add the 'Merrifield' word in front of 'Basin Cove PMIC' as following:
> - Merrifield Basic Cove PMIC
> 
>>> +MODULE_LICENSE("GPL v2");
>>> -- 
>>> 2.20.1
>>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC

[Andy Shevchenko]

On Mon, Mar 18, 2019 at 07:38:26PM +0900, Chanwoo Choi wrote:

> Thanks for comment. I add my comments
> and then you have to rebase it on latest v5.0-rc1
> because the merge conflict happen on v5.0-rc1.

Thanks for review, see my answers below.
Non-answered items will be fixed accordingly.

> >> +config EXTCON_INTEL_MRFLD
> > 
> >> +	tristate "Intel MErrifield Basin Cove PMIC extcon driver"
> > 
> > ME -> Me (will be fixed)
> > 
> >> +	depends on INTEL_SOC_PMIC_MRFLD
> 
> This driver uses the regmap interface. So, you better to add
> following dependency?

> - select REGMAP_I2C or REGMAP_SPI

None of them fits this or MFD driver. See below.

> But, if 'INTEL_SOC_PMIC_MRFLE' selects already REGMAP_*
> configuration. It is not necessary.

https://lore.kernel.org/lkml/20190318095316.69278-1-andriy.shevchenko@linux.intel.com/

It selects REGMAP_IRQ which selects necessary bits from regmap API.

> >> +	help
> >> +	  Say Y here to enable extcon support for charger detection / control
> >> +	  on the Intel Merrifiel Basin Cove PMIC.
> 
> What is correct word?
> - Merrifield? is used on above
> - Merrifiel?

Merrifield is a correct one. Thanks for spotting this.

> >> +static int mrfld_extcon_set(struct mrfld_extcon_data *data, unsigned int reg,
> >> +			    unsigned int mask)
> >> +{
> >> +	return regmap_update_bits(data->regmap, reg, mask, 0xff);
> >> +}
> 
> mrfld_extcon_clear() and mrfld_extcon_set() are just wrapper function
> for regmap interface. I think that you better to define
> the meaningful defintion for '0x00' and '0xff' as following:
> 
> (just example, you may make the more correct name)
> #define INTEL_MRFLD_RESET	0x00
> #define INTEL_MRFLD_SET		0xff

It makes a little sense here, the idea is to reduce parameters.

I could ideally write
(..., mask, ~mask) for clear
and
(..., mask, mask) for set

> And then you better to use the 'regmap_update_bits()' function
> directly instead of mrfld_extcon_clear/set'.

It will bring duplication of long definitions and reduce readability of the
code.

> >> +	/*
> >> +	 * It seems SCU firmware clears the content of BCOVE_CHGRIRQ1
> >> +	 * and makes it useless for OS. Instead we compare a previously
> >> +	 * stored status to the current one, provided by BCOVE_SCHGRIRQ1.
> >> +	 */
> >> +	ret = regmap_read(regmap, BCOVE_SCHGRIRQ1, &status);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (!(status ^ data->status))
> >> +		return -ENODATA;
> >> +
> >> +	if ((status ^ data->status) & BCOVE_CHGRIRQ_USBIDDET)
> >> +		ret = mrfld_extcon_role_detect(data);
> This line gets the return value from mrfld_extcon_role_detect(data)
> without any error handling and then the below line just saves 'status'
> to 'data->status' regardless of 'ret' value.
> 
> I think that you have to handle the error case of
> 'ret = mrfld_extcon_role_detect(data)'.

I'm not sure of the consequences of such change.
I will give it some tests, and then will proceed accordingly.

> >> +		.name	= KBUILD_MODNAME,
> 
> Where is the definition of KBUILD_MODNAME? Are you missing?

In the Makefile.
Nothing is missed here.

But I could put its content explicitly here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC

[Chanwoo Choi]

Hi Andy,

On 19. 3. 18. 오후 9:46, Andy Shevchenko wrote:
> On Mon, Mar 18, 2019 at 07:38:26PM +0900, Chanwoo Choi wrote:
> 
>> Thanks for comment. I add my comments
>> and then you have to rebase it on latest v5.0-rc1
>> because the merge conflict happen on v5.0-rc1.
> 
> Thanks for review, see my answers below.
> Non-answered items will be fixed accordingly.
> 
>>>> +config EXTCON_INTEL_MRFLD
>>>
>>>> +	tristate "Intel MErrifield Basin Cove PMIC extcon driver"
>>>
>>> ME -> Me (will be fixed)
>>>
>>>> +	depends on INTEL_SOC_PMIC_MRFLD
>>
>> This driver uses the regmap interface. So, you better to add
>> following dependency?
> 
>> - select REGMAP_I2C or REGMAP_SPI
> 
> None of them fits this or MFD driver. See below.
> 
>> But, if 'INTEL_SOC_PMIC_MRFLE' selects already REGMAP_*
>> configuration. It is not necessary.
> 
> https://lore.kernel.org/lkml/20190318095316.69278-1-andriy.shevchenko@linux.intel.com/
> 
> It selects REGMAP_IRQ which selects necessary bits from regmap API.

OK.

> 
>>>> +	help
>>>> +	  Say Y here to enable extcon support for charger detection / control
>>>> +	  on the Intel Merrifiel Basin Cove PMIC.
>>
>> What is correct word?
>> - Merrifield? is used on above
>> - Merrifiel?
> 
> Merrifield is a correct one. Thanks for spotting this.
> 
>>>> +static int mrfld_extcon_set(struct mrfld_extcon_data *data, unsigned int reg,
>>>> +			    unsigned int mask)
>>>> +{
>>>> +	return regmap_update_bits(data->regmap, reg, mask, 0xff);
>>>> +}
>>
>> mrfld_extcon_clear() and mrfld_extcon_set() are just wrapper function
>> for regmap interface. I think that you better to define
>> the meaningful defintion for '0x00' and '0xff' as following:
>>
>> (just example, you may make the more correct name)
>> #define INTEL_MRFLD_RESET	0x00
>> #define INTEL_MRFLD_SET		0xff
> 
> It makes a little sense here, the idea is to reduce parameters.
> 
> I could ideally write
> (..., mask, ~mask) for clear
> and
> (..., mask, mask) for set
> 
>> And then you better to use the 'regmap_update_bits()' function
>> directly instead of mrfld_extcon_clear/set'.
> 
> It will bring duplication of long definitions and reduce readability of the
> code.

Actually, it is not critical issue. If you don't agree my comments,
you keep your style on next version. I have no any strong objection.

> 
>>>> +	/*
>>>> +	 * It seems SCU firmware clears the content of BCOVE_CHGRIRQ1
>>>> +	 * and makes it useless for OS. Instead we compare a previously
>>>> +	 * stored status to the current one, provided by BCOVE_SCHGRIRQ1.
>>>> +	 */
>>>> +	ret = regmap_read(regmap, BCOVE_SCHGRIRQ1, &status);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (!(status ^ data->status))
>>>> +		return -ENODATA;
>>>> +
>>>> +	if ((status ^ data->status) & BCOVE_CHGRIRQ_USBIDDET)
>>>> +		ret = mrfld_extcon_role_detect(data);
>> This line gets the return value from mrfld_extcon_role_detect(data)
>> without any error handling and then the below line just saves 'status'
>> to 'data->status' regardless of 'ret' value.
>>
>> I think that you have to handle the error case of
>> 'ret = mrfld_extcon_role_detect(data)'.
> 
> I'm not sure of the consequences of such change.
> I will give it some tests, and then will proceed accordingly.

OK. Thanks.

> 
>>>> +		.name	= KBUILD_MODNAME,
>>
>> Where is the definition of KBUILD_MODNAME? Are you missing?
> 
> In the Makefile.
> Nothing is missed here.
> 
> But I could put its content explicitly here.

OK. Thanks.

> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics