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

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

[Lee Jones]

On Fri, 03 Apr 2015, 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 |  479 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h     |    5 +

Acked-by: Lee Jones <lee.jones@linaro.org>

>  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"
> +	depends on MFD_AXP20X && USB_PHY
> +	help
> +	  Say Y here to enable support for USB peripheral detection
> +	  and USB MUX switching by AXP288 PMIC.
> +
>  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
> 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 <ramakrishna.pallala@intel.com>
> + *
> + * 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 <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/usb/phy.h>
> +#include <linux/notifier.h>
> +#include <linux/extcon.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/axp20x.h>
> +
> +#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
> +
> +#define AXP288_PWRSRC_INTR_NUM		4
> +
> +#define AXP288_DRV_NAME			"axp288_extcon"
> +
> +#define AXP288_EXTCON_CABLE_SDP		"Slow-charger"
> +#define AXP288_EXTCON_CABLE_CDP		"Charge-downstream"
> +#define AXP288_EXTCON_CABLE_DCP		"Fast-charger"
> +
> +#define EXTCON_GPIO_MUX_SEL_PMIC	0
> +#define EXTCON_GPIO_MUX_SEL_SOC		1
> +
> +enum {
> +	VBUS_FALLING_IRQ = 0,
> +	VBUS_RISING_IRQ,
> +	MV_CHNG_IRQ,
> +	BC_USB_CHNG_IRQ,
> +};
> +
> +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;
> +	struct regmap *regmap;
> +	struct regmap_irq_chip_data *regmap_irqc;
> +	struct axp288_extcon_pdata *pdata;
> +	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;
> +	bool usb_id_short;
> +};
> +
> +static char *pwr_up_down_info[] = {
> +	/* 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",
> +	NULL,
> +};
> +
> +/*
> + * 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;
> +	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);
> +	if (!info->edev)
> +		return -ENOMEM;
> +
> +	info->edev->name = "extcon-axp288";
> +	info->edev->supported_cable = axp288_extcon_cables;
> +	ret = extcon_dev_register(info->edev);
> +	if (ret)
> +		dev_err(&info->pdev->dev, "extcon registration failed!!\n");
> +
> +	return ret;
> +}
> +
> +static inline bool is_usb_host_mode(struct extcon_dev *evdev)
> +{
> +	return !!evdev->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",
> +		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) {
> +		dev_dbg(&info->pdev->dev,
> +			"usb_id_short=%d\n", info->usb_id_short);
> +		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));
> +		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);
> +	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;
> +	info->pdata = pdev->dev.platform_data;
> +
> +	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);
> +
> +	/* Register extcon device */
> +	ret = axp288_extcon_registration(info);
> +	if (ret < 0)
> +		goto extcon_reg_failed;
> +
> +	/* 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],
> +				NULL, axp288_extcon_isr,
> +				IRQF_ONESHOT, AXP288_DRV_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) {
> +		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);
> +	usb_put_phy(info->otg);
> +	extcon_dev_unregister(info->edev);
> +	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 <ramakrishna.pallala@intel.com>");
> +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 */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/2] mfd/axp20x: add support for extcon cell

[Lee Jones]

On Fri, 03 Apr 2015, Ramakrishna Pallala wrote:

> This patch adds the mfd cell info for axp288 extcon device.
> 
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> ---
>  drivers/mfd/axp20x.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

