Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 25 Feb 2020 08:50:12 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     Baolin Wang <baolin.wang7@gmail.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [RESEND PATCH] mfd: sc27xx: Add USB charger type detection
 support
Message-ID: <20200225085012.GW3494@dell>
References: <049eb16cf995d3a2dd0de01f4c0ed09965e36f92.1581906151.git.baolin.wang7@gmail.com>
 <20200224113926.GU3494@dell>
 <CADBw62ry=+2Rm-Xnar-oeGe_JipvZ9zw=stT7vMHd+QR_m-JEw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CADBw62ry=+2Rm-Xnar-oeGe_JipvZ9zw=stT7vMHd+QR_m-JEw@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
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.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog