[PATCH v3 0/3] Fix the Build error for fsl_mxc_udc.c

[PATCH v3 0/3] Fix the Build error for fsl_mxc_udc.c

[Peter Chen]

Changes for v3:
- Split the one big patch into three patches

Changes for v2:
- Add const for fsl_udc_devtype
- Do ioremap for phy address at fsl-mxc-udc

Peter Chen (3):
  usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
  usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap
  ARM: i.MX clock: Change the connection-id for fsl-usb2-udc

 arch/arm/mach-imx/clk-imx25.c                     |    6 +-
 arch/arm/mach-imx/clk-imx27.c                     |    6 +-
 arch/arm/mach-imx/clk-imx31.c                     |    6 +-
 arch/arm/mach-imx/clk-imx35.c                     |    6 +-
 arch/arm/mach-imx/clk-imx51-imx53.c               |    6 +-
 arch/arm/mach-imx/devices/devices-common.h        |    1 +
 arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c |   15 +++---
 drivers/usb/gadget/fsl_mxc_udc.c                  |   23 +++++----
 drivers/usb/gadget/fsl_udc_core.c                 |   52 +++++++++++++-------
 drivers/usb/gadget/fsl_usb2_udc.h                 |   13 ++++--
 include/linux/fsl_devices.h                       |    8 +++
 11 files changed, 87 insertions(+), 55 deletions(-)

[PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Peter Chen]

As mach/hardware.h is deleted, we need to use platform_device_id to
differentiate SoCs.

Besides we update the platform code accordingly.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/mach-imx/devices/devices-common.h        |    1 +
 arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c |   15 +++---
 drivers/usb/gadget/fsl_mxc_udc.c                  |   11 ++--
 drivers/usb/gadget/fsl_udc_core.c                 |   52 +++++++++++++-------
 drivers/usb/gadget/fsl_usb2_udc.h                 |   13 ++++--
 include/linux/fsl_devices.h                       |    8 +++
 6 files changed, 65 insertions(+), 35 deletions(-)

diff --git a/arch/arm/mach-imx/devices/devices-common.h b/arch/arm/mach-imx/devices/devices-common.h
index 6277baf..9bd5777 100644
--- a/arch/arm/mach-imx/devices/devices-common.h
+++ b/arch/arm/mach-imx/devices/devices-common.h
@@ -63,6 +63,7 @@ struct platform_device *__init imx_add_flexcan(
 
 #include <linux/fsl_devices.h>
 struct imx_fsl_usb2_udc_data {
+	const char *devid;
 	resource_size_t iobase;
 	resource_size_t irq;
 };
diff --git a/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c b/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
index 37e4439..fb527c7 100644
--- a/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
+++ b/arch/arm/mach-imx/devices/platform-fsl-usb2-udc.c
@@ -11,35 +11,36 @@
 #include "../hardware.h"
 #include "devices-common.h"
 
-#define imx_fsl_usb2_udc_data_entry_single(soc)				\
+#define imx_fsl_usb2_udc_data_entry_single(soc, _devid)			\
 	{								\
+		.devid = _devid,					\
 		.iobase = soc ## _USB_OTG_BASE_ADDR,			\
 		.irq = soc ## _INT_USB_OTG,				\
 	}
 
 #ifdef CONFIG_SOC_IMX25
 const struct imx_fsl_usb2_udc_data imx25_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX25);
+	imx_fsl_usb2_udc_data_entry_single(MX25, "imx-udc-mx25");
 #endif /* ifdef CONFIG_SOC_IMX25 */
 
 #ifdef CONFIG_SOC_IMX27
 const struct imx_fsl_usb2_udc_data imx27_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX27);
+	imx_fsl_usb2_udc_data_entry_single(MX27, "imx-udc-mx27");
 #endif /* ifdef CONFIG_SOC_IMX27 */
 
 #ifdef CONFIG_SOC_IMX31
 const struct imx_fsl_usb2_udc_data imx31_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX31);
+	imx_fsl_usb2_udc_data_entry_single(MX31, "imx-udc-mx31");
 #endif /* ifdef CONFIG_SOC_IMX31 */
 
 #ifdef CONFIG_SOC_IMX35
 const struct imx_fsl_usb2_udc_data imx35_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX35);
+	imx_fsl_usb2_udc_data_entry_single(MX35, "imx-udc-mx35");
 #endif /* ifdef CONFIG_SOC_IMX35 */
 
 #ifdef CONFIG_SOC_IMX51
 const struct imx_fsl_usb2_udc_data imx51_fsl_usb2_udc_data __initconst =
-	imx_fsl_usb2_udc_data_entry_single(MX51);
+	imx_fsl_usb2_udc_data_entry_single(MX51, "imx-udc-mx51");
 #endif
 
 struct platform_device *__init imx_add_fsl_usb2_udc(
@@ -57,7 +58,7 @@ struct platform_device *__init imx_add_fsl_usb2_udc(
 			.flags = IORESOURCE_IRQ,
 		},
 	};
-	return imx_add_platform_device_dmamask("fsl-usb2-udc", -1,
+	return imx_add_platform_device_dmamask(data->devid, -1,
 			res, ARRAY_SIZE(res),
 			pdata, sizeof(*pdata), DMA_BIT_MASK(32));
 }
diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
index 1b0f086..6df45f7 100644
--- a/drivers/usb/gadget/fsl_mxc_udc.c
+++ b/drivers/usb/gadget/fsl_mxc_udc.c
@@ -18,8 +18,6 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 
-#include <mach/hardware.h>
-
 static struct clk *mxc_ahb_clk;
 static struct clk *mxc_per_clk;
 static struct clk *mxc_ipg_clk;
@@ -28,7 +26,7 @@ static struct clk *mxc_ipg_clk;
 #define USBPHYCTRL_OTGBASE_OFFSET	0x608
 #define USBPHYCTRL_EVDO			(1 << 23)
 
