| Message ID | 1421519013-22123-1-git-send-email-sylvain.rochet@finsecur.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sylvain, On Sat, 17 Jan 2015 19:23:31 +0100 Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > This patch move Atmel EHCI global variables (clocks ptr and clocked > boolean) to private struct atmel_ehci, appended at the end of the parent > struct usb_hcd. Maybe you should add a cover letter to clearly state that this series depends on another one: [1]. Anyway, thanks for removing these global variables. > > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> To the whole series: Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> [1]https://www.mail-archive.com/linux-usb@vger.kernel.org/msg54675.html
On Sat, 17 Jan 2015, Sylvain Rochet wrote: > This patch move Atmel EHCI global variables (clocks ptr and clocked > boolean) to private struct atmel_ehci, appended at the end of the parent > struct usb_hcd. > > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> > --- > drivers/usb/host/ehci-atmel.c | 75 +++++++++++++++++++++++++++---------------- > 1 file changed, 48 insertions(+), 27 deletions(-) > > diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c > index 2e0043b..a47fe3f 100644 > --- a/drivers/usb/host/ehci-atmel.c > +++ b/drivers/usb/host/ehci-atmel.c > @@ -30,45 +30,62 @@ static const char hcd_name[] = "ehci-atmel"; > static struct hc_driver __read_mostly ehci_atmel_hc_driver; > > /* interface and function clocks */ > -static struct clk *iclk, *fclk, *uclk; > -static int clocked; > +struct atmel_ehci { > + struct ehci_hcd ehci; > + > + struct clk *iclk; > + struct clk *fclk; > + struct clk *uclk; > + bool clocked; > +}; > > /*-------------------------------------------------------------------------*/ > > -static void atmel_start_clock(void) > +static inline struct atmel_ehci *hcd_to_atmel_ehci(struct usb_hcd *hcd) > +{ > + return (struct atmel_ehci *) hcd->hcd_priv; > +} This is not the right way to do it. For an example of the right way, see the ehci-platform.c file. Your private structure is stored in ehci->priv, and its size is specified by the .extra_priv_size member of an ehci_driver_overrides structure passed to ehci_init_driver(). Nevertheless, I approve the idea of getting rid of global variables. Alan Stern
Sylvain Rochet (5):
USB: host: ehci_atmel: Add suspend/resume support
USB: host: ohci_at91: Stop/start USB PLL for all sleep modes
USB: host: ehci_atmel: Move global variables to private struct
USB: host: ohci_at91: Move global variables to private struct
USB: host: ohci_at91: usb_hcd_at91_probe(), remove useless stack
initialisation
drivers/usb/host/ehci-atmel.c | 102 +++++++++++++++++++++++++++++++----------
drivers/usb/host/ohci-at91.c | 104 +++++++++++++++++++++++++-----------------
2 files changed, 138 insertions(+), 68 deletions(-)
Hello Alan, On Sat, Jan 17, 2015 at 03:30:45PM -0500, Alan Stern wrote: > > This is not the right way to do it. For an example of the right way, > see the ehci-platform.c file. Your private structure is stored in > ehci->priv, and its size is specified by the .extra_priv_size member of > an ehci_driver_overrides structure passed to ehci_init_driver(). Dammit… reworked this way, thanks for the heads-up ! Sylvain
On Sat, 17 Jan 2015, Sylvain Rochet wrote: > Sylvain Rochet (5): > USB: host: ehci_atmel: Add suspend/resume support > USB: host: ohci_at91: Stop/start USB PLL for all sleep modes > USB: host: ehci_atmel: Move global variables to private struct > USB: host: ohci_at91: Move global variables to private struct > USB: host: ohci_at91: usb_hcd_at91_probe(), remove useless stack > initialisation > > drivers/usb/host/ehci-atmel.c | 102 +++++++++++++++++++++++++++++++---------- > drivers/usb/host/ohci-at91.c | 104 +++++++++++++++++++++++++----------------- > 2 files changed, 138 insertions(+), 68 deletions(-) These patches look pretty good to me. Have you run them through checkpatch.pl? Alan Stern
Suspend/resume support for EHCI.
struct dev_pm_ops for OHCI.
PLL stop for all sleep modes for OHCI.
Removed global variables from both.
Removed at91_suspend_entering_slow_clock() from both.
Changes since v3:
* Using struct dev_pm_ops instead of static struct platform_driver
resume and suspend bindings for both EHCI and OHCI
* Fixed inconsistency in patch subjects, _ intead of - for file names
* Patch cleaning with the help of checkpatch.pl, fixed lines over
80 characters
Changes since v2:
* Added patchs from an other submission, because this series
depended on this one
* EHCI: Move global variables to private struct
* OHCI: Move global variables to private struct
* Using ohci->priv and ehci->priv instead of hcd->hcd_priv,
which were not the right way to do that
Changes since v1:
* Don't use at91_suspend_entering_slow_clock() on EHCI,
we are trying to get read of this of this function
* Removed at91_suspend_entering_slow_clock() from OHCI
Sylvain Rochet (6):
USB: host: ehci-atmel: Add suspend/resume support
USB: host: ohci-at91: Use struct dev_pm_ops instead of struct
platform_driver
USB: host: ohci-at91: Stop/start USB PLL for all sleep modes
USB: host: ehci-atmel: Move global variables to private struct
USB: host: ohci-at91: Move global variables to private struct
USB: host: ohci-at91: usb_hcd_at91_probe(), remove useless stack
initialisation
drivers/usb/host/ehci-atmel.c | 102 +++++++++++++++++++++++++---------
drivers/usb/host/ohci-at91.c | 126 ++++++++++++++++++++++++------------------
2 files changed, 149 insertions(+), 79 deletions(-)
Hello Alan, On Sun, Jan 18, 2015 at 12:20:49PM -0500, Alan Stern wrote: > On Sat, 17 Jan 2015, Sylvain Rochet wrote: > > > Sylvain Rochet (5): > > USB: host: ehci_atmel: Add suspend/resume support > > USB: host: ohci_at91: Stop/start USB PLL for all sleep modes > > USB: host: ehci_atmel: Move global variables to private struct > > USB: host: ohci_at91: Move global variables to private struct > > USB: host: ohci_at91: usb_hcd_at91_probe(), remove useless stack > > initialisation > > > > drivers/usb/host/ehci-atmel.c | 102 +++++++++++++++++++++++++++++++---------- > > drivers/usb/host/ohci-at91.c | 104 +++++++++++++++++++++++++----------------- > > 2 files changed, 138 insertions(+), 68 deletions(-) > > These patches look pretty good to me. Have you run them through > checkpatch.pl? Just did, fixed some lines over 80 characters + Sergei suggestion about using struct dev_pm_ops instead of struct platform_driver.{resume,suspend}. Sylvain
diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c index 2e0043b..a47fe3f 100644 --- a/drivers/usb/host/ehci-atmel.c +++ b/drivers/usb/host/ehci-atmel.c @@ -30,45 +30,62 @@ static const char hcd_name[] = "ehci-atmel"; static struct hc_driver __read_mostly ehci_atmel_hc_driver; /* interface and function clocks */ -static struct clk *iclk, *fclk, *uclk; -static int clocked; +struct atmel_ehci { + struct ehci_hcd ehci; + + struct clk *iclk; + struct clk *fclk; + struct clk *uclk; + bool clocked; +}; /*-------------------------------------------------------------------------*/ -static void atmel_start_clock(void) +static inline struct atmel_ehci *hcd_to_atmel_ehci(struct usb_hcd *hcd) +{ + return (struct atmel_ehci *) hcd->hcd_priv; +} + +static void atmel_start_clock(struct atmel_ehci *atmel_ehci) { - if (clocked) + if (atmel_ehci->clocked) return; if (IS_ENABLED(CONFIG_COMMON_CLK)) { - clk_set_rate(uclk, 48000000); - clk_prepare_enable(uclk); + clk_set_rate(atmel_ehci->uclk, 48000000); + clk_prepare_enable(atmel_ehci->uclk); } - clk_prepare_enable(iclk); - clk_prepare_enable(fclk); - clocked = 1; + clk_prepare_enable(atmel_ehci->iclk); + clk_prepare_enable(atmel_ehci->fclk); + atmel_ehci->clocked = true; } -static void atmel_stop_clock(void) +static void atmel_stop_clock(struct atmel_ehci *atmel_ehci) { - if (!clocked) + if (!atmel_ehci->clocked) return; - clk_disable_unprepare(fclk); - clk_disable_unprepare(iclk); + clk_disable_unprepare(atmel_ehci->fclk); + clk_disable_unprepare(atmel_ehci->iclk); if (IS_ENABLED(CONFIG_COMMON_CLK)) - clk_disable_unprepare(uclk); - clocked = 0; + clk_disable_unprepare(atmel_ehci->uclk); + atmel_ehci->clocked = false; } static void atmel_start_ehci(struct platform_device *pdev) { + struct usb_hcd *hcd = platform_get_drvdata(pdev); + struct atmel_ehci *atmel_ehci = hcd_to_atmel_ehci(hcd); + dev_dbg(&pdev->dev, "start\n"); - atmel_start_clock(); + atmel_start_clock(atmel_ehci); } static void atmel_stop_ehci(struct platform_device *pdev) { + struct usb_hcd *hcd = platform_get_drvdata(pdev); + struct atmel_ehci *atmel_ehci = hcd_to_atmel_ehci(hcd); + dev_dbg(&pdev->dev, "stop\n"); - atmel_stop_clock(); + atmel_stop_clock(atmel_ehci); } /*-------------------------------------------------------------------------*/ @@ -79,6 +96,7 @@ static int ehci_atmel_drv_probe(struct platform_device *pdev) const struct hc_driver *driver = &ehci_atmel_hc_driver; struct resource *res; struct ehci_hcd *ehci; + struct atmel_ehci *atmel_ehci; int irq; int retval; @@ -109,6 +127,7 @@ static int ehci_atmel_drv_probe(struct platform_device *pdev) retval = -ENOMEM; goto fail_create_hcd; } + atmel_ehci = hcd_to_atmel_ehci(hcd); res = platform_get_resource(pdev, IORESOURCE_MEM, 0); hcd->regs = devm_ioremap_resource(&pdev->dev, res); @@ -120,23 +139,23 @@ static int ehci_atmel_drv_probe(struct platform_device *pdev) hcd->rsrc_start = res->start; hcd->rsrc_len = resource_size(res); - iclk = devm_clk_get(&pdev->dev, "ehci_clk"); - if (IS_ERR(iclk)) { + atmel_ehci->iclk = devm_clk_get(&pdev->dev, "ehci_clk"); + if (IS_ERR(atmel_ehci->iclk)) { dev_err(&pdev->dev, "Error getting interface clock\n"); retval = -ENOENT; goto fail_request_resource; } - fclk = devm_clk_get(&pdev->dev, "uhpck"); - if (IS_ERR(fclk)) { + atmel_ehci->fclk = devm_clk_get(&pdev->dev, "uhpck"); + if (IS_ERR(atmel_ehci->fclk)) { dev_err(&pdev->dev, "Error getting function clock\n"); retval = -ENOENT; goto fail_request_resource; } if (IS_ENABLED(CONFIG_COMMON_CLK)) { - uclk = devm_clk_get(&pdev->dev, "usb_clk"); - if (IS_ERR(uclk)) { + atmel_ehci->uclk = devm_clk_get(&pdev->dev, "usb_clk"); + if (IS_ERR(atmel_ehci->uclk)) { dev_err(&pdev->dev, "failed to get uclk\n"); - retval = PTR_ERR(uclk); + retval = PTR_ERR(atmel_ehci->uclk); goto fail_request_resource; } } @@ -173,7 +192,6 @@ static int ehci_atmel_drv_remove(struct platform_device *pdev) usb_put_hcd(hcd); atmel_stop_ehci(pdev); - fclk = iclk = NULL; return 0; } @@ -182,21 +200,23 @@ static int ehci_atmel_drv_remove(struct platform_device *pdev) static int ehci_atmel_drv_suspend(struct platform_device *pdev, pm_message_t mesg) { struct usb_hcd *hcd = platform_get_drvdata(pdev); + struct atmel_ehci *atmel_ehci = hcd_to_atmel_ehci(hcd); int ret; ret = ehci_suspend(hcd, false); if (ret) return ret; - atmel_stop_clock(); + atmel_stop_clock(atmel_ehci); return 0; } static int ehci_atmel_drv_resume(struct platform_device *pdev) { struct usb_hcd *hcd = platform_get_drvdata(pdev); + struct atmel_ehci *atmel_ehci = hcd_to_atmel_ehci(hcd); - atmel_start_clock(); + atmel_start_clock(atmel_ehci); return ehci_resume(hcd, false); } #else @@ -232,6 +252,7 @@ static int __init ehci_atmel_init(void) pr_info("%s: " DRIVER_DESC "\n", hcd_name); ehci_init_driver(&ehci_atmel_hc_driver, NULL); + ehci_atmel_hc_driver.hcd_priv_size = sizeof(struct atmel_ehci); return platform_driver_register(&ehci_atmel_driver); } module_init(ehci_atmel_init);
This patch move Atmel EHCI global variables (clocks ptr and clocked boolean) to private struct atmel_ehci, appended at the end of the parent struct usb_hcd. Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> --- drivers/usb/host/ehci-atmel.c | 75 +++++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 27 deletions(-)