* [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header
@ 2019-03-18 9:52 ` Andy Shevchenko
2019-03-18 9:52 ` [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC Andy Shevchenko
2019-03-18 10:05 ` [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header Chanwoo Choi
0 siblings, 2 replies; 8+ messages in thread
From: Andy Shevchenko @ 2019-03-18 9:52 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, linux-kernel, Hans de Goede; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC
2019-03-18 9:52 ` [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header Andy Shevchenko
@ 2019-03-18 9:52 ` Andy Shevchenko
2019-03-18 10:11 ` Andy Shevchenko
2019-03-18 10:05 ` [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header Chanwoo Choi
1 sibling, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2019-03-18 9:52 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, linux-kernel, Hans de Goede; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header
2019-03-18 9:52 ` [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header Andy Shevchenko
2019-03-18 9:52 ` [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC Andy Shevchenko
@ 2019-03-18 10:05 ` Chanwoo Choi
1 sibling, 0 replies; 8+ messages in thread
From: Chanwoo Choi @ 2019-03-18 10:05 UTC (permalink / raw)
To: Andy Shevchenko, MyungJoo Ham, linux-kernel, Hans de Goede
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC
2019-03-18 9:52 ` [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC Andy Shevchenko
@ 2019-03-18 10:11 ` Andy Shevchenko
2019-03-18 10:38 ` Chanwoo Choi
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2019-03-18 10:11 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, linux-kernel, Hans de Goede
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC
2019-03-18 10:11 ` Andy Shevchenko
@ 2019-03-18 10:38 ` Chanwoo Choi
2019-03-18 10:50 ` Chanwoo Choi
2019-03-18 12:46 ` Andy Shevchenko
0 siblings, 2 replies; 8+ messages in thread
From: Chanwoo Choi @ 2019-03-18 10:38 UTC (permalink / raw)
To: Andy Shevchenko, MyungJoo Ham, linux-kernel, Hans de Goede
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC
2019-03-18 10:38 ` Chanwoo Choi
@ 2019-03-18 10:50 ` Chanwoo Choi
2019-03-18 12:46 ` Andy Shevchenko
1 sibling, 0 replies; 8+ messages in thread
From: Chanwoo Choi @ 2019-03-18 10:50 UTC (permalink / raw)
To: Andy Shevchenko, MyungJoo Ham, linux-kernel, Hans de Goede
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC
2019-03-18 10:38 ` Chanwoo Choi
2019-03-18 10:50 ` Chanwoo Choi
@ 2019-03-18 12:46 ` Andy Shevchenko
2019-03-19 0:45 ` Chanwoo Choi
1 sibling, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2019-03-18 12:46 UTC (permalink / raw)
To: Chanwoo Choi; +Cc: MyungJoo Ham, linux-kernel, Hans de Goede
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC
2019-03-18 12:46 ` Andy Shevchenko
@ 2019-03-19 0:45 ` Chanwoo Choi
0 siblings, 0 replies; 8+ messages in thread
From: Chanwoo Choi @ 2019-03-19 0:45 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: MyungJoo Ham, linux-kernel, Hans de Goede
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-03-19 0:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20190318095232epcas5p27d6bafb732b79cf76d84f0de0fccda6c@epcas5p2.samsung.com>
2019-03-18 9:52 ` [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header Andy Shevchenko
2019-03-18 9:52 ` [PATCH v1 2/2] extcon: mrfld: Introduce extcon driver for Basin Cove PMIC Andy Shevchenko
2019-03-18 10:11 ` Andy Shevchenko
2019-03-18 10:38 ` Chanwoo Choi
2019-03-18 10:50 ` Chanwoo Choi
2019-03-18 12:46 ` Andy Shevchenko
2019-03-19 0:45 ` Chanwoo Choi
2019-03-18 10:05 ` [PATCH v1 1/2] extcon: intel: Split out some definitions to a common header Chanwoo Choi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).