-int fsl_udc_clk_init(struct platform_device *pdev)
+int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev)
 {
 	struct fsl_usb2_platform_data *pdata;
 	unsigned long freq;
@@ -59,7 +57,7 @@ int fsl_udc_clk_init(struct platform_device *pdev)
 	clk_prepare_enable(mxc_per_clk);
 
 	/* make sure USB_CLK is running at 60 MHz +/- 1000 Hz */
-	if (!cpu_is_mx51()) {
+	if (!(devtype == IMX51_UDC)) {
 		freq = clk_get_rate(mxc_per_clk);
 		if (pdata->phy_mode != FSL_USB2_PHY_ULPI &&
 		    (freq < 59999000 || freq > 60001000)) {
@@ -79,10 +77,11 @@ eclkrate:
 	return ret;
 }
 
-void fsl_udc_clk_finalize(struct platform_device *pdev)
+void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+	struct platform_device *pdev)
 {
 	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
-	if (cpu_is_mx35()) {
+	if (devtype == IMX35_UDC) {
 		unsigned int v;
 
 		/* workaround ENGcm09152 for i.MX35 */
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index c19f7f1..c32119b 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -41,6 +41,7 @@
 #include <linux/fsl_devices.h>
 #include <linux/dmapool.h>
 #include <linux/delay.h>
+#include <linux/of_device.h>
 
 #include <asm/byteorder.h>
 #include <asm/io.h>
@@ -2438,17 +2439,13 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 	unsigned int i;
 	u32 dccparams;
 
-	if (strcmp(pdev->name, driver_name)) {
-		VDBG("Wrong device");
-		return -ENODEV;
-	}
-
 	udc_controller = kzalloc(sizeof(struct fsl_udc), GFP_KERNEL);
 	if (udc_controller == NULL) {
 		ERR("malloc udc failed\n");
 		return -ENOMEM;
 	}
 
+	udc_controller->devtype = pdev->id_entry->driver_data;
 	pdata = pdev->dev.platform_data;
 	udc_controller->pdata = pdata;
 	spin_lock_init(&udc_controller->lock);
@@ -2505,7 +2502,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 #endif
 
 	/* Initialize USB clocks */
-	ret = fsl_udc_clk_init(pdev);
+	ret = fsl_udc_clk_init(udc_controller->devtype, pdev);
 	if (ret < 0)
 		goto err_iounmap_noclk;
 
@@ -2547,7 +2544,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 		dr_controller_setup(udc_controller);
 	}
 
-	fsl_udc_clk_finalize(pdev);
+	fsl_udc_clk_finalize(udc_controller->devtype, pdev);
 
 	/* Setup gadget structure */
 	udc_controller->gadget.ops = &fsl_gadget_ops;
@@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
 
 	return fsl_udc_resume(NULL);
 }
-
 /*-------------------------------------------------------------------------
 	Register entry point for the peripheral controller driver
 --------------------------------------------------------------------------*/
-
+static const struct platform_device_id fsl_udc_devtype[] = {
+	{
+		.name = "imx-udc-mx25",
+		.driver_data = IMX25_UDC,
+	}, {
+		.name = "imx-udc-mx27",
+		.driver_data = IMX27_UDC,
+	}, {
+		.name = "imx-udc-mx31",
+		.driver_data = IMX31_UDC,
+	}, {
+		.name = "imx-udc-mx35",
+		.driver_data = IMX35_UDC,
+	}, {
+		.name = "imx-udc-mx51",
+		.driver_data = IMX51_UDC,
+	}
+};
+MODULE_DEVICE_TABLE(platform, fsl_udc_devtype);
 static struct platform_driver udc_driver = {
-	.remove  = __exit_p(fsl_udc_remove),
+	.remove		= __exit_p(fsl_udc_remove),
+	/* Just for FSL i.mx SoC currently */
+	.id_table	= fsl_udc_devtype,
 	/* these suspend and resume are not usb suspend and resume */
-	.suspend = fsl_udc_suspend,
-	.resume  = fsl_udc_resume,
-	.driver  = {
-		.name = (char *)driver_name,
-		.owner = THIS_MODULE,
-		/* udc suspend/resume called from OTG driver */
-		.suspend = fsl_udc_otg_suspend,
-		.resume  = fsl_udc_otg_resume,
+	.suspend	= fsl_udc_suspend,
+	.resume		= fsl_udc_resume,
+	.driver		= {
+			.name = (char *)driver_name,
+			.owner = THIS_MODULE,
+			/* udc suspend/resume called from OTG driver */
+			.suspend = fsl_udc_otg_suspend,
+			.resume  = fsl_udc_otg_resume,
 	},
 };
 
diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
index f61a967..bc1f6d0 100644
--- a/drivers/usb/gadget/fsl_usb2_udc.h
+++ b/drivers/usb/gadget/fsl_usb2_udc.h
@@ -505,6 +505,8 @@ struct fsl_udc {
 	u32 ep0_dir;		/* Endpoint zero direction: can be
 				   USB_DIR_IN or USB_DIR_OUT */
 	u8 device_address;	/* Device USB address */
+	/* devtype for kinds of SoC, only i.mx uses it now */
+	enum fsl_udc_type devtype;
 };
 
 /*-------------------------------------------------------------------------*/
@@ -591,15 +593,18 @@ static inline struct ep_queue_head *get_qh_by_ep(struct fsl_ep *ep)
 
 struct platform_device;
 #ifdef CONFIG_ARCH_MXC
-int fsl_udc_clk_init(struct platform_device *pdev);
-void fsl_udc_clk_finalize(struct platform_device *pdev);
+int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev);
+void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+		struct platform_device *pdev);
 void fsl_udc_clk_release(void);
 #else
-static inline int fsl_udc_clk_init(struct platform_device *pdev)
+static inline int fsl_udc_clk_init(enum fsl_udc_type devtype,
+		struct platform_device *pdev)
 {
 	return 0;
 }
-static inline void fsl_udc_clk_finalize(struct platform_device *pdev)
+static inline void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
+		struct platform_device *pdev)
 {
 }
 static inline void fsl_udc_clk_release(void)
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index a82296a..7cb3fe0 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -66,6 +66,14 @@ enum fsl_usb2_phy_modes {
 	FSL_USB2_PHY_SERIAL,
 };
 
+enum fsl_udc_type {
+	IMX25_UDC,
+	IMX27_UDC,
+	IMX31_UDC,
+	IMX35_UDC,
+	IMX51_UDC,
+};
+
 struct clk;
 struct platform_device;
 
-- 
1.7.0.4

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

Hi,

On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>  
>  	return fsl_udc_resume(NULL);
>  }
> -
>  /*-------------------------------------------------------------------------
>  	Register entry point for the peripheral controller driver
>  --------------------------------------------------------------------------*/
> -
> +static const struct platform_device_id fsl_udc_devtype[] = {
> +	{
> +		.name = "imx-udc-mx25",
> +		.driver_data = IMX25_UDC,
> +	}, {
> +		.name = "imx-udc-mx27",
> +		.driver_data = IMX27_UDC,
> +	}, {
> +		.name = "imx-udc-mx31",
> +		.driver_data = IMX31_UDC,
> +	}, {
> +		.name = "imx-udc-mx35",
> +		.driver_data = IMX35_UDC,
> +	}, {
> +		.name = "imx-udc-mx51",
> +		.driver_data = IMX51_UDC,
> +	}
> +};

