RE: [PATCH v7] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

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.pallala@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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.pallala@xxxxxxxxx>");
> > +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