| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200227093530.GU3494@dell>
Date: Thu, 27 Feb 2020 09:35:30 +0000
From: Lee Jones <lee.jones@...aro.org>
To: Baolin Wang <baolin.wang7@...il.com>
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
On Thu, 27 Feb 2020, Baolin Wang wrote:
> 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.
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
Powered by blists - more mailing lists