I wonder if your driver-data is actually needed since you can use string
comparisson to achieve the exact same outcome.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]

On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>>  
>>  	return fsl_udc_resume(NULL);
>>  }
>> -
>>  /*-------------------------------------------------------------------------
>>  	Register entry point for the peripheral controller driver
>>  --------------------------------------------------------------------------*/
>> -
>> +static const struct platform_device_id fsl_udc_devtype[] = {
>> +	{
>> +		.name = "imx-udc-mx25",
>> +		.driver_data = IMX25_UDC,
>> +	}, {
>> +		.name = "imx-udc-mx27",
>> +		.driver_data = IMX27_UDC,
>> +	}, {
>> +		.name = "imx-udc-mx31",
>> +		.driver_data = IMX31_UDC,
>> +	}, {
>> +		.name = "imx-udc-mx35",
>> +		.driver_data = IMX35_UDC,
>> +	}, {
>> +		.name = "imx-udc-mx51",
>> +		.driver_data = IMX51_UDC,
>> +	}
>> +};
> 
> I wonder if your driver-data is actually needed since you can use string
> comparisson to achieve the exact same outcome.

Why use a string compare, if the kernel infrastructure already does this
for you?

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
> >> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
> >>  
> >>  	return fsl_udc_resume(NULL);
> >>  }
> >> -
> >>  /*-------------------------------------------------------------------------
> >>  	Register entry point for the peripheral controller driver
> >>  --------------------------------------------------------------------------*/
> >> -
> >> +static const struct platform_device_id fsl_udc_devtype[] = {
> >> +	{
> >> +		.name = "imx-udc-mx25",
> >> +		.driver_data = IMX25_UDC,
> >> +	}, {
> >> +		.name = "imx-udc-mx27",
> >> +		.driver_data = IMX27_UDC,
> >> +	}, {
> >> +		.name = "imx-udc-mx31",
> >> +		.driver_data = IMX31_UDC,
> >> +	}, {
> >> +		.name = "imx-udc-mx35",
> >> +		.driver_data = IMX35_UDC,
> >> +	}, {
> >> +		.name = "imx-udc-mx51",
> >> +		.driver_data = IMX51_UDC,
> >> +	}
> >> +};
> > 
> > I wonder if your driver-data is actually needed since you can use string
> > comparisson to achieve the exact same outcome.
> 
> Why use a string compare, if the kernel infrastructure already does this
> for you?

what do you mean ? What kernel infrastructure is doing waht for me ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

On 01/14/2013 11:24 AM, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>>>>  
>>>>  	return fsl_udc_resume(NULL);
>>>>  }
>>>> -
>>>>  /*-------------------------------------------------------------------------
>>>>  	Register entry point for the peripheral controller driver
>>>>  --------------------------------------------------------------------------*/
>>>> -
>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
>>>> +	{
>>>> +		.name = "imx-udc-mx25",
>>>> +		.driver_data = IMX25_UDC,
>>>> +	}, {
>>>> +		.name = "imx-udc-mx27",
>>>> +		.driver_data = IMX27_UDC,
>>>> +	}, {
>>>> +		.name = "imx-udc-mx31",
>>>> +		.driver_data = IMX31_UDC,
>>>> +	}, {
>>>> +		.name = "imx-udc-mx35",
>>>> +		.driver_data = IMX35_UDC,
>>>> +	}, {
>>>> +		.name = "imx-udc-mx51",
>>>> +		.driver_data = IMX51_UDC,
>>>> +	}
>>>> +};
>>>
>>> I wonder if your driver-data is actually needed since you can use string
>>> comparisson to achieve the exact same outcome.
>>
>> Why use a string compare, if the kernel infrastructure already does this
>> for you?
> 
> what do you mean ? What kernel infrastructure is doing waht for me ?

The kernel infrastructure is doing the string compare for you to match
the device against the driver (via platform_device_id->name). You get
the a pointer to the driver_data for free. So you don't need any string
compare in the driver later.

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]

On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote:
> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
> > On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
> >> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> >>> Hi,
> >>>
> >>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
> >>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
> >>>>  
> >>>>  	return fsl_udc_resume(NULL);
> >>>>  }
> >>>> -
> >>>>  /*-------------------------------------------------------------------------
> >>>>  	Register entry point for the peripheral controller driver
> >>>>  --------------------------------------------------------------------------*/
> >>>> -
> >>>> +static const struct platform_device_id fsl_udc_devtype[] = {
> >>>> +	{
> >>>> +		.name = "imx-udc-mx25",
> >>>> +		.driver_data = IMX25_UDC,
> >>>> +	}, {
> >>>> +		.name = "imx-udc-mx27",
> >>>> +		.driver_data = IMX27_UDC,
> >>>> +	}, {
> >>>> +		.name = "imx-udc-mx31",
> >>>> +		.driver_data = IMX31_UDC,
> >>>> +	}, {
> >>>> +		.name = "imx-udc-mx35",
> >>>> +		.driver_data = IMX35_UDC,
> >>>> +	}, {
> >>>> +		.name = "imx-udc-mx51",
> >>>> +		.driver_data = IMX51_UDC,
> >>>> +	}
> >>>> +};
> >>>
> >>> I wonder if your driver-data is actually needed since you can use string
> >>> comparisson to achieve the exact same outcome.
> >>
> >> Why use a string compare, if the kernel infrastructure already does this
> >> for you?
> > 
> > what do you mean ? What kernel infrastructure is doing waht for me ?
> 
> The kernel infrastructure is doing the string compare for you to match
> the device against the driver (via platform_device_id->name). You get
> the a pointer to the driver_data for free. So you don't need any string
> compare in the driver later.

but current driver data is just duplicating name with an integer, it's
pretty useless driver data.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2666 bytes --]

