From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932600AbbD0LAB (ORCPT ); Mon, 27 Apr 2015 07:00:01 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:50261 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932448AbbD0K77 (ORCPT ); Mon, 27 Apr 2015 06:59:59 -0400 X-AuditID: cbfee68d-f79266d0000049c9-41-553e16ad48f0 Message-id: <553E16AD.7010207@samsung.com> Date: Mon, 27 Apr 2015 19:59:57 +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: "Pallala, Ramakrishna" Cc: "linux-kernel@vger.kernel.org" , Lee Jones , Samuel Ortiz , Carlo Caione , Jacob Pan Subject: Re: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support References: <1428513134-31402-1-git-send-email-ramakrishna.pallala@intel.com> <1428513134-31402-3-git-send-email-ramakrishna.pallala@intel.com> <5539C62E.1000907@samsung.com> In-reply-to: Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprLIsWRmVeSWpSXmKPExsWyRsSkSHetmF2owaNTChYP1qxntlh6cD2L xf2vRxktLu+aw2ax8M1NJovT3awObB5dJ/uYPBbvecnkcefaHjaPeScDPT5vkgtgjeKySUnN ySxLLdK3S+DKOPuqm7GgQbvi9YyX7A2MJ5S6GDk5JARMJLZf72SDsMUkLtxbD2RzcQgJLGWU WPDjGwtM0ZpT15khEtMZJS7OegxV9QCo6uMtdpAqXgEtieu7/7GC2CwCqhI31+0Ai7MBxfe/ uAG2QlQgTGLl9CssEPWCEj8m3wOzRQSsJNoPbmYGsZkFbjJKzFopD2ILC3hLdE+6zw6x7A+j xKqzv5lAEpxAgy6032GBaNCR2N86jQ3ClpfYvOYtM8TZ+9glZm6PgzhIQOLb5ENA9RxAcVmJ TQegSiQlDq64wTKBUWwWkpNmIZk6C8nUBYzMqxhFUwuSC4qT0osM9YoTc4tL89L1kvNzNzEC Y+30v2e9OxhvH7A+xCjAwajEwysx0TZUiDWxrLgy9xCjKdAVE5mlRJPzgRGdVxJvaGxmZGFq YmpsZG5ppiTOqyj1M1hIID2xJDU7NbUgtSi+qDQntfgQIxMHp1QD45L7CiFuq2r8cjpNQnRv Kvx6/qL4i2/v/1uFN1qr3D+ab46d6f2/vm3Oztq4Sx56szNc/65XnXx7o9f0v1VVvidmfNx+ 1KLC9IPO0g8v1irGVAv7LjdlfDVTTuP47rWh9Q8Vnvyd/GDexBTvkPUtx23+PJBSv5bU/kpo 08L2A4dsprhfl1l7770SS3FGoqEWc1FxIgD69oqmsAIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrBIsWRmVeSWpSXmKPExsVy+t9jQd21YnahBpN6pS0erFnPbLH04HoW i/tfjzJaXN41h81i4ZubTBanu1kd2Dy6TvYxeSze85LJ4861PWwe804GenzeJBfAGtXAaJOR mpiSWqSQmpecn5KZl26r5B0c7xxvamZgqGtoaWGupJCXmJtqq+TiE6DrlpkDdICSQlliTilQ KCCxuFhJ3w7ThNAQN10LmMYIXd+QILgeIwM0kLCGMePsq27GggbtitczXrI3MJ5Q6mLk5JAQ MJFYc+o6M4QtJnHh3nq2LkYuDiGB6YwSF2c9hnIeMEos+HiLHaSKV0BL4vruf6wgNouAqsTN dTvA4mxA8f0vbrCB2KICYRIrp19hgagXlPgx+R6YLSJgJdF+cDPYNmaBm4wSs1bKg9jCAt4S 3ZPus0Ms+8MosersbyaQBCfQoAvtd1ggGnQk9rdOY4Ow5SU2r3nLPIFRYBaSHbOQlM1CUraA kXkVo2hqQXJBcVJ6rpFecWJucWleul5yfu4mRnAsP5PewbiqweIQowAHoxIPr8RE21Ah1sSy 4srcQ4wSHMxKIrzLRexChXhTEiurUovy44tKc1KLDzGaAoNgIrOUaHI+MM3klcQbGpuYGVka mRtaGBmbK4nzztGVCxUSSE8sSc1OTS1ILYLpY+LglGpg7L+4xo1J2/DAxkt7LilfPenp4em6 4MjPB3dPNn50fD81MTf9yZTLzZ21JW3R2XtkLB/M5v/L6bfpojtD0iY7v9Wz0iQS9B2iueSm Xf31vm5WkNf1dovlWec7UuwerevOLPFZ4M9pHOq1RS1OyqDRyn8934pN/3rSE2S+/v21M1TO +MOUU/83KLEUZyQaajEXFScCAJCxQd37AgAA 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 Pallala, On 04/27/2015 07:44 PM, Pallala, Ramakrishna wrote: > Hi Choi, > > Please find my response below. > >> On 04/09/2015 02:12 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 | 462 >> ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/mfd/axp20x.h | 5 + >>> 4 files changed, 475 insertions(+) >>> create mode 100644 drivers/extcon/extcon-axp288.c [snip] >>> +enum axp288_extcon_reg { >>> + AXP288_PS_STAT_REG = 0x00, >>> + AXP288_PS_BOOT_REASON_REG = 0x02, >>> + AXP288_BC_GLOBAL_REG = 0x2c, >>> + AXP288_BC_VBUS_CNTL_REG = 0x2d, >>> + AXP288_BC_USB_STAT_REG = 0x2e, >>> + AXP288_BC_DET_STAT_REG = 0x2f, >>> + AXP288_PWRSRC_IRQ_CFG_REG = 0x40, >>> + AXP288_BC12_IRQ_CFG_REG = 0x45, >>> +}; >>> + >>> +#define AXP288_DRV_NAME "axp288_extcon" >> >> Use the '-' instead of '_' >> - axp288_extcon -> axp288-extcon > > All axp20x pmic mfd cell names are starting with "axp288_*" this one comment I got from Lee Jones on mfd patch. OK. [snip] >>> +struct axp288_extcon_info { >>> + struct device *dev; >>> + struct regmap *regmap; >>> + struct regmap_irq_chip_data *regmap_irqc; >>> + struct axp288_extcon_pdata *pdata; >>> + int irq[EXTCON_IRQ_END]; >>> + struct extcon_dev *edev; >>> + struct usb_phy *otg; >>> + struct notifier_block extcon_nb; >>> + struct extcon_specific_cable_nb cable; >>> + /* detect OTG or ID events */ >> >> This driver have the feature of both extcon provider driver and extcon consumer >> driver. > Yes, it's acts as both...I will remove the consumer support from this patch as you suggested in later comments. OK. [snip] > >> >>> + ret = PTR_ERR(info->otg); >>> + return ret; >>> + } >>> + >>> + for (i = 0; i < EXTCON_IRQ_END; 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(&pdev->dev, >>> + "regmap_irq get virq failed for IRQ %d: %d\n", >>> + pirq, info->irq[i]); >>> + info->irq[i] = -1; >>> + goto intr_reg_failed; >>> + } >> >> Need to add blank line > Not sure how it came here...i don't see the line in vim I think that need the blank line between '}' and devm_request_threaded_irq() sentence. > >> >>> + ret = devm_request_threaded_irq(&pdev->dev, info->irq[i], >>> + NULL, axp288_extcon_isr, >>> + IRQF_ONESHOT | IRQF_NO_SUSPEND, >>> + pdev->name, info); >>> + 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) { >> >> Modify if statement as following simply: >> if (info->pdata->gpio_mux_cntl) > Ok.. > >> >>> + 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); >> >> I think that you better to move the initialization code before extcon registration. >> The initialization should be executed before extcon and interrupt registration. > My intention is to enable the interrupts once after setting all the interrupt and extcon handlers. OK. But, I'd like you to add following functions to include the init code for enabling interrupt. axp288_extcon_enable_irq() or axp288_extcon_init() > >> >>> + >>> + return 0; >>> + >>> +intr_reg_failed: >>> + usb_put_phy(info->otg); >>> + return ret; >>> +} >>> + >>> +static int axp288_extcon_remove(struct platform_device *pdev) { >>> + struct axp288_extcon_info *info = platform_get_drvdata(pdev); >>> + >>> + usb_put_phy(info->otg); >>> + return 0; >>> +} >>> + >>> +static struct platform_driver axp288_extcon_driver = { >>> + .probe = axp288_extcon_probe, >>> + .remove = axp288_extcon_remove, >>> + .driver = { >>> + .name = "axp288_extcon", >>> + }, >> >> You should add the suspend/resume funciton to control the interrupt during >> suspend state. I discussed the same issue on patch[2]. >> And you can refer to extcon-usb-gpio.c[3] for controlling interrupt in >> suspend/resume function. >> >> [2] https://lkml.org/lkml/2015/1/27/1069 >> [3] >> http://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extc >> on-next&id=e52817faae359ce95c93c2b6eb88b16d4b430181 > The actual HW interrupt is controlled/handled by mfd driver. This driver only gets the virtual interrupts. > I don't see the need to enable/disable the interrupts in this case... OK. I understand. Thanks, Chanwoo Choi