Hi Choi, Thank you for the comments. I will be fixing them and submit the v8 patch.
> On Wed, Apr 29, 2015 at 11:22 AM, Ramakrishna Pallala > <ramakrishna.pall...@intel.com> wrote: > > This patch adds the extcon support for AXP288 PMIC which has the BC1.2 > > charger detection capability. Additionally it also adds the USB mux > > switching support b/w SOC and PMIC based on GPIO control. > > > > Signed-off-by: Ramakrishna Pallala <ramakrishna.pall...@intel.com> > > --- > > drivers/extcon/Kconfig | 7 + > > drivers/extcon/Makefile | 1 + > > drivers/extcon/extcon-axp288.c | 403 > ++++++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/axp20x.h | 5 + > > 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..7d72d44 > > --- /dev/null > > +++ b/drivers/extcon/extcon-axp288.c > > @@ -0,0 +1,403 @@ > > +/* > > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection > > +driver > > + * > > + * Copyright (C) 2014 Intel Corporation > > 2014 -> 2015 Ok > > + * 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) > > Add 'axp288' prefix to function name as following: > - handle_chrg_det_event -> axp288_handle_chrg_det_event Ok > > > +{ > > + 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"); > > I recommend that you use the consistency log pattern as following: > "failed to read vbus status\n" > Ok > > + 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; > > + } > > I think that you can simplify upper if/else statement as following without any > unneeded debug log. > if (!vbus_attach) { > goto notify_otg; Ok. > > + > > + /* 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"); > > ditto. Don't need to add '!' at the end of log message. > "charger detection not complete!!\n" -> "can't complete the charger > detection\n" Ok. > > > + 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); > > I don't prefer to show the just register value without any log message. Ok. > > > + > > + chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_SHIFT; > > + > > + if (chrg_type == DET_STAT_SDP) { > > + dev_dbg(info->dev, "sdp cable connecetd\n"); > > "sdp cable connecetd\n"-> "sdp cable is connected\n" > > > + notify_otg = true; > > + notify_charger = true; > > + cable = AXP288_EXTCON_SLOW_CHARGER; > > + } else if (chrg_type == DET_STAT_CDP) { > > + dev_dbg(info->dev, "cdp cable connecetd\n"); > > "cdp cable connecetd\n" -> "cdp cable is connected" > > > + notify_otg = true; > > + notify_charger = true; > > + cable = AXP288_EXTCON_DOWNSTREAM_CHARGER; > > + } else if (chrg_type == DET_STAT_DCP) { > > + dev_dbg(info->dev, "dcp cable connecetd\n"); > > "dcp cable connecetd\n" -> "dcp cable is connecetd\n" > > > + notify_charger = true; > > + cable = AXP288_EXTCON_FAST_CHARGER; > > + } else { > > + dev_warn(info->dev, > > + "disconnect or unknown or ID event\n"); > > + } > > How about using the 'switch' statement instead of 'if/else'? Ok. > > > + > > +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; > > + } > > You better to modify if statement as following without brace: > if (!vbus_attach) > notify_otg = notify_charger = false; > Ok. > > + > > + return 0; > > + > > +dev_det_ret: > > + if (ret < 0) > > + dev_warn(info->dev, "BC Mod detection error\n"); > > Use the dev_err() instead of dev_warn() because log message means error state. > Ok. > > + > > + 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"); > > for and if statement is not necessary because 'int irq' must be included in > 'info- > >irq[]' array. Ok. > > > + return IRQ_NONE; > > + } > > + > > + ret = handle_chrg_det_event(info); > > ditto. > - axp288_handle_chrg_det_event() Ok. > > > + if (ret < 0) > > + dev_warn(info->dev, "error in PWRSRC INT handling\n"); > > Use the 'dev_err' instead of 'dev_warn' and modify the log message on upper > comment. > "failed to handle PWRSRC interrupt\n" Ok. > > > + > > + 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 */ > > complte? complete? Ok. > > > + 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 > > + */ > > I prefer to remove "TODO" comment. Ok. > > > + if (!device_property_present(&pdev->dev, > > + "axp288_extcon_data\n")) > > + dev_err(&pdev->dev, "no platform data\n"); > > ditto. > - "failed to get platform data' Ok. > > > + 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; > > Return 'PTR_ERR(info->edev)' directly without allocation to 'ret' variable. OK > > > + } > > + > > + /* 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; > > ditto. Ok > > > + } > > + > > + /* 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", > > I recommend that you use the consistency log pattern as following: > "failed to get virtual interrupt ..." Ok. > > > + pirq, info->irq[i]); > > + info->irq[i] = -1; > > + goto intr_reg_failed; > > Use 'gpio_req_failed' instead of '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", > > ditto. "failed to request interrupt > Ok. > > + info->irq[i], ret); > > + goto intr_reg_failed; > > Use 'gpio_req_failed' instead of 'intr_reg_failed'. > > > + } > > + } > > + > > + /* Enable interrupts */ > > + axp288_extcon_enable_irq(info); > > + > > + return 0; > > + > > +intr_reg_failed: > > 'intr_reg_failed' is not necessary. I prefer to remove it. > > > +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 <ramakrishna.pall...@intel.com>"); > > +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..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, Ram