On 01/14/2013 11:39 AM, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote:
>> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
>>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
>>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
>>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>>>>>>  
>>>>>>  	return fsl_udc_resume(NULL);
>>>>>>  }
>>>>>> -
>>>>>>  /*-------------------------------------------------------------------------
>>>>>>  	Register entry point for the peripheral controller driver
>>>>>>  --------------------------------------------------------------------------*/
>>>>>> -
>>>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
>>>>>> +	{
>>>>>> +		.name = "imx-udc-mx25",
>>>>>> +		.driver_data = IMX25_UDC,
>>>>>> +	}, {
>>>>>> +		.name = "imx-udc-mx27",
>>>>>> +		.driver_data = IMX27_UDC,
>>>>>> +	}, {
>>>>>> +		.name = "imx-udc-mx31",
>>>>>> +		.driver_data = IMX31_UDC,
>>>>>> +	}, {
>>>>>> +		.name = "imx-udc-mx35",
>>>>>> +		.driver_data = IMX35_UDC,
>>>>>> +	}, {
>>>>>> +		.name = "imx-udc-mx51",
>>>>>> +		.driver_data = IMX51_UDC,
>>>>>> +	}
>>>>>> +};
>>>>>
>>>>> I wonder if your driver-data is actually needed since you can use string
>>>>> comparisson to achieve the exact same outcome.
>>>>
>>>> Why use a string compare, if the kernel infrastructure already does this
>>>> for you?
>>>
>>> what do you mean ? What kernel infrastructure is doing waht for me ?
>>
>> The kernel infrastructure is doing the string compare for you to match
>> the device against the driver (via platform_device_id->name). You get
>> the a pointer to the driver_data for free. So you don't need any string
>> compare in the driver later.
> 
> but current driver data is just duplicating name with an integer, it's
> pretty useless driver data.

I don't think so - another argument:
Less code. As struct platform_device_id is a static array the space is
allocated anyway. So it doesn't make any difference if driver_data is
NULL or not. Later you just need to make an integer comparison instead
of a call to a strcmp(), if you have a switch/case and IMX*_UDC is an
enum, the compiler will warn you if you've missed an IMX variant.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]

Hi,

On Mon, Jan 14, 2013 at 11:50:41AM +0100, Marc Kleine-Budde wrote:
> On 01/14/2013 11:39 AM, Felipe Balbi wrote:
> > On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote:
> >> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
> >>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
> >>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
> >>>>>>  
> >>>>>>  	return fsl_udc_resume(NULL);
> >>>>>>  }
> >>>>>> -
> >>>>>>  /*-------------------------------------------------------------------------
> >>>>>>  	Register entry point for the peripheral controller driver
> >>>>>>  --------------------------------------------------------------------------*/
> >>>>>> -
> >>>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
> >>>>>> +	{
> >>>>>> +		.name = "imx-udc-mx25",
> >>>>>> +		.driver_data = IMX25_UDC,
> >>>>>> +	}, {
> >>>>>> +		.name = "imx-udc-mx27",
> >>>>>> +		.driver_data = IMX27_UDC,
> >>>>>> +	}, {
> >>>>>> +		.name = "imx-udc-mx31",
> >>>>>> +		.driver_data = IMX31_UDC,
> >>>>>> +	}, {
> >>>>>> +		.name = "imx-udc-mx35",
> >>>>>> +		.driver_data = IMX35_UDC,
> >>>>>> +	}, {
> >>>>>> +		.name = "imx-udc-mx51",
> >>>>>> +		.driver_data = IMX51_UDC,
> >>>>>> +	}
> >>>>>> +};
> >>>>>
> >>>>> I wonder if your driver-data is actually needed since you can use string
> >>>>> comparisson to achieve the exact same outcome.
> >>>>
> >>>> Why use a string compare, if the kernel infrastructure already does this
> >>>> for you?
> >>>
> >>> what do you mean ? What kernel infrastructure is doing waht for me ?
> >>
> >> The kernel infrastructure is doing the string compare for you to match
> >> the device against the driver (via platform_device_id->name). You get
> >> the a pointer to the driver_data for free. So you don't need any string
> >> compare in the driver later.
> > 
> > but current driver data is just duplicating name with an integer, it's
> > pretty useless driver data.
> 
> I don't think so - another argument:
> Less code. As struct platform_device_id is a static array the space is
> allocated anyway. So it doesn't make any difference if driver_data is
> NULL or not. Later you just need to make an integer comparison instead
> of a call to a strcmp(), if you have a switch/case and IMX*_UDC is an
> enum, the compiler will warn you if you've missed an IMX variant.

fair enough, but then don't create a different enum value for each imx
instance if they're mostly the same. Differentiate only what's actually
different.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3428 bytes --]

On 01/14/2013 11:53 AM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Jan 14, 2013 at 11:50:41AM +0100, Marc Kleine-Budde wrote:
>> On 01/14/2013 11:39 AM, Felipe Balbi wrote:
>>> On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote:
>>>> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
>>>>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
>>>>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
>>>>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>>>>>>>>  
>>>>>>>>  	return fsl_udc_resume(NULL);
>>>>>>>>  }
>>>>>>>> -
>>>>>>>>  /*-------------------------------------------------------------------------
>>>>>>>>  	Register entry point for the peripheral controller driver
>>>>>>>>  --------------------------------------------------------------------------*/
>>>>>>>> -
>>>>>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
>>>>>>>> +	{
>>>>>>>> +		.name = "imx-udc-mx25",
>>>>>>>> +		.driver_data = IMX25_UDC,
>>>>>>>> +	}, {
>>>>>>>> +		.name = "imx-udc-mx27",
>>>>>>>> +		.driver_data = IMX27_UDC,
>>>>>>>> +	}, {
>>>>>>>> +		.name = "imx-udc-mx31",
>>>>>>>> +		.driver_data = IMX31_UDC,
>>>>>>>> +	}, {
>>>>>>>> +		.name = "imx-udc-mx35",
>>>>>>>> +		.driver_data = IMX35_UDC,
>>>>>>>> +	}, {
>>>>>>>> +		.name = "imx-udc-mx51",
>>>>>>>> +		.driver_data = IMX51_UDC,
>>>>>>>> +	}
>>>>>>>> +};
>>>>>>>
>>>>>>> I wonder if your driver-data is actually needed since you can use string
>>>>>>> comparisson to achieve the exact same outcome.
>>>>>>
>>>>>> Why use a string compare, if the kernel infrastructure already does this
>>>>>> for you?
>>>>>
>>>>> what do you mean ? What kernel infrastructure is doing waht for me ?
>>>>
>>>> The kernel infrastructure is doing the string compare for you to match
>>>> the device against the driver (via platform_device_id->name). You get
>>>> the a pointer to the driver_data for free. So you don't need any string
>>>> compare in the driver later.
>>>
>>> but current driver data is just duplicating name with an integer, it's
>>> pretty useless driver data.
>>
>> I don't think so - another argument:
>> Less code. As struct platform_device_id is a static array the space is
>> allocated anyway. So it doesn't make any difference if driver_data is
>> NULL or not. Later you just need to make an integer comparison instead
>> of a call to a strcmp(), if you have a switch/case and IMX*_UDC is an
>> enum, the compiler will warn you if you've missed an IMX variant.
> 
> fair enough, but then don't create a different enum value for each imx
> instance if they're mostly the same. Differentiate only what's actually
> different.

