2014-10-30 11:36:52

by Antoine Tenart

[permalink] [raw]
Subject: [PATCH 1/6] usb: chipidea: setup ci_hdrc_platform_data in core driver

Hello,

This series introduce the ci_hdrc_get_platdata function to help setting up the
ChipIdea internal ci_hdrc_platform_data structure. This helps avoiding
duplicating code.

This series comes from a duscission on Berlin's USB pacthes where it was asked
to move the PHY phandle handling in the ChipIdea core[1].

With the introduction of the ci_hdrc_get_platdata function, the old
ci_get_platdata function is removed. Changes in ChipIdea drivers have also been
made.

This series is needed for the Berlin USB support and has been tested with an
updated version of the USB Berlin driver (not in mainline yet).

Thanks,

Antoine

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/292383.html

Antoine Tenart (6):
usb: chipidea: add a core function to setup ci_hdrc_platform_data
usb: chipidea: use ci_hdrc_get_platdata in ci_hdrc_imx
usb: chipidea: use ci_hdrc_get_platdata in ci_hdrc_msm
usb: chipidea: use ci_hdrc_get_platdata in ci_hdrc_pci
usb: chipidea: use ci_hdrc_get_platdata in ci_hdrc_zevio
usb: chipidea: remove obsolete ci_get_platdata function

drivers/usb/chipidea/ci_hdrc_imx.c | 14 ++--
drivers/usb/chipidea/ci_hdrc_msm.c | 10 ++-
drivers/usb/chipidea/ci_hdrc_pci.c | 12 ++-
drivers/usb/chipidea/ci_hdrc_zevio.c | 8 +-
drivers/usb/chipidea/core.c | 146 ++++++++++++++++++++++++++++-------
include/linux/usb/chipidea.h | 2 +
6 files changed, 150 insertions(+), 42 deletions(-)

--
2.1.0


2014-10-30 11:36:53

by Antoine Tenart

[permalink] [raw]
Subject: [PATCH 1/6] usb: chipidea: add a core function to setup ci_hdrc_platform_data

Add a function into the chipidea core to help drivers setup the internal
ci_hdrc_platform_data structure. This helps not duplicating common code.

The ci_hdrc_get_platdata function only setup non filled members of the
structure so that is is possible to give an already filled one. This is
what the ci_pdata_default parameter is for.

Signed-off-by: Antoine Tenart <[email protected]>
---
drivers/usb/chipidea/core.c | 129 +++++++++++++++++++++++++++++++++++++++++++
include/linux/usb/chipidea.h | 2 +
2 files changed, 131 insertions(+)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index ba0ac2723098..0ad55c10a903 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -535,6 +535,135 @@ static int ci_get_platdata(struct device *dev,
return 0;
}

+/*
+ * Getting a PHY or an USB PHY is optional:
+ * If no PHY or USB PHY is found, or if their subsystems aren't enabled,
+ * PHY and/or USB PHY will be set to NULL. Otherwise returns an error.
+ */
+static int ci_hdrc_get_phy(struct device *dev,
+ struct ci_hdrc_platform_data *ci_pdata)
+{
+ ci_pdata->phy = devm_phy_get(dev, "usb");
+ ci_pdata->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+
+ if (PTR_ERR(ci_pdata->phy) == -EPROBE_DEFER ||
+ PTR_ERR(ci_pdata->usb_phy) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ if (IS_ERR(ci_pdata->phy)) {
+ if (PTR_ERR(ci_pdata->phy) == -ENOSYS ||
+ PTR_ERR(ci_pdata->phy) == -ENODEV) {
+ ci_pdata->phy = NULL;
+ } else {
+ dev_err(dev, "Could not get PHY: %ld\n",
+ PTR_ERR(ci_pdata->phy));
+ return PTR_ERR(ci_pdata->phy);
+ }
+ }
+
+ if (IS_ERR(ci_pdata->usb_phy)) {
+ if (PTR_ERR(ci_pdata->usb_phy) == -ENXIO ||
+ PTR_ERR(ci_pdata->usb_phy) == -ENODEV) {
+ ci_pdata->usb_phy = NULL;
+ } else {
+ dev_err(dev, "Could not get USB PHY: %ld\n",
+ PTR_ERR(ci_pdata->usb_phy));
+ return PTR_ERR(ci_pdata->usb_phy);
+ }
+ }
+
+ return 0;
+}
+
+static int ci_hdrc_get_usb_phy_mode(struct device *dev,
+ struct ci_hdrc_platform_data *ci_pdata)
+{
+ if (!ci_pdata->phy_mode)
+ ci_pdata->phy_mode = of_usb_get_phy_mode(dev->of_node);
+
+ if (!ci_pdata->dr_mode)
+ ci_pdata->dr_mode = of_usb_get_dr_mode(dev->of_node);
+
+ if (of_usb_get_maximum_speed(dev->of_node) == USB_SPEED_FULL)
+ ci_pdata->flags |= CI_HDRC_FORCE_FULLSPEED;
+
+ return 0;
+}
+
+/*
+ * Getting a regulator is optional:
+ * If no regulator is found, or if the regulator subsystem isn't enabled,
+ * the regulator will be set to NULL. Otherwise returns an error.
+ */
+static int ci_hdrc_get_regulator(struct device *dev,
+ struct ci_hdrc_platform_data *ci_pdata)
+{
+ ci_pdata->reg_vbus = devm_regulator_get(dev, "vbus");
+
+ if (IS_ERR(ci_pdata->reg_vbus)) {
+ if (PTR_ERR(ci_pdata->reg_vbus) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ if (PTR_ERR(ci_pdata->reg_vbus) == -ENODEV) {
+ ci_pdata->reg_vbus = NULL;
+ } else {
+ dev_err(dev, "Could not get regulator for vbus: %ld\n",
+ PTR_ERR(ci_pdata->reg_vbus));
+ return PTR_ERR(ci_pdata->reg_vbus);
+ }
+ }
+
+ return 0;
+}
+
+struct ci_hdrc_platform_data *ci_hdrc_get_platdata(struct device *dev,
+ struct ci_hdrc_platform_data *ci_pdata_default)
+{
+ struct ci_hdrc_platform_data *ci_pdata;
+ int ret;
+
+ if (!ci_pdata_default) {
+ ci_pdata = devm_kzalloc(dev, sizeof(*ci_pdata), GFP_KERNEL);
+ if (!ci_pdata)
+ return ERR_PTR(-ENOMEM);
+ } else {
+ ci_pdata = ci_pdata_default;
+ }
+
+ if (!ci_pdata->name)
+ ci_pdata->name = dev_name(dev);
+
+ if (!ci_pdata->phy && !ci_pdata->usb_phy) {
+ ret = ci_hdrc_get_phy(dev, ci_pdata);
+ if (ret)
+ return ERR_PTR(ret);
+ }
+
+ if (ci_pdata->usb_phy) {
+ ret = ci_hdrc_get_usb_phy_mode(dev, ci_pdata);
+ if (ret)
+ return ERR_PTR(ret);
+ }
+
+ if (ci_pdata->dr_mode == USB_DR_MODE_UNKNOWN)
+ ci_pdata->dr_mode = USB_DR_MODE_OTG;
+
+ if (ci_pdata->dr_mode != USB_DR_MODE_PERIPHERAL) {
+ if (!ci_pdata->reg_vbus) {
+ ret = ci_hdrc_get_regulator(dev, ci_pdata);
+ if (ret)
+ return ERR_PTR(ret);
+ }
+
+ if (!ci_pdata->tpl_support)
+ ci_pdata->tpl_support =
+ of_usb_host_tpl_support(dev->of_node);
+ }
+
+ return ci_pdata;
+}
+EXPORT_SYMBOL_GPL(ci_hdrc_get_platdata);
+
static DEFINE_IDA(ci_ida);

struct platform_device *ci_hdrc_add_device(struct device *dev,
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index c01bf4ea27b9..7bb7520da59b 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -39,6 +39,8 @@ struct ci_hdrc_platform_data {
/* Default offset of capability registers */
#define DEF_CAPOFFSET 0x100

+struct ci_hdrc_platform_data *ci_hdrc_get_platdata(struct device *dev,
+ struct ci_hdrc_platform_data *ci_pdata_default);
/* Add ci hdrc device */
struct platform_device *ci_hdrc_add_device(struct device *dev,
struct resource *res, int nres,
--
2.1.0

2014-10-30 11:37:00

by Antoine Tenart

[permalink] [raw]
Subject: [PATCH 5/6] usb: chipidea: use ci_hdrc_get_platdata in ci_hdrc_zevio

Use the newly introduced ci_hdrc_get_platdata function to help setup the
chipidea internal ci_hdrc_platform_data structure.

Signed-off-by: Antoine Tenart <[email protected]>
---
drivers/usb/chipidea/ci_hdrc_zevio.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_zevio.c b/drivers/usb/chipidea/ci_hdrc_zevio.c
index 3bf6489ef5ec..d50a6d9038bd 100644
--- a/drivers/usb/chipidea/ci_hdrc_zevio.c
+++ b/drivers/usb/chipidea/ci_hdrc_zevio.c
@@ -25,12 +25,16 @@ static struct ci_hdrc_platform_data ci_hdrc_zevio_platdata = {
static int ci_hdrc_zevio_probe(struct platform_device *pdev)
{
struct platform_device *ci_pdev;
+ struct ci_hdrc_platform_data *ci_pdata;

dev_dbg(&pdev->dev, "ci_hdrc_zevio_probe\n");

+ ci_pdata = ci_hdrc_get_platdata(&pdev->dev, &ci_hdrc_zevio_platdata);
+ if (IS_ERR(ci_pdata))
+ return PTR_ERR(ci_pdata);
+
ci_pdev = ci_hdrc_add_device(&pdev->dev,
- pdev->resource, pdev->num_resources,
- &ci_hdrc_zevio_platdata);
+ pdev->resource, pdev->num_resources, ci_pdata);

if (IS_ERR(ci_pdev)) {
dev_err(&pdev->dev, "ci_hdrc_add_device failed!\n");
--
2.1.0

2014-10-30 11:36:58

by Antoine Tenart

[permalink] [raw]
Subject: [PATCH 6/6] usb: chipidea: remove obsolete ci_get_platdata function

The addition of the ci_hdrc_get_platdata function makes the use of
ci_get_platdata obsolete. Remove it.

Signed-off-by: Antoine Tenart <[email protected]>
---
drivers/usb/chipidea/core.c | 41 -----------------------------------------
1 file changed, 41 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 0ad55c10a903..8562a9c6154d 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -498,43 +498,6 @@ static irqreturn_t ci_irq(int irq, void *data)
return ret;
}

-static int ci_get_platdata(struct device *dev,
- struct ci_hdrc_platform_data *platdata)
-{
- if (!platdata->phy_mode)
- platdata->phy_mode = of_usb_get_phy_mode(dev->of_node);
-
- if (!platdata->dr_mode)
- platdata->dr_mode = of_usb_get_dr_mode(dev->of_node);
-
- if (platdata->dr_mode == USB_DR_MODE_UNKNOWN)
- platdata->dr_mode = USB_DR_MODE_OTG;
-
- if (platdata->dr_mode != USB_DR_MODE_PERIPHERAL) {
- /* Get the vbus regulator */
- platdata->reg_vbus = devm_regulator_get(dev, "vbus");
- if (PTR_ERR(platdata->reg_vbus) == -EPROBE_DEFER) {
- return -EPROBE_DEFER;
- } else if (PTR_ERR(platdata->reg_vbus) == -ENODEV) {
- /* no vbus regualator is needed */
- platdata->reg_vbus = NULL;
- } else if (IS_ERR(platdata->reg_vbus)) {
- dev_err(dev, "Getting regulator error: %ld\n",
- PTR_ERR(platdata->reg_vbus));
- return PTR_ERR(platdata->reg_vbus);
- }
- /* Get TPL support */
- if (!platdata->tpl_support)
- platdata->tpl_support =
- of_usb_host_tpl_support(dev->of_node);
- }
-
- if (of_usb_get_maximum_speed(dev->of_node) == USB_SPEED_FULL)
- platdata->flags |= CI_HDRC_FORCE_FULLSPEED;
-
- return 0;
-}
-
/*
* Getting a PHY or an USB PHY is optional:
* If no PHY or USB PHY is found, or if their subsystems aren't enabled,
@@ -673,10 +636,6 @@ struct platform_device *ci_hdrc_add_device(struct device *dev,
struct platform_device *pdev;
int id, ret;

- ret = ci_get_platdata(dev, platdata);
- if (ret)
- return ERR_PTR(ret);
-
id = ida_simple_get(&ci_ida, 0, 0, GFP_KERNEL);
if (id < 0)
return ERR_PTR(id);
--
2.1.0

2014-10-30 11:37:56

by Antoine Tenart

[permalink] [raw]
Subject: [PATCH 4/6] usb: chipidea: use ci_hdrc_get_platdata in ci_hdrc_pci

Use the newly introduced ci_hdrc_get_platdata function to help setup the
chipidea internal ci_hdrc_platform_data structure.

Signed-off-by: Antoine Tenart <[email protected]>
---
drivers/usb/chipidea/ci_hdrc_pci.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_pci.c b/drivers/usb/chipidea/ci_hdrc_pci.c
index 241ae3444fde..d2ffc651b650 100644
--- a/drivers/usb/chipidea/ci_hdrc_pci.c
+++ b/drivers/usb/chipidea/ci_hdrc_pci.c
@@ -51,16 +51,22 @@ static struct ci_hdrc_platform_data penwell_pci_platdata = {
static int ci_hdrc_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
- struct ci_hdrc_platform_data *platdata = (void *)id->driver_data;
+ struct ci_hdrc_platform_data *ci_pdata_default =
+ (void *)id->driver_data;
+ struct ci_hdrc_platform_data *ci_pdata;
struct platform_device *plat_ci;
struct resource res[3];
int retval = 0, nres = 2;

- if (!platdata) {
+ if (!ci_pdata_default) {
dev_err(&pdev->dev, "device doesn't provide driver data\n");
return -ENODEV;
}

+ ci_pdata = ci_hdrc_get_platdata(&pdev->dev, ci_pdata_default);
+ if (IS_ERR(ci_pdata))
+ return PTR_ERR(ci_pdata);
+
retval = pcim_enable_device(pdev);
if (retval)
return retval;
@@ -80,7 +86,7 @@ static int ci_hdrc_pci_probe(struct pci_dev *pdev,
res[1].start = pdev->irq;
res[1].flags = IORESOURCE_IRQ;

- plat_ci = ci_hdrc_add_device(&pdev->dev, res, nres, platdata);
+ plat_ci = ci_hdrc_add_device(&pdev->dev, res, nres, ci_pdata);
if (IS_ERR(plat_ci)) {
dev_err(&pdev->dev, "ci_hdrc_add_device failed!\n");
return PTR_ERR(plat_ci);
--
2.1.0

2014-10-30 11:38:27

by Antoine Tenart

[permalink] [raw]
Subject: [PATCH 3/6] usb: chipidea: use ci_hdrc_get_platdata in ci_hdrc_msm

Use the newly introduced ci_hdrc_get_platdata function to help setup the
chipidea internal ci_hdrc_platform_data structure.

Signed-off-by: Antoine Tenart <[email protected]>
---
drivers/usb/chipidea/ci_hdrc_msm.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c
index 3edf969ed797..32eeab404911 100644
--- a/drivers/usb/chipidea/ci_hdrc_msm.c
+++ b/drivers/usb/chipidea/ci_hdrc_msm.c
@@ -55,10 +55,15 @@ static struct ci_hdrc_platform_data ci_hdrc_msm_platdata = {
static int ci_hdrc_msm_probe(struct platform_device *pdev)
{
struct platform_device *plat_ci;
+ struct ci_hdrc_platform_data *ci_pdata;
struct usb_phy *phy;

dev_dbg(&pdev->dev, "ci_hdrc_msm_probe\n");

+ ci_pdata = ci_hdrc_get_platdata(&pdev->dev, &ci_hdrc_msm_platdata);
+ if (IS_ERR(ci_pdata))
+ return PTR_ERR(ci_pdata);
+
/*
* OTG(PHY) driver takes care of PHY initialization, clock management,
* powering up VBUS, mapping of registers address space and power
@@ -68,11 +73,10 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
if (IS_ERR(phy))
return PTR_ERR(phy);

- ci_hdrc_msm_platdata.usb_phy = phy;
+ ci_pdata->usb_phy = phy;

plat_ci = ci_hdrc_add_device(&pdev->dev,
- pdev->resource, pdev->num_resources,
- &ci_hdrc_msm_platdata);
+ pdev->resource, pdev->num_resources, ci_pdata);
if (IS_ERR(plat_ci)) {
dev_err(&pdev->dev, "ci_hdrc_add_device failed!\n");
return PTR_ERR(plat_ci);
--
2.1.0

2014-10-30 11:38:49

by Antoine Tenart

[permalink] [raw]
Subject: [PATCH 2/6] usb: chipidea: use ci_hdrc_get_platdata in ci_hdrc_imx

Use the newly introduced ci_hdrc_get_platdata function to help setup the
chipidea internal ci_hdrc_platform_data structure.

Signed-off-by: Antoine Tenart <[email protected]>
---
drivers/usb/chipidea/ci_hdrc_imx.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 6f8b1b1045b5..25f3b465cb73 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -103,7 +103,8 @@ static struct imx_usbmisc_data *usbmisc_get_init_data(struct device *dev)
static int ci_hdrc_imx_probe(struct platform_device *pdev)
{
struct ci_hdrc_imx_data *data;
- struct ci_hdrc_platform_data pdata = {
+ struct ci_hdrc_platform_data *ci_pdata;
+ struct ci_hdrc_platform_data ci_pdata_default = {
.name = dev_name(&pdev->dev),
.capoffset = DEF_CAPOFFSET,
.flags = CI_HDRC_REQUIRE_TRANSCEIVER |
@@ -114,6 +115,10 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
of_match_device(ci_hdrc_imx_dt_ids, &pdev->dev);
const struct ci_hdrc_imx_platform_flag *imx_platform_flag = of_id->data;

+ ci_pdata = ci_hdrc_get_platdata(&pdev->dev, &ci_pdata_default);
+ if (IS_ERR(ci_pdata))
+ return PTR_ERR(ci_pdata);
+
data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
if (!data) {
dev_err(&pdev->dev, "Failed to allocate ci_hdrc-imx data!\n");
@@ -147,10 +152,10 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
goto err_clk;
}

- pdata.usb_phy = data->phy;
+ ci_pdata->usb_phy = data->phy;

if (imx_platform_flag->flags & CI_HDRC_IMX_IMX28_WRITE_FIX)
- pdata.flags |= CI_HDRC_IMX28_WRITE_FIX;
+ ci_pdata->flags |= CI_HDRC_IMX28_WRITE_FIX;

ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
if (ret)
@@ -166,8 +171,7 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
}

data->ci_pdev = ci_hdrc_add_device(&pdev->dev,
- pdev->resource, pdev->num_resources,
- &pdata);
+ pdev->resource, pdev->num_resources, ci_pdata);
if (IS_ERR(data->ci_pdev)) {
ret = PTR_ERR(data->ci_pdev);
dev_err(&pdev->dev,
--
2.1.0

2014-10-30 11:43:46

by Antoine Tenart

[permalink] [raw]
Subject: Re: [PATCH 1/6] usb: chipidea: setup ci_hdrc_platform_data in core driver

On Thu, Oct 30, 2014 at 12:36:41PM +0100, Antoine Tenart wrote:
> Hello,
>
> This series introduce the ci_hdrc_get_platdata function to help setting up the
> ChipIdea internal ci_hdrc_platform_data structure. This helps avoiding
> duplicating code.
>
> This series comes from a duscission on Berlin's USB pacthes where it was asked
> to move the PHY phandle handling in the ChipIdea core[1].
>
> With the introduction of the ci_hdrc_get_platdata function, the old
> ci_get_platdata function is removed. Changes in ChipIdea drivers have also been
> made.
>
> This series is needed for the Berlin USB support and has been tested with an
> updated version of the USB Berlin driver (not in mainline yet).
>
> Thanks,
>
> Antoine
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/292383.html
>

Of course, this is the cover letter so it should be [PATCH 0/6].

This series depends on the generic PHY framework support in USB and CI:
https://lkml.org/lkml/2014/10/28/807


Antoine

--
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2014-11-06 08:37:55

by Antoine Tenart

[permalink] [raw]
Subject: Re: [PATCH 1/6] usb: chipidea: setup ci_hdrc_platform_data in core driver

Hi guys,

On Thu, Oct 30, 2014 at 12:43:41PM +0100, Antoine Tenart wrote:
> On Thu, Oct 30, 2014 at 12:36:41PM +0100, Antoine Tenart wrote:
> > Hello,
> >
> > This series introduce the ci_hdrc_get_platdata function to help setting up the
> > ChipIdea internal ci_hdrc_platform_data structure. This helps avoiding
> > duplicating code.
> >
> > This series comes from a duscission on Berlin's USB pacthes where it was asked
> > to move the PHY phandle handling in the ChipIdea core[1].
> >
> > With the introduction of the ci_hdrc_get_platdata function, the old
> > ci_get_platdata function is removed. Changes in ChipIdea drivers have also been
> > made.
> >
> > This series is needed for the Berlin USB support and has been tested with an
> > updated version of the USB Berlin driver (not in mainline yet).
> >
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/292383.html
> >
>
> Of course, this is the cover letter so it should be [PATCH 0/6].
>
> This series depends on the generic PHY framework support in USB and CI:
> https://lkml.org/lkml/2014/10/28/807

Any news? I have another series depending on this one waiting in my
queue.

Antoine

--
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2014-11-06 10:02:11

by Peter Chen

[permalink] [raw]
Subject: RE: [PATCH 1/6] usb: chipidea: setup ci_hdrc_platform_data in core driver


> Subject: Re: [PATCH 1/6] usb: chipidea: setup ci_hdrc_platform_data in core
> driver
>
> Hi guys,
>
> On Thu, Oct 30, 2014 at 12:43:41PM +0100, Antoine Tenart wrote:
> > On Thu, Oct 30, 2014 at 12:36:41PM +0100, Antoine Tenart wrote:
> > > Hello,
> > >
> > > This series introduce the ci_hdrc_get_platdata function to help
> > > setting up the ChipIdea internal ci_hdrc_platform_data structure.
> > > This helps avoiding duplicating code.
> > >
> > > This series comes from a duscission on Berlin's USB pacthes where it
> > > was asked to move the PHY phandle handling in the ChipIdea core[1].
> > >
> > > With the introduction of the ci_hdrc_get_platdata function, the old
> > > ci_get_platdata function is removed. Changes in ChipIdea drivers
> > > have also been made.
> > >
> > > This series is needed for the Berlin USB support and has been tested
> > > with an updated version of the USB Berlin driver (not in mainline yet).
> > >
> > > [1]
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/2
> > > 92383.html
> > >
> >
> > Of course, this is the cover letter so it should be [PATCH 0/6].
> >
> > This series depends on the generic PHY framework support in USB and CI:
> > https://lkml.org/lkml/2014/10/28/807
>
> Any news? I have another series depending on this one waiting in my queue.
>

I have some issues on hand now, will review them next week.

Peter

2014-11-14 02:23:40

by Peter Chen

[permalink] [raw]
Subject: Re: [PATCH 1/6] usb: chipidea: add a core function to setup ci_hdrc_platform_data

On Thu, Oct 30, 2014 at 12:36:42PM +0100, Antoine Tenart wrote:
> Add a function into the chipidea core to help drivers setup the internal
> ci_hdrc_platform_data structure. This helps not duplicating common code.
>
> The ci_hdrc_get_platdata function only setup non filled members of the
> structure so that is is possible to give an already filled one. This is
> what the ci_pdata_default parameter is for.
>
> Signed-off-by: Antoine Tenart <[email protected]>
> ---
> drivers/usb/chipidea/core.c | 129 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/usb/chipidea.h | 2 +
> 2 files changed, 131 insertions(+)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index ba0ac2723098..0ad55c10a903 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -535,6 +535,135 @@ static int ci_get_platdata(struct device *dev,
> return 0;
> }
>
> +/*
> + * Getting a PHY or an USB PHY is optional:
> + * If no PHY or USB PHY is found, or if their subsystems aren't enabled,
> + * PHY and/or USB PHY will be set to NULL. Otherwise returns an error.
> + */
> +static int ci_hdrc_get_phy(struct device *dev,
> + struct ci_hdrc_platform_data *ci_pdata)
> +{
> + ci_pdata->phy = devm_phy_get(dev, "usb");
> + ci_pdata->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +
> + if (PTR_ERR(ci_pdata->phy) == -EPROBE_DEFER ||
> + PTR_ERR(ci_pdata->usb_phy) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + if (IS_ERR(ci_pdata->phy)) {
> + if (PTR_ERR(ci_pdata->phy) == -ENOSYS ||
> + PTR_ERR(ci_pdata->phy) == -ENODEV) {
> + ci_pdata->phy = NULL;
> + } else {
> + dev_err(dev, "Could not get PHY: %ld\n",
> + PTR_ERR(ci_pdata->phy));
> + return PTR_ERR(ci_pdata->phy);
> + }
> + }
> +
> + if (IS_ERR(ci_pdata->usb_phy)) {
> + if (PTR_ERR(ci_pdata->usb_phy) == -ENXIO ||
> + PTR_ERR(ci_pdata->usb_phy) == -ENODEV) {
> + ci_pdata->usb_phy = NULL;
> + } else {
> + dev_err(dev, "Could not get USB PHY: %ld\n",
> + PTR_ERR(ci_pdata->usb_phy));
> + return PTR_ERR(ci_pdata->usb_phy);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int ci_hdrc_get_usb_phy_mode(struct device *dev,
> + struct ci_hdrc_platform_data *ci_pdata)
> +{
> + if (!ci_pdata->phy_mode)
> + ci_pdata->phy_mode = of_usb_get_phy_mode(dev->of_node);
> +
> + if (!ci_pdata->dr_mode)
> + ci_pdata->dr_mode = of_usb_get_dr_mode(dev->of_node);
> +
> + if (of_usb_get_maximum_speed(dev->of_node) == USB_SPEED_FULL)
> + ci_pdata->flags |= CI_HDRC_FORCE_FULLSPEED;
> +
> + return 0;
> +}
> +

The things in above function are no relationship.

> +/*
> + * Getting a regulator is optional:
> + * If no regulator is found, or if the regulator subsystem isn't enabled,
> + * the regulator will be set to NULL. Otherwise returns an error.
> + */
> +static int ci_hdrc_get_regulator(struct device *dev,
> + struct ci_hdrc_platform_data *ci_pdata)
> +{
> + ci_pdata->reg_vbus = devm_regulator_get(dev, "vbus");
> +
> + if (IS_ERR(ci_pdata->reg_vbus)) {
> + if (PTR_ERR(ci_pdata->reg_vbus) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + if (PTR_ERR(ci_pdata->reg_vbus) == -ENODEV) {
> + ci_pdata->reg_vbus = NULL;
> + } else {
> + dev_err(dev, "Could not get regulator for vbus: %ld\n",
> + PTR_ERR(ci_pdata->reg_vbus));
> + return PTR_ERR(ci_pdata->reg_vbus);
> + }
> + }
> +
> + return 0;
> +}
> +
> +struct ci_hdrc_platform_data *ci_hdrc_get_platdata(struct device *dev,
> + struct ci_hdrc_platform_data *ci_pdata_default)
> +{
> + struct ci_hdrc_platform_data *ci_pdata;
> + int ret;
> +
> + if (!ci_pdata_default) {
> + ci_pdata = devm_kzalloc(dev, sizeof(*ci_pdata), GFP_KERNEL);
> + if (!ci_pdata)
> + return ERR_PTR(-ENOMEM);
> + } else {
> + ci_pdata = ci_pdata_default;
> + }
> +
> + if (!ci_pdata->name)
> + ci_pdata->name = dev_name(dev);
> +
> + if (!ci_pdata->phy && !ci_pdata->usb_phy) {
> + ret = ci_hdrc_get_phy(dev, ci_pdata);
> + if (ret)
> + return ERR_PTR(ret);
> + }
> +
> + if (ci_pdata->usb_phy) {
> + ret = ci_hdrc_get_usb_phy_mode(dev, ci_pdata);
> + if (ret)
> + return ERR_PTR(ret);
> + }
> +
> + if (ci_pdata->dr_mode == USB_DR_MODE_UNKNOWN)
> + ci_pdata->dr_mode = USB_DR_MODE_OTG;
> +
> + if (ci_pdata->dr_mode != USB_DR_MODE_PERIPHERAL) {
> + if (!ci_pdata->reg_vbus) {
> + ret = ci_hdrc_get_regulator(dev, ci_pdata);
> + if (ret)
> + return ERR_PTR(ret);
> + }
> +
> + if (!ci_pdata->tpl_support)
> + ci_pdata->tpl_support =
> + of_usb_host_tpl_support(dev->of_node);
> + }
> +
> + return ci_pdata;
> +}
> +EXPORT_SYMBOL_GPL(ci_hdrc_get_platdata);
> +
> static DEFINE_IDA(ci_ida);
>
> struct platform_device *ci_hdrc_add_device(struct device *dev,
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index c01bf4ea27b9..7bb7520da59b 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -39,6 +39,8 @@ struct ci_hdrc_platform_data {
> /* Default offset of capability registers */
> #define DEF_CAPOFFSET 0x100
>
> +struct ci_hdrc_platform_data *ci_hdrc_get_platdata(struct device *dev,
> + struct ci_hdrc_platform_data *ci_pdata_default);
> /* Add ci hdrc device */
> struct platform_device *ci_hdrc_add_device(struct device *dev,
> struct resource *res, int nres,
> --
> 2.1.0
>

Ok, Antoine, I find this patch set may not have many benefits due to
below reasons:
- There is already function ci_get_platdata to do the similar things
- If the PHY can't get from the DT, it will use devm_phy_get or
devm_usb_get_phy to get, this code has already in core.

I am apologized that I wanted you to do a patch for moving get PHY
operation to core, it doesn't have many benefits currently
- For non-dt user, the phy get function has already in core like I said
above.
- For dt generic phy user, it uses of_phy_get, the second parameter
con_id may be different for platforms.
- For dt usb phy, it uses devm_usb_get_phy_by_phandle, the second
parameters phandle may be different for platforms.

So, please rebase my next tree which your generic phy patch set has
already in, and send your generic usb2 chipidea patch base on it.

--

Best Regards,
Peter Chen

2014-11-14 15:49:35

by Antoine Tenart

[permalink] [raw]
Subject: Re: [PATCH 1/6] usb: chipidea: add a core function to setup ci_hdrc_platform_data

Peter,

On Fri, Nov 14, 2014 at 09:16:55AM +0800, Peter Chen wrote:
>
> Ok, Antoine, I find this patch set may not have many benefits due to
> below reasons:
> - There is already function ci_get_platdata to do the similar things
> - If the PHY can't get from the DT, it will use devm_phy_get or
> devm_usb_get_phy to get, this code has already in core.
>
> I am apologized that I wanted you to do a patch for moving get PHY
> operation to core, it doesn't have many benefits currently
> - For non-dt user, the phy get function has already in core like I said
> above.
> - For dt generic phy user, it uses of_phy_get, the second parameter
> con_id may be different for platforms.
> - For dt usb phy, it uses devm_usb_get_phy_by_phandle, the second
> parameters phandle may be different for platforms.

Ok.

> So, please rebase my next tree which your generic phy patch set has
> already in, and send your generic usb2 chipidea patch base on it.

I just rebased my usb2 ci generic driver series and sent it to you and
to the mailing lists. Since there is not reason from now to delay it, I
expect it to be merged soon. I wouldn't want to miss yet another release.

Thanks,

Antoine

--
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2014-11-17 06:37:43

by Peter Chen

[permalink] [raw]
Subject: Re: [PATCH 1/6] usb: chipidea: add a core function to setup ci_hdrc_platform_data

On Fri, Nov 14, 2014 at 04:34:55PM +0100, Antoine Tenart wrote:
> Peter,
>
> On Fri, Nov 14, 2014 at 09:16:55AM +0800, Peter Chen wrote:
> >
> > Ok, Antoine, I find this patch set may not have many benefits due to
> > below reasons:
> > - There is already function ci_get_platdata to do the similar things
> > - If the PHY can't get from the DT, it will use devm_phy_get or
> > devm_usb_get_phy to get, this code has already in core.
> >
> > I am apologized that I wanted you to do a patch for moving get PHY
> > operation to core, it doesn't have many benefits currently
> > - For non-dt user, the phy get function has already in core like I said
> > above.
> > - For dt generic phy user, it uses of_phy_get, the second parameter
> > con_id may be different for platforms.
> > - For dt usb phy, it uses devm_usb_get_phy_by_phandle, the second
> > parameters phandle may be different for platforms.
>
> Ok.
>
> > So, please rebase my next tree which your generic phy patch set has
> > already in, and send your generic usb2 chipidea patch base on it.
>
> I just rebased my usb2 ci generic driver series and sent it to you and
> to the mailing lists. Since there is not reason from now to delay it, I
> expect it to be merged soon. I wouldn't want to miss yet another release.
>
> Thanks,
>

I hope soon, just your generic phy patch set changed lots of things, and
spent too much time on reviewing.

--

Best Regards,
Peter Chen