Re: [PATCH 03/15] extcon: cht-wc: Add Intel Cherry Trail Whiskey Cove PMIC extcon driver

Hans de Goede
2017-03-23T15:22:33Z

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