Re: [PATCH 03/15] extcon: cht-wc: Add Intel Cherry Trail Whiskey Cove PMIC extcon driver
From: Hans de Goede
Date: Thu Mar 23 2017 - 11:22:48 EST
Hi,
On 21-03-17 06:16, Chanwoo Choi wrote:
Hi,
On 2017ë 03ì 21ì 04:57, Hans de Goede wrote:
Hi,
On 20-03-17 02:33, Chanwoo Choi wrote:
Hi,
On 2017ë 03ì 17ì 18:55, Hans de Goede wrote:
Add a driver for charger detection / control on the Intel Cherrytrail
Whiskey Cove PMIC.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/extcon/Kconfig | 7 +
drivers/extcon/Makefile | 1 +
drivers/extcon/extcon-cht-wc.c | 356 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 364 insertions(+)
create mode 100644 drivers/extcon/extcon-cht-wc.c
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 96bbae5..4cace6b 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -52,6 +52,13 @@ config EXTCON_INTEL_INT3496
This ACPI device is typically found on Intel Baytrail or Cherrytrail
based tablets, or other Baytrail / Cherrytrail devices.
+config EXTCON_CHT_WC
Need to reorder it alpabetically as the following Makefile.
The idea is to have the items alphabetically listed in "make menuconfig"
and the name of the menu item starts with Intel:
If you want to locate it under the EXTCON_INTEL_INT3496,
you should change the filename as following style:
- extcon-intel-cht-wc.c
I want to locate all entry alphabetically.
Ok, will fix for v3
+ tristate "Intel Cherrytrail Whiskey Cove PMIC extcon driver"
+ depends on INTEL_SOC_PMIC_CHTWC
+ help
+ Say Y here to enable extcon support for charger detection / control
+ on the Intel Cherrytrail Whiskey 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 237ac3f..160f88b 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -7,6 +7,7 @@ extcon-core-objs += extcon.o devres.o
obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o
+obj-$(CONFIG_EXTCON_CHT_WC) += extcon-cht-wc.o
obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o
obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o
obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o
diff --git a/drivers/extcon/extcon-cht-wc.c b/drivers/extcon/extcon-cht-wc.c
new file mode 100644
index 0000000..896eee6
--- /dev/null
+++ b/drivers/extcon/extcon-cht-wc.c
@@ -0,0 +1,356 @@
+/*
+ * Extcon charger detection driver for Intel Cherrytrail Whiskey Cove PMIC
+ * Copyright (C) 2017 Hans de Goede <hdegoede@xxxxxxxxxx>
+ *
+ * Based on various non upstream patches to support the CHT Whiskey Cove PMIC:
Maybe, you don't need to add ':' at the end of line.
Th ':' is there because the following copyright line:
+ * Copyright (C) 2013-2015 Intel Corporation. All rights reserved.
Comes from those various non upstream patches.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/extcon.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define CHT_WC_PWRSRC_IRQ 0x6e03
+#define CHT_WC_PWRSRC_IRQ_MASK 0x6e0f
+#define CHT_WC_PWRSRC_STS 0x6e1e
+#define CHT_WC_PWRSRC_VBUS BIT(0)
+#define CHT_WC_PWRSRC_DC BIT(1)
+#define CHT_WC_PWRSRC_BAT BIT(2)
+#define CHT_WC_PWRSRC_ID_GND BIT(3)
+#define CHT_WC_PWRSRC_ID_FLOAT BIT(4)
+
+#define CHT_WC_PHYCTRL 0x5e07
+
+#define CHT_WC_CHGRCTRL0 0x5e16
+
+#define CHT_WC_CHGRCTRL0 0x5e16
+#define CHT_WC_CHGRCTRL0_CHGRRESET BIT(0)
+#define CHT_WC_CHGRCTRL0_EMRGCHREN BIT(1)
+#define CHT_WC_CHGRCTRL0_EXTCHRDIS BIT(2)
+#define CHT_WC_CHGRCTRL0_SWCONTROL BIT(3)
+#define CHT_WC_CHGRCTRL0_TTLCK_MASK BIT(4)
+#define CHT_WC_CHGRCTRL0_CCSM_OFF_MASK BIT(5)
+#define CHT_WC_CHGRCTRL0_DBPOFF_MASK BIT(6)
+#define CHT_WC_CHGRCTRL0_WDT_NOKICK BIT(7)
+
+#define CHT_WC_CHGRCTRL1 0x5e17
+
+#define CHT_WC_USBSRC 0x5e29
+#define CHT_WC_USBSRC_STS_MASK GENMASK(1, 0)
+#define CHT_WC_USBSRC_STS_SUCCESS 2
+#define CHT_WC_USBSRC_STS_FAIL 3
+#define CHT_WC_USBSRC_TYPE_SHIFT 2
+#define CHT_WC_USBSRC_TYPE_MASK GENMASK(5, 2)
+#define CHT_WC_USBSRC_TYPE_NONE 0
+#define CHT_WC_USBSRC_TYPE_SDP 1
+#define CHT_WC_USBSRC_TYPE_DCP 2
+#define CHT_WC_USBSRC_TYPE_CDP 3
+#define CHT_WC_USBSRC_TYPE_ACA 4
+#define CHT_WC_USBSRC_TYPE_SE1 5
+#define CHT_WC_USBSRC_TYPE_MHL 6
+#define CHT_WC_USBSRC_TYPE_FLOAT_DP_DN 7
+#define CHT_WC_USBSRC_TYPE_OTHER 8
+#define CHT_WC_USBSRC_TYPE_DCP_EXTPHY 9
+
+enum cht_wc_usb_id {
+ USB_ID_OTG,
+ USB_ID_GND,
+ USB_ID_FLOAT,
+ USB_RID_A,
+ USB_RID_B,
+ USB_RID_C,
+};
+
+/* Strings matching the cht_wc_usb_id enum labels */
+static const char * const usb_id_str[] = {
+ "otg", "gnd", "float", "rid_a", "rid_b", "rid_c" };
+
+enum cht_wc_mux_select {
+ MUX_SEL_PMIC = 0,
+ MUX_SEL_SOC,
+};
+
+static const unsigned int cht_wc_extcon_cables[] = {
+ EXTCON_USB,
+ EXTCON_USB_HOST,
+ EXTCON_CHG_USB_SDP,
+ EXTCON_CHG_USB_CDP,
+ EXTCON_CHG_USB_DCP,
+ EXTCON_NONE,
+};
+
+struct cht_wc_extcon_data {
+ struct device *dev;
+ struct regmap *regmap;
+ struct extcon_dev *edev;
+ unsigned int previous_cable;
+ int usb_id;
+};
+
+static int cht_wc_extcon_get_id(struct cht_wc_extcon_data *ext, int pwrsrc_sts)
+{
+ if (ext->usb_id)
+ return ext->usb_id;
+
+ if (pwrsrc_sts & CHT_WC_PWRSRC_ID_GND)
+ return USB_ID_GND;
+ if (pwrsrc_sts & CHT_WC_PWRSRC_ID_FLOAT)
+ return USB_ID_FLOAT;
+
+ /*
+ * Once we have iio support for the gpadc we should read the USBID
+ * gpadc channel here and determine ACA role based on that.
+ */
+ return USB_ID_FLOAT;
+}
+
+static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext)
+{
+ int ret, usbsrc, status, retries = 5;
You have to define the constant for '5' because the name of constant
definition indicates what is meaning. So, maybe you will use the 'for' loope
instead of 'do..while'.
Right, already fixed as a result of Andy's review.
+
+ do {
+ ret = regmap_read(ext->regmap, CHT_WC_USBSRC, &usbsrc);
+ if (ret) {
+ dev_err(ext->dev, "Error reading usbsrc: %d\n", ret);
+ return ret;
+ }
Need to add one blank line.
+ status = usbsrc & CHT_WC_USBSRC_STS_MASK;
+ if (status == CHT_WC_USBSRC_STS_SUCCESS ||
+ status == CHT_WC_USBSRC_STS_FAIL)
+ break;
+
+ msleep(200);
You have to define the constant for '200' because the name of constant
definition indicates what is meaning.
+ } while (retries--);
+
+ if (status != CHT_WC_USBSRC_STS_SUCCESS) {
+ if (status == CHT_WC_USBSRC_STS_FAIL)
+ dev_warn(ext->dev, "Could not detect charger type\n");
+ else
+ dev_warn(ext->dev, "Timeout detecting charger type\n");
+ return EXTCON_CHG_USB_SDP; /* Save fallback */
+ }
+
+ ret = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
'ret' is not proper indicates the meaning of 'CHT_WC_USBSRC_TYPE'.
You have to use the more correct local variable such as 'usbsrc_type'.
Fixed for v2.
+ switch (ret) {
+ default:
+ dev_warn(ext->dev, "Unhandled charger type %d\n", ret);
+ /* Fall through treat as SDP */
Is it right? Why do you located the 'default' on the top in the switch?
So that I can use fall-through, there is no rule in C where the default goes.
The fallthrough is there because assuming SDP (and thus max 500mA current
draw) is always safe.
If in the default statement, you treat the unhandled charger type as the SDP,
you don't remove the warning message. It makes the confusion.
Warning message is 'unhandled charger type'. But, the extcon
detects the 'SDP' connector type. It is not reasonable.
Ok, I will change the warning message to say that we are defaulting to
SDP.
+ case CHT_WC_USBSRC_TYPE_SDP:
+ case CHT_WC_USBSRC_TYPE_FLOAT_DP_DN:
+ case CHT_WC_USBSRC_TYPE_OTHER:
+ return EXTCON_CHG_USB_SDP;
+ case CHT_WC_USBSRC_TYPE_CDP:
+ return EXTCON_CHG_USB_CDP;
+ case CHT_WC_USBSRC_TYPE_DCP:
+ case CHT_WC_USBSRC_TYPE_DCP_EXTPHY:
+ case CHT_WC_USBSRC_TYPE_MHL: /* MHL2+ delivers upto 2A, treat as DCP */
+ return EXTCON_CHG_USB_DCP;
+ case CHT_WC_USBSRC_TYPE_ACA:
+ return EXTCON_CHG_USB_ACA;
+ }
+}
+
+static void cht_wc_extcon_set_phymux(struct cht_wc_extcon_data *ext, u8 state)
+{
+ int ret;
+
+ ret = regmap_write(ext->regmap, CHT_WC_PHYCTRL, state);
+ if (ret)
+ dev_err(ext->dev, "Error writing phyctrl: %d\n", ret);
This function is only called in the cht_wc_extcon_det_event().
Also, this funciton write only one register. It is too short.
So, you don't need to add the separate function.
You better to include this code in the cht_wc_extcon_det_event().
This is used multiple times in cht_wc_extcon_det_event() and is
also used in probe() so it saves having to copy and paste the error
check. Also the flow of cht_wc_extcon_det_event() is more readable
this way. If it is more efficient to have this inline the compiler
will auto-inline it.
OK. I recognized it was called only on the cht_wc_extcon_det_event().
But, it is called on multiple points. It's OK.
+}
+
+static void cht_wc_extcon_det_event(struct cht_wc_extcon_data *ext)
+{
+ int ret, pwrsrc_sts, id;
+ unsigned int cable = EXTCON_NONE;
+
+ ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
+ if (ret) {
+ dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
+ return;
+ }
+
+ id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
+ if (id == USB_ID_GND) {
+ /* The 5v boost causes a false VBUS / SDP detect, skip */
+ goto charger_det_done;
+ }
+
+ /* Plugged into a host/charger or not connected? */
+ if (!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) {
+ /* Route D+ and D- to PMIC for future charger detection */
+ cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
+ goto set_state;
+ }
The cht_wc_extcon_get_id() and cht_wc_extcon_det_event() use the value
of CHT_WC_PWRSRC_STS register. So, I think you better to gather the
code related to the CHT_WC_PWRSRC_STS for readability.
- First suggestion, remove the separate the cht_wc_extcon_get_id()
- Second suggestion, The code from regmap_read() to "!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)"
move into the cht_wc_extcon_get_id().
In my opinion, I recommend that second way.
These register reads are i2c register reads which are quite costly,
so we really want to do this only once, which is why the code is
as it is.
Sure, If you use the my second suggestion, you can read i2c register
only one time for all codes as following:
cht_wc_extcon_get_id(ext)
// Read the CHT_WC_PWRSRC_STS register through i2c
// Check the usb_id and pwersrc_sts with CHT_WC_PWRSRC_ID_GND/CHT_WC_PWRSRC_ID_FLOAT.
// Plugged into a host/charger or not connected?
+
+ ret = cht_wc_extcon_get_charger(ext);
+ if (ret >= 0)
+ cable = ret;
+
+charger_det_done:
+ /* Route D+ and D- to SoC for the host / gadget controller */
Minor comment.
You better to use '&' instead of '/'
The data lines get used by either the host OR the gadget controller,
as there is another mux inside the SoC.
If '/' means the 'or', you can use the 'or' instead of '/'.
Ok text changed to use or for v3.
+ cht_wc_extcon_set_phymux(ext, MUX_SEL_SOC);
+
+set_state:
+ extcon_set_state_sync(ext->edev, cable, true);
+ extcon_set_state_sync(ext->edev, ext->previous_cable, false);
+ extcon_set_state_sync(ext->edev, EXTCON_USB_HOST,
+ id == USB_ID_GND || id == USB_RID_A);
+ ext->previous_cable = cable;
+}
+
+static irqreturn_t cht_wc_extcon_isr(int irq, void *data)
+{
+ struct cht_wc_extcon_data *ext = data;
+ int ret, irqs;
+
+ ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_IRQ, &irqs);
+ if (ret)
+ dev_err(ext->dev, "Error reading irqs: %d\n", ret);
+
+ cht_wc_extcon_det_event(ext);
+
+ ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ, irqs);
+ if (ret)
+ dev_err(ext->dev, "Error writing irqs: %d\n", ret);
+
+ return IRQ_HANDLED;
+}
+
+/* usb_id sysfs attribute for debug / testing purposes */
+static ssize_t usb_id_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct cht_wc_extcon_data *ext = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%s\n", usb_id_str[ext->usb_id]);
+}
+
+static ssize_t usb_id_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t n)
+{
+ struct cht_wc_extcon_data *ext = dev_get_drvdata(dev);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(usb_id_str); i++) {
+ if (sysfs_streq(buf, usb_id_str[i])) {
+ dev_info(ext->dev, "New usb_id %s\n", usb_id_str[i]);
+ ext->usb_id = i;
+ cht_wc_extcon_det_event(ext);
+ return n;
+ }
+ }
+
+ return -EINVAL;
+}
+
+static DEVICE_ATTR(usb_id, 0644, usb_id_show, usb_id_store);
I think it is not good to add specific sysfs for only this device driver.
The sysfs entry of framework must include the only common and standard interfarce
for all extcon device drivers. Because the sysfs entry affects the ABI interface.
So, It is not proper.
Unfortunately these kinda sysfs files are somewhat normal when
dealing with USB-OTG. For example my board does not have the
id-pin hooked up to the connector, so to test host mode
I need to echo "gnd" to the sysfs attr. But also if I actually
want to use host-mode (or anyone else with the same or a similar
board).
As I said, I don't want to create the non-standard sysfs interface
for only specific device driver.
If you want to change the some mode of device driver,
we should implement the something in the extcon framework
by keeping the standard interface for ABI. I don't want to
make such a special case.
Ok, I will drop this part of the driver for now, we can revisit
this later.
See for example also the "mode" sysfs attribute of the musb driver.
Since the id-pin setting influences multiple other drivers through
extcon the best place for this is in the extcon driver, as that
it the canonical source of the EXTCON_USB_HOST cable value.
+
+static int cht_wc_extcon_probe(struct platform_device *pdev)
+{
+ struct cht_wc_extcon_data *ext;
+ struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+ int irq, ret;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ext = devm_kzalloc(&pdev->dev, sizeof(*ext), GFP_KERNEL);
+ if (!ext)
+ return -ENOMEM;
+
+ ext->dev = &pdev->dev;
+ ext->regmap = pmic->regmap;
+ ext->previous_cable = EXTCON_NONE;
+
+ /* Initialize extcon device */
+ ext->edev = devm_extcon_dev_allocate(ext->dev, cht_wc_extcon_cables);
+ if (IS_ERR(ext->edev))
+ return PTR_ERR(ext->edev);
+
+ ret = regmap_update_bits(ext->regmap, CHT_WC_CHGRCTRL0,
+ CHT_WC_CHGRCTRL0_SWCONTROL | CHT_WC_CHGRCTRL0_CCSM_OFF_MASK,
+ CHT_WC_CHGRCTRL0_SWCONTROL | CHT_WC_CHGRCTRL0_CCSM_OFF_MASK);
+ if (ret) {
+ dev_err(ext->dev, "Error enabling sw control\n");
+ return ret;
+ }
+
+ /* Register extcon device */
+ ret = devm_extcon_dev_register(ext->dev, ext->edev);
+ if (ret) {
+ dev_err(ext->dev, "Failed to register extcon device\n");
+ return ret;
+ }
+
+ /* Route D+ and D- to PMIC for initial charger detection */
+ cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC);
+
+ /* Get initial state */
+ cht_wc_extcon_det_event(ext);
+
+ ret = devm_request_threaded_irq(ext->dev, irq, NULL, cht_wc_extcon_isr,
+ IRQF_ONESHOT, pdev->name, ext);
+ if (ret) {
+ dev_err(ext->dev, "Failed to request interrupt\n");
+ return ret;
+ }
+
+ /* Unmask irqs */
+ ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ_MASK,
+ (int)~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_ID_GND |
+ CHT_WC_PWRSRC_ID_FLOAT));
+ if (ret) {
+ dev_err(ext->dev, "Error writing irq-mask: %d\n", ret);
I prefer to use the consistent error log. In the probe function,
you use the 'Failed to ...' when error hanppen. So, You better
to use the consistent format for errr log as following:
- "Failed to write the irq-mask: %d\n", ret);
Fixed for v2.
I think it improve the readability of your device driver.
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, ext);
+ device_create_file(ext->dev, &dev_attr_usb_id);
+
+ return 0;
In the probe function, you touch the some register for initialization.
But, if error happen, the probe function don't restore the register value.
Is it ok? I think you need to handle the error case.
Fixed for v2.
+}
+
+static int cht_wc_extcon_remove(struct platform_device *pdev)
+{
+ struct cht_wc_extcon_data *ext = platform_get_drvdata(pdev);
+
+ device_remove_file(ext->dev, &dev_attr_usb_id);
Don't need it.
+
+ return 0;
+}
+
+static const struct platform_device_id cht_wc_extcon_table[] = {
+ { .name = "cht_wcove_pwrsrc" },
You use the 'cht_wc' word instead of 'cht_wcove_pwrsrc'.
So, To maintain the consistency, you better to use the 'cht-wc' as the name.
- I prefer to use '-' instead of '_' in the name.
.name ="cht-wc"
Already answered by Andy.
I replied again from the Andy's reply.
+ {},
+};
+MODULE_DEVICE_TABLE(platform, cht_wc_extcon_table);
+
+static struct platform_driver cht_wc_extcon_driver = {
+ .probe = cht_wc_extcon_probe,
+ .remove = cht_wc_extcon_remove,
+ .id_table = cht_wc_extcon_table,
+ .driver = {
+ .name = "cht_wcove_pwrsrc",
+ },
+};
+module_platform_driver(cht_wc_extcon_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@xxxxxxxxxx>");
+MODULE_DESCRIPTION("Intel Cherrytrail Whiskey Cove PMIC extcon driver");
Minor comment.
You better to locate the MODULE_DESCRIPTION at the first line
and then MODULE_AUTHOR is at second line.
Fixed for v2.
Regards,
Hans