Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1203031 > unrolled thread
| Started by | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| First post | 2015-08-07 22:40 +0200 |
| Last post | 2015-08-10 16:40 +0200 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver Alan Stern <stern@rowland.harvard.edu> - 2015-08-07 22:40 +0200
RE: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver Ramneek Mehresh <ramneek.mehresh@freescale.com> - 2015-08-10 11:40 +0200
RE: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver Alan Stern <stern@rowland.harvard.edu> - 2015-08-10 16:50 +0200
RE: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver Ramneek Mehresh <ramneek.mehresh@freescale.com> - 2015-08-10 17:40 +0200
Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver Fabio Estevam <festevam@gmail.com> - 2015-08-10 17:50 +0200
Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver Felipe Balbi <balbi@ti.com> - 2015-08-10 16:40 +0200
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2015-08-07 22:40 +0200 |
| Subject | Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver |
| Message-ID | <pUWAN-24q-5@gated-at.bofh.it> |
On Wed, 15 Jul 2015, Ramneek Mehresh wrote:
> Add workqueue to add/remove host driver (outside
> interrupt context) upon each id change.
>
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> ---
> drivers/usb/host/ehci-fsl.c | 83 ++++++++++++++++++++++++++++++++++-----------
> drivers/usb/host/ehci-fsl.h | 20 +++++++++++
> 2 files changed, 84 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 5352e74..81e4bf5 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -44,6 +44,34 @@
>
> static struct hc_driver __read_mostly fsl_ehci_hc_driver;
>
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
You've got these #if lines all over the place. They look ugly and make
the code hard to read. Consider removing them. Or even if you can't
remove them entirely, removing most of them would help.
Also, instead of testing both CONFIG_FSL_USB2_OTG and
CONFIG_FSL_USB2_OTG_MODULE, how about testing a single symbol? For
example:
#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
#define CHANGE_HCD 1
#else
#define CHANGE_HCD 0
#endif
Then all you need later on is "#if CHANGE_HCD". Or if it's inside a
code block, just "if (CHANGE_HCD)".
> +static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd)
> +{
> + return (struct ehci_fsl *)hcd_to_ehci(hcd)->priv;
> +}
> +
> +static void do_change_hcd(struct work_struct *work)
> +{
> + struct ehci_fsl *ehci_fsl = container_of(work, struct ehci_fsl,
> + change_hcd_work);
> + struct usb_hcd *hcd = ehci_fsl->hcd;
> + void __iomem *non_ehci = hcd->regs;
> + int retval;
> +
> + if (ehci_fsl->hcd_add && !ehci_fsl->have_hcd) {
> + writel(USBMODE_CM_HOST, non_ehci + FSL_SOC_USB_USBMODE);
> + /* host, gadget and otg share same int line */
> + retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
> + if (retval == 0)
> + ehci_fsl->have_hcd = 1;
> + } else if (!ehci_fsl->hcd_add && ehci_fsl->have_hcd) {
> + usb_remove_hcd(hcd);
> + ehci_fsl->have_hcd = 0;
Don't you have to turn off the USBMODE_CM_HOST bit here? It looks
strange to turn it on above but not turn it off again.
> + }
> +}
> +#endif
> static int ehci_fsl_drv_suspend(struct device *dev)
> {
> struct usb_hcd *hcd = dev_get_drvdata(dev);
> - struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> void __iomem *non_ehci = hcd->regs;
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> + struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> + struct usb_bus host = hcd->self;
> +#endif
> +
>
> if (of_device_is_compatible(dev->parent->of_node,
> "fsl,mpc5121-usb2-dr")) {
> return ehci_fsl_mpc512x_drv_suspend(dev);
> }
>
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> + if (host.is_otg) {
> + /* remove hcd */
> + ehci_fsl->hcd_add = 0;
> + schedule_work(&ehci_fsl->change_hcd_work);
> + host.is_otg = 0;
Why do you set host.is_otg to 0 here? Why not do it in the work
routine?
> + return 0;
> + }
> +#endif
> +
> ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
> device_may_wakeup(dev));
> if (!fsl_deep_sleep())
> @@ -540,15 +571,29 @@ static int ehci_fsl_drv_suspend(struct device *dev)
> static int ehci_fsl_drv_resume(struct device *dev)
> {
> struct usb_hcd *hcd = dev_get_drvdata(dev);
> - struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> void __iomem *non_ehci = hcd->regs;
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> + struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> + struct usb_bus host = hcd->self;
> +#endif
>
> if (of_device_is_compatible(dev->parent->of_node,
> "fsl,mpc5121-usb2-dr")) {
> return ehci_fsl_mpc512x_drv_resume(dev);
> }
>
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> + if (host.is_otg) {
> + /* add hcd */
> + ehci_fsl->hcd_add = 1;
> + schedule_work(&ehci_fsl->change_hcd_work);
> + usb_hcd_resume_root_hub(hcd);
> + host.is_otg = 0;
Again, why change host.is_otg here? And for that matter, where does
host.is_otg ever get set to 1?
Also, what is the reason for calling usb_hcd_resume_root_hub()? It
won't do anything, because it will run before the scheduled work, so
there won't be a root hub for it to resume.
> + return 0;
> + }
> +#endif
> +
> ehci_prepare_ports_for_controller_resume(ehci);
> if (!fsl_deep_sleep())
> return 0;
> diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
> index dbd292e..3fd1fd0 100644
> --- a/drivers/usb/host/ehci-fsl.h
> +++ b/drivers/usb/host/ehci-fsl.h
> @@ -62,4 +62,24 @@
> #define UTMI_PHY_EN (1<<9)
> #define ULPI_PHY_CLK_SEL (1<<10)
> #define PHY_CLK_VALID (1<<17)
> +
> +struct ehci_fsl {
> +#ifdef CONFIG_PM
> + /* Saved USB PHY settings, need to restore after deep sleep. */
> + u32 usb_ctrl;
> +#endif
> + struct usb_hcd *hcd;
> +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> + struct work_struct change_hcd_work;
> +#endif
Again, try to eliminate these #if's. There really isn't anything wrong
with allocating the space for these things even in a non-OTG build.
> + /*
> + * store current hcd state for otg;
> + * have_hcd is true when host drv al already part of otg framework,
> + * otherwise false;
> + * hcd_add is true when otg framework wants to add host
> + * drv as part of otg;flase when it wants to remove it
> + */
> + unsigned have_hcd:1;
> + unsigned hcd_add:1;
> +};
> #endif /* _EHCI_FSL_H */
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Ramneek Mehresh <ramneek.mehresh@freescale.com> |
|---|---|
| Date | 2015-08-10 11:40 +0200 |
| Message-ID | <pVRIK-25h-11@gated-at.bofh.it> |
| In reply to | #1203031 |
> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Saturday, August 08, 2015 2:04 AM
> To: Mehresh Ramneek-B31383
> Cc: linux-kernel@vger.kernel.org; balbi@ti.com;
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Li Yang-Leo-R58472
> Subject: Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb
> host driver
>
> On Wed, 15 Jul 2015, Ramneek Mehresh wrote:
>
> > Add workqueue to add/remove host driver (outside interrupt context)
> > upon each id change.
> >
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> > ---
> > drivers/usb/host/ehci-fsl.c | 83
> > ++++++++++++++++++++++++++++++++++-----------
> > drivers/usb/host/ehci-fsl.h | 20 +++++++++++
> > 2 files changed, 84 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > index 5352e74..81e4bf5 100644
> > --- a/drivers/usb/host/ehci-fsl.c
> > +++ b/drivers/usb/host/ehci-fsl.c
> > @@ -44,6 +44,34 @@
> >
> > static struct hc_driver __read_mostly fsl_ehci_hc_driver;
> >
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> > +defined(CONFIG_FSL_USB2_OTG_MODULE)
>
> You've got these #if lines all over the place. They look ugly and make the
> code hard to read. Consider removing them. Or even if you can't remove
> them entirely, removing most of them would help.
>
> Also, instead of testing both CONFIG_FSL_USB2_OTG and
> CONFIG_FSL_USB2_OTG_MODULE, how about testing a single symbol? For
> example:
>
> #if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> #define CHANGE_HCD 1
> #else
> #define CHANGE_HCD 0
> #endif
>
> Then all you need later on is "#if CHANGE_HCD". Or if it's inside a code block,
> just "if (CHANGE_HCD)".
>
Ok, let me see what can be done to minimize these macros. Thanks
> > +static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd) {
> > + return (struct ehci_fsl *)hcd_to_ehci(hcd)->priv; }
> > +
> > +static void do_change_hcd(struct work_struct *work) {
> > + struct ehci_fsl *ehci_fsl = container_of(work, struct ehci_fsl,
> > + change_hcd_work);
> > + struct usb_hcd *hcd = ehci_fsl->hcd;
> > + void __iomem *non_ehci = hcd->regs;
> > + int retval;
> > +
> > + if (ehci_fsl->hcd_add && !ehci_fsl->have_hcd) {
> > + writel(USBMODE_CM_HOST, non_ehci +
> FSL_SOC_USB_USBMODE);
> > + /* host, gadget and otg share same int line */
> > + retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
> > + if (retval == 0)
> > + ehci_fsl->have_hcd = 1;
> > + } else if (!ehci_fsl->hcd_add && ehci_fsl->have_hcd) {
> > + usb_remove_hcd(hcd);
> > + ehci_fsl->have_hcd = 0;
>
> Don't you have to turn off the USBMODE_CM_HOST bit here? It looks
> strange to turn it on above but not turn it off again.
>
USBMODE bit defines mode for the controller. It gets auto cleared when controller is
reset in host/gadget driver. After resetting, it's mandatory to write this bit once to define
controller's mode.
> > + }
> > +}
> > +#endif
>
>
> > static int ehci_fsl_drv_suspend(struct device *dev) {
> > struct usb_hcd *hcd = dev_get_drvdata(dev);
> > - struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> > void __iomem *non_ehci = hcd->regs;
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > + struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> > + struct usb_bus host = hcd->self;
> > +#endif
> > +
> >
> > if (of_device_is_compatible(dev->parent->of_node,
> > "fsl,mpc5121-usb2-dr")) {
> > return ehci_fsl_mpc512x_drv_suspend(dev);
> > }
> >
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > + if (host.is_otg) {
> > + /* remove hcd */
> > + ehci_fsl->hcd_add = 0;
> > + schedule_work(&ehci_fsl->change_hcd_work);
> > + host.is_otg = 0;
>
> Why do you set host.is_otg to 0 here? Why not do it in the work routine?
Well, it can be done there also. I just wanted to keep host-bus house-keeping out
of otg role-changing specific workqueue (where host driver is brought-up/shut-down).
>
> > + return 0;
> > + }
> > +#endif
> > +
> > ehci_prepare_ports_for_controller_suspend(hcd_to_ehci(hcd),
> > device_may_wakeup(dev));
> > if (!fsl_deep_sleep())
> > @@ -540,15 +571,29 @@ static int ehci_fsl_drv_suspend(struct device
> > *dev) static int ehci_fsl_drv_resume(struct device *dev) {
> > struct usb_hcd *hcd = dev_get_drvdata(dev);
> > - struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> > struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > void __iomem *non_ehci = hcd->regs;
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > + struct ehci_fsl *ehci_fsl = hcd_to_ehci_fsl(hcd);
> > + struct usb_bus host = hcd->self;
> > +#endif
> >
> > if (of_device_is_compatible(dev->parent->of_node,
> > "fsl,mpc5121-usb2-dr")) {
> > return ehci_fsl_mpc512x_drv_resume(dev);
> > }
> >
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > + if (host.is_otg) {
> > + /* add hcd */
> > + ehci_fsl->hcd_add = 1;
> > + schedule_work(&ehci_fsl->change_hcd_work);
> > + usb_hcd_resume_root_hub(hcd);
> > + host.is_otg = 0;
>
> Again, why change host.is_otg here? And for that matter, where does
> host.is_otg ever get set to 1?
host.is_otg sets to 1 in phy driver before calling ehci drv suspend/resume.
This is donr to make it clear in host drv suspend/resume that these functions
Are called in context of otg drv
>
> Also, what is the reason for calling usb_hcd_resume_root_hub()? It won't
> do anything, because it will run before the scheduled work, so there won't
> be a root hub for it to resume.
>
Well, you're right...it's not a surety that usb_hcd_resume_root_hub() will run after
scheduled work...i'll move it inside do_change_hcd() after usb_add_hcd(). Thanks.
> > + return 0;
> > + }
> > +#endif
> > +
> > ehci_prepare_ports_for_controller_resume(ehci);
> > if (!fsl_deep_sleep())
> > return 0;
> > diff --git a/drivers/usb/host/ehci-fsl.h b/drivers/usb/host/ehci-fsl.h
> > index dbd292e..3fd1fd0 100644
> > --- a/drivers/usb/host/ehci-fsl.h
> > +++ b/drivers/usb/host/ehci-fsl.h
> > @@ -62,4 +62,24 @@
> > #define UTMI_PHY_EN (1<<9)
> > #define ULPI_PHY_CLK_SEL (1<<10)
> > #define PHY_CLK_VALID (1<<17)
> > +
> > +struct ehci_fsl {
> > +#ifdef CONFIG_PM
> > + /* Saved USB PHY settings, need to restore after deep sleep. */
> > + u32 usb_ctrl;
> > +#endif
> > + struct usb_hcd *hcd;
> > +#if defined(CONFIG_FSL_USB2_OTG) ||
> defined(CONFIG_FSL_USB2_OTG_MODULE)
> > + struct work_struct change_hcd_work;
> > +#endif
>
> Again, try to eliminate these #if's. There really isn't anything wrong with
> allocating the space for these things even in a non-OTG build.
>
> > + /*
> > + * store current hcd state for otg;
> > + * have_hcd is true when host drv al already part of otg framework,
> > + * otherwise false;
> > + * hcd_add is true when otg framework wants to add host
> > + * drv as part of otg;flase when it wants to remove it
> > + */
> > + unsigned have_hcd:1;
> > + unsigned hcd_add:1;
> > +};
> > #endif /* _EHCI_FSL_H */
>
> Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2015-08-10 16:50 +0200 |
| Message-ID | <pVWyK-RP-21@gated-at.bofh.it> |
| In reply to | #1203935 |
On Mon, 10 Aug 2015, Ramneek Mehresh wrote: > > Also, what is the reason for calling usb_hcd_resume_root_hub()? It won't > > do anything, because it will run before the scheduled work, so there won't > > be a root hub for it to resume. > > > Well, you're right...it's not a surety that usb_hcd_resume_root_hub() will run after > scheduled work...i'll move it inside do_change_hcd() after usb_add_hcd(). Thanks. You're missing the point. There's no reason ever to call usb_hcd_resume_root_hub() immediately after usb_add_hcd(). The hub driver does everything necessary when the root hub is registered. Resuming it (if it is suspended at that point) won't accomplish anything because all the important work has just been done. Now, if you _weren't_ calling usb_add_hcd() (for example if host.is_otg were 0) then it might make sense to call usb_hcd_resume_root_hub() -- although I don't see why you would need to. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ramneek Mehresh <ramneek.mehresh@freescale.com> |
|---|---|
| Date | 2015-08-10 17:40 +0200 |
| Message-ID | <pVXl8-21m-27@gated-at.bofh.it> |
| In reply to | #1204345 |
> -----Original Message----- > From: Alan Stern [mailto:stern@rowland.harvard.edu] > Sent: Monday, August 10, 2015 8:14 PM > To: Mehresh Ramneek-B31383 > Cc: linux-kernel@vger.kernel.org; balbi@ti.com; > gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Li Yang-Leo-R58472 > Subject: RE: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb > host driver > > On Mon, 10 Aug 2015, Ramneek Mehresh wrote: > > > > Also, what is the reason for calling usb_hcd_resume_root_hub()? It > won't > > > do anything, because it will run before the scheduled work, so there > won't > > > be a root hub for it to resume. > > > > > Well, you're right...it's not a surety that usb_hcd_resume_root_hub() will > run after > > scheduled work...i'll move it inside do_change_hcd() after usb_add_hcd(). > Thanks. > > You're missing the point. There's no reason ever to call > usb_hcd_resume_root_hub() immediately after usb_add_hcd(). The hub > driver does everything necessary when the root hub is registered. > Resuming it (if it is suspended at that point) won't accomplish > anything because all the important work has just been done. > > Now, if you _weren't_ calling usb_add_hcd() (for example if host.is_otg > were 0) then it might make sense to call usb_hcd_resume_root_hub() -- > although I don't see why you would need to. > > Alan Stern Understood...let me check this once again. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2015-08-10 17:50 +0200 |
| Subject | Re: [PATCH 3/8][v2]usb:fsl:otg: Add support to add/remove usb host driver |
| Message-ID | <pVXuO-2cJ-11@gated-at.bofh.it> |
| In reply to | #1204373 |
On Mon, Aug 10, 2015 at 12:35 PM, Ramneek Mehresh <ramneek.mehresh@freescale.com> wrote: > Understood...let me check this once again. Thanks. Why don't use the chipidea driver instead? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@ti.com> |
|---|---|
| Date | 2015-08-10 16:40 +0200 |
| Message-ID | <pVWp4-FY-1@gated-at.bofh.it> |
| In reply to | #1203031 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, Aug 07, 2015 at 04:33:33PM -0400, Alan Stern wrote:
> On Wed, 15 Jul 2015, Ramneek Mehresh wrote:
>
> > Add workqueue to add/remove host driver (outside
> > interrupt context) upon each id change.
> >
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Ramneek Mehresh <ramneek.mehresh@freescale.com>
> > ---
> > drivers/usb/host/ehci-fsl.c | 83 ++++++++++++++++++++++++++++++++++-----------
> > drivers/usb/host/ehci-fsl.h | 20 +++++++++++
> > 2 files changed, 84 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > index 5352e74..81e4bf5 100644
> > --- a/drivers/usb/host/ehci-fsl.c
> > +++ b/drivers/usb/host/ehci-fsl.c
> > @@ -44,6 +44,34 @@
> >
> > static struct hc_driver __read_mostly fsl_ehci_hc_driver;
> >
> > +#if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
>
> You've got these #if lines all over the place. They look ugly and make
> the code hard to read. Consider removing them. Or even if you can't
> remove them entirely, removing most of them would help.
>
> Also, instead of testing both CONFIG_FSL_USB2_OTG and
> CONFIG_FSL_USB2_OTG_MODULE, how about testing a single symbol? For
> example:
>
> #if defined(CONFIG_FSL_USB2_OTG) || defined(CONFIG_FSL_USB2_OTG_MODULE)
> #define CHANGE_HCD 1
> #else
> #define CHANGE_HCD 0
> #endif
>
> Then all you need later on is "#if CHANGE_HCD". Or if it's inside a
> code block, just "if (CHANGE_HCD)".
what about IS_ENABLED() ?
if (IS_ENABLED(CONFIG_FSL_USB2_OTG)) {
foo();
} else {
bar();
}
--
balbi
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web