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

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

[Baolin Wang]

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

diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c
index c0529a1..ebdf2f1 100644
--- a/drivers/mfd/sprd-sc27xx-spi.c
+++ b/drivers/mfd/sprd-sc27xx-spi.c
@@ -10,6 +10,7 @@
 #include <linux/of_device.h>
 #include <linux/regmap.h>
 #include <linux/spi/spi.h>
+#include <uapi/linux/usb/charger.h>
 
 #define SPRD_PMIC_INT_MASK_STATUS	0x0
 #define SPRD_PMIC_INT_RAW_STATUS	0x4
@@ -17,6 +18,16 @@
 
 #define SPRD_SC2731_IRQ_BASE		0x140
 #define SPRD_SC2731_IRQ_NUMS		16
+#define SPRD_SC2731_CHG_DET		0xedc
+
+/* PMIC charger detection definition */
+#define SPRD_PMIC_CHG_DET_DELAY_US	200000
+#define SPRD_PMIC_CHG_DET_TIMEOUT	2000000
+#define SPRD_PMIC_CHG_DET_DONE		BIT(11)
+#define SPRD_PMIC_SDP_TYPE		BIT(7)
+#define SPRD_PMIC_DCP_TYPE		BIT(6)
+#define SPRD_PMIC_CDP_TYPE		BIT(5)
+#define SPRD_PMIC_CHG_TYPE_MASK		GENMASK(7, 5)
 
 struct sprd_pmic {
 	struct regmap *regmap;
@@ -24,12 +35,14 @@ struct sprd_pmic {
 	struct regmap_irq *irqs;
 	struct regmap_irq_chip irq_chip;
 	struct regmap_irq_chip_data *irq_data;
+	const struct sprd_pmic_data *pdata;
 	int irq;
 };
 
 struct sprd_pmic_data {
 	u32 irq_base;
 	u32 num_irqs;
+	u32 charger_det;
 };
 
 /*
@@ -40,8 +53,46 @@ struct sprd_pmic_data {
 static const struct sprd_pmic_data sc2731_data = {
 	.irq_base = SPRD_SC2731_IRQ_BASE,
 	.num_irqs = SPRD_SC2731_IRQ_NUMS,
+	.charger_det = SPRD_SC2731_CHG_DET,
 };
 
+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);
+
 static const struct mfd_cell sprd_pmic_devs[] = {
 	{
 		.name = "sc27xx-wdt",
@@ -181,6 +232,7 @@ static int sprd_pmic_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, ddata);
 	ddata->dev = &spi->dev;
 	ddata->irq = spi->irq;
+	ddata->pdata = pdata;
 
 	ddata->irq_chip.name = dev_name(&spi->dev);
 	ddata->irq_chip.status_base =
diff --git a/include/linux/mfd/sc27xx-pmic.h b/include/linux/mfd/sc27xx-pmic.h
new file mode 100644
index 0000000..57e45c0
--- /dev/null
+++ b/include/linux/mfd/sc27xx-pmic.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_MFD_SC27XX_PMIC_H
+#define __LINUX_MFD_SC27XX_PMIC_H
+
+extern enum usb_charger_type sprd_pmic_detect_charger_type(struct device *dev);
+
+#endif /* __LINUX_MFD_SC27XX_PMIC_H */
-- 
1.7.9.5

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

[Lee Jones]

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?

Why isn't the charger type detected in the charger driver?

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

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

[Baolin Wang]

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.

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

[Lee Jones]

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

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

[Baolin Wang]

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?

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

[Baolin Wang]

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.

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

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

[Lee Jones]

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

Applied, thanks.

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

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