2015-04-28 08:05:04

by Pallala, Ramakrishna

[permalink] [raw]
Subject: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

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 <[email protected]>
---
drivers/extcon/Kconfig | 7 +
drivers/extcon/Makefile | 1 +
drivers/extcon/extcon-axp288.c | 399 ++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/axp20x.h | 9 +
4 files changed, 416 insertions(+)
create mode 100644 drivers/extcon/extcon-axp288.c

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index fdc0bf0..2305edc 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -28,6 +28,13 @@ config EXTCON_ARIZONA
with Wolfson Arizona devices. These are audio CODECs with
advanced audio accessory detection support.

+config EXTCON_AXP288
+ tristate "X-Power 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 X-Power AXP288 PMIC.
+
config EXTCON_GPIO
tristate "GPIO extcon support"
depends on GPIOLIB
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 9204114..ba787d0 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -5,6 +5,7 @@
obj-$(CONFIG_EXTCON) += extcon.o
obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
+obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o
obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o
obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o
obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o
diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
new file mode 100644
index 0000000..91d764c
--- /dev/null
+++ b/drivers/extcon/extcon-axp288.c
@@ -0,0 +1,399 @@
+/*
+ * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
+ *
+ * Copyright (C) 2014 Intel Corporation
+ * Author: Ramakrishna Pallala <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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>
+
+/* Power source status register */
+#define PS_STAT_VBUS_TRIGGER BIT(0)
+#define PS_STAT_BAT_CHRG_DIR BIT(2)
+#define PS_STAT_VBUS_ABOVE_VHOLD BIT(3)
+#define PS_STAT_VBUS_VALID BIT(4)
+#define PS_STAT_VBUS_PRESENT BIT(5)
+
+/* BC module global register */
+#define BC_GLOBAL_RUN BIT(0)
+#define BC_GLOBAL_DET_STAT BIT(2)
+#define BC_GLOBAL_DBP_TOUT BIT(3)
+#define BC_GLOBAL_VLGC_COM_SEL BIT(4)
+#define BC_GLOBAL_DCD_TOUT_MASK (BIT(6)|BIT(5))
+#define BC_GLOBAL_DCD_TOUT_300MS 0
+#define BC_GLOBAL_DCD_TOUT_100MS 1
+#define BC_GLOBAL_DCD_TOUT_500MS 2
+#define BC_GLOBAL_DCD_TOUT_900MS 3
+#define BC_GLOBAL_DCD_DET_SEL BIT(7)
+
+/* BC module vbus control and status register */
+#define VBUS_CNTL_DPDM_PD_EN BIT(4)
+#define VBUS_CNTL_DPDM_FD_EN BIT(5)
+#define VBUS_CNTL_FIRST_PO_STAT BIT(6)
+
+/* BC USB status register */
+#define USB_STAT_BUS_STAT_MASK (BIT(3)|BIT(2)|BIT(1)|BIT(0))
+#define USB_STAT_BUS_STAT_SHIFT 0
+#define USB_STAT_BUS_STAT_ATHD 0
+#define USB_STAT_BUS_STAT_CONN 1
+#define USB_STAT_BUS_STAT_SUSP 2
+#define USB_STAT_BUS_STAT_CONF 3
+#define USB_STAT_USB_SS_MODE BIT(4)
+#define USB_STAT_DEAD_BAT_DET BIT(6)
+#define USB_STAT_DBP_UNCFG BIT(7)
+
+/* BC detect status register */
+#define DET_STAT_MASK (BIT(7)|BIT(6)|BIT(5))
+#define DET_STAT_SHIFT 5
+#define DET_STAT_SDP 1
+#define DET_STAT_CDP 2
+#define DET_STAT_DCP 3
+
+/* IRQ enable-1 register */
+#define PWRSRC_IRQ_CFG_MASK (BIT(4)|BIT(3)|BIT(2))
+
+/* IRQ enable-6 register */
+#define BC12_IRQ_CFG_MASK BIT(1)
+
+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,
+};
+
+enum axp288_mux_select {
+ EXTCON_GPIO_MUX_SEL_PMIC = 0,
+ EXTCON_GPIO_MUX_SEL_SOC,
+};
+
+enum axp288_extcon_irq {
+ VBUS_FALLING_IRQ = 0,
+ VBUS_RISING_IRQ,
+ MV_CHNG_IRQ,
+ BC_USB_CHNG_IRQ,
+ EXTCON_IRQ_END,
+};
+
+static const char *axp288_extcon_cables[] = {
+ AXP288_EXTCON_CABLE_SDP,
+ AXP288_EXTCON_CABLE_CDP,
+ AXP288_EXTCON_CABLE_DCP,
+ NULL,
+};
+
+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 notifier_block extcon_nb;
+ struct usb_phy *otg;
+};
+
+/* Power up/down reason string array */
+static char *axp288_pwr_up_down_info[] = {
+ "Last wake caused by user pressing the power button",
+ "Last wake caused by a charger insertion",
+ "Last wake caused by a battery insertion",
+ "Last wake caused by SOC initiated global reset",
+ "Last wake caused by cold reset",
+ "Last shutdown caused by PMIC UVLO threshold",
+ "Last shutdown caused by SOC initiated cold off",
+ "Last shutdown caused by user pressing the power button",
+ NULL,
+};
+
+/*
+ * Decode and log the given "reset source indicator" (rsi)
+ * register and then clear it.
+ */
+static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
+{
+ char **rsi;
+ unsigned int val, i, clear_mask = 0;
+ int ret;
+
+ ret = regmap_read(info->regmap, AXP288_PS_BOOT_REASON_REG, &val);
+ for (i = 0, rsi = axp288_pwr_up_down_info; *rsi; rsi++, i++) {
+ if (val & BIT(i)) {
+ dev_dbg(info->dev, "%s\n", *rsi);
+ clear_mask |= BIT(i);
+ }
+ }
+
+ /* Clear the register value for next reboot (write 1 to clear bit) */
+ regmap_write(info->regmap, AXP288_PS_BOOT_REASON_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->dev, "vbus status read error\n");
+ return ret;
+ }
+
+ vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
+ if (vbus_attach) {
+ dev_dbg(info->dev, "vbus present\n");
+ } else {
+ dev_dbg(info->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->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->dev, "stat:%x, cfg:%x\n", stat, cfg);
+
+ chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_SHIFT;
+
+ if (chrg_type == DET_STAT_SDP) {
+ dev_dbg(info->dev, "sdp cable connecetd\n");
+ notify_otg = true;
+ notify_charger = true;
+ cable = AXP288_EXTCON_CABLE_SDP;
+ } else if (chrg_type == DET_STAT_CDP) {
+ dev_dbg(info->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->dev, "dcp cable connecetd\n");
+ notify_charger = true;
+ cable = AXP288_EXTCON_CABLE_DCP;
+ } else {
+ 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 = false;
+ notify_charger = false;
+ }
+
+ return 0;
+
+dev_det_ret:
+ if (ret < 0)
+ dev_warn(info->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 < EXTCON_IRQ_END; i++) {
+ if (info->irq[i] == irq)
+ break;
+ }
+
+ if (i == EXTCON_IRQ_END) {
+ dev_warn(info->dev, "spurious interrupt!!\n");
+ return IRQ_NONE;
+ }
+
+ ret = handle_chrg_det_event(info);
+ if (ret < 0)
+ dev_warn(info->dev, "error in PWRSRC INT handling\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 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);
+}
+
+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, gpio;
+
+ info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ info->dev = &pdev->dev;
+ 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);
+
+ /* Initialize extcon device */
+ info->edev = devm_extcon_dev_allocate(&pdev->dev,
+ axp288_extcon_cables);
+ if (IS_ERR(info->edev)) {
+ dev_err(&pdev->dev, "failed to allocate memory for extcon\n");
+ ret = PTR_ERR(info->edev);
+ return ret;
+ }
+
+ /* 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");
+ ret = PTR_ERR(info->otg);
+ return ret;
+ }
+
+ /* 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,
+ "regmap_irq get virq failed for IRQ %d: %d\n",
+ pirq, info->irq[i]);
+ info->irq[i] = -1;
+ goto intr_reg_failed;
+ }
+
+ 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;
+ }
+ }
+
+ /* Enable interrupts */
+ axp288_extcon_enable_irq(info);
+
+ return 0;
+
+intr_reg_failed:
+gpio_req_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",
+ },
+};
+module_platform_driver(axp288_extcon_driver);
+
+MODULE_AUTHOR("Ramakrishna Pallala <[email protected]>");
+MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index dfabd6d..81152e2 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
};

