On Mon, Aug 12, 2013 at 12:29:42PM +0200, Sascha Hauer wrote:
> The chipidea i.MX driver is split into two drivers. The ci_hdrc_imx driver
> handles the chipidea cores and the usbmisc_imx driver handles the noncore
> registers common to all chipidea cores (but SoC specific). Current flow is:
>
> - usbmisc sets an ops pointer in the ci_hdrc_imx driver during probe
> - ci_hdrc_imx checks if the pointer is valid during probe, if yes calls
> the functions in the ops pointer.
> - usbmisc_imx calls back into the ci_hdrc_imx driver to get additional
> data
>
> This is overly complicated and has problems if the drivers are compiled
> as modules. In this case the usbmisc_imx driver can be unloaded even if
> the ci_hdrc_imx driver still needs usbmisc functionality.
>
> This patch changes this by letting the ci_hdrc_imx driver calling functions
> from the usbmisc_imx driver. This way the symbol resolving during module
> load makes sure the ci_hdrc_imx driver depends on the usbmisc_imx driver.
>
> Also instead of letting the usbmisc_imx driver call back into the ci_hdrc_imx
> driver, pass the needed data in the first place.
>
> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> ---
> drivers/usb/chipidea/ci_hdrc_imx.c | 71 +++++++++++++---------------
> drivers/usb/chipidea/ci_hdrc_imx.h | 17 ++-----
> drivers/usb/chipidea/usbmisc_imx.c | 95 +++++++++++++++++---------------------
> 3 files changed, 76 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 11ed423..b9464ff 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -29,57 +29,48 @@ struct ci_hdrc_imx_data {
> struct platform_device *ci_pdev;
> struct clk *clk;
> struct regulator *reg_vbus;
> + struct imx_usbmisc_data *usbmisc_data;
> };
>
> -static const struct usbmisc_ops *usbmisc_ops;
> -
> /* Common functions shared by usbmisc drivers */
>
> -int usbmisc_set_ops(const struct usbmisc_ops *ops)
> -{
> - if (usbmisc_ops)
> - return -EBUSY;
> -
> - usbmisc_ops = ops;
> -
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(usbmisc_set_ops);
> -
> -void usbmisc_unset_ops(const struct usbmisc_ops *ops)
> -{
> - usbmisc_ops = NULL;
> -}
> -EXPORT_SYMBOL_GPL(usbmisc_unset_ops);
> -
> -int usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device *usbdev)
> +static struct imx_usbmisc_data *usbmisc_get_init_data(struct device *dev)
> {
> struct device_node *np = dev->of_node;
> struct of_phandle_args args;
> + struct imx_usbmisc_data *data;
> int ret;
>
> - usbdev->dev = dev;
> + /*
> + * In case the fsl,usbmisc property is not present this device doesn't
> + * need usbmisc. Return NULL (which is no error here)
> + */
> + if (!of_get_property(np, "fsl,usbmisc", NULL))
> + return NULL;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return ERR_PTR(-ENOMEM);
>
> ret = of_parse_phandle_with_args(np, "fsl,usbmisc", "#index-cells",
> 0, &args);
> if (ret) {
> dev_err(dev, "Failed to parse property fsl,usbmisc, errno %d\n",
> ret);
> - memset(usbdev, 0, sizeof(*usbdev));
> - return ret;
> + return ERR_PTR(ret);
> }
> - usbdev->index = args.args[0];
> +
> + data->index = args.args[0];
> of_node_put(args.np);
>
> if (of_find_property(np, "disable-over-current", NULL))
> - usbdev->disable_oc = 1;
> + data->disable_oc = 1;
>
> if (of_find_property(np, "external-vbus-divider", NULL))
> - usbdev->evdo = 1;
> + data->evdo = 1;
>
> - return 0;
> + return data;
> }
> -EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
>
> /* End of common functions shared by usbmisc drivers*/
>
> @@ -96,10 +87,6 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> struct resource *res;
> int ret;
>
> - if (of_find_property(pdev->dev.of_node, "fsl,usbmisc", NULL)
> - && !usbmisc_ops)
> - return -EPROBE_DEFER;
> -
> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> if (!data) {
> dev_err(&pdev->dev, "Failed to allocate ci_hdrc-imx data!\n");
> @@ -112,6 +99,10 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> return -ENOENT;
> }
>
> + data->usbmisc_data = usbmisc_get_init_data(&pdev->dev);
> + if (IS_ERR(data->usbmisc_data))
> + return PTR_ERR(data->usbmisc_data);
> +
> data->clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(data->clk)) {
> dev_err(&pdev->dev,
> @@ -159,11 +150,11 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> if (!pdev->dev.coherent_dma_mask)
> pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>
> - if (usbmisc_ops && usbmisc_ops->init) {
> - ret = usbmisc_ops->init(&pdev->dev);
> + if (data->usbmisc_data) {
> + ret = imx_usbmisc_init(data->usbmisc_data);
> if (ret) {
> - dev_err(&pdev->dev,
> - "usbmisc init failed, ret=%d\n", ret);
> + dev_err(&pdev->dev, "usbmisc init failed, ret=%d\n",
> + ret);
> goto err;
> }
> }
> @@ -179,11 +170,11 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> goto err;
> }
>
> - if (usbmisc_ops && usbmisc_ops->post) {
> - ret = usbmisc_ops->post(&pdev->dev);
> + if (data->usbmisc_data) {
> + ret = imx_usbmisc_init_post(data->usbmisc_data);
> if (ret) {
> - dev_err(&pdev->dev,
> - "usbmisc post failed, ret=%d\n", ret);
> + dev_err(&pdev->dev, "usbmisc post failed, ret=%d\n",
> + ret);
> goto disable_device;
> }
> }
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h b/drivers/usb/chipidea/ci_hdrc_imx.h
> index 550bfa4..c727159 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> @@ -9,23 +9,12 @@
> * http://www.gnu.org/copyleft/gpl.html
> */
>
> -/* Used to set SoC specific callbacks */
> -struct usbmisc_ops {
> - /* It's called once when probe a usb device */
> - int (*init)(struct device *dev);
> - /* It's called once after adding a usb device */
> - int (*post)(struct device *dev);
> -};
> -
> -struct usbmisc_usb_device {
> - struct device *dev; /* usb controller device */
> +struct imx_usbmisc_data {
> int index;
>
> unsigned int disable_oc:1; /* over current detect disabled */
> unsigned int evdo:1; /* set external vbus divider option */
> };
>
> -int usbmisc_set_ops(const struct usbmisc_ops *ops);
> -void usbmisc_unset_ops(const struct usbmisc_ops *ops);
> -int
> -usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device *usbdev);
> +int imx_usbmisc_init(struct imx_usbmisc_data *);
> +int imx_usbmisc_init_post(struct imx_usbmisc_data *);
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> index ac5a461..8a1094b 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -18,8 +18,6 @@
>
> #include "ci_hdrc_imx.h"
>
> -#define USB_DEV_MAX 4
> -
> #define MX25_USB_PHY_CTRL_OFFSET 0x08
> #define MX25_BM_EXTERNAL_VBUS_DIVIDER BIT(23)
>
> @@ -32,51 +30,34 @@
>
> #define MX6_BM_OVER_CUR_DIS BIT(7)
>
> +struct usbmisc_ops {
> + /* It's called once when probe a usb device */
> + int (*init)(struct imx_usbmisc_data *data);
> + /* It's called once after adding a usb device */
> + int (*post)(struct imx_usbmisc_data *data);
> +};
> +
> struct imx_usbmisc {
> void __iomem *base;
> spinlock_t lock;
> struct clk *clk;
> - struct usbmisc_usb_device usbdev[USB_DEV_MAX];
> const struct usbmisc_ops *ops;
> };
>
> static struct imx_usbmisc *usbmisc;
>
> -static struct usbmisc_usb_device *get_usbdev(struct device *dev)
> -{
> - int i, ret;
> -
> - for (i = 0; i < USB_DEV_MAX; i++) {
> - if (usbmisc->usbdev[i].dev == dev)
> - return &usbmisc->usbdev[i];
> - else if (!usbmisc->usbdev[i].dev)
> - break;
> - }
> -
> - if (i >= USB_DEV_MAX)
> - return ERR_PTR(-EBUSY);
> -
> - ret = usbmisc_get_init_data(dev, &usbmisc->usbdev[i]);
> - if (ret)
> - return ERR_PTR(ret);
> -
> - return &usbmisc->usbdev[i];
> -}
> -
> -static int usbmisc_imx25_post(struct device *dev)
> +static int usbmisc_imx25_post(struct imx_usbmisc_data *data)
> {
> - struct usbmisc_usb_device *usbdev;
> void __iomem *reg;
> unsigned long flags;
> u32 val;
>
> - usbdev = get_usbdev(dev);
> - if (IS_ERR(usbdev))
> - return PTR_ERR(usbdev);
> + if (data->index > 2)
> + return -EINVAL;
>
> reg = usbmisc->base + MX25_USB_PHY_CTRL_OFFSET;
>
> - if (usbdev->evdo) {
> + if (data->evdo) {
> spin_lock_irqsave(&usbmisc->lock, flags);
> val = readl(reg);
> writel(val | MX25_BM_EXTERNAL_VBUS_DIVIDER, reg);
> @@ -87,20 +68,18 @@ static int usbmisc_imx25_post(struct device *dev)
> return 0;
> }
>
> -static int usbmisc_imx53_init(struct device *dev)
> +static int usbmisc_imx53_init(struct imx_usbmisc_data *data)
> {
> - struct usbmisc_usb_device *usbdev;
> void __iomem *reg = NULL;
> unsigned long flags;
> u32 val = 0;
>
> - usbdev = get_usbdev(dev);
> - if (IS_ERR(usbdev))
> - return PTR_ERR(usbdev);
> + if (data->index > 3)
> + return -EINVAL;
>
> - if (usbdev->disable_oc) {
> + if (data->disable_oc) {
> spin_lock_irqsave(&usbmisc->lock, flags);
> - switch (usbdev->index) {
> + switch (data->index) {
> case 0:
> reg = usbmisc->base + MX53_USB_OTG_PHY_CTRL_0_OFFSET;
> val = readl(reg) | MX53_BM_OVER_CUR_DIS_OTG;
> @@ -126,22 +105,19 @@ static int usbmisc_imx53_init(struct device *dev)
> return 0;
> }
>
> -static int usbmisc_imx6q_init(struct device *dev)
> +static int usbmisc_imx6q_init(struct imx_usbmisc_data *data)
> {
> -
> - struct usbmisc_usb_device *usbdev;
> unsigned long flags;
> u32 reg;
>
> - usbdev = get_usbdev(dev);
> - if (IS_ERR(usbdev))
> - return PTR_ERR(usbdev);
> + if (data->index > 3)
> + return -EINVAL;
>
> - if (usbdev->disable_oc) {
> + if (data->disable_oc) {
> spin_lock_irqsave(&usbmisc->lock, flags);
> - reg = readl(usbmisc->base + usbdev->index * 4);
> + reg = readl(usbmisc->base + data->index * 4);
> writel(reg | MX6_BM_OVER_CUR_DIS,
> - usbmisc->base + usbdev->index * 4);
> + usbmisc->base + data->index * 4);
> spin_unlock_irqrestore(&usbmisc->lock, flags);
> }
>
> @@ -160,6 +136,26 @@ static const struct usbmisc_ops imx6q_usbmisc_ops = {
> .init = usbmisc_imx6q_init,
> };
>
> +int imx_usbmisc_init(struct imx_usbmisc_data *data)
> +{
> + if (!usbmisc)
> + return -EPROBE_DEFER;
> + if (!usbmisc->ops->init)
> + return 0;
> + return usbmisc->ops->init(data);
> +}
> +EXPORT_SYMBOL_GPL(imx_usbmisc_init);
> +
> +int imx_usbmisc_init_post(struct imx_usbmisc_data *data)
> +{
> + if (!usbmisc)
> + return -EPROBE_DEFER;
> + if (!usbmisc->ops->post)
> + return 0;
> + return usbmisc->ops->post(data);
> +}
> +EXPORT_SYMBOL_GPL(imx_usbmisc_init_post);
> +
> static const struct of_device_id usbmisc_imx_dt_ids[] = {
> {
> .compatible = "fsl,imx25-usbmisc",
> @@ -216,19 +212,12 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
> of_match_device(usbmisc_imx_dt_ids, &pdev->dev);
> data->ops = (const struct usbmisc_ops *)tmp_dev->data;
> usbmisc = data;
> - ret = usbmisc_set_ops(data->ops);
> - if (ret) {
> - usbmisc = NULL;
> - clk_disable_unprepare(data->clk);
> - return ret;
> - }
>
> return 0;
> }
>
> static int usbmisc_imx_remove(struct platform_device *pdev)
> {
> - usbmisc_unset_ops(usbmisc->ops);
> clk_disable_unprepare(usbmisc->clk);
> usbmisc = NULL;
> return 0;
> --
> 1.8.4.rc1
>
Acked-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