Usually there isn't any Changelog between IP cores used in the different
fsl processors (at least available outside of fsl), that makes it quite
difficult to say if something found on one imx is really the same as on
the other one. And they (usually) don't provide any versioning
information in a register or the documentation.

just my 2¢
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3564 bytes --]

On Mon, Jan 14, 2013 at 12:03:04PM +0100, Marc Kleine-Budde wrote:
> On 01/14/2013 11:53 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Jan 14, 2013 at 11:50:41AM +0100, Marc Kleine-Budde wrote:
> >> On 01/14/2013 11:39 AM, Felipe Balbi wrote:
> >>> On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote:
> >>>> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
> >>>>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
> >>>>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
> >>>>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
> >>>>>>>>  
> >>>>>>>>  	return fsl_udc_resume(NULL);
> >>>>>>>>  }
> >>>>>>>> -
> >>>>>>>>  /*-------------------------------------------------------------------------
> >>>>>>>>  	Register entry point for the peripheral controller driver
> >>>>>>>>  --------------------------------------------------------------------------*/
> >>>>>>>> -
> >>>>>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
> >>>>>>>> +	{
> >>>>>>>> +		.name = "imx-udc-mx25",
> >>>>>>>> +		.driver_data = IMX25_UDC,
> >>>>>>>> +	}, {
> >>>>>>>> +		.name = "imx-udc-mx27",
> >>>>>>>> +		.driver_data = IMX27_UDC,
> >>>>>>>> +	}, {
> >>>>>>>> +		.name = "imx-udc-mx31",
> >>>>>>>> +		.driver_data = IMX31_UDC,
> >>>>>>>> +	}, {
> >>>>>>>> +		.name = "imx-udc-mx35",
> >>>>>>>> +		.driver_data = IMX35_UDC,
> >>>>>>>> +	}, {
> >>>>>>>> +		.name = "imx-udc-mx51",
> >>>>>>>> +		.driver_data = IMX51_UDC,
> >>>>>>>> +	}
> >>>>>>>> +};
> >>>>>>>
> >>>>>>> I wonder if your driver-data is actually needed since you can use string
> >>>>>>> comparisson to achieve the exact same outcome.
> >>>>>>
> >>>>>> Why use a string compare, if the kernel infrastructure already does this
> >>>>>> for you?
> >>>>>
> >>>>> what do you mean ? What kernel infrastructure is doing waht for me ?
> >>>>
> >>>> The kernel infrastructure is doing the string compare for you to match
> >>>> the device against the driver (via platform_device_id->name). You get
> >>>> the a pointer to the driver_data for free. So you don't need any string
> >>>> compare in the driver later.
> >>>
> >>> but current driver data is just duplicating name with an integer, it's
> >>> pretty useless driver data.
> >>
> >> I don't think so - another argument:
> >> Less code. As struct platform_device_id is a static array the space is
> >> allocated anyway. So it doesn't make any difference if driver_data is
> >> NULL or not. Later you just need to make an integer comparison instead
> >> of a call to a strcmp(), if you have a switch/case and IMX*_UDC is an
> >> enum, the compiler will warn you if you've missed an IMX variant.
> > 
> > fair enough, but then don't create a different enum value for each imx
> > instance if they're mostly the same. Differentiate only what's actually
> > different.
> 
> Usually there isn't any Changelog between IP cores used in the different
> fsl processors (at least available outside of fsl), that makes it quite
> difficult to say if something found on one imx is really the same as on
> the other one. And they (usually) don't provide any versioning
> information in a register or the documentation.
> 
> just my 2¢

$SUBJECT is trying to differentiate a single feature (or maybe two) to
replace cpu_is_xxx(), then expose that on driver_data without creating
one enum value for each release from fsl.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Peter Chen]

On Mon, Jan 14, 2013 at 01:06:00PM +0200, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 12:03:04PM +0100, Marc Kleine-Budde wrote:
> > On 01/14/2013 11:53 AM, Felipe Balbi wrote:
> > > Hi,
> > >=20
> > > On Mon, Jan 14, 2013 at 11:50:41AM +0100, Marc Kleine-Budde wrote:
> > >> On 01/14/2013 11:39 AM, Felipe Balbi wrote:
> > >>> On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote=
:
> > >>>> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
> > >>>>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wro=
te:
> > >>>>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
> > >>>>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct=
 device *dev)
> > >>>>>>>> =20
> > >>>>>>>>  	return fsl_udc_resume(NULL);
> > >>>>>>>>  }
> > >>>>>>>> -
> > >>>>>>>>  /*---------------------------------------------------------=
----------------
> > >>>>>>>>  	Register entry point for the peripheral controller driver
> > >>>>>>>>  -----------------------------------------------------------=
---------------*/
> > >>>>>>>> -
> > >>>>>>>> +static const struct platform_device_id fsl_udc_devtype[] =3D=
 {
> > >>>>>>>> +	{
> > >>>>>>>> +		.name =3D "imx-udc-mx25",
> > >>>>>>>> +		.driver_data =3D IMX25_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name =3D "imx-udc-mx27",
> > >>>>>>>> +		.driver_data =3D IMX27_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name =3D "imx-udc-mx31",
> > >>>>>>>> +		.driver_data =3D IMX31_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name =3D "imx-udc-mx35",
> > >>>>>>>> +		.driver_data =3D IMX35_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name =3D "imx-udc-mx51",
> > >>>>>>>> +		.driver_data =3D IMX51_UDC,
> > >>>>>>>> +	}
> > >>>>>>>> +};
> > >>>>>>>
> > >>>>>>> I wonder if your driver-data is actually needed since you can=
 use string
> > >>>>>>> comparisson to achieve the exact same outcome.
> > >>>>>>
> > >>>>>> Why use a string compare, if the kernel infrastructure already=
 does this
> > >>>>>> for you?
> > >>>>>
> > >>>>> what do you mean ? What kernel infrastructure is doing waht for=
 me ?
> > >>>>
> > >>>> The kernel infrastructure is doing the string compare for you to=
 match
> > >>>> the device against the driver (via platform_device_id->name). Yo=
u get
> > >>>> the a pointer to the driver_data for free. So you don't need any=
 string
> > >>>> compare in the driver later.
> > >>>
> > >>> but current driver data is just duplicating name with an integer,=
 it's
> > >>> pretty useless driver data.
> > >>
> > >> I don't think so - another argument:
> > >> Less code. As struct platform_device_id is a static array the spac=
e is
> > >> allocated anyway. So it doesn't make any difference if driver_data=
 is
> > >> NULL or not. Later you just need to make an integer comparison ins=
tead
> > >> of a call to a strcmp(), if you have a switch/case and IMX*_UDC is=
 an
