Re: [RESEND PATCH] mfd: sc27xx: Add USB charger type detection support

[Lee Jones]

On Thu, 27 Feb 2020, Baolin Wang wrote:

> Hi Lee,
> 
> On Tue, Feb 25, 2020 at 5:27 PM Baolin Wang <baolin.wang7@xxxxxxxxx> wrote:
> >
> > On Tue, Feb 25, 2020 at 4:49 PM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 25 Feb 2020, Baolin Wang wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > On Mon, Feb 24, 2020 at 7:38 PM Lee Jones <lee.jones@xxxxxxxxxx> 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@xxxxxxxxx>
> > > > > > ---
> > > > > >  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.

Thanks for the explanation.  Patch applied.

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

-- 
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog