From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Chen Subject: Re: [PATCH 17/21] usb: chipidea: msm: Make platform data driver local instead of global Date: Wed, 29 Jun 2016 19:29:25 +0800 Message-ID: <20160629112925.GN25236@shlinux2> References: <20160626072838.28082-1-stephen.boyd@linaro.org> <20160626072838.28082-18-stephen.boyd@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-pa0-f68.google.com ([209.85.220.68]:34863 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751169AbcF2LgZ (ORCPT ); Wed, 29 Jun 2016 07:36:25 -0400 Content-Disposition: inline In-Reply-To: <20160626072838.28082-18-stephen.boyd@linaro.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Stephen Boyd Cc: linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, Andy Gross , Bjorn Andersson , Neil Armstrong , Arnd Bergmann , Felipe Balbi , Peter Chen , Greg Kroah-Hartman On Sun, Jun 26, 2016 at 12:28:34AM -0700, Stephen Boyd wrote: > If two devices are probed with this same driver, they'll share > the same platform data structure, while the chipidea core layer > writes and modifies it. This can lead to interesting results > especially if one device is an OTG type chipidea controller and > another is a host. Let's create a copy of this structure per each > device instance so that odd things don't happen. > > Cc: Peter Chen > Cc: Greg Kroah-Hartman > Signed-off-by: Stephen Boyd > --- > drivers/usb/chipidea/ci_hdrc_msm.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c > index cc6f9b0df9d5..fb4340f02c16 100644 > --- a/drivers/usb/chipidea/ci_hdrc_msm.c > +++ b/drivers/usb/chipidea/ci_hdrc_msm.c > @@ -37,6 +37,7 @@ struct ci_hdrc_msm { > struct clk *core_clk; > struct clk *iface_clk; > struct extcon_dev *vbus_edev; > + struct ci_hdrc_platform_data pdata; > bool secondary_phy; > bool hsic; > void __iomem *base; > @@ -79,16 +80,6 @@ static void ci_hdrc_msm_notify_event(struct ci_hdrc *ci, unsigned event) > } > } > > -static struct ci_hdrc_platform_data ci_hdrc_msm_platdata = { > - .name = "ci_hdrc_msm", > - .capoffset = DEF_CAPOFFSET, > - .flags = CI_HDRC_REGS_SHARED | > - CI_HDRC_DISABLE_STREAMING | > - CI_HDRC_OVERRIDE_AHB_BURST, > - > - .notify_event = ci_hdrc_msm_notify_event, > -}; > - > static int ci_hdrc_msm_mux_phy(struct ci_hdrc_msm *ci, > struct platform_device *pdev) > { > @@ -151,6 +142,12 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) > return -ENOMEM; > platform_set_drvdata(pdev, ci); > > + ci->pdata.name = "ci_hdrc_msm"; > + ci->pdata.capoffset = DEF_CAPOFFSET; > + ci->pdata.flags = CI_HDRC_REGS_SHARED | CI_HDRC_DISABLE_STREAMING | > + CI_HDRC_OVERRIDE_AHB_BURST; > + ci->pdata.notify_event = ci_hdrc_msm_notify_event; > + > reset = devm_reset_control_get(&pdev->dev, "core"); > if (IS_ERR(reset)) > return PTR_ERR(reset); > @@ -204,7 +201,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) > of_node_put(ulpi_node); > > plat_ci = ci_hdrc_add_device(&pdev->dev, pdev->resource, > - pdev->num_resources, &ci_hdrc_msm_platdata); > + pdev->num_resources, &ci->pdata); > if (IS_ERR(plat_ci)) { > dev_err(&pdev->dev, "ci_hdrc_add_device failed!\n"); > ret = PTR_ERR(plat_ci); You can do something like ci_hdrc_usb2.c, it looks simpler. -- Best Regards, Peter Chen From mboxrd@z Thu Jan 1 00:00:00 1970 From: hzpeterchen@gmail.com (Peter Chen) Date: Wed, 29 Jun 2016 19:29:25 +0800 Subject: [PATCH 17/21] usb: chipidea: msm: Make platform data driver local instead of global In-Reply-To: <20160626072838.28082-18-stephen.boyd@linaro.org> References: <20160626072838.28082-1-stephen.boyd@linaro.org> <20160626072838.28082-18-stephen.boyd@linaro.org> Message-ID: <20160629112925.GN25236@shlinux2> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sun, Jun 26, 2016 at 12:28:34AM -0700, Stephen Boyd wrote: > If two devices are probed with this same driver, they'll share > the same platform data structure, while the chipidea core layer > writes and modifies it. This can lead to interesting results > especially if one device is an OTG type chipidea controller and > another is a host. Let's create a copy of this structure per each > device instance so that odd things don't happen. > > Cc: Peter Chen > Cc: Greg Kroah-Hartman > Signed-off-by: Stephen Boyd > --- > drivers/usb/chipidea/ci_hdrc_msm.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c > index cc6f9b0df9d5..fb4340f02c16 100644 > --- a/drivers/usb/chipidea/ci_hdrc_msm.c > +++ b/drivers/usb/chipidea/ci_hdrc_msm.c > @@ -37,6 +37,7 @@ struct ci_hdrc_msm { > struct clk *core_clk; > struct clk *iface_clk; > struct extcon_dev *vbus_edev; > + struct ci_hdrc_platform_data pdata; > bool secondary_phy; > bool hsic; > void __iomem *base; > @@ -79,16 +80,6 @@ static void ci_hdrc_msm_notify_event(struct ci_hdrc *ci, unsigned event) > } > } > > -static struct ci_hdrc_platform_data ci_hdrc_msm_platdata = { > - .name = "ci_hdrc_msm", > - .capoffset = DEF_CAPOFFSET, > - .flags = CI_HDRC_REGS_SHARED | > - CI_HDRC_DISABLE_STREAMING | > - CI_HDRC_OVERRIDE_AHB_BURST, > - > - .notify_event = ci_hdrc_msm_notify_event, > -}; > - > static int ci_hdrc_msm_mux_phy(struct ci_hdrc_msm *ci, > struct platform_device *pdev) > { > @@ -151,6 +142,12 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) > return -ENOMEM; > platform_set_drvdata(pdev, ci); > > + ci->pdata.name = "ci_hdrc_msm"; > + ci->pdata.capoffset = DEF_CAPOFFSET; > + ci->pdata.flags = CI_HDRC_REGS_SHARED | CI_HDRC_DISABLE_STREAMING | > + CI_HDRC_OVERRIDE_AHB_BURST; > + ci->pdata.notify_event = ci_hdrc_msm_notify_event; > + > reset = devm_reset_control_get(&pdev->dev, "core"); > if (IS_ERR(reset)) > return PTR_ERR(reset); > @@ -204,7 +201,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) > of_node_put(ulpi_node); > > plat_ci = ci_hdrc_add_device(&pdev->dev, pdev->resource, > - pdev->num_resources, &ci_hdrc_msm_platdata); > + pdev->num_resources, &ci->pdata); > if (IS_ERR(plat_ci)) { > dev_err(&pdev->dev, "ci_hdrc_add_device failed!\n"); > ret = PTR_ERR(plat_ci); You can do something like ci_hdrc_usb2.c, it looks simpler. -- Best Regards, Peter Chen