+#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
+#define AXP288_EXTCON_CABLE_CDP "CHARGE-DOWNSTREAM"
+#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"
+
+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


2015-04-28 10:04:50

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

On Tue, 28 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 <[email protected]>
> ---
> drivers/extcon/Kconfig | 7 +
> drivers/extcon/Makefile | 1 +
> drivers/extcon/extcon-axp288.c | 399 ++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/axp20x.h | 9 +
> 4 files changed, 416 insertions(+)
> create mode 100644 drivers/extcon/extcon-axp288.c
>
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index fdc0bf0..2305edc 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> with Wolfson Arizona devices. These are audio CODECs with
> advanced audio accessory detection support.
>
> +config EXTCON_AXP288
> + tristate "X-Power 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 X-Power AXP288 PMIC.
> +
> config EXTCON_GPIO
> tristate "GPIO extcon support"
> depends on GPIOLIB
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 9204114..ba787d0 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -5,6 +5,7 @@
> obj-$(CONFIG_EXTCON) += extcon.o
> obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
> obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
> +obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o
> obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o
> obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o
> obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> new file mode 100644
> index 0000000..91d764c
> --- /dev/null
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -0,0 +1,399 @@
> +/*
> + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
> + *
> + * Copyright (C) 2014 Intel Corporation
> + * Author: Ramakrishna Pallala <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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>
> +
> +/* Power source status register */
> +#define PS_STAT_VBUS_TRIGGER BIT(0)
> +#define PS_STAT_BAT_CHRG_DIR BIT(2)
> +#define PS_STAT_VBUS_ABOVE_VHOLD BIT(3)
> +#define PS_STAT_VBUS_VALID BIT(4)
> +#define PS_STAT_VBUS_PRESENT BIT(5)
> +
> +/* BC module global register */
> +#define BC_GLOBAL_RUN BIT(0)
> +#define BC_GLOBAL_DET_STAT BIT(2)
> +#define BC_GLOBAL_DBP_TOUT BIT(3)
> +#define BC_GLOBAL_VLGC_COM_SEL BIT(4)
> +#define BC_GLOBAL_DCD_TOUT_MASK (BIT(6)|BIT(5))
> +#define BC_GLOBAL_DCD_TOUT_300MS 0
> +#define BC_GLOBAL_DCD_TOUT_100MS 1
> +#define BC_GLOBAL_DCD_TOUT_500MS 2
> +#define BC_GLOBAL_DCD_TOUT_900MS 3
> +#define BC_GLOBAL_DCD_DET_SEL BIT(7)
> +
> +/* BC module vbus control and status register */
> +#define VBUS_CNTL_DPDM_PD_EN BIT(4)
> +#define VBUS_CNTL_DPDM_FD_EN BIT(5)
> +#define VBUS_CNTL_FIRST_PO_STAT BIT(6)
> +
> +/* BC USB status register */
> +#define USB_STAT_BUS_STAT_MASK (BIT(3)|BIT(2)|BIT(1)|BIT(0))
> +#define USB_STAT_BUS_STAT_SHIFT 0
> +#define USB_STAT_BUS_STAT_ATHD 0
> +#define USB_STAT_BUS_STAT_CONN 1
> +#define USB_STAT_BUS_STAT_SUSP 2
> +#define USB_STAT_BUS_STAT_CONF 3
> +#define USB_STAT_USB_SS_MODE BIT(4)
> +#define USB_STAT_DEAD_BAT_DET BIT(6)
> +#define USB_STAT_DBP_UNCFG BIT(7)
> +
> +/* BC detect status register */
> +#define DET_STAT_MASK (BIT(7)|BIT(6)|BIT(5))
> +#define DET_STAT_SHIFT 5
> +#define DET_STAT_SDP 1
> +#define DET_STAT_CDP 2
> +#define DET_STAT_DCP 3
> +
> +/* IRQ enable-1 register */
> +#define PWRSRC_IRQ_CFG_MASK (BIT(4)|BIT(3)|BIT(2))
> +
> +/* IRQ enable-6 register */
> +#define BC12_IRQ_CFG_MASK BIT(1)
> +
> +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,
> +};
> +
> +enum axp288_mux_select {
> + EXTCON_GPIO_MUX_SEL_PMIC = 0,
> + EXTCON_GPIO_MUX_SEL_SOC,
> +};
> +
> +enum axp288_extcon_irq {
> + VBUS_FALLING_IRQ = 0,
> + VBUS_RISING_IRQ,
> + MV_CHNG_IRQ,
> + BC_USB_CHNG_IRQ,
> + EXTCON_IRQ_END,
> +};
> +
> +static const char *axp288_extcon_cables[] = {
> + AXP288_EXTCON_CABLE_SDP,
> + AXP288_EXTCON_CABLE_CDP,
> + AXP288_EXTCON_CABLE_DCP,
> + NULL,
> +};
> +
> +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 notifier_block extcon_nb;
> + struct usb_phy *otg;
> +};
> +
> +/* Power up/down reason string array */
> +static char *axp288_pwr_up_down_info[] = {
> + "Last wake caused by user pressing the power button",
> + "Last wake caused by a charger insertion",
> + "Last wake caused by a battery insertion",
> + "Last wake caused by SOC initiated global reset",
> + "Last wake caused by cold reset",
> + "Last shutdown caused by PMIC UVLO threshold",
> + "Last shutdown caused by SOC initiated cold off",
> + "Last shutdown caused by user pressing the power button",
> + NULL,
> +};
> +
> +/*
> + * Decode and log the given "reset source indicator" (rsi)
> + * register and then clear it.
> + */
> +static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
> +{
> + char **rsi;
> + unsigned int val, i, clear_mask = 0;
> + int ret;
> +
> + ret = regmap_read(info->regmap, AXP288_PS_BOOT_REASON_REG, &val);
> + for (i = 0, rsi = axp288_pwr_up_down_info; *rsi; rsi++, i++) {
> + if (val & BIT(i)) {
> + dev_dbg(info->dev, "%s\n", *rsi);
> + clear_mask |= BIT(i);
> + }
> + }
> +
> + /* Clear the register value for next reboot (write 1 to clear bit) */
> + regmap_write(info->regmap, AXP288_PS_BOOT_REASON_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->dev, "vbus status read error\n");
> + return ret;
> + }
> +
> + vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
> + if (vbus_attach) {
> + dev_dbg(info->dev, "vbus present\n");
> + } else {
> + dev_dbg(info->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->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->dev, "stat:%x, cfg:%x\n", stat, cfg);
> +
> + chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_SHIFT;
> +
> + if (chrg_type == DET_STAT_SDP) {
> + dev_dbg(info->dev, "sdp cable connecetd\n");
> + notify_otg = true;
> + notify_charger = true;
> + cable = AXP288_EXTCON_CABLE_SDP;
> + } else if (chrg_type == DET_STAT_CDP) {
> + dev_dbg(info->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->dev, "dcp cable connecetd\n");
> + notify_charger = true;
> + cable = AXP288_EXTCON_CABLE_DCP;
> + } else {
> + 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 = false;
> + notify_charger = false;
> + }
> +
> + return 0;
> +
> +dev_det_ret:
> + if (ret < 0)
> + dev_warn(info->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 < EXTCON_IRQ_END; i++) {
> + if (info->irq[i] == irq)
> + break;
> + }
> +
> + if (i == EXTCON_IRQ_END) {
> + dev_warn(info->dev, "spurious interrupt!!\n");
> + return IRQ_NONE;
> + }
> +
> + ret = handle_chrg_det_event(info);
> + if (ret < 0)
> + dev_warn(info->dev, "error in PWRSRC INT handling\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 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);
> +}
> +
> +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, gpio;
> +
> + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> +
> + info->dev = &pdev->dev;
> + 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);
> +
> + /* Initialize extcon device */
> + info->edev = devm_extcon_dev_allocate(&pdev->dev,
> + axp288_extcon_cables);
> + if (IS_ERR(info->edev)) {
> + dev_err(&pdev->dev, "failed to allocate memory for extcon\n");
> + ret = PTR_ERR(info->edev);
> + return ret;
> + }
> +
> + /* 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");
> + ret = PTR_ERR(info->otg);
> + return ret;
> + }
> +
> + /* 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,
> + "regmap_irq get virq failed for IRQ %d: %d\n",
> + pirq, info->irq[i]);
> + info->irq[i] = -1;
> + goto intr_reg_failed;
> + }
> +
> + 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;
> + }
> + }
> +
> + /* Enable interrupts */
> + axp288_extcon_enable_irq(info);
> +
> + return 0;
> +
> +intr_reg_failed:
> +gpio_req_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",
> + },
> +};
> +module_platform_driver(axp288_extcon_driver);
> +
> +MODULE_AUTHOR("Ramakrishna Pallala <[email protected]>");
> +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index dfabd6d..81152e2 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> };
>
> +#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
> +#define AXP288_EXTCON_CABLE_CDP "CHARGE-DOWNSTREAM"
> +#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"