> > >> enum, the compiler will warn you if you've missed an IMX variant.
> > >=20
> > > fair enough, but then don't create a different enum value for each =
imx
> > > instance if they're mostly the same. Differentiate only what's actu=
ally
> > > different.
> >=20
> > Usually there isn't any Changelog between IP cores used in the differ=
ent
> > fsl processors (at least available outside of fsl), that makes it qui=
te
> > difficult to say if something found on one imx is really the same as =
on
> > the other one. And they (usually) don't provide any versioning
> > information in a register or the documentation.
> >=20
> > just my 2=A2
>=20
> $SUBJECT is trying to differentiate a single feature (or maybe two) to
> replace cpu_is_xxx(), then expose that on driver_data without creating
> one enum value for each release from fsl.

Felipe, every one or two SoCs may have their special operations for
integrate PHY interface, clk operation, or workaround for IC limitation.

Maybe, it will add more future or SoCs (maybe not for this driver)
in the future, using enum is easier than string comparison for expanding =
something.
>=20
> --=20
> balbi



--=20

Best Regards,
Peter Chen

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

Hi,

On Mon, Jan 14, 2013 at 08:56:33PM +0800, Peter Chen wrote:

<snip>

> > > Usually there isn't any Changelog between IP cores used in the different
> > > fsl processors (at least available outside of fsl), that makes it quite
> > > difficult to say if something found on one imx is really the same as on
> > > the other one. And they (usually) don't provide any versioning
> > > information in a register or the documentation.
> > > 
> > > just my 2¢
> > 
> > $SUBJECT is trying to differentiate a single feature (or maybe two) to
> > replace cpu_is_xxx(), then expose that on driver_data without creating
> > one enum value for each release from fsl.
> 
> Felipe, every one or two SoCs may have their special operations for
> integrate PHY interface, clk operation, or workaround for IC
> limitation.

the particular PHY and clk used should be hidden by phy layer and clk
API respectively. Workarounds, fair enough, we need to handle them; but
ideally those should be based on runtime revision detection, not some
hackery using driver_data.

> Maybe, it will add more future or SoCs (maybe not for this driver) in
> the future, using enum is easier than string comparison for expanding
> something.

a) I never told you to *not* use enum. I said that creating DEVICE_A,
DEVICE_B, DEVICE_C, DEVICE_D and DEVICE_E values when DEVICE_B,
DEVICE_C and DEVICE_E behave exactly the same is unnecessary.

b) you can't be expecting to add future SoCs support to fsl udc, I have
already said and will repeat for the last time: move to chipidea ASAP.

New SoCs cannot be added to fsl udc, you *must* use chipidea for
anything new and move the legacy to chipidea eventually. I will wait for
a full year for you to do that, but after that I will have to start
deleting drivers for the sake of avoid duplication of effort.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

On 01/14/2013 06:40 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Jan 14, 2013 at 08:56:33PM +0800, Peter Chen wrote:
> 
> <snip>
> 
>>>> Usually there isn't any Changelog between IP cores used in the different
>>>> fsl processors (at least available outside of fsl), that makes it quite
>>>> difficult to say if something found on one imx is really the same as on
>>>> the other one. And they (usually) don't provide any versioning
>>>> information in a register or the documentation.
>>>>
>>>> just my 2¢
>>>
>>> $SUBJECT is trying to differentiate a single feature (or maybe two) to
>>> replace cpu_is_xxx(), then expose that on driver_data without creating
>>> one enum value for each release from fsl.
>>
>> Felipe, every one or two SoCs may have their special operations for
>> integrate PHY interface, clk operation, or workaround for IC
>> limitation.
> 
> the particular PHY and clk used should be hidden by phy layer and clk
> API respectively. Workarounds, fair enough, we need to handle them; but
> ideally those should be based on runtime revision detection, not some
> hackery using driver_data.

If this is actually possible, I'd love to do this. But IP vendor don't
include a version register in their cores. :(

>> Maybe, it will add more future or SoCs (maybe not for this driver) in
>> the future, using enum is easier than string comparison for expanding
>> something.
> 
> a) I never told you to *not* use enum. I said that creating DEVICE_A,
> DEVICE_B, DEVICE_C, DEVICE_D and DEVICE_E values when DEVICE_B,
> DEVICE_C and DEVICE_E behave exactly the same is unnecessary.
> 
> b) you can't be expecting to add future SoCs support to fsl udc, I have
> already said and will repeat for the last time: move to chipidea ASAP.
> 
> New SoCs cannot be added to fsl udc, you *must* use chipidea for
> anything new and move the legacy to chipidea eventually. I will wait for
> a full year for you to do that, but after that I will have to start
> deleting drivers for the sake of avoid duplication of effort.

+1

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On Mon, Jan 14, 2013 at 06:54:22PM +0100, Marc Kleine-Budde wrote:
> On 01/14/2013 06:40 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Jan 14, 2013 at 08:56:33PM +0800, Peter Chen wrote:
> > 
> > <snip>
> > 
> >>>> Usually there isn't any Changelog between IP cores used in the different
> >>>> fsl processors (at least available outside of fsl), that makes it quite
> >>>> difficult to say if something found on one imx is really the same as on
> >>>> the other one. And they (usually) don't provide any versioning
> >>>> information in a register or the documentation.
> >>>>
> >>>> just my 2¢
> >>>
> >>> $SUBJECT is trying to differentiate a single feature (or maybe two) to
> >>> replace cpu_is_xxx(), then expose that on driver_data without creating
> >>> one enum value for each release from fsl.
> >>
> >> Felipe, every one or two SoCs may have their special operations for
> >> integrate PHY interface, clk operation, or workaround for IC
> >> limitation.
> > 
> > the particular PHY and clk used should be hidden by phy layer and clk
> > API respectively. Workarounds, fair enough, we need to handle them; but
> > ideally those should be based on runtime revision detection, not some
> > hackery using driver_data.
> 
> If this is actually possible, I'd love to do this. But IP vendor don't
> include a version register in their cores. :(

then fair enough, driver_data or platform_data is the way to go, still
my point (a) below is valid.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Peter Chen]

