lkml.org 
[lkml]   [2020]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RESEND PATCH] mfd: sc27xx: Add USB charger type detection support
    On Thu, 27 Feb 2020, Baolin Wang wrote:

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

    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

    \
     
     \ /
      Last update: 2020-02-27 10:36    [W:6.259 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site