Re: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Chanwoo Choi <cw00.choi@samsung.com>]

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.pallala@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