diff mbox

[v2,02/14] usb: phy-mxs: Add platform judgement code

Message ID 1382421528-17897-3-git-send-email-peter.chen@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Chen Oct. 22, 2013, 5:58 a.m. UTC 
The mxs-phy has three versions until now, each versions have
some differences among PHY operations. the 1st version is
for mx23/mx28 SoC, The 2nd version is for mx6q and mx6dl, the
3rd version is for mx6sl and later mx6 platform.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/phy/phy-mxs-usb.c |   65 ++++++++++++++++++++++++++++++++++++-----
 1 files changed, 57 insertions(+), 8 deletions(-)

Comments

Shawn Guo Oct. 23, 2013, 6:13 a.m. UTC | #1 
On Tue, Oct 22, 2013 at 01:58:36PM +0800, Peter Chen wrote:
> The mxs-phy has three versions until now, each versions have
> some differences among PHY operations. the 1st version is
> for mx23/mx28 SoC, The 2nd version is for mx6q and mx6dl, the
> 3rd version is for mx6sl and later mx6 platform.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/phy/phy-mxs-usb.c |   65 ++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 57 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> index fdd33b4..a0628d6 100644
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012-2013 Freescale Semiconductor, Inc.
>   * Copyright (C) 2012 Marek Vasut <marex@denx.de>
>   * on behalf of DENX Software Engineering GmbH
>   *
> @@ -20,6 +20,7 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/of_device.h>
>  
>  #define DRIVER_NAME "mxs_phy"
>  
> @@ -34,12 +35,57 @@
>  #define BM_USBPHY_CTRL_ENUTMILEVEL2		BIT(14)
>  #define BM_USBPHY_CTRL_ENHOSTDISCONDETECT	BIT(1)
>  
> +#define to_mxs_phy(p) container_of((p), struct mxs_phy, phy)
> +
> +enum imx_phy_type {
> +	IMX6Q_USB_PHY,
> +	IMX6SL_USB_PHY,
> +	IMX23_USB_PHY,
> +};
> +
>  struct mxs_phy {
>  	struct usb_phy phy;
>  	struct clk *clk;
> +	enum imx_phy_type devtype;
>  };
>  
> -#define to_mxs_phy(p) container_of((p), struct mxs_phy, phy)
> +static inline int is_mx6q_phy(struct mxs_phy *data)
> +{
> +	return data->devtype == IMX6Q_USB_PHY;
> +}
> +
> +static inline int is_mx6sl_phy(struct mxs_phy *data)
> +{
> +	return data->devtype == IMX6SL_USB_PHY;
> +}
> +
> +static inline int is_mx23_phy(struct mxs_phy *data)
> +{
> +	return data->devtype == IMX23_USB_PHY;
> +}
> +
> +static struct platform_device_id imx_phy_devtype[] = {
> +	{
> +		.name = "usb-phy-imx6q",
> +		.driver_data = IMX6Q_USB_PHY,
> +	}, {
> +		.name = "usb-phy-imx6sl",
> +		.driver_data = IMX6SL_USB_PHY,
> +	}, {
> +		.name = "usb-phy-imx23",
> +		.driver_data = IMX23_USB_PHY,
> +	}, {
> +		/* sentinel */
> +	}
> +};

I know many imx device drivers have this platform_device_id table, but
that's because they need to support both non-DT and DT probe.  Since
this driver supports DT probe only, we can save this table by passing
imx_phy_type value through of_device_id.data directly.

