Message-ID: <CADBw62o=nw_S9Q4x8S-NK+8Xa6jhzXxRgy0PxEJvcPKrs6WbOg@mail.gmail.com>
Date:   Thu, 27 Feb 2020 13:41:26 +0800
From:   Baolin Wang <baolin.wang7@...il.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        Orson Zhai <orsonzhai@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH] mfd: sc27xx: Add USB charger type detection support

Hi Lee,

On Tue, Feb 25, 2020 at 5:27 PM Baolin Wang <baolin.wang7@...il.com> wrote:
>
> On Tue, Feb 25, 2020 at 4:49 PM Lee Jones <lee.jones@...aro.org> wrote:
> >
> > On Tue, 25 Feb 2020, Baolin Wang wrote:
> >
> > > Hi Lee,
> > >
> > > On Mon, Feb 24, 2020 at 7:38 PM Lee Jones <lee.jones@...aro.org> wrote:
> > > >
> > > > On Mon, 17 Feb 2020, Baolin Wang wrote:
> > > >
> > > > > The Spreadtrum SC27XX series PMICs supply the USB charger type detection
> > > > > function, and related registers are located on the PMIC global registers
> > > > > region, thus we implement and export this function in the MFD driver for
> > > > > users to get the USB charger type.
> > > > >
> > > > > Signed-off-by: Baolin Wang <baolin.wang7@...il.com>
> > > > > ---
> > > > >  drivers/mfd/sprd-sc27xx-spi.c   |   52 +++++++++++++++++++++++++++++++++++++++
> > > > >  include/linux/mfd/sc27xx-pmic.h |    7 ++++++
> > > > >  2 files changed, 59 insertions(+)
> > > > >  create mode 100644 include/linux/mfd/sc27xx-pmic.h
> > > >
> > > > [...]
> > > >
> > > > > +enum usb_charger_type sprd_pmic_detect_charger_type(struct device *dev)
> > > > > +{
> > > > > +     struct spi_device *spi = to_spi_device(dev);
> > > > > +     struct sprd_pmic *ddata = spi_get_drvdata(spi);
> > > > > +     const struct sprd_pmic_data *pdata = ddata->pdata;
> > > > > +     enum usb_charger_type type;
> > > > > +     u32 val;
> > > > > +     int ret;
> > > > > +
> > > > > +     ret = regmap_read_poll_timeout(ddata->regmap, pdata->charger_det, val,
> > > > > +                                    (val & SPRD_PMIC_CHG_DET_DONE),
> > > > > +                                    SPRD_PMIC_CHG_DET_DELAY_US,
> > > > > +                                    SPRD_PMIC_CHG_DET_TIMEOUT);
> > > > > +     if (ret) {
> > > > > +             dev_err(&spi->dev, "failed to detect charger type\n");
> > > > > +             return UNKNOWN_TYPE;
> > > > > +     }
> > > > > +
> > > > > +     switch (val & SPRD_PMIC_CHG_TYPE_MASK) {
> > > > > +     case SPRD_PMIC_CDP_TYPE:
> > > > > +             type = CDP_TYPE;
> > > > > +             break;
> > > > > +     case SPRD_PMIC_DCP_TYPE:
> > > > > +             type = DCP_TYPE;
> > > > > +             break;
> > > > > +     case SPRD_PMIC_SDP_TYPE:
> > > > > +             type = SDP_TYPE;
> > > > > +             break;
> > > > > +     default:
> > > > > +             type = UNKNOWN_TYPE;
> > > > > +             break;
> > > > > +     }
> > > > > +
> > > > > +     return type;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(sprd_pmic_detect_charger_type);
> > > >
> > > > Where is this called from?
> > >
> > > Our USB phy driver will call this API to get the charger type, which
> > > is used to notify the corresponding current can be drawn to charger
> > > drivers. And we will introduce users after this patch getting applied.
> > >
> > > > Why isn't the charger type detected in the charger driver?
> > >
> > > The charger type detection operation is not a part of charger, and its
> > > related registers are located on the PMIC global registers area. So I
> > > think the PMIC driver is the right place to implement. Moreover Arnd
> > > also suggested us to implement these APIs in the PMIC driver if I
> > > remember correctly.
> >
> > You shouldn't think of this as a PMIC driver.  This is a device's
> > parent were functional drivers are allocated and registered.  Any
>
> Right.
>
> > useful functionality should be farmed out to the child devices which
> > are to be appropriately dispersed and located into the subsystems.
> >
> > It looks like the charger has access to the same register map as this
> > parent driver.  I do not see any compelling reason to provide charger
> > specific functionality in the parent driver at this point.
>
> Actually the charger detection is not belonging to the charger
> subsystem, at least in the hardware design level. The charger
> detection's theory is detetcing the USB phy D+/D- line to get the
> charger type, then the hardware will save the charger type into the
> PMIC global reigsters automatically for users to get. So this is not
> related with the charger driver, which only supplies charging
> services, and this is also not belonging to the USB phy, since the
> related registers are located on the PMIC gloabl registers area. So
> you still think we should not provide this funcion here?

After more investigation, I found I can not move the charger detection
into the charger driver.

Cause the USB phy will implement the USB charger support by
implementing phy->charger_detect() ops (which will call
sprd_pmic_detect_charger_type() to get the charger type), which means
the USB phy driver need to get a power supply object by a
'power-supply' phandle firstly, if we move the charger detection part
into the charger driver.

But our charger driver also need to register a USB phy notifier to be
notified how much current can be drawn from the USB charger framework,
which means the charger driver need to get a usb_phy object by a
'phys' phandle[1]. So this two drivers are interdependent and
dead-lock.

If we implement the charger type detection in the MFD, the USB phy
driver can get the PMIC device by a phandle easily to get the charger
type. Moreover from my previous description of the hardware design, I
still think implementing the charger type detection in the MFD driver
is a good way now.

What do you think? Thanks.

[1] https://elixir.bootlin.com/linux/v5.6-rc3/source/drivers/power/supply/sc2731_charger.c#L496

Powered by blists - more mailing lists