Hi,
On Thu, Apr 04, 2013 at 09:50:12PM +0200, Daniel Mack wrote:
> Initialize the host and gagdet subsystems of the musb driver only when
> the appropriate mode is selected from platform data, or device-tree
> information, respectively.
>
> Refuse to start the gadget part if the port is in host-only mode.
>
> Signed-off-by: Daniel Mack <zon...@gmail.com>
> ---
> drivers/usb/musb/musb_core.c | 25 +++++++++++++++++--------
> drivers/usb/musb/musb_core.h | 7 +++++++
> drivers/usb/musb/musb_gadget.c | 5 +++++
> 3 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index cb1631e..c021058 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -943,10 +943,12 @@ void musb_start(struct musb *musb)
> * (b) vbus present/connect IRQ, peripheral mode;
> * (c) peripheral initiates, using SRP
> */
> - if ((devctl & MUSB_DEVCTL_VBUS) == MUSB_DEVCTL_VBUS)
> + if (musb->port_mode != MUSB_PORT_MODE_HOST &&
> + (devctl & MUSB_DEVCTL_VBUS) == MUSB_DEVCTL_VBUS) {
> musb->is_active = 1;
> - else
> + } else {
> devctl |= MUSB_DEVCTL_SESSION;
> + }
this is the kind of code which shouldn't exist in musb_core.c. This file
should only know about calling musb_host_setup() and
musb_gadget_cleanup(). Those two functions should take care of checking
details such as VBUS.
The only thing this file should do is, as stated before:
switch(mode)
case host:
musb_host_setup();
break;
case peripheral:
musb_peripheral_setup();
break;
case otg:
musb_peripheral_setup();
musb_host_setup();
break;
default:
bail_out();
}
> @@ -1868,6 +1870,7 @@ musb_init_controller(struct device *dev, int nIrq, void
> __iomem *ctrl)
> musb->board_set_power = plat->set_power;
> musb->min_power = plat->min_power;
> musb->ops = plat->platform_ops;
> + musb->port_mode = plat->mode;
>
> /* The musb_platform_init() call:
> * - adjusts musb->mregs
> @@ -1958,13 +1961,19 @@ musb_init_controller(struct device *dev, int nIrq,
> void __iomem *ctrl)
> musb->xceiv->state = OTG_STATE_B_IDLE;
> }
>
> - status = musb_host_setup(musb, plat->power);
> - if (status < 0)
> - goto fail3;
> + if (musb->port_mode == MUSB_PORT_MODE_HOST ||
> + musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE) {
> + status = musb_host_setup(musb, plat->power);
> + if (status < 0)
> + goto fail3;
> + }
>
> - status = musb_gadget_setup(musb);
> - if (status < 0)
> - goto fail3;
> + if (musb->port_mode == MUSB_PORT_MODE_GADGET ||
> + musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE) {
> + status = musb_gadget_setup(musb);
> + if (status < 0)
> + goto fail3;
> + }
this is quite convoluted, to me at least. A switch statement, as I
suggested before, looks a lot cleaner.
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index ef5b4e6..ed1644f 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -77,6 +77,12 @@ struct musb_ep;
> #define is_peripheral_active(m) (!(m)->is_host)
> #define is_host_active(m) ((m)->is_host)
>
> +enum {
> + MUSB_PORT_MODE_HOST = 1,
> + MUSB_PORT_MODE_GADGET = 2,
> + MUSB_PORT_MODE_DUAL_ROLE = 3,
no need to assign numbers yourself. You can let the compiler do it.
should be part of a separate patch, btw.
> @@ -356,6 +362,7 @@ struct musb {
>
> u8 min_power; /* vbus for periph, in mA/2 */
>
> + int port_mode; /* MUSB_PORT_MODE_* */
should be part of a separate patch. First you add what you need, then
you use it.
It helps keeping patches very small, which makes my review time a lot
better as I can quickly look over patches adding fields to structures
and focus review on the actual meat.
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index 0414bc1..c606088 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -1823,6 +1823,11 @@ static int musb_gadget_start(struct usb_gadget *g,
> unsigned long flags;
> int retval = 0;
>
> + if (musb->port_mode == MUSB_PORT_MODE_HOST) {
> + retval = -EINVAL;
> + goto err;
> + }
why ? You won't start the gadget side unless port mode is gadget or otg,
this should *NEVER* be true.
--
balbi
signature.asc
Description: Digital signature