#defines are meant to make things *easier* for humans to read. It is
my opinion that these negatively impact the readability of the code.

> +struct axp288_extcon_pdata {
> + /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> + struct gpio_desc *gpio_mux_cntl;
> +};

Are you going to add to this?

> #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

2015-04-28 10:10:18

by Pallala, Ramakrishna

[permalink] [raw]
Subject: RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

> > 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 <[email protected]>
> > ---
> > drivers/extcon/Kconfig | 7 +
> > drivers/extcon/Makefile | 1 +
> > drivers/extcon/extcon-axp288.c | 399
> ++++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/axp20x.h | 9 +
> > 4 files changed, 416 insertions(+)
> > create mode 100644 drivers/extcon/extcon-axp288.c
> >
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > fdc0bf0..2305edc 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> > with Wolfson Arizona devices. These are audio CODECs with
> > advanced audio accessory detection support.
> >
> > +config EXTCON_AXP288
> > + tristate "X-Power 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 X-Power AXP288 PMIC.
> > +
> > config EXTCON_GPIO
> > tristate "GPIO extcon support"
> > depends on GPIOLIB
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > 9204114..ba787d0 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -5,6 +5,7 @@
> > obj-$(CONFIG_EXTCON) += extcon.o
> > obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
> > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
> > +obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o
> > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o
> > obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o
> > obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o
> > diff --git a/drivers/extcon/extcon-axp288.c
> > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > 0000000..91d764c
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> > @@ -0,0 +1,399 @@
> > +/*
> > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > +driver
> > + *
> > + * Copyright (C) 2014 Intel Corporation
> > + * Author: Ramakrishna Pallala <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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>

[snip]

> > +module_platform_driver(axp288_extcon_driver);
> > +
> > +MODULE_AUTHOR("Ramakrishna Pallala <[email protected]>");
> > +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index dfabd6d..81152e2 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> > };
> >
> > +#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
> > +#define AXP288_EXTCON_CABLE_CDP "CHARGE-
> DOWNSTREAM"
> > +#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"
>
> #defines are meant to make things *easier* for humans to read. It is my opinion
> that these negatively impact the readability of the code.
These macros will be used in extcon and charger drivers. How can I make it more readable?

> > +struct axp288_extcon_pdata {
> > + /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> > + struct gpio_desc *gpio_mux_cntl;
> > +};
>
> Are you going to add to this?
Yes, this is needed.

Thanks,
Ram
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-04-28 11:13:40

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

On Tue, 28 Apr 2015, Pallala, Ramakrishna 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 <[email protected]>
> > > ---
> > > drivers/extcon/Kconfig | 7 +
> > > drivers/extcon/Makefile | 1 +
> > > drivers/extcon/extcon-axp288.c | 399
> > ++++++++++++++++++++++++++++++++++++++++
> > > include/linux/mfd/axp20x.h | 9 +
> > > 4 files changed, 416 insertions(+)
> > > create mode 100644 drivers/extcon/extcon-axp288.c
> > >
> > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > > fdc0bf0..2305edc 100644
> > > --- a/drivers/extcon/Kconfig
> > > +++ b/drivers/extcon/Kconfig
> > > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> > > with Wolfson Arizona devices. These are audio CODECs with
> > > advanced audio accessory detection support.
> > >
> > > +config EXTCON_AXP288
> > > + tristate "X-Power 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 X-Power AXP288 PMIC.
> > > +
> > > config EXTCON_GPIO
> > > tristate "GPIO extcon support"
> > > depends on GPIOLIB
> > > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > > 9204114..ba787d0 100644
> > > --- a/drivers/extcon/Makefile
> > > +++ b/drivers/extcon/Makefile
> > > @@ -5,6 +5,7 @@
> > > obj-$(CONFIG_EXTCON) += extcon.o
> > > obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
> > > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
> > > +obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o
> > > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o
> > > obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o
> > > obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o
> > > diff --git a/drivers/extcon/extcon-axp288.c
> > > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > > 0000000..91d764c
> > > --- /dev/null
> > > +++ b/drivers/extcon/extcon-axp288.c
> > > @@ -0,0 +1,399 @@
> > > +/*
> > > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > > +driver
> > > + *
> > > + * Copyright (C) 2014 Intel Corporation
> > > + * Author: Ramakrishna Pallala <[email protected]>
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > +modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * 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>
>
> [snip]
>
> > > +module_platform_driver(axp288_extcon_driver);
> > > +
> > > +MODULE_AUTHOR("Ramakrishna Pallala <[email protected]>");
> > > +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > > index dfabd6d..81152e2 100644
> > > --- a/include/linux/mfd/axp20x.h
> > > +++ b/include/linux/mfd/axp20x.h
> > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> > > };
> > >
> > > +#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
> > > +#define AXP288_EXTCON_CABLE_CDP "CHARGE-
> > DOWNSTREAM"
> > > +#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"
> >
> > #defines are meant to make things *easier* for humans to read. It is my opinion
> > that these negatively impact the readability of the code.
> These macros will be used in extcon and charger drivers. How can I make it more readable?

If the things you're defining are more readable than the defines
themselves you're doing the opposite of what I'd expect.

How about:

AXP288_EXTCON_SLOW_CHARGER_CABLE
AXP288_EXTCON_FAST_CHARGER_CABLE
AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE

> > > +struct axp288_extcon_pdata {
> > > + /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> > > + struct gpio_desc *gpio_mux_cntl;
> > > +};
> >
> > Are you going to add to this?
> Yes, this is needed.
>
> Thanks,
> Ram

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

2015-04-28 11:20:45

by Pallala, Ramakrishna

[permalink] [raw]
Subject: RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

> On Tue, 28 Apr 2015, Pallala, Ramakrishna 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 <[email protected]>
> > > > ---
> > > > drivers/extcon/Kconfig | 7 +
> > > > drivers/extcon/Makefile | 1 +
> > > > drivers/extcon/extcon-axp288.c | 399
> > > ++++++++++++++++++++++++++++++++++++++++
> > > > include/linux/mfd/axp20x.h | 9 +
> > > > 4 files changed, 416 insertions(+) create mode 100644
> > > > drivers/extcon/extcon-axp288.c
> > > >
> > > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > > > fdc0bf0..2305edc 100644
> > > > --- a/drivers/extcon/Kconfig
> > > > +++ b/drivers/extcon/Kconfig
> > > > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> > > > with Wolfson Arizona devices. These are audio CODECs with
> > > > advanced audio accessory detection support.
> > > >
> > > > +config EXTCON_AXP288
> > > > + tristate "X-Power 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 X-Power AXP288 PMIC.
> > > > +
> > > > config EXTCON_GPIO
> > > > tristate "GPIO extcon support"
> > > > depends on GPIOLIB
> > > > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > > > index
> > > > 9204114..ba787d0 100644
> > > > --- a/drivers/extcon/Makefile
> > > > +++ b/drivers/extcon/Makefile
> > > > @@ -5,6 +5,7 @@
> > > > obj-$(CONFIG_EXTCON) += extcon.o
> > > > obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
> > > > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
> > > > +obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o
> > > > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o
> > > > obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o
> > > > obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o
> > > > diff --git a/drivers/extcon/extcon-axp288.c
> > > > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > > > 0000000..91d764c
> > > > --- /dev/null
> > > > +++ b/drivers/extcon/extcon-axp288.c
> > > > @@ -0,0 +1,399 @@
> > > > +/*
> > > > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > > > +driver
> > > > + *
> > > > + * Copyright (C) 2014 Intel Corporation
> > > > + * Author: Ramakrishna Pallala <[email protected]>
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or
> > > > +modify
> > > > + * it under the terms of the GNU General Public License version 2
> > > > +as
> > > > + * published by the Free Software Foundation.
> > > > + *
> > > > + * 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>
> >
> > [snip]
> >
> > > > +module_platform_driver(axp288_extcon_driver);
> > > > +
> > > > +MODULE_AUTHOR("Ramakrishna Pallala
> > > > +<[email protected]>");
> > > > +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > > diff --git a/include/linux/mfd/axp20x.h
> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
> > > > --- a/include/linux/mfd/axp20x.h
> > > > +++ b/include/linux/mfd/axp20x.h
> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> > > > };
> > > >
> > > > +#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
> > > > +#define AXP288_EXTCON_CABLE_CDP "CHARGE-
> > > DOWNSTREAM"
> > > > +#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"
> > >
> > > #defines are meant to make things *easier* for humans to read. It is
> > > my opinion that these negatively impact the readability of the code.
> > These macros will be used in extcon and charger drivers. How can I make it
> more readable?
>
> If the things you're defining are more readable than the defines themselves
> you're doing the opposite of what I'd expect.
>
> How about:
>
> AXP288_EXTCON_SLOW_CHARGER_CABLE
> AXP288_EXTCON_FAST_CHARGER_CABLE
> AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE

Ok I will change the names similar to the above ones.

Thanks,
Ram
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-04-28 11:33:26

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

Hi Ram,

On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
<[email protected]> wrote:
>> On Tue, 28 Apr 2015, Pallala, Ramakrishna 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 <[email protected]>
>> > > > ---
>> > > > drivers/extcon/Kconfig | 7 +
>> > > > drivers/extcon/Makefile | 1 +
>> > > > drivers/extcon/extcon-axp288.c | 399
>> > > ++++++++++++++++++++++++++++++++++++++++
>> > > > include/linux/mfd/axp20x.h | 9 +
>> > > > 4 files changed, 416 insertions(+) create mode 100644
>> > > > drivers/extcon/extcon-axp288.c
>> > > >
>> > > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
>> > > > fdc0bf0..2305edc 100644
>> > > > --- a/drivers/extcon/Kconfig
>> > > > +++ b/drivers/extcon/Kconfig
>> > > > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
>> > > > with Wolfson Arizona devices. These are audio CODECs with
>> > > > advanced audio accessory detection support.
>> > > >
>> > > > +config EXTCON_AXP288
>> > > > + tristate "X-Power 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 X-Power AXP288 PMIC.
>> > > > +
>> > > > config EXTCON_GPIO
>> > > > tristate "GPIO extcon support"
>> > > > depends on GPIOLIB
>> > > > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> > > > index
>> > > > 9204114..ba787d0 100644
>> > > > --- a/drivers/extcon/Makefile
>> > > > +++ b/drivers/extcon/Makefile
>> > > > @@ -5,6 +5,7 @@
>> > > > obj-$(CONFIG_EXTCON) += extcon.o
>> > > > obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
>> > > > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
>> > > > +obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o
>> > > > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o
>> > > > obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o
>> > > > obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o
>> > > > diff --git a/drivers/extcon/extcon-axp288.c
>> > > > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
>> > > > 0000000..91d764c
>> > > > --- /dev/null
>> > > > +++ b/drivers/extcon/extcon-axp288.c
>> > > > @@ -0,0 +1,399 @@
>> > > > +/*
>> > > > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
>> > > > +driver
>> > > > + *
>> > > > + * Copyright (C) 2014 Intel Corporation
>> > > > + * Author: Ramakrishna Pallala <[email protected]>
>> > > > + *
>> > > > + * This program is free software; you can redistribute it and/or
>> > > > +modify
>> > > > + * it under the terms of the GNU General Public License version 2
>> > > > +as
>> > > > + * published by the Free Software Foundation.
>> > > > + *
>> > > > + * 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>
>> >
>> > [snip]
>> >
>> > > > +module_platform_driver(axp288_extcon_driver);
>> > > > +
>> > > > +MODULE_AUTHOR("Ramakrishna Pallala
>> > > > +<[email protected]>");
>> > > > +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
>> > > > +MODULE_LICENSE("GPL v2");
>> > > > diff --git a/include/linux/mfd/axp20x.h
>> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
>> > > > --- a/include/linux/mfd/axp20x.h
>> > > > +++ b/include/linux/mfd/axp20x.h
>> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
>> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
>> > > > };
>> > > >
>> > > > +#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
>> > > > +#define AXP288_EXTCON_CABLE_CDP "CHARGE-
>> > > DOWNSTREAM"
>> > > > +#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"
>> > >
>> > > #defines are meant to make things *easier* for humans to read. It is
>> > > my opinion that these negatively impact the readability of the code.
>> > These macros will be used in extcon and charger drivers. How can I make it
>> more readable?
>>
>> If the things you're defining are more readable than the defines themselves
>> you're doing the opposite of what I'd expect.
>>
>> How about:
>>
>> AXP288_EXTCON_SLOW_CHARGER_CABLE
>> AXP288_EXTCON_FAST_CHARGER_CABLE
>> AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
>
> Ok I will change the names similar to the above ones.

This cable names are used in only extcon driver.
I think that you don't need to define them on header file.
You better to move this definition to extcon driver.

Also, I'll review your patch on other mail.

Thanks,
Chanwoo Choi

2015-04-28 12:17:18

by Pallala, Ramakrishna

[permalink] [raw]
Subject: RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

Hi Choi,

> On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
> >> > > > diff --git a/include/linux/mfd/axp20x.h
> >> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
> >> > > > --- a/include/linux/mfd/axp20x.h
> >> > > > +++ b/include/linux/mfd/axp20x.h
> >> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> >> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> >> > > > };
> >> > > >
> >> > > > +#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
> >> > > > +#define AXP288_EXTCON_CABLE_CDP "CHARGE-
> >> > > DOWNSTREAM"
> >> > > > +#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"
> >> > >
> >> > > #defines are meant to make things *easier* for humans to read. It
> >> > > is my opinion that these negatively impact the readability of the code.
> >> > These macros will be used in extcon and charger drivers. How can I
> >> > make it
> >> more readable?
> >>
> >> If the things you're defining are more readable than the defines
> >> themselves you're doing the opposite of what I'd expect.
> >>
> >> How about:
> >>
> >> AXP288_EXTCON_SLOW_CHARGER_CABLE
> >> AXP288_EXTCON_FAST_CHARGER_CABLE
> >> AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
> >
> > Ok I will change the names similar to the above ones.
>
> This cable names are used in only extcon driver.
> I think that you don't need to define them on header file.
> You better to move this definition to extcon driver.
>
> Also, I'll review your patch on other mail.

Based on the these cable types I will be setting the charging parameters.
How will I know which charger cable is connected in extcon consumer driver?

Thanks,
Ram
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-04-28 13:02:37

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

Hi Ram,

On Tue, Apr 28, 2015 at 9:17 PM, Pallala, Ramakrishna
<[email protected]> wrote:
> Hi Choi,
>
>> On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
>> >> > > > diff --git a/include/linux/mfd/axp20x.h
>> >> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
>> >> > > > --- a/include/linux/mfd/axp20x.h
>> >> > > > +++ b/include/linux/mfd/axp20x.h
>> >> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
>> >> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
>> >> > > > };
>> >> > > >
>> >> > > > +#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
>> >> > > > +#define AXP288_EXTCON_CABLE_CDP "CHARGE-
>> >> > > DOWNSTREAM"
>> >> > > > +#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"
>> >> > >
>> >> > > #defines are meant to make things *easier* for humans to read. It
>> >> > > is my opinion that these negatively impact the readability of the code.
>> >> > These macros will be used in extcon and charger drivers. How can I
>> >> > make it
>> >> more readable?
>> >>
>> >> If the things you're defining are more readable than the defines
>> >> themselves you're doing the opposite of what I'd expect.
>> >>
>> >> How about:
>> >>
>> >> AXP288_EXTCON_SLOW_CHARGER_CABLE
>> >> AXP288_EXTCON_FAST_CHARGER_CABLE
>> >> AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
>> >
>> > Ok I will change the names similar to the above ones.
>>
>> This cable names are used in only extcon driver.
>> I think that you don't need to define them on header file.
>> You better to move this definition to extcon driver.
>>
>> Also, I'll review your patch on other mail.
>
> Based on the these cable types I will be setting the charging parameters.
> How will I know which charger cable is connected in extcon consumer driver?

I'm working to implement the unique id for all external connectors instead of
string name when registering the external connectors [1]. But It is
not completed and ongoing.
[1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=v4.2/extcon-next&id=52d1f16a650cc3b20ab5d5e281b874cfc5f659bc

Also, extcon consumer driver can use the unique id (enum extcon) when
executing extcon_register_interest() function.
After implementing patch[1], all extcon provider/consumer driver would
not pass the string of external connector directly.

Thanks,
Chanwoo Choi

2015-04-28 13:19:06

by Pallala, Ramakrishna

[permalink] [raw]
Subject: RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

> On Tue, Apr 28, 2015 at 9:17 PM, Pallala, Ramakrishna
> <[email protected]> wrote:
> > Hi Choi,
> >
> >> On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
> >> >> > > > diff --git a/include/linux/mfd/axp20x.h
> >> >> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
> >> >> > > > --- a/include/linux/mfd/axp20x.h
> >> >> > > > +++ b/include/linux/mfd/axp20x.h
> >> >> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> >> >> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> >> >> > > > };
> >> >> > > >
> >> >> > > > +#define AXP288_EXTCON_CABLE_SDP "SLOW-CHARGER"
> >> >> > > > +#define AXP288_EXTCON_CABLE_CDP "CHARGE-
> >> >> > > DOWNSTREAM"
> >> >> > > > +#define AXP288_EXTCON_CABLE_DCP "FAST-CHARGER"
> >> >> > >
> >> >> > > #defines are meant to make things *easier* for humans to read.
> >> >> > > It is my opinion that these negatively impact the readability of the
> code.
> >> >> > These macros will be used in extcon and charger drivers. How can
> >> >> > I make it
> >> >> more readable?
> >> >>
> >> >> If the things you're defining are more readable than the defines
> >> >> themselves you're doing the opposite of what I'd expect.
> >> >>
> >> >> How about:
> >> >>
> >> >> AXP288_EXTCON_SLOW_CHARGER_CABLE
> >> >> AXP288_EXTCON_FAST_CHARGER_CABLE
> >> >> AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
> >> >
> >> > Ok I will change the names similar to the above ones.
> >>
> >> This cable names are used in only extcon driver.
> >> I think that you don't need to define them on header file.
> >> You better to move this definition to extcon driver.
> >>
> >> Also, I'll review your patch on other mail.
> >
> > Based on the these cable types I will be setting the charging parameters.
> > How will I know which charger cable is connected in extcon consumer driver?
>
> I'm working to implement the unique id for all external connectors instead of
> string name when registering the external connectors [1]. But It is not
> completed and ongoing.
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=v4.
> 2/extcon-next&id=52d1f16a650cc3b20ab5d5e281b874cfc5f659bc
>
> Also, extcon consumer driver can use the unique id (enum extcon) when
> executing extcon_register_interest() function.
> After implementing patch[1], all extcon provider/consumer driver would not
> pass the string of external connector directly.

Sounds good. Can you review the patch v6 and let me know if you have
any other comments else I can submit the patch with these changes.

Thanks,
Ram
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?