On Mon, Jan 14, 2013 at 07:57:24PM +0200, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 06:54:22PM +0100, Marc Kleine-Budde wrote:
> > On 01/14/2013 06:40 PM, Felipe Balbi wrote:
> > > Hi,
> > >=20
> > > On Mon, Jan 14, 2013 at 08:56:33PM +0800, Peter Chen wrote:
> > >=20
> > > <snip>
> > >=20
> > >>>> Usually there isn't any Changelog between IP cores used in the d=
ifferent
> > >>>> fsl processors (at least available outside of fsl), that makes i=
t quite
> > >>>> difficult to say if something found on one imx is really the sam=
e as on
> > >>>> the other one. And they (usually) don't provide any versioning
> > >>>> information in a register or the documentation.
> > >>>>
> > >>>> just my 2=A2
> > >>>
> > >>> $SUBJECT is trying to differentiate a single feature (or maybe tw=
o) to
> > >>> replace cpu_is_xxx(), then expose that on driver_data without cre=
ating
> > >>> one enum value for each release from fsl.
> > >>
> > >> Felipe, every one or two SoCs may have their special operations fo=
r
> > >> integrate PHY interface, clk operation, or workaround for IC
> > >> limitation.
> > >=20
> > > the particular PHY and clk used should be hidden by phy layer and c=
lk
> > > API respectively. Workarounds, fair enough, we need to handle them;=
 but
> > > ideally those should be based on runtime revision detection, not so=
me
> > > hackery using driver_data.
> >=20
> > If this is actually possible, I'd love to do this. But IP vendor don'=
t
> > include a version register in their cores. :(
>=20
> then fair enough, driver_data or platform_data is the way to go, still
> my point (a) below is valid.
I will send v5 patch with your suggestion.

>=20
> --=20
> balbi



--=20

Best Regards,
Peter Chen

[PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap

[Peter Chen]

As mach/hardware.h is deleted, we can't visit platform code at driver.
It has no phy driver to combine with this controller, so it has to use
ioremap to map phy address as a workaround.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/gadget/fsl_mxc_udc.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
index 6df45f7..0e858e6 100644
--- a/drivers/usb/gadget/fsl_mxc_udc.c
+++ b/drivers/usb/gadget/fsl_mxc_udc.c
@@ -23,7 +23,8 @@ static struct clk *mxc_per_clk;
 static struct clk *mxc_ipg_clk;
 
 /* workaround ENGcm09152 for i.MX35 */
-#define USBPHYCTRL_OTGBASE_OFFSET	0x608
+#define MX35_USBPHYCTRL_OFFSET		0x600
+#define USBPHYCTRL_OTGBASE_OFFSET	0x8
 #define USBPHYCTRL_EVDO			(1 << 23)
 
 int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev)
@@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
 	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
 	if (devtype == IMX35_UDC) {
 		unsigned int v;
+		void __iomem *phy_regs = ioremap((unsigned long)pdata->regs +
+						MX35_USBPHYCTRL_OFFSET, 512);
 
 		/* workaround ENGcm09152 for i.MX35 */
 		if (pdata->workaround & FLS_USB2_WORKAROUND_ENGCM09152) {
-			v = readl(MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
-					USBPHYCTRL_OTGBASE_OFFSET));
+			v = readl(phy_regs + USBPHYCTRL_OTGBASE_OFFSET);
 			writel(v | USBPHYCTRL_EVDO,
-				MX35_IO_ADDRESS(MX35_USB_BASE_ADDR +
-					USBPHYCTRL_OTGBASE_OFFSET));
+				phy_regs + USBPHYCTRL_OTGBASE_OFFSET);
 		}
+		iounmap(phy_regs);
 	}
 
 	/* ULPI transceivers don't need usbpll */
-- 
1.7.0.4

Re: [PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

On Mon, Jan 14, 2013 at 06:12:40PM +0800, Peter Chen wrote:
> As mach/hardware.h is deleted, we can't visit platform code at driver.
> It has no phy driver to combine with this controller, so it has to use
> ioremap to map phy address as a workaround.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/gadget/fsl_mxc_udc.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
> index 6df45f7..0e858e6 100644
> --- a/drivers/usb/gadget/fsl_mxc_udc.c
> +++ b/drivers/usb/gadget/fsl_mxc_udc.c
> @@ -23,7 +23,8 @@ static struct clk *mxc_per_clk;
>  static struct clk *mxc_ipg_clk;
>  
>  /* workaround ENGcm09152 for i.MX35 */
> -#define USBPHYCTRL_OTGBASE_OFFSET	0x608
> +#define MX35_USBPHYCTRL_OFFSET		0x600
> +#define USBPHYCTRL_OTGBASE_OFFSET	0x8
>  #define USBPHYCTRL_EVDO			(1 << 23)
>  
>  int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev)
> @@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
>  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
>  	if (devtype == IMX35_UDC) {
>  		unsigned int v;
> +		void __iomem *phy_regs = ioremap((unsigned long)pdata->regs +
> +						MX35_USBPHYCTRL_OFFSET, 512);

as I said before, this should be passed via struct resource, not pdata.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap

[Peter Chen]

On Mon, Jan 14, 2013 at 12:17:08PM +0200, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 06:12:40PM +0800, Peter Chen wrote:
> > As mach/hardware.h is deleted, we can't visit platform code at driver.
> > It has no phy driver to combine with this controller, so it has to use
> > ioremap to map phy address as a workaround.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  drivers/usb/gadget/fsl_mxc_udc.c |   12 +++++++-----
> >  1 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/fsl_mxc_udc.c b/drivers/usb/gadget/fsl_mxc_udc.c
> > index 6df45f7..0e858e6 100644
> > --- a/drivers/usb/gadget/fsl_mxc_udc.c
> > +++ b/drivers/usb/gadget/fsl_mxc_udc.c
> > @@ -23,7 +23,8 @@ static struct clk *mxc_per_clk;
> >  static struct clk *mxc_ipg_clk;
> >  
> >  /* workaround ENGcm09152 for i.MX35 */
> > -#define USBPHYCTRL_OTGBASE_OFFSET	0x608
> > +#define MX35_USBPHYCTRL_OFFSET		0x600
> > +#define USBPHYCTRL_OTGBASE_OFFSET	0x8
> >  #define USBPHYCTRL_EVDO			(1 << 23)
> >  
> >  int fsl_udc_clk_init(enum fsl_udc_type devtype, struct platform_device *pdev)
> > @@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
> >  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
> >  	if (devtype == IMX35_UDC) {
> >  		unsigned int v;
> > +		void __iomem *phy_regs = ioremap((unsigned long)pdata->regs +
> > +						MX35_USBPHYCTRL_OFFSET, 512);
> 
> as I said before, this should be passed via struct resource, not pdata.
My careless, will change at next version
> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

Re: [PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap

[Russell King - ARM Linux]

On Mon, Jan 14, 2013 at 06:12:40PM +0800, Peter Chen wrote:
> @@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
>  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
>  	if (devtype == IMX35_UDC) {
>  		unsigned int v;
> +		void __iomem *phy_regs = ioremap((unsigned long)pdata->regs +
> +						MX35_USBPHYCTRL_OFFSET, 512);

Consider that ioremap() can fail.

Re: [PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap

[Peter Chen]

On Mon, Jan 14, 2013 at 01:10:56PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 14, 2013 at 06:12:40PM +0800, Peter Chen wrote:
> > @@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
> >  	struct fsl_usb2_platform_data *pdata = pdev->dev.platform_data;
> >  	if (devtype == IMX35_UDC) {
> >  		unsigned int v;
> > +		void __iomem *phy_regs = ioremap((unsigned long)pdata->regs +
> > +						MX35_USBPHYCTRL_OFFSET, 512);
> 
> Consider that ioremap() can fail.
> 
Thanks, will check NULL pointer.

-- 

Best Regards,
Peter Chen

[PATCH v3 3/3] ARM: i.MX clock: Change the connection-id for fsl-usb2-udc

[Peter Chen]

As we use platform_device_id for fsl-usb2-udc driver, it needs to
change clk connection-id, or the related devm_clk_get will be failed.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/mach-imx/clk-imx25.c       |    6 +++---
 arch/arm/mach-imx/clk-imx27.c       |    6 +++---
 arch/arm/mach-imx/clk-imx31.c       |    6 +++---
 arch/arm/mach-imx/clk-imx35.c       |    6 +++---
 arch/arm/mach-imx/clk-imx51-imx53.c |    6 +++---
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-imx/clk-imx25.c b/arch/arm/mach-imx/clk-imx25.c
index b197aa7..67e353d 100644
--- a/arch/arm/mach-imx/clk-imx25.c
+++ b/arch/arm/mach-imx/clk-imx25.c
@@ -254,9 +254,9 @@ int __init mx25_clocks_init(void)
 	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
 	clk_register_clkdev(clk[usbotg_ahb], "ahb", "mxc-ehci.2");
 	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.2");
-	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usbotg_ahb], "ahb", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
+	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx25");
+	clk_register_clkdev(clk[usbotg_ahb], "ahb", "imx-udc-mx25");
+	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx25");
 	clk_register_clkdev(clk[nfc_ipg_per], NULL, "imx25-nand.0");
 	/* i.mx25 has the i.mx35 type cspi */
 	clk_register_clkdev(clk[cspi1_ipg], NULL, "imx35-cspi.0");
diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
index 4c1d1e4..1ffe3b5 100644
--- a/arch/arm/mach-imx/clk-imx27.c
+++ b/arch/arm/mach-imx/clk-imx27.c
@@ -236,9 +236,9 @@ int __init mx27_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[lcdc_ahb_gate], "ahb", "imx21-fb.0");
 	clk_register_clkdev(clk[csi_ahb_gate], "ahb", "imx27-camera.0");
 	clk_register_clkdev(clk[per4_gate], "per", "imx27-camera.0");
-	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "fsl-usb2-udc");
+	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx27");
+	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "imx-udc-mx27");
+	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "imx-udc-mx27");
 	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.0");
 	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "mxc-ehci.0");
 	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "mxc-ehci.0");