> +static const struct of_device_id mxs_phy_dt_ids[] = {
> +	{ .compatible = "fsl,imx6q-usbphy", .data = &imx_phy_devtype[IMX6Q_USB_PHY], },
> +	{ .compatible = "fsl,imx6sl-usbphy", .data = &imx_phy_devtype[IMX6SL_USB_PHY], },
> +	{ .compatible = "fsl,imx23-usbphy", .data = &imx_phy_devtype[IMX23_USB_PHY], },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
>  
>  static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
>  {
> @@ -131,6 +177,14 @@ static int mxs_phy_probe(struct platform_device *pdev)
>  	struct clk *clk;
>  	struct mxs_phy *mxs_phy;
>  	int ret;
> +	const struct of_device_id *of_id =
> +			of_match_device(mxs_phy_dt_ids, &pdev->dev);
> +
> +	/* This driver is DT-only version now */
> +	if (!of_id)
> +		return -ENXIO;

Since it's DT-only, I'm not sure you will run into the case that
mxs_phy_probe() is called with a NULL of_id.  The check looks
unnecessary to me.

> +
> +	pdev->id_entry = of_id->data;

Some imx device drivers did the same thing, but we should keep
pdev->id_entry immutable.  The removal of that platform_device_id table
will help save this.

Shawn

>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	base = devm_ioremap_resource(&pdev->dev, res);
> @@ -163,6 +217,7 @@ static int mxs_phy_probe(struct platform_device *pdev)
>  	ATOMIC_INIT_NOTIFIER_HEAD(&mxs_phy->phy.notifier);
>  
>  	mxs_phy->clk = clk;
> +	mxs_phy->devtype = pdev->id_entry->driver_data;
>  
>  	platform_set_drvdata(pdev, &mxs_phy->phy);
>  
> @@ -182,12 +237,6 @@ static int mxs_phy_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id mxs_phy_dt_ids[] = {
> -	{ .compatible = "fsl,imx23-usbphy", },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
> -
>  static struct platform_driver mxs_phy_driver = {
>  	.probe = mxs_phy_probe,
>  	.remove = mxs_phy_remove,
> -- 
> 1.7.1
> 
>
Peter Chen Oct. 23, 2013, 6:46 a.m. UTC | #2 
On Wed, Oct 23, 2013 at 02:13:24PM +0800, Shawn Guo wrote:
> > +
> > +enum imx_phy_type {
> > +	IMX6Q_USB_PHY,
> > +	IMX6SL_USB_PHY,
> > +	IMX23_USB_PHY,
> > +};
> > +
> >  struct mxs_phy {
> >  	struct usb_phy phy;
> >  	struct clk *clk;
> > +	enum imx_phy_type devtype;
> >  };
> >  
> > -#define to_mxs_phy(p) container_of((p), struct mxs_phy, phy)
> > +static inline int is_mx6q_phy(struct mxs_phy *data)
> > +{
> > +	return data->devtype == IMX6Q_USB_PHY;
> > +}
> > +
> > +static inline int is_mx6sl_phy(struct mxs_phy *data)
> > +{
> > +	return data->devtype == IMX6SL_USB_PHY;
> > +}
> > +
> > +static inline int is_mx23_phy(struct mxs_phy *data)
> > +{
> > +	return data->devtype == IMX23_USB_PHY;
> > +}
> > +
> > +static struct platform_device_id imx_phy_devtype[] = {
> > +	{
> > +		.name = "usb-phy-imx6q",
> > +		.driver_data = IMX6Q_USB_PHY,
> > +	}, {
> > +		.name = "usb-phy-imx6sl",
> > +		.driver_data = IMX6SL_USB_PHY,
> > +	}, {
> > +		.name = "usb-phy-imx23",
> > +		.driver_data = IMX23_USB_PHY,
> > +	}, {
> > +		/* sentinel */
> > +	}
> > +};
> 
> I know many imx device drivers have this platform_device_id table, but
> that's because they need to support both non-DT and DT probe.  Since
> this driver supports DT probe only, we can save this table by passing
> imx_phy_type value through of_device_id.data directly.

How about compare compatible string directly at probe?

	if (of_device_is_compatible(np, "fsl,imx6q-usbphy"))
		mxs_phy->devtype = IMX6Q_USB_PHY;
	else if ((of_device_is_compatible(np, "fsl,imx6sl-usbphy"))
		mxs_phy->devtype = IMX6SL_USB_PHY;
	else if (...)
		...;

I don't know how to passing imx_phy_type value through below table
directly? imx_phy_type is a enum variable, not a arrary.

> 
> > +static const struct of_device_id mxs_phy_dt_ids[] = {
> > +	{ .compatible = "fsl,imx6q-usbphy", .data = &imx_phy_devtype[IMX6Q_USB_PHY], },
> > +	{ .compatible = "fsl,imx6sl-usbphy", .data = &imx_phy_devtype[IMX6SL_USB_PHY], },
> > +	{ .compatible = "fsl,imx23-usbphy", .data = &imx_phy_devtype[IMX23_USB_PHY], },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
> >  
> >  static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
> >  {
> > @@ -131,6 +177,14 @@ static int mxs_phy_probe(struct platform_device *pdev)
> >  	struct clk *clk;
> >  	struct mxs_phy *mxs_phy;
> >  	int ret;
> > +	const struct of_device_id *of_id =
> > +			of_match_device(mxs_phy_dt_ids, &pdev->dev);
> > +
> > +	/* This driver is DT-only version now */
> > +	if (!of_id)
> > +		return -ENXIO;
> 
> Since it's DT-only, I'm not sure you will run into the case that
> mxs_phy_probe() is called with a NULL of_id.  The check looks
> unnecessary to me.

Will change.

> 
> > +
> > +	pdev->id_entry = of_id->data;
> 
> Some imx device drivers did the same thing, but we should keep
> pdev->id_entry immutable.  The removal of that platform_device_id table
> will help save this.

Surely we can delete this line if no platform_device_id table.
Peter Chen Oct. 23, 2013, 8:55 a.m. UTC | #3 
On Wed, Oct 23, 2013 at 05:04:44PM +0800, Shawn Guo wrote:
> On Wed, Oct 23, 2013 at 02:46:04PM +0800, Peter Chen wrote:
> > How about compare compatible string directly at probe?
> > 
> > 	if (of_device_is_compatible(np, "fsl,imx6q-usbphy"))
> > 		mxs_phy->devtype = IMX6Q_USB_PHY;
> > 	else if ((of_device_is_compatible(np, "fsl,imx6sl-usbphy"))
> > 		mxs_phy->devtype = IMX6SL_USB_PHY;
> > 	else if (...)
> > 		...;
> 
> It can work, but in general, we should avoid unnecessary device tree
> lookup.  I would suggest something like below.
> 
> #define MXS_FLAGS_SUSPEND_ISSUE_1	BIT(0)
> #define MXS_FLAGS_SUSPEND_ISSUE_2	BIT(1)
> #define MXS_FLAGS_LINE_DISCONNECT	BIT(2)
> 
> struct mxs_phy_data {
> 	unsigned int flags;
> };
> 
> struct mxs_phy {
> 	...
> 	mxs_phy_data *data;
> };
> 
> static struct mxs_phy_data imx6sl_usbphy_data = {
>         .flags = MXS_FLAGS_LINE_DISCONNECT,
> };
> 
> static struct mxs_phy_data imx6q_usbphy_data = {
>         .flags = MXS_FLAGS_SUSPEND_ISSUE_2 | MXS_FLAGS_LINE_DISCONNECT,
> };
> 
> static struct mxs_phy_data imx23_usbphy_data = {
>         .flags = MXS_FLAGS_SUSPEND_ISSUE_1 | MXS_FLAGS_SUSPEND_ISSUE_2,
> };
> 
> static const struct of_device_id mxs_phy_dt_ids[] = {
>        { .compatible = "fsl,imx6sl-usbphy", .data = &imx6sl_usbphy_data, },
>        { .compatible = "fsl,imx6q-usbphy", .data = &imx6q_usbphy_data, },
>        { .compatible = "fsl,imx23-usbphy", .data = &imx23_usbphy_data, },
>        { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
> 
> Then you can check the flags for handling different cases.  This would
> be more flexible and future proof.
> 

Great, I will use that way at chipidea driver too.
Shawn Guo Oct. 23, 2013, 9:04 a.m. UTC | #4 
On Wed, Oct 23, 2013 at 02:46:04PM +0800, Peter Chen wrote:
> How about compare compatible string directly at probe?
> 
> 	if (of_device_is_compatible(np, "fsl,imx6q-usbphy"))
> 		mxs_phy->devtype = IMX6Q_USB_PHY;
> 	else if ((of_device_is_compatible(np, "fsl,imx6sl-usbphy"))
> 		mxs_phy->devtype = IMX6SL_USB_PHY;
> 	else if (...)
> 		...;

It can work, but in general, we should avoid unnecessary device tree
lookup.  I would suggest something like below.

#define MXS_FLAGS_SUSPEND_ISSUE_1	BIT(0)
#define MXS_FLAGS_SUSPEND_ISSUE_2	BIT(1)
#define MXS_FLAGS_LINE_DISCONNECT	BIT(2)

struct mxs_phy_data {
	unsigned int flags;
};

struct mxs_phy {
	...
	mxs_phy_data *data;
};

static struct mxs_phy_data imx6sl_usbphy_data = {
        .flags = MXS_FLAGS_LINE_DISCONNECT,
};

static struct mxs_phy_data imx6q_usbphy_data = {
        .flags = MXS_FLAGS_SUSPEND_ISSUE_2 | MXS_FLAGS_LINE_DISCONNECT,
};

static struct mxs_phy_data imx23_usbphy_data = {
        .flags = MXS_FLAGS_SUSPEND_ISSUE_1 | MXS_FLAGS_SUSPEND_ISSUE_2,
};

static const struct of_device_id mxs_phy_dt_ids[] = {
       { .compatible = "fsl,imx6sl-usbphy", .data = &imx6sl_usbphy_data, },
       { .compatible = "fsl,imx6q-usbphy", .data = &imx6q_usbphy_data, },
       { .compatible = "fsl,imx23-usbphy", .data = &imx23_usbphy_data, },
       { /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);

Then you can check the flags for handling different cases.  This would
be more flexible and future proof.

Shawn
diff mbox

Patch

diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
index fdd33b4..a0628d6 100644
--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -1,5 +1,5 @@ 
 /*
- * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2012-2013 Freescale Semiconductor, Inc.
  * Copyright (C) 2012 Marek Vasut <marex@denx.de>
  * on behalf of DENX Software Engineering GmbH
  *
@@ -20,6 +20,7 @@ 
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/of_device.h>
 
 #define DRIVER_NAME "mxs_phy"
 
@@ -34,12 +35,57 @@ 
 #define BM_USBPHY_CTRL_ENUTMILEVEL2		BIT(14)
 #define BM_USBPHY_CTRL_ENHOSTDISCONDETECT	BIT(1)
 
+#define to_mxs_phy(p) container_of((p), struct mxs_phy, phy)
+
+enum imx_phy_type {
+	IMX6Q_USB_PHY,
+	IMX6SL_USB_PHY,
+	IMX23_USB_PHY,
+};
+
 struct mxs_phy {
 	struct usb_phy phy;
 	struct clk *clk;
+	enum imx_phy_type devtype;
 };
 
-#define to_mxs_phy(p) container_of((p), struct mxs_phy, phy)
+static inline int is_mx6q_phy(struct mxs_phy *data)
+{
+	return data->devtype == IMX6Q_USB_PHY;
+}
+
+static inline int is_mx6sl_phy(struct mxs_phy *data)
+{
+	return data->devtype == IMX6SL_USB_PHY;
+}
+
+static inline int is_mx23_phy(struct mxs_phy *data)
+{
+	return data->devtype == IMX23_USB_PHY;
+}
+
+static struct platform_device_id imx_phy_devtype[] = {
+	{
+		.name = "usb-phy-imx6q",
+		.driver_data = IMX6Q_USB_PHY,
+	}, {
+		.name = "usb-phy-imx6sl",
+		.driver_data = IMX6SL_USB_PHY,
+	}, {
+		.name = "usb-phy-imx23",
+		.driver_data = IMX23_USB_PHY,
+	}, {
+		/* sentinel */
+	}
+};
+
+static const struct of_device_id mxs_phy_dt_ids[] = {
+	{ .compatible = "fsl,imx6q-usbphy", .data = &imx_phy_devtype[IMX6Q_USB_PHY], },
+	{ .compatible = "fsl,imx6sl-usbphy", .data = &imx_phy_devtype[IMX6SL_USB_PHY], },
+	{ .compatible = "fsl,imx23-usbphy", .data = &imx_phy_devtype[IMX23_USB_PHY], },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
 
 static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
 {
@@ -131,6 +177,14 @@  static int mxs_phy_probe(struct platform_device *pdev)
 	struct clk *clk;
 	struct mxs_phy *mxs_phy;
 	int ret;
+	const struct of_device_id *of_id =
+			of_match_device(mxs_phy_dt_ids, &pdev->dev);
+
+	/* This driver is DT-only version now */
+	if (!of_id)
+		return -ENXIO;
+
+	pdev->id_entry = of_id->data;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
@@ -163,6 +217,7 @@  static int mxs_phy_probe(struct platform_device *pdev)
 	ATOMIC_INIT_NOTIFIER_HEAD(&mxs_phy->phy.notifier);
 
 	mxs_phy->clk = clk;
+	mxs_phy->devtype = pdev->id_entry->driver_data;
 
 	platform_set_drvdata(pdev, &mxs_phy->phy);
 
@@ -182,12 +237,6 @@  static int mxs_phy_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id mxs_phy_dt_ids[] = {
-	{ .compatible = "fsl,imx23-usbphy", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
-
 static struct platform_driver mxs_phy_driver = {
 	.probe = mxs_phy_probe,
 	.remove = mxs_phy_remove,