lkml.org 
[lkml]   [2017]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 03/15] extcon: cht-wc: Add Intel Cherry Trail Whiskey Cove PMIC extcon driver
    From
    Date
    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

    \
     
     \ /
      Last update: 2017-03-23 16:23    [W:3.016 / U:0.264 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site