diff --git a/arch/arm/mach-imx/clk-imx31.c b/arch/arm/mach-imx/clk-imx31.c
index 8be64e0..ef66eaf 100644
--- a/arch/arm/mach-imx/clk-imx31.c
+++ b/arch/arm/mach-imx/clk-imx31.c
@@ -139,9 +139,9 @@ int __init mx31_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[usb_div_post], "per", "mxc-ehci.2");
 	clk_register_clkdev(clk[usb_gate], "ahb", "mxc-ehci.2");
 	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
-	clk_register_clkdev(clk[usb_div_post], "per", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usb_gate], "ahb", "fsl-usb2-udc");
-	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
+	clk_register_clkdev(clk[usb_div_post], "per", "imx-udc-mx31");
+	clk_register_clkdev(clk[usb_gate], "ahb", "imx-udc-mx31");
+	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx31");
 	clk_register_clkdev(clk[csi_gate], NULL, "mx3-camera.0");
 	/* i.mx31 has the i.mx21 type uart */
 	clk_register_clkdev(clk[uart1_gate], "per", "imx21-uart.0");
diff --git a/arch/arm/mach-imx/clk-imx35.c b/arch/arm/mach-imx/clk-imx35.c
index 66f3d65..69fe9c8 100644
--- a/arch/arm/mach-imx/clk-imx35.c
+++ b/arch/arm/mach-imx/clk-imx35.c
@@ -251,9 +251,9 @@ int __init mx35_clocks_init()
 	clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.2");
 	clk_register_clkdev(clk[ipg], "ipg", "mxc-ehci.2");
 	clk_register_clkdev(clk[usbotg_gate], "ahb", "mxc-ehci.2");
-	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
-	clk_register_clkdev(clk[ipg], "ipg", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usbotg_gate], "ahb", "fsl-usb2-udc");
+	clk_register_clkdev(clk[usb_div], "per", "imx-udc-mx35");
+	clk_register_clkdev(clk[ipg], "ipg", "imx-udc-mx35");
+	clk_register_clkdev(clk[usbotg_gate], "ahb", "imx-udc-mx35");
 	clk_register_clkdev(clk[wdog_gate], NULL, "imx2-wdt.0");
 	clk_register_clkdev(clk[nfc_div], NULL, "imx25-nand.0");
 	clk_register_clkdev(clk[csi_gate], NULL, "mx3-camera.0");
diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
index 579023f..fb7cb84 100644
--- a/arch/arm/mach-imx/clk-imx51-imx53.c
+++ b/arch/arm/mach-imx/clk-imx51-imx53.c
@@ -269,9 +269,9 @@ static void __init mx5_clocks_common_init(unsigned long rate_ckil,
 	clk_register_clkdev(clk[usboh3_per_gate], "per", "mxc-ehci.2");
 	clk_register_clkdev(clk[usboh3_gate], "ipg", "mxc-ehci.2");
 	clk_register_clkdev(clk[usboh3_gate], "ahb", "mxc-ehci.2");
-	clk_register_clkdev(clk[usboh3_per_gate], "per", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usboh3_gate], "ipg", "fsl-usb2-udc");
-	clk_register_clkdev(clk[usboh3_gate], "ahb", "fsl-usb2-udc");
+	clk_register_clkdev(clk[usboh3_per_gate], "per", "imx-udc-mx51");
+	clk_register_clkdev(clk[usboh3_gate], "ipg", "imx-udc-mx51");
+	clk_register_clkdev(clk[usboh3_gate], "ahb", "imx-udc-mx51");
 	clk_register_clkdev(clk[nfc_gate], NULL, "imx51-nand");
 	clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "imx-ssi.0");
 	clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "imx-ssi.1");
-- 
1.7.0.4