| Message ID | 20170317095527.10487-4-hdegoede@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote: > Add a driver for charger detection / control on the Intel Cherrytrail > Whiskey Cove PMIC. +Cc: Felipe for some question(s) below. > drivers/extcon/extcon-cht-wc.c | 356 I would use same pattern across drivers, i.e. "chtwc" (same for the rest of the drivers in this series). > +#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) Not obvious for which register those bit definitions are. Also, keep them ordered by offset. > + > +#define CHT_WC_PHYCTRL 0x5e07 > + > +#define CHT_WC_CHGRCTRL0 0x5e16 Dup! > + > +#define CHT_WC_CHGRCTRL0 0x5e16 > +static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext) > +{ > + int ret, usbsrc, status, retries = 5; > + > + do { > + ret = regmap_read(ext->regmap, CHT_WC_USBSRC, > &usbsrc); > + if (ret) { > + dev_err(ext->dev, "Error reading usbsrc: > %d\n", ret); > + return ret; > + } > + status = usbsrc & CHT_WC_USBSRC_STS_MASK; > + if (status == CHT_WC_USBSRC_STS_SUCCESS || > + status == CHT_WC_USBSRC_STS_FAIL) > + break; > + > + msleep(200); Comment why and why so long? > + } while (retries--); > +static void cht_wc_extcon_det_event(struct cht_wc_extcon_data *ext) det -> detect ? > +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); Shouldn't we return IRQ_NONE here? Perhaps comment is needed. > + > + 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 */ Hmm... I would use debugfs for debug, otherwise it looks like it should be framework (extcon) wide. Perhaps Felipe can advise something here. > +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); Exchange them (assignment first). > + int irq, ret; > + > + 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 | Hmm... Do you need explicit casting here? > CHT_WC_PWRSRC_ID_GND | > + CHT_WC_PWRSRC_ID_FLOAT)); > + if (ret) { > + dev_err(ext->dev, "Error writing irq-mask: %d\n", > ret); > + return ret; > + }
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@redhat.com> > --- > 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. > + 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@redhat.com> > + * > + * 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. > + * Copyright (C) 2013-2015 Intel Corporation. All rights reserved. > + * > + * 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'. > + > + 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'. > + 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? > + 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(). > +} > + > +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. > + > + 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 '/' > + 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. > + > +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); 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. > +} > + > +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" > + {}, > +}; > +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@redhat.com>"); > +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. > +MODULE_LICENSE("GPL v2"); >
On Mon, 2017-03-20 at 10:33 +0900, Chanwoo Choi wrote: > On 2017년 03월 17일 18:55, Hans de Goede wrote: > > +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" I would keep as Hans did for the sake of consistency among all Whiskey Cove device drivers (and predecessors like Crystal Cove). I understand your point from extcon subsystem view, but PMICs like Whiskey Cove are multi-functional devices, and thus naming across them (same prefix in use to be precise) is better idea. > > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(platform, cht_wc_extcon_table);
Hi, On 17-03-17 18:18, Andy Shevchenko wrote: > On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote: >> Add a driver for charger detection / control on the Intel Cherrytrail >> Whiskey Cove PMIC. > > +Cc: Felipe for some question(s) below. > >> drivers/extcon/extcon-cht-wc.c | 356 > > I would use same pattern across drivers, i.e. "chtwc" (same for the rest > of the drivers in this series). I already answered this in another part of the thread, but for the archives sake let me copy and paste my answer: "One thing I disagree with is the cht_wc > chtwc rename you are proposing for one Cherry Trail and Whiskey Cove are 2 different words (4 if you look at spelling but 2 if you look at pronunciation, so IMHO cht_wc is more readable other then that I see the suggested rename as a lot of extra churn without any tangible benefits." > >> +#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) > > Not obvious for which register those bit definitions are. They are for all 3 CHT_WC_PWRSRC_* registers, this is the more or less usual irq setup for some i2c devices where there is a status register, an irq register where the hardware sets bits to 1 (and we write 1 to clear) when the corresponding status bits changes and a mask register to select which irq register bits actually will raise the interrupt line. > Also, keep them ordered by offset. Will fix. > >> + >> +#define CHT_WC_PHYCTRL 0x5e07 >> + > >> +#define CHT_WC_CHGRCTRL0 0x5e16 > > Dup! Good catch, will fix. > >> + >> +#define CHT_WC_CHGRCTRL0 0x5e16 > >> +static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext) >> +{ >> + int ret, usbsrc, status, retries = 5; >> + >> + do { >> + ret = regmap_read(ext->regmap, CHT_WC_USBSRC, >> &usbsrc); >> + if (ret) { >> + dev_err(ext->dev, "Error reading usbsrc: >> %d\n", ret); >> + return ret; >> + } >> + status = usbsrc & CHT_WC_USBSRC_STS_MASK; >> + if (status == CHT_WC_USBSRC_STS_SUCCESS || >> + status == CHT_WC_USBSRC_STS_FAIL) >> + break; >> + > >> + msleep(200); > > Comment why and why so long? Fixed for v2 (actually switched to using jiffies + time_before for a more accurate timeout). >> + } while (retries--); > >> +static void cht_wc_extcon_det_event(struct cht_wc_extcon_data *ext) > > det -> detect ? Renamed to more accurate cht_wc_extcon_pwrsrc_event > +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); > > Shouldn't we return IRQ_NONE here? I was wondering the same myself here, there is no good answer here, this should simply never fail ... which does make returning IRQ_NONE a good idea, I was worried that would lead to a "nobody cared, disabling irq", but as said this should never happen, so when it does, disabling the irq is probably for the best. Will fix. > >> + >> + 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 */ > > Hmm... I would use debugfs for debug, otherwise it looks like it should > be framework (extcon) wide. 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). This definitely is not something which belongs in the extcon-core. > Perhaps Felipe can advise something here. > >> +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); > > Exchange them (assignment first). Fixed. > >> + int irq, ret; >> + > > >> + 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 | > > Hmm... Do you need explicit casting here? Yes because the BIT(x) macros are unsigned longs and the ~ sets the MSB so then the compiler complaints about truncating the variable without the cast. > >> CHT_WC_PWRSRC_ID_GND | >> + CHT_WC_PWRSRC_ID_FLOAT)); >> + if (ret) { >> + dev_err(ext->dev, "Error writing irq-mask: %d\n", >> ret); >> + return ret; >> + } > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> >> --- >> 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: > >> + 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@redhat.com> >> + * >> + * 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. > >> + 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. > >> +} >> + >> +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. >> + >> + 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. > >> + 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). 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. >> + {}, >> +}; >> +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@redhat.com>"); >> +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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017년 03월 20일 22:00, Andy Shevchenko wrote: > On Mon, 2017-03-20 at 10:33 +0900, Chanwoo Choi wrote: >> On 2017년 03월 17일 18:55, Hans de Goede wrote: > >>> +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" > > I would keep as Hans did for the sake of consistency among all Whiskey > Cove device drivers (and predecessors like Crystal Cove). The 'wcove' short word is not used in this patch. If the author want to use the 'wcove', I recommend that you should use the 'wcove' instead of 'wc' in this patch. And, I think that 'pwrsrc' is not ambiguous. Hans might use the 'pwrsrc' as the 'Power Source'. But, this driver is not power source. Instead, this driver supports the detection of external connector. I think 'power source' means the power supply instead of detector. > > I understand your point from extcon subsystem view, but PMICs like > Whiskey Cove are multi-functional devices, and thus naming across them > (same prefix in use to be precise) is better idea. > >> >>> + {}, >>> +}; >>> +MODULE_DEVICE_TABLE(platform, cht_wc_extcon_table); >
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@redhat.com> >>> --- >>> 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. > >> >>> + 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@redhat.com> >>> + * >>> + * 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. > >> >>> + 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 '/'. > >> >>> + 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. > > 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@redhat.com>"); >>> +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 > > >
On 2017년 03월 21일 12:54, Chanwoo Choi wrote: > On 2017년 03월 20일 22:00, Andy Shevchenko wrote: >> On Mon, 2017-03-20 at 10:33 +0900, Chanwoo Choi wrote: >>> On 2017년 03월 17일 18:55, Hans de Goede wrote: >> >>>> +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" >> >> I would keep as Hans did for the sake of consistency among all Whiskey >> Cove device drivers (and predecessors like Crystal Cove). > > The 'wcove' short word is not used in this patch. > If the author want to use the 'wcove', I recommend that > you should use the 'wcove' instead of 'wc' in this patch. > > And, I think that 'pwrsrc' is not ambiguous. I'm sorry. I used the wrong word. I mean that 'pwrsrc' is not correct. > Hans might use the 'pwrsrc' as the 'Power Source'. > But, this driver is not power source. Instead, > this driver supports the detection of external connector. > > I think 'power source' means the power supply instead of detector. > >> >> I understand your point from extcon subsystem view, but PMICs like >> Whiskey Cove are multi-functional devices, and thus naming across them >> (same prefix in use to be precise) is better idea. >> >>> >>>> + {}, >>>> +}; >>>> +MODULE_DEVICE_TABLE(platform, cht_wc_extcon_table); >> > >
Hi, I think it again. The "cht_wcove_pwrsrc' is ok, if the register use the 'pwrsrc' word and using the 'wcove' instead of 'wc'. It is not big matter. I agree to use the 'cht_wcove_pwrsrc'. Best Regards, Chanwoo Choi On 2017년 03월 21일 14:21, Chanwoo Choi wrote: > On 2017년 03월 21일 12:54, Chanwoo Choi wrote: >> On 2017년 03월 20일 22:00, Andy Shevchenko wrote: >>> On Mon, 2017-03-20 at 10:33 +0900, Chanwoo Choi wrote: >>>> On 2017년 03월 17일 18:55, Hans de Goede wrote: >>> >>>>> +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" >>> >>> I would keep as Hans did for the sake of consistency among all Whiskey >>> Cove device drivers (and predecessors like Crystal Cove). >> >> The 'wcove' short word is not used in this patch. >> If the author want to use the 'wcove', I recommend that >> you should use the 'wcove' instead of 'wc' in this patch. >> >> And, I think that 'pwrsrc' is not ambiguous. > > I'm sorry. I used the wrong word. I mean that 'pwrsrc' is not correct. > >> Hans might use the 'pwrsrc' as the 'Power Source'. >> But, this driver is not power source. Instead, >> this driver supports the detection of external connector. >> >> I think 'power source' means the power supply instead of detector. >> >>> >>> I understand your point from extcon subsystem view, but PMICs like >>> Whiskey Cove are multi-functional devices, and thus naming across them >>> (same prefix in use to be precise) is better idea. >>> >>>> >>>>> + {}, >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(platform, cht_wc_extcon_table); >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> >>>> --- >>>> 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@redhat.com> >>>> + * >>>> + * 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@redhat.com>"); >>>> +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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 + 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@redhat.com> + * + * Based on various non upstream patches to support the CHT Whiskey Cove PMIC: + * Copyright (C) 2013-2015 Intel Corporation. All rights reserved. + * + * 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; + + do { + ret = regmap_read(ext->regmap, CHT_WC_USBSRC, &usbsrc); + if (ret) { + dev_err(ext->dev, "Error reading usbsrc: %d\n", ret); + return ret; + } + status = usbsrc & CHT_WC_USBSRC_STS_MASK; + if (status == CHT_WC_USBSRC_STS_SUCCESS || + status == CHT_WC_USBSRC_STS_FAIL) + break; + + msleep(200); + } 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; + switch (ret) { + default: + dev_warn(ext->dev, "Unhandled charger type %d\n", ret); + /* Fall through treat as 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); +} + +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; + } + + 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 */ + 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); + +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); + return ret; + } + + platform_set_drvdata(pdev, ext); + device_create_file(ext->dev, &dev_attr_usb_id); + + return 0; +} + +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); + + return 0; +} + +static const struct platform_device_id cht_wc_extcon_table[] = { + { .name = "cht_wcove_pwrsrc" }, + {}, +}; +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@redhat.com>"); +MODULE_DESCRIPTION("Intel Cherrytrail Whiskey Cove PMIC extcon driver"); +MODULE_LICENSE("GPL v2");
Add a driver for charger detection / control on the Intel Cherrytrail Whiskey Cove PMIC. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- 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