On Fri, Aug 09, 2013 at 08:56:56AM +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 <[email protected]>
> ---
> drivers/usb/chipidea/ci_hdrc_imx.c | 74 +++++++++++------------------
> drivers/usb/chipidea/ci_hdrc_imx.h | 17 ++-----
> drivers/usb/chipidea/usbmisc_imx.c | 95
> +++++++++++++++++---------------------
> 3 files changed, 72 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 11ed423..7e722fe 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -29,57 +29,43 @@ 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;
> + if (!of_get_property(np, "fsl,usbmisc", NULL))
> + return NULL;
>
> 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];
> - of_node_put(args.np);
Is it a bug fix?
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return ERR_PTR(-ENOMEM);
> +
> + data->index = args.args[0];
>
> 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 +82,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 +94,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);
> +
You need to consider mx23/mx28 case which doesn't have usbmisc.
Your version can fix current loadable module problem, and it is also
easy to add new ops in future. After fixing mx23/mx28 case, we can use
your version.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
