lkml.org 
[lkml]   [2016]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/8] extcon: axp288: Fix possibly reporting 2 cables in state true
    Hi Hans,

    This patch looks good to me. But I have one comment
    when setting the previous_cable in probe().

    On 2016년 12월 19일 09:13, Hans de Goede wrote:
    > When the charger type changes from e.g. SDP to CDP, without Vbus being
    > seen as low in between axp288_handle_chrg_det_event would set the state
    > for the new cable type to true, without clearing the state of the
    > previous cable type to false.
    >
    > This commit fixes this and also gets rid of the function local static
    > cable variable, properly storing all drv state in the axp288_extcon_info
    > struct.
    >
    > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
    > ---
    > drivers/extcon/extcon-axp288.c | 10 ++++++++--
    > 1 file changed, 8 insertions(+), 2 deletions(-)
    >
    > diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
    > index 43b3637..ded0bd9 100644
    > --- a/drivers/extcon/extcon-axp288.c
    > +++ b/drivers/extcon/extcon-axp288.c
    > @@ -115,6 +115,7 @@ struct axp288_extcon_info {
    > int irq[EXTCON_IRQ_END];
    > struct extcon_dev *edev;
    > struct notifier_block extcon_nb;
    > + unsigned int previous_cable;
    > };
    >
    > /* Power up/down reason string array */
    > @@ -154,9 +155,9 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
    >
    > static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
    > {
    > - static unsigned int cable;
    > int ret, stat, cfg, pwr_stat;
    > u8 chrg_type;
    > + unsigned int cable = info->previous_cable;
    > bool vbus_attach = false;
    >
    > ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, &pwr_stat);
    > @@ -212,7 +213,11 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
    > vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
    > : EXTCON_GPIO_MUX_SEL_PMIC);
    >
    > - extcon_set_state_sync(info->edev, cable, vbus_attach);
    > + extcon_set_state_sync(info->edev, info->previous_cable, false);
    > + if (vbus_attach) {
    > + extcon_set_state_sync(info->edev, cable, vbus_attach);
    > + info->previous_cable = cable;
    > + }
    >
    > return 0;
    >
    > @@ -263,6 +268,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
    > info->dev = &pdev->dev;
    > info->regmap = axp20x->regmap;
    > info->regmap_irqc = axp20x->regmap_irqc;
    > + info->previous_cable = axp288_extcon_cables[0];

    I think that you better to use "EXTCON_NONE" instead of
    "axp288_extcon_cables[0]" as following:
    info->previous_cable = EXTCON_NONE;


    > if (pdata)
    > info->gpio_mux_cntl = pdata->gpio_mux_cntl;
    >
    >

    --
    Regards,
    Chanwoo Choi

    \
     
     \ /
      Last update: 2016-12-19 07:28    [W:3.761 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site