Date:   Thu, 21 Feb 2019 00:24:11 +0300
From:   Yauhen Kharuzhy <jekhor@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v2 2/2] extcon intel-cht-wc: Enable external charger

ср, 20 февр. 2019 г. в 18:53, Hans de Goede <hdegoede@...hat.com>:
>
> Hi,
>
> On 2/19/19 10:24 PM, Yauhen Kharuzhy wrote:
> > In some configuration external charger "#charge enable" signal is
> > connected to PMIC. Enable it at device probing to allow charging.
> >
> > Save CHGRCTRL0 and CHGDISCTR registers at driver probing and restore
> > them at driver unbind to re-enable hardware charging control if it was
> > enabled before.
> >
> > Tested at Lenovo Yoga Book (YB1-X91L).
> >
> > Signed-off-by: Yauhen Kharuzhy <jekhor@...il.com>
> > ---
> >   drivers/extcon/extcon-intel-cht-wc.c | 91 +++++++++++++++++++++++++++-
> >   1 file changed, 90 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> > index 4f6ba249bc30..ac009929d244 100644
> > --- a/drivers/extcon/extcon-intel-cht-wc.c
> > +++ b/drivers/extcon/extcon-intel-cht-wc.c
> > @@ -57,6 +57,16 @@
> >   #define CHT_WC_USBSRC_TYPE_OTHER    8
> >   #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY       9
> >
> > +#define CHT_WC_CHGDISCTRL            0x5e2f
> > +#define CHT_WC_CHGDISCTRL_OUTPUT     BIT(0)
> > +/* 0 - open drain, 1 - regular output */
> > +#define CHT_WC_CHGDISCTRL_DRV_OD_DIS BIT(4)
> > +#define CHT_WC_CHGDISCTRL_MODE_HW    BIT(6)
> > +
> > +#define CHT_WC_CHGDISCTRL_CCSM_DIS   0x11
> > +#define CHT_WC_CHGDISCTRL_CCSM_EN    0x00
> > +#define CHT_WC_CHGDISCTRL_CCSM_MASK  0x51
> > +
>
> You no longer need the last 3 defines and IMHO keeping them
> will only confuse the reader of the code, please drop these 3.

My mistake, thanks.

> > +static int cht_wc_save_initial_state(struct cht_wc_extcon_data *ext)
> > +{
> > +     int ret;
> > +
> > +     /*
> > +      * Save the external charger control output state for restoring it at
> > +      * driver unbinding
> > +      */
> > +     ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL,
> > +                       &ext->chgdisctrl_saved);
> > +     if (ret) {
> > +             dev_err(ext->dev, "Error reading CHGDISCTRL: %d\n",
> > +                     ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL0,
> > +                       &ext->chgrctrl0_saved);
> > +     if (ret) {
> > +             dev_err(ext->dev, "Error reading CHGRCTRL0: %d\n",
> > +                     ret);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int cht_wc_restore_initial_state(struct cht_wc_extcon_data *ext)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_write(ext->regmap, CHT_WC_CHGDISCTRL,
> > +                        ext->chgdisctrl_saved);
> > +     if (ret)
> > +             dev_err(ext->dev, "Error restoring of CHGDISCTRL reg: %d\n",
> > +                     ret);
> > +
> > +     ret = regmap_write(ext->regmap, CHT_WC_CHGRCTRL0,
> > +                        ext->chgrctrl0_saved);
> > +     if (ret)
> > +             dev_err(ext->dev, "Error restoring of CHGRCTRL0 reg: %d\n",
> > +                     ret);
> > +
> > +     return ret;
> > +}
> > +
> >   static int cht_wc_extcon_probe(struct platform_device *pdev)
> >   {
> >       struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> > @@ -347,6 +429,8 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> >       if (IS_ERR(ext->edev))
> >               return PTR_ERR(ext->edev);
> >
> > +     cht_wc_save_initial_state(ext);
> > +
> >       /*
> >        * When a host-cable is detected the BIOS enables an external 5v boost
> >        * converter to power connected devices there are 2 problems with this:
> > @@ -365,7 +449,10 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> >       /* Enable sw control */
> >       ret = cht_wc_extcon_sw_control(ext, true);
> >       if (ret)
> > -             return ret;
> > +             goto disable_sw_control;
> > +
> > +     /* Disable charging by external battery charger */
> > +     cht_wc_extcon_enable_charging(ext, false);
> >
> >       /* Register extcon device */
> >       ret = devm_extcon_dev_register(ext->dev, ext->edev);
> > @@ -400,6 +487,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> >
> >   disable_sw_control:
> >       cht_wc_extcon_sw_control(ext, false);
> > +     cht_wc_restore_initial_state(ext);
>
> The restore_initial_state will clober al changes made by the
> cht_wc_extcon_sw_control call, so we do not need both. Thinking a bit
> more about this I think it might be best to drop the state save/restore
> code and just enable hw-control on exit unconditionally. We cannot be
> sure that te initial state is sane, so just switching to hw-control on
> exit seem best and requires less code.  Andy, what is your take on this?

On the other hand we don't know if HW mode is suitable for given
hardware configuration.

We can suppose that if existing code with unconditionally switching to
HW mode didn't cause
any issues before than we can safely leave this for future discussions
and add CHGDISCTRL restoring only.

-- 
Yauhen Kharuzhy

Powered by blists - more mailing lists