Applied, thanks.

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 0acbe52..a569721 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -290,6 +290,29 @@ static struct resource axp288_adc_resources[] = {
>  	},
>  };
>  
> +static struct resource axp288_extcon_resources[] = {
> +	{
> +		.start = AXP288_IRQ_VBUS_FALL,
> +		.end   = AXP288_IRQ_VBUS_FALL,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	{
> +		.start = AXP288_IRQ_VBUS_RISE,
> +		.end   = AXP288_IRQ_VBUS_RISE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	{
> +		.start = AXP288_IRQ_MV_CHNG,
> +		.end   = AXP288_IRQ_MV_CHNG,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	{
> +		.start = AXP288_IRQ_BC_USB_CHNG,
> +		.end   = AXP288_IRQ_BC_USB_CHNG,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
>  static struct resource axp288_charger_resources[] = {
>  	{
>  		.start = AXP288_IRQ_OV,
> @@ -345,6 +368,11 @@ static struct mfd_cell axp288_cells[] = {
>  		.resources = axp288_adc_resources,
>  	},
>  	{
> +		.name = "axp288_extcon",
> +		.num_resources = ARRAY_SIZE(axp288_extcon_resources),
> +		.resources = axp288_extcon_resources,
> +	},
> +	{
>  		.name = "axp288_charger",
>  		.num_resources = ARRAY_SIZE(axp288_charger_resources),
>  		.resources = axp288_charger_resources,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 0/2] Add extcon support for AXP288 PMIC

[Ramakrishna Pallala]

This patch series adds the support for axp288 extcon driver
and also adds the cell info for extcon device in axp20x mfd driver.

Ramakrishna Pallala (2):
  mfd/axp20x: add support for extcon cell
  extcon-axp288: Add axp288 extcon driver support

 drivers/extcon/Kconfig         |    7 +
 drivers/extcon/Makefile        |    1 +
 drivers/extcon/extcon-axp288.c |  479 ++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/axp20x.c           |   28 +++
 include/linux/mfd/axp20x.h     |    5 +
 5 files changed, 520 insertions(+)
 create mode 100644 drivers/extcon/extcon-axp288.c

-- 
1.7.9.5

[PATCH v2 1/2] mfd/axp20x: add support for extcon cell

[Ramakrishna Pallala]

This patch adds the mfd cell info for axp288 extcon device.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
---
 drivers/mfd/axp20x.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 0acbe52..a569721 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -290,6 +290,29 @@ static struct resource axp288_adc_resources[] = {
 	},
 };
 
+static struct resource axp288_extcon_resources[] = {
+	{
+		.start = AXP288_IRQ_VBUS_FALL,
+		.end   = AXP288_IRQ_VBUS_FALL,
+		.flags = IORESOURCE_IRQ,
+	},
+	{
+		.start = AXP288_IRQ_VBUS_RISE,
+		.end   = AXP288_IRQ_VBUS_RISE,
+		.flags = IORESOURCE_IRQ,
+	},
+	{
+		.start = AXP288_IRQ_MV_CHNG,
+		.end   = AXP288_IRQ_MV_CHNG,
+		.flags = IORESOURCE_IRQ,
+	},
+	{
+		.start = AXP288_IRQ_BC_USB_CHNG,
+		.end   = AXP288_IRQ_BC_USB_CHNG,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
 static struct resource axp288_charger_resources[] = {
 	{
 		.start = AXP288_IRQ_OV,
@@ -345,6 +368,11 @@ static struct mfd_cell axp288_cells[] = {
 		.resources = axp288_adc_resources,
 	},
 	{
+		.name = "axp288_extcon",
+		.num_resources = ARRAY_SIZE(axp288_extcon_resources),
+		.resources = axp288_extcon_resources,
+	},
+	{
 		.name = "axp288_charger",
 		.num_resources = ARRAY_SIZE(axp288_charger_resources),
 		.resources = axp288_charger_resources,
-- 
1.7.9.5

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

[Ramakrishna Pallala]

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 |  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"
+	depends on MFD_AXP20X && USB_PHY
+	help
+	  Say Y here to enable support for USB peripheral detection
+	  and USB MUX switching by AXP288 PMIC.
+
 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
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 <ramakrishna.pallala@intel.com>
+ *
+ * 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 <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/usb/phy.h>
+#include <linux/notifier.h>
+#include <linux/extcon.h>
+#include <linux/regmap.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mfd/axp20x.h>
+
+#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
+
+#define AXP288_PWRSRC_INTR_NUM		4
+
+#define AXP288_DRV_NAME			"axp288_extcon"
+
+#define AXP288_EXTCON_CABLE_SDP		"Slow-charger"
+#define AXP288_EXTCON_CABLE_CDP		"Charge-downstream"
+#define AXP288_EXTCON_CABLE_DCP		"Fast-charger"
+
+#define EXTCON_GPIO_MUX_SEL_PMIC	0
+#define EXTCON_GPIO_MUX_SEL_SOC		1
+
+enum {
+	VBUS_FALLING_IRQ = 0,
+	VBUS_RISING_IRQ,
+	MV_CHNG_IRQ,
+	BC_USB_CHNG_IRQ,
+};
+
+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;
+	struct regmap *regmap;
+	struct regmap_irq_chip_data *regmap_irqc;
+	struct axp288_extcon_pdata *pdata;
+	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;
+	bool usb_id_short;
+};
+
+static char *pwr_up_down_info[] = {
+	/* 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",
+	NULL,
+};
+
+/*
+ * 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;
+	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);
+	if (!info->edev)
+		return -ENOMEM;
+
+	info->edev->name = "extcon-axp288";
+	info->edev->supported_cable = axp288_extcon_cables;
+	ret = extcon_dev_register(info->edev);
+	if (ret)
+		dev_err(&info->pdev->dev, "extcon registration failed!!\n");
+
+	return ret;
+}
+
+static inline bool is_usb_host_mode(struct extcon_dev *evdev)
+{
+	return !!evdev->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",
+		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) {
+		dev_dbg(&info->pdev->dev,
+			"usb_id_short=%d\n", info->usb_id_short);
+		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));
+		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);
+	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;
+	info->pdata = pdev->dev.platform_data;
+
+	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);
+
+	/* Register extcon device */
+	ret = axp288_extcon_registration(info);
+	if (ret < 0)
+		goto extcon_reg_failed;
+
+	/* 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],
+				NULL, axp288_extcon_isr,
+				IRQF_ONESHOT, AXP288_DRV_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) {
+		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);
+	usb_put_phy(info->otg);
+	extcon_dev_unregister(info->edev);
+	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 <ramakrishna.pallala@intel.com>");
+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 */
-- 
1.7.9.5

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

[Chanwoo Choi]

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 <ramakrishna.pallala@intel.com>
> ---
>  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 <ramakrishna.pallala@intel.com>

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 <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/usb/phy.h>
> +#include <linux/notifier.h>
> +#include <linux/extcon.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/axp20x.h>
> +
> +#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 <ramakrishna.pallala@intel.com>");
> +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

RE: [PATCH v2 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

> 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.
Ok I will create the patches on git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

> 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.
Ok.

> > --- 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.
Ok.

> > +	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.
Ok.

> >  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.
Ok.

> > 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 <ramakrishna.pallala@intel.com>
> 
> Need to add 'Author' in front of name.
Ok.

> > +#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.
Ok.

> 
> > +
> > +#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 capital letter.
You mean like this... "Slow-charger" => "SLOW-CHARGER"?

> 
> > +
> > +#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.
Ok.

> > +	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).
Ok.

> > +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.
Ok I will check.

> > +	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.
Yes, as I have plans to include more info to pdata struct.

> 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");
> 
I will check.

> > +	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'.
I forgot to remove this. I will fix it now.

> 
> 
> > +	bool usb_id_short;
> 
> What is meaning of usb_id_short? You need to add the description.
> 
Ok.

> > +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.
Ok.

> > +
> > +/*
> > + * 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'?
"reset source indicator"

> > +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.
Ok.
 
> > +
> > +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().
Ok.

> > +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");
As this handler is specifically registered for usb-host cable I think we have specific message?
If you not agree, please suggest any standard log message.

> > +
> > +	/*
> > +	 * 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.
This message is redundant. I will remove it.

> > +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;
> > +	}
This driver acts as consumer for charger detection logic and also acts as consumer for OTG detection for mux switching.
And also to avoid false charger notification for Host mode case.

> > +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.
Then I would need "axp20x_dev->" every time I use regmap...

> 
> > +	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.
> 
My platform don't use device tree....it uses SFI or ACPI based approach...I cannot remove this at the moment..

> > +
> > +	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.
This functions basically logs the system power up/down reason for debug purpose. Also there is user space daemon which parses these logs in my board.


> > +
> > +	/* Register extcon device */
> > +	ret = axp288_extcon_registration(info);
> > +	if (ret < 0)
> > +		goto extcon_reg_failed;
> 
> Remove the call of axp288_extcon_registration()
> 
Ok.

> > +
> > +	/* 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.
Ok.

> > +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().
Ok.


> > +	usb_put_phy(info->otg);
> > +	extcon_dev_unregister(info->edev);
> 
> Don't need to unregister extcon device if you use devm_extcon_*.
Ok.

Thanks,
Ram