Messages in this thread | | | Date | Thu, 27 Feb 2020 09:35:30 +0000 | From | Lee Jones <> | 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@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
| |