From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752376AbbDCDyc (ORCPT ); Thu, 2 Apr 2015 23:54:32 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:9460 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752249AbbDCDy3 (ORCPT ); Thu, 2 Apr 2015 23:54:29 -0400 X-AuditID: cbfee68d-f79296d000004278-40-551e0ef3d2e5 Message-id: <551E0EF3.4000600@samsung.com> Date: Fri, 03 Apr 2015 12:54:27 +0900 From: Chanwoo Choi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130106 Thunderbird/17.0.2 MIME-version: 1.0 To: Ramakrishna Pallala Cc: linux-kernel@vger.kernel.org, MyungJoo Ham , Lee Jones , Samuel Ortiz , Carlo Caione , Jacob Pan Subject: Re: [PATCH v2 2/2] extcon-axp288: Add axp288 extcon driver support References: <1428002388-16879-1-git-send-email-ramakrishna.pallala@intel.com> <1428002388-16879-3-git-send-email-ramakrishna.pallala@intel.com> In-reply-to: <1428002388-16879-3-git-send-email-ramakrishna.pallala@intel.com> Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrJIsWRmVeSWpSXmKPExsWyRsSkQPczn1yowdk+dYsHa9YzWyw9uJ7F 4v7Xo4wWl3fNYbO43biCzWLhm5tMFqe7WR3YPbpO9jF5LN7zksnjzrU9bB7zTgZ69G1Zxejx eZNcAFsUl01Kak5mWWqRvl0CV8btVbdZCuYuY6zYNmU9UwPjilbGLkZODgkBE4nvuw+yQthi EhfurWfrYuTiEBJYyijx8NFNdpiiN8c2MoPYQgKLGCWerueBKHrAKLHx2XQ2kASvgJbE6yU/ wBpYBFQljlydB9bABhTf/+IGWI2oQJjEyulXWCDqBSV+TL4HZHNwiAiYS9z7rwIyk1ngIdDM Lw1gvcIC3hKbD+5mhlg2kVFi7o5pTCAJTgE/ifW7NoEtYxbQkdjfOo0NwpaX2LzmLViDhMAl donNV1eyQVwkIPFt8iGwbRICshKbDjBDfCYpcXDFDZYJjGKzkNw0C8nYWUjGLmBkXsUomlqQ XFCclF5kqFecmFtcmpeul5yfu4kRGIOn/z3r3cF4+4D1IUYBDkYlHt6MPTKhQqyJZcWVuYcY TYGumMgsJZqcD4z0vJJ4Q2MzIwtTE1NjI3NLMyVxXkWpn8FCAumJJanZqakFqUXxRaU5qcWH GJk4OKUaGEMUvj5rPzWp3Pp9vfu72Jt18WbfD2oUarRPFn7faam+RDTx01zh+rSICJ75Emv4 6vbZV7KZyh+f+Cvbuy/sAUdH52b+/ITTpXdn3j4ZOHfOYUk7nsnXaqrUZDxmLD7/64VRmILB sa8nklUOBvxQqfQTkxP8fNFswaTsBS6X+g6UrLy3ut2BS4mlOCPRUIu5qDgRAA+Te3K8AgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrMIsWRmVeSWpSXmKPExsVy+t9jAd3PfHKhBhP+S1o8WLOe2WLpwfUs Fve/HmW0uLxrDpvF7cYVbBYL39xksjjdzerA7tF1so/JY/Gel0wed67tYfOYdzLQo2/LKkaP z5vkAtiiGhhtMlITU1KLFFLzkvNTMvPSbZW8g+Od403NDAx1DS0tzJUU8hJzU22VXHwCdN0y c4BOUVIoS8wpBQoFJBYXK+nbYZoQGuKmawHTGKHrGxIE12NkgAYS1jBm3F51m6Vg7jLGim1T 1jM1MK5oZexi5OSQEDCReHNsIzOELSZx4d56NhBbSGARo8TT9TxdjFxA9gNGiY3PpoMleAW0 JF4v+cEOYrMIqEocuToPrJkNKL7/xQ2wGlGBMImV06+wQNQLSvyYfA/I5uAQETCXuPdfBWQm s8BDoJlfGsB6hQW8JTYf3M0MsWwio8TcHdOYQBKcAn4S63dtAlvGLKAjsb91GhuELS+xec1b 5gmMArOQ7JiFpGwWkrIFjMyrGEVTC5ILipPScw31ihNzi0vz0vWS83M3MYIj/JnUDsaVDRaH GAU4GJV4eDP2yIQKsSaWFVfmHmKU4GBWEuFdsVs2VIg3JbGyKrUoP76oNCe1+BCjKTAIJjJL iSbnA5NPXkm8obGJmZGlkbmhhZGxuZI4r5J9W4iQQHpiSWp2ampBahFMHxMHp1QD4+G5a10+ /gySna6oFtl2sCtEf7beUqNjZkuOnAyYY/5zgkriE2ZLR/aut+/jA50+iPgLTvvp1vzptU7f 7cLrbwNN3myXPKFiwpWeUm5iM3HTmTofxxjWq99bsv/vvL1X2/vb7MapgjvUSreEz0j6PHfh y9PHHiSd5j726Miyg+GGJYJfvSXsO5RYijMSDbWYi4oTARn6OTYGAwAA DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ramakrishna, When I apply this patch for build test on extcon-next branch, conflict happen. you have to implement this patchset on latest extcon-next branch. Also, This patch must need more clean-up and use latest extcon helper API. When you implement extcon-axp288.c, I recommend you refer to merged extcon drivers. On 04/03/2015 04:19 AM, Ramakrishna Pallala wrote: > This patch adds the extcon support for AXP288 PMIC which > has the BC1.2 charger detection capability. Additionally > it also adds the USB mux switching support b/w SOC and PMIC > based on GPIO control. > > Signed-off-by: Ramakrishna Pallala > --- > drivers/extcon/Kconfig | 7 + > drivers/extcon/Makefile | 1 + > drivers/extcon/extcon-axp288.c | 479 ++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/axp20x.h | 5 + > 4 files changed, 492 insertions(+) > create mode 100644 drivers/extcon/extcon-axp288.c > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > index 6a1f7de..b8627f7 100644 > --- a/drivers/extcon/Kconfig > +++ b/drivers/extcon/Kconfig > @@ -93,4 +93,11 @@ config EXTCON_SM5502 > Silicon Mitus SM5502. The SM5502 is a USB port accessory > detector and switch. > > +config EXTCON_AXP288 > + tristate "AXP288 EXTCON support" I recommend to add manufactor name of AXP288 PMIC chip. > + depends on MFD_AXP20X && USB_PHY > + help > + Say Y here to enable support for USB peripheral detection > + and USB MUX switching by AXP288 PMIC. > + Need to relocate EXTCON_AXP288 configuration alpabetically. > endif # MULTISTATE_SWITCH > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile > index 0370b42..832ad79 100644 > --- a/drivers/extcon/Makefile > +++ b/drivers/extcon/Makefile > @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o > obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o > obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o > obj-$(CONFIG_EXTCON_SM5502) += extcon-sm5502.o > +obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o ditto. > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c > new file mode 100644 > index 0000000..2e75d45 > --- /dev/null > +++ b/drivers/extcon/extcon-axp288.c > @@ -0,0 +1,479 @@ > +/* > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver > + * > + * Copyright (C) 2015 Intel Corporation > + * Ramakrishna Pallala Need to add 'Author' in front of name. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define AXP288_PS_STAT_REG 0x00 > +#define PS_STAT_VBUS_TRIGGER (1 << 0) > +#define PS_STAT_BAT_CHRG_DIR (1 << 2) > +#define PS_STAT_VBUS_ABOVE_VHOLD (1 << 3) > +#define PS_STAT_VBUS_VALID (1 << 4) > +#define PS_STAT_VBUS_PRESENT (1 << 5) > + > +#define AXP288_BC_GLOBAL_REG 0x2c > +#define BC_GLOBAL_RUN (1 << 0) > +#define BC_GLOBAL_DET_STAT (1 << 2) > +#define BC_GLOBAL_DBP_TOUT (1 << 3) > +#define BC_GLOBAL_VLGC_COM_SEL (1 << 4) > +#define BC_GLOBAL_DCD_TOUT_MASK 0x60 > +#define BC_GLOBAL_DCD_TOUT_300MS 0x0 > +#define BC_GLOBAL_DCD_TOUT_100MS 0x1 > +#define BC_GLOBAL_DCD_TOUT_500MS 0x2 > +#define BC_GLOBAL_DCD_TOUT_900MS 0x3 > +#define BC_GLOBAL_DCD_DET_SEL (1 << 7) > + > +#define AXP288_BC_VBUS_CNTL_REG 0x2d > +#define VBUS_CNTL_DPDM_PD_EN (1 << 4) > +#define VBUS_CNTL_DPDM_FD_EN (1 << 5) > +#define VBUS_CNTL_FIRST_PO_STAT (1 << 6) > + > +#define AXP288_BC_USB_STAT_REG 0x2e > +#define USB_STAT_BUS_STAT_MASK 0x0f > +#define USB_STAT_BUS_STAT_OFFSET 0 > +#define USB_STAT_BUS_STAT_ATHD 0x0 > +#define USB_STAT_BUS_STAT_CONN 0x1 > +#define USB_STAT_BUS_STAT_SUSP 0x2 > +#define USB_STAT_BUS_STAT_CONF 0x3 > +#define USB_STAT_USB_SS_MODE (1 << 4) > +#define USB_STAT_DEAD_BAT_DET (1 << 6) > +#define USB_STAT_DBP_UNCFG (1 << 7) > + > +#define AXP288_BC_DET_STAT_REG 0x2f > +#define DET_STAT_MASK 0xe0 > +#define DET_STAT_OFFSET 5 > +#define DET_STAT_SDP 0x1 > +#define DET_STAT_CDP 0x2 > +#define DET_STAT_DCP 0x3 > + > +#define AXP288_PS_BOOT_REASON_REG 0x2 > + > +#define AXP288_PWRSRC_IRQ_CFG_REG 0x40 > +#define PWRSRC_IRQ_CFG_MASK 0x1c > + > +#define AXP288_BC12_IRQ_CFG_REG 0x45 > +#define BC12_IRQ_CFG_MASK 0x2 I recommend to gather axp288 registers as following by using 'enum' keyword and then add the mask definition using the BIT(n) macro. enum axp288_reg { AXP288_REG_PS_STAT_REG = 0x00, AXP288_REG_BC_GLOBAL_REG = 0x2c, AXP288_REG_BC_VBUS_CNTL_REG = 0x2d, AXP288_REG_BC_USB_STAT_REG = 0x2e, AXP288_REG_BC_DET_STAT_REG = 0x2f, ... AXP288_REG_END }; Also, I think need more description about mask/offset definition. > + > +#define AXP288_PWRSRC_INTR_NUM 4 Don't need this definition. You can add AXP288_ > + > +#define AXP288_DRV_NAME "axp288_extcon" Don't need this definition. You use drive name in structure axp288_extcon_driver. > + > +#define AXP288_EXTCON_CABLE_SDP "Slow-charger" > +#define AXP288_EXTCON_CABLE_CDP "Charge-downstream" > +#define AXP288_EXTCON_CABLE_DCP "Fast-charger" Use the capital letter for cable name instead of small letter. I'll change all cable names by using captical letter. > + > +#define EXTCON_GPIO_MUX_SEL_PMIC 0 > +#define EXTCON_GPIO_MUX_SEL_SOC 1 > + > +enum { Need to add enum name to improve readability and catch the correct meaning of this definitions. > + VBUS_FALLING_IRQ = 0, > + VBUS_RISING_IRQ, > + MV_CHNG_IRQ, > + BC_USB_CHNG_IRQ, If AXP288_PWRSRC_INTR_NUM means the number of this interrupt, you can add AXP288_PWRSRC_INTR_NUM value in this enum list without separate defintion (#define AXP288_PWRSRC_INTR_NUM). > +}; > + > +static const char *axp288_extcon_cables[] = { > + AXP288_EXTCON_CABLE_SDP, > + AXP288_EXTCON_CABLE_CDP, > + AXP288_EXTCON_CABLE_DCP, > + NULL, > +}; > + > +struct axp288_extcon_info { > + struct platform_device *pdev; Define 'struct device *dev' instead of 'struct platform_device *pdev'. I think you can support all case by using 'dev' instance in extcon driver. > + struct regmap *regmap; > + struct regmap_irq_chip_data *regmap_irqc; > + struct axp288_extcon_pdata *pdata; Are you sure that pdata is necessary? The axp288_extcon_pdata includes only gpio. You have to get the gpio value by using gpiod helper funciton as following: You can refer other extcon driver(extcon-usb-gpio.c)/ devm_gpiod_get(dev, "mux_cntl"); > + int irq[AXP288_PWRSRC_INTR_NUM]; > + struct extcon_dev *edev; > + struct usb_phy *otg; > + struct notifier_block extcon_nb; > + struct extcon_specific_cable_nb cable; > + bool is_sdp; I can't find any 'if statemenet' using 'is_sdp' variable. Just this driver store the false or true to 'is_sdp'. Remove the 'is_sdp'. > + bool usb_id_short; What is meaning of usb_id_short? You need to add the description. > +}; > + > +static char *pwr_up_down_info[] = { Add 'axp288' prefix to array name. > + /* bit 0 */ "Last wake caused by user pressing the power button", > + /* bit 2 */ "Last wake caused by a charger insertion", > + /* bit 1 */ "Last wake caused by a battery insertion", > + /* bit 3 */ "Last wake caused by SOC initiated global reset", > + /* bit 4 */ "Last wake caused by cold reset", > + /* bit 5 */ "Last shutdown caused by PMIC UVLO threshold", > + /* bit 6 */ "Last shutdown caused by SOC initiated cold off", > + /* bit 7 */ "Last shutdown caused by user pressing the power button", I don't prefer to add following comment in front of each entry. /* bit x */ > + NULL, > +}; Need to add more detailed description why this array is necessary. > + > +/* > + * Decode and log the given "reset source indicator" > + * register and then clear it. > + */ > +static void axp288_extcon_log_rsi(struct axp288_extcon_info *info, > + char **pwrsrc_rsi_info, int reg) > +{ > + char **rsi; What is meaning of 'rsi'? > + unsigned int val, i, clear_mask = 0; > + int ret; > + > + ret = regmap_read(info->regmap, reg, &val); > + for (i = 0, rsi = pwrsrc_rsi_info; *rsi; rsi++, i++) { > + if (val & BIT(i)) { > + dev_dbg(&info->pdev->dev, "%s\n", *rsi); > + clear_mask |= BIT(i); > + } > + } > + > + /* Clear the register value for next reboot (write 1 to clear bit) */ > + regmap_write(info->regmap, reg, clear_mask); > +} > + > +static int handle_chrg_det_event(struct axp288_extcon_info *info) > +{ > + static bool notify_otg, notify_charger; > + static char *cable; > + int ret, stat, cfg, pwr_stat; > + u8 chrg_type; > + bool vbus_attach = false; > + > + ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, &pwr_stat); > + if (ret < 0) { > + dev_err(&info->pdev->dev, "vbus status read error\n"); > + return ret; > + } > + > + vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT) && !info->usb_id_short; > + if (vbus_attach) { > + dev_dbg(&info->pdev->dev, "vbus present\n"); > + } else { > + dev_dbg(&info->pdev->dev, "vbus not present\n"); > + goto notify_otg; > + } > + > + /* Check charger detection completion status */ > + ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, &cfg); > + if (ret < 0) > + goto dev_det_ret; > + if (cfg & BC_GLOBAL_DET_STAT) { > + dev_dbg(&info->pdev->dev, "charger detection not complete!!\n"); > + goto dev_det_ret; > + } > + > + ret = regmap_read(info->regmap, AXP288_BC_DET_STAT_REG, &stat); > + if (ret < 0) > + goto dev_det_ret; > + dev_dbg(&info->pdev->dev, "stat:%x, cfg:%x\n", stat, cfg); > + > + chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_OFFSET; > + info->is_sdp = false; > + > + if (chrg_type == DET_STAT_SDP) { > + dev_dbg(&info->pdev->dev, "sdp cable connecetd\n"); > + notify_otg = true; > + notify_charger = true; > + info->is_sdp = true; > + cable = AXP288_EXTCON_CABLE_SDP; > + } else if (chrg_type == DET_STAT_CDP) { > + dev_dbg(&info->pdev->dev, "cdp cable connecetd\n"); > + notify_otg = true; > + notify_charger = true; > + cable = AXP288_EXTCON_CABLE_CDP; > + } else if (chrg_type == DET_STAT_DCP) { > + dev_dbg(&info->pdev->dev, "dcp cable connecetd\n"); > + notify_charger = true; > + cable = AXP288_EXTCON_CABLE_DCP; > + } else { > + dev_warn(&info->pdev->dev, > + "disconnect or unknown or ID event\n"); > + } > + > +notify_otg: > + if (notify_otg) { > + /* > + * If VBUS is absent Connect D+/D- lines to PMIC for BC > + * detection. Else connect them to SOC for USB communication. > + */ > + if (info->pdata->gpio_mux_cntl != NULL) > + gpiod_set_value(info->pdata->gpio_mux_cntl, > + vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC > + : EXTCON_GPIO_MUX_SEL_PMIC); > + > + atomic_notifier_call_chain(&info->otg->notifier, > + vbus_attach ? USB_EVENT_VBUS : USB_EVENT_NONE, NULL); > + } > + > + if (notify_charger) > + extcon_set_cable_state(info->edev, cable, vbus_attach); > + > + /* Clear the flags on disconnect event */ > + if (!vbus_attach) { > + notify_otg = false; > + notify_charger = false; > + } > + > + return 0; > + > +dev_det_ret: > + if (ret < 0) > + dev_warn(&info->pdev->dev, "BC Mod detection error\n"); > + > + return ret; > +} > + > +static irqreturn_t axp288_extcon_isr(int irq, void *data) > +{ > + struct axp288_extcon_info *info = data; > + unsigned int i; > + int ret; > + > + for (i = 0; i < AXP288_PWRSRC_INTR_NUM; i++) { > + if (info->irq[i] == irq) > + break; > + } > + > + if (i == AXP288_PWRSRC_INTR_NUM) { > + dev_warn(&info->pdev->dev, "spurious interrupt!!\n"); > + return IRQ_NONE; > + } > + > + ret = handle_chrg_det_event(info); > + if (ret < 0) > + dev_warn(&info->pdev->dev, "error in PWRSRC INT handling\n"); > + > + return IRQ_HANDLED; > +} > + > +static int axp288_extcon_registration(struct axp288_extcon_info *info) > +{ > + int ret; > + > + /* Register with extcon */ > + info->edev = devm_kzalloc(&info->pdev->dev, > + sizeof(struct extcon_dev), GFP_KERNEL); Use devm_extcon_dev_allocate() instead of devm_kzalloc() > + if (!info->edev) > + return -ENOMEM; > + > + info->edev->name = "extcon-axp288"; Don't need to assing extcon device name. extcon_dev_register() will use the device name as extcon device name. > + info->edev->supported_cable = axp288_extcon_cables; > + ret = extcon_dev_register(info->edev); Use devm_extcon_dev_register() instead of extcon_dev_register. > + if (ret) > + dev_err(&info->pdev->dev, "extcon registration failed!!\n"); > + > + return ret; > +} Don't need seprate this function. I prefer to execute devm_extcon_* function in probe funtion. > + > +static inline bool is_usb_host_mode(struct extcon_dev *evdev) > +{ > + return !!evdev->state; > +} The state of extcon_dev means the state of all supported cables. If you want to get the state of specific cable, you can use extcon_get_cable_state(). > + > +static int axp288_handle_extcon_event(struct notifier_block *nb, > + unsigned long event, void *param) > +{ > + struct axp288_extcon_info *info = > + container_of(nb, struct axp288_extcon_info, extcon_nb); > + struct extcon_dev *edev = param; > + int usb_host = is_usb_host_mode(edev); > + > + dev_info(&info->pdev->dev, > + "[extcon notification] evt:USB-Host val:%s\n", I recommend to use the standard log mesasge and use dev_dbg instead of dev_info. I think following message is very specific log. [extcon notification] evt:USB-Host val: > + usb_host ? "Connected" : "Disconnected"); > + > + /* > + * Set usb_id_short flag to avoid running charger detection logic > + * in case usb host. > + */ > + info->usb_id_short = usb_host; > + > + /* > + * Connect the USB mux to SOC in case of usb host else connect > + * it to PMIC. > + */ > + if (info->pdata->gpio_mux_cntl != NULL) { As I commented, need to remove pdata. > + dev_dbg(&info->pdev->dev, > + "usb_id_short=%d\n", info->usb_id_short); Ditto. Need to modify log message. > + if (info->usb_id_short) > + gpiod_set_value(info->pdata->gpio_mux_cntl, > + EXTCON_GPIO_MUX_SEL_SOC); > + else > + gpiod_set_value(info->pdata->gpio_mux_cntl, > + EXTCON_GPIO_MUX_SEL_PMIC); > + } > + > + return NOTIFY_OK; > +} > + > +static int axp288_init_gpio_mux_cntl(struct axp288_extcon_info *info) > +{ > + int ret; > + > + ret = gpio_request(desc_to_gpio(info->pdata->gpio_mux_cntl), "USB_MUX"); > + if (ret < 0) { > + dev_err(&info->pdev->dev, > + "usb mux gpio request failed:gpio=%d\n", > + desc_to_gpio(info->pdata->gpio_mux_cntl)); You can change the error message as following: "Failed to request the gpio" > + return ret; > + } > + gpiod_direction_output(info->pdata->gpio_mux_cntl, > + EXTCON_GPIO_MUX_SEL_PMIC); > + > + info->extcon_nb.notifier_call = axp288_handle_extcon_event; > + ret = extcon_register_interest(&info->cable, NULL, > + "USB-Host", &info->extcon_nb); NAK. Why do you register notifier_chain in this driver? Only extcon provider driver can be included int drivers/extcon/ directory. extcon_register_interest() function should be used for extcon consumer driver. > + if (ret) { > + dev_err(&info->pdev->dev, "failed to register extcon notifier\n"); > + return ret; > + } > + > + if (info->cable.edev) > + info->usb_id_short = > + is_usb_host_mode(info->cable.edev); > + if (info->usb_id_short) > + gpiod_set_value(info->pdata->gpio_mux_cntl, > + EXTCON_GPIO_MUX_SEL_SOC); > + > + return 0; > +} > + > +static int axp288_extcon_probe(struct platform_device *pdev) > +{ > + struct axp288_extcon_info *info; > + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent); > + int ret, i, pirq; > + > + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + info->pdev = pdev; > + info->regmap = axp20x->regmap; > + info->regmap_irqc = axp20x->regmap_irqc; If axp288_extcon_info structure includes the axp20x_dev structure, info->regmap and info->regmap_irqc is not necessary. > + info->pdata = pdev->dev.platform_data; I don't prefer to use platform_data. You have to get the platform data from devicetree by using OF helper function. > + > + if (!info->pdata) { > + /* TODO: Try ACPI provided pdata via device properties */ > + if (!device_property_present(&pdev->dev, > + "axp288_extcon_data\n")) > + dev_err(&pdev->dev, "no platform data\n"); > + return -ENODEV; > + } > + platform_set_drvdata(pdev, info); > + > + axp288_extcon_log_rsi(info, pwr_up_down_info, > + AXP288_PS_BOOT_REASON_REG); I need more detailed description. > + > + /* Register extcon device */ > + ret = axp288_extcon_registration(info); > + if (ret < 0) > + goto extcon_reg_failed; Remove the call of axp288_extcon_registration() > + > + /* Get otg transceiver phy */ > + info->otg = usb_get_phy(USB_PHY_TYPE_USB2); > + if (IS_ERR(info->otg)) { > + dev_warn(&info->pdev->dev, "Failed to get otg transceiver!!\n"); > + ret = PTR_ERR(info->otg); > + goto otg_reg_failed; > + } > + > + for (i = 0; i < AXP288_PWRSRC_INTR_NUM; i++) { > + pirq = platform_get_irq(pdev, i); > + info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq); > + if (info->irq[i] < 0) { > + dev_warn(&info->pdev->dev, > + "regmap_irq get virq failed for IRQ %d: %d\n", > + pirq, info->irq[i]); > + info->irq[i] = -1; > + goto intr_reg_failed; > + } > + ret = request_threaded_irq(info->irq[i], Use devm_request_threaded_irq(). > + NULL, axp288_extcon_isr, > + IRQF_ONESHOT, AXP288_DRV_NAME, info); You can use pdev->name instead of AXP288_DRV_NAME. > + if (ret) { > + dev_err(&pdev->dev, "request_irq fail :%d err:%d\n", > + info->irq[i], ret); > + goto intr_reg_failed; > + } > + } > + > + /* Set up gpio control for USB Mux */ > + if (info->pdata->gpio_mux_cntl != NULL) { > + ret = axp288_init_gpio_mux_cntl(info); > + if (ret < 0) > + goto intr_reg_failed; > + } > + > + /* Unmask VBUS interrupt */ > + regmap_write(info->regmap, AXP288_PWRSRC_IRQ_CFG_REG, > + PWRSRC_IRQ_CFG_MASK); > + regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG, > + BC_GLOBAL_RUN, 0); > + /* Unmask the BC1.2 complte interrupts */ > + regmap_write(info->regmap, AXP288_BC12_IRQ_CFG_REG, BC12_IRQ_CFG_MASK); > + /* Enable the charger detection logic */ > + regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG, > + BC_GLOBAL_RUN, BC_GLOBAL_RUN); > + > + return 0; > + > +intr_reg_failed: > + for (; i > 0; i--) { > + free_irq(info->irq[i - 1], info); > + info->irq[i - 1] = -1; > + } > + usb_put_phy(info->otg); > +otg_reg_failed: > + extcon_dev_unregister(info->edev); > +extcon_reg_failed: > + return ret; > +} > + > +static int axp288_extcon_remove(struct platform_device *pdev) > +{ > + struct axp288_extcon_info *info = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < AXP288_PWRSRC_INTR_NUM; i++) > + free_irq(info->irq[i], info); Don't need to free interrupt if you use devm_request_threaded_irq(). > + usb_put_phy(info->otg); > + extcon_dev_unregister(info->edev); Don't need to unregister extcon device if you use devm_extcon_*. > + return 0; > +} > + > +static struct platform_driver axp288_extcon_driver = { > + .probe = axp288_extcon_probe, > + .remove = axp288_extcon_remove, > + .driver = { > + .name = AXP288_DRV_NAME, > + }, > +}; > +module_platform_driver(axp288_extcon_driver); > + > +MODULE_AUTHOR("Ramakrishna Pallala "); > +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h > index dfabd6d..4ed8071 100644 > --- a/include/linux/mfd/axp20x.h > +++ b/include/linux/mfd/axp20x.h > @@ -275,4 +275,9 @@ struct axp20x_fg_pdata { > int thermistor_curve[MAX_THERM_CURVE_SIZE][2]; > }; > > +struct axp288_extcon_pdata { > + /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */ > + struct gpio_desc *gpio_mux_cntl; > +}; > + > #endif /* __LINUX_MFD_AXP20X_H */ > Thanks, Chanwoo Choi