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 <ramakrishna.pall...@intel.com>
>>> ---
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
