Re: [PATCH v8] extcon-axp288: Add axp288 extcon driver support

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

Hi Ram,

I added some comment.
If you fix minor issue according to comment, I'll apply it.

On 04/30/2015 10:43 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 |  386 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h     |    5 +
>  4 files changed, 399 insertions(+)
>  create mode 100644 drivers/extcon/extcon-axp288.c

[snip]

> +static int axp288_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->dev, "failed to read vbus status\n");
> +		return ret;
> +	}
> +
> +	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
> +	if (!vbus_attach)
> +		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->dev, "can't complete the charger detection\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->dev, "status:%x, config:%x\n", stat, cfg);

As I said on previous reply, it is not necessary about just register value
without appropriate log message.

> +
> +	chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_SHIFT;
> +
> +	switch (chrg_type) {
> +	case DET_STAT_SDP:
> +		dev_dbg(info->dev, "sdp cable is connecetd\n");
> +		notify_otg = true;
> +		notify_charger = true;
> +		cable = AXP288_EXTCON_SLOW_CHARGER;
> +		break;
> +	case DET_STAT_CDP:
> +		dev_dbg(info->dev, "cdp cable is connecetd\n");
> +		notify_otg = true;
> +		notify_charger = true;
> +		cable = AXP288_EXTCON_DOWNSTREAM_CHARGER;
> +		break;
> +	case DET_STAT_DCP:
> +		dev_dbg(info->dev, "dcp cable is connecetd\n");
> +		notify_charger = true;
> +		cable = AXP288_EXTCON_FAST_CHARGER;
> +		break;
> +	default:
> +		dev_warn(info->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)
> +			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 = notify_charger = false;
> +
> +	return 0;
> +
> +dev_det_ret:
> +	if (ret < 0)
> +		dev_err(info->dev, "BC Mod detection error\n");

"BC Mod detection error\n" -> "failed to detect BC Mod\n"

> +
> +	return ret;
> +}
> +
> +static irqreturn_t axp288_extcon_isr(int irq, void *data)
> +{
> +	struct axp288_extcon_info *info = data;
> +	int ret;
> +
> +	ret = axp288_handle_chrg_det_event(info);
> +	if (ret < 0)
> +		dev_err(info->dev, "error in PWRSRC INT handling\n");

As I saied on previous reply, you better to modify log message as following:
	"failed to handle the interrupt\n"

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void axp288_extcon_enable_irq(struct axp288_extcon_info *info)
> +{
> +	/* 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 complete 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);
> +}
> +
> +static int axp288_extcon_probe(struct platform_device *pdev)
> +{

[snip]

> +
> +	/* Register extcon device */
> +	ret = devm_extcon_dev_register(&pdev->dev, info->edev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	/* Get otg transceiver phy */
> +	info->otg = usb_get_phy(USB_PHY_TYPE_USB2);
> +	if (IS_ERR(info->otg)) {
> +		dev_warn(&pdev->dev, "failed to get otg transceiver\n");

dev_warn -> dev_err

> +		return PTR_ERR(info->otg);
> +	}
> +
> +	/* Set up gpio control for USB Mux */
> +	if (info->pdata->gpio_mux_cntl) {
> +		gpio = desc_to_gpio(info->pdata->gpio_mux_cntl);
> +		ret = gpio_request(gpio, "USB_MUX");
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to request the gpio=%d\n", gpio);
> +			goto gpio_req_failed;
> +		}
> +		gpiod_direction_output(info->pdata->gpio_mux_cntl,
> +						EXTCON_GPIO_MUX_SEL_PMIC);
> +	}
> +
> +	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,
> +				"failed to get virtual interrupt=%d\n", pirq);
> +			ret = info->irq[i];

dev_warn -> dev_err

[snip]

Thanks,
Chanwoo Choi