RE: [PATCH 03/12] usb: dwc2: update gadget portion to use common dwc2_hsotg structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: dinguyen@xxxxxxxxxx [mailto:dinguyen@xxxxxxxxxx]
> Sent: Wednesday, July 16, 2014 1:33 PM
> 
> Update gadget.c to use the dwc2_hsotg and new placement of the gadget
> data structure that has been moved into the common structure. Along
> with the updating the gadget to use the common dwc2_hsotg data structure,
> a few other things are required in order for this patch to build properly.
> Those are:
> 
> - Add proper externs for functions that need to be shared between host
>   and peripheral drivers.
> - Remove gadget module defines. Since the driver probing will be handled
>   by either the platform or pci code.
> - Change the gadget probe function into gadget_init.
> - Move the allocation of the core_params to a share place for use by both
>   host and gadget.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> ---
>  drivers/usb/dwc2/core.h     |   61 ++-
>  drivers/usb/dwc2/gadget.c   | 1238 ++++++++++++++++++++-----------------------
>  drivers/usb/dwc2/hcd.c      |    5 -
>  drivers/usb/dwc2/hcd.h      |   10 -
>  drivers/usb/dwc2/platform.c |    4 +
>  5 files changed, 649 insertions(+), 669 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 0c6efbf..0fc4e41 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -84,7 +84,7 @@ static const char * const s3c_hsotg_supply_names[] = {
>   */
>  #define EP0_MPS_LIMIT   64
> 
> -struct s3c_hsotg;
> +struct dwc2_hsotg;
>  struct s3c_hsotg_req;
> 
>  /**
> @@ -130,7 +130,7 @@ struct s3c_hsotg_req;
>  struct s3c_hsotg_ep {
>  	struct usb_ep           ep;
>  	struct list_head        queue;
> -	struct s3c_hsotg        *parent;
> +	struct dwc2_hsotg       *parent;
>  	struct s3c_hsotg_req    *req;
>  	struct dentry           *debugfs;

At some point we should rename s3c_hsotg_ep and s3c_hsotg_req as well.
Same for all the s3c_* function names. That could be done as an
additional patch.

> @@ -953,4 +953,61 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg);
>   */
>  extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg);
> 
> +/* Gadget defines */
> +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE)
> +extern void s3c_hsotg_irq_enumdone(struct dwc2_hsotg *dwc2);
> +extern void s3c_hsotg_epint(struct dwc2_hsotg *dwc2, unsigned int idx,
> +			int dir_in);

...

> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index f3c56a2..42d1d60 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -38,6 +38,7 @@
>  #include <linux/platform_data/s3c-hsotg.h>
> 
>  #include "core.h"
> +#include "hw.h"
> 
>  /* conversion functions */
>  static inline struct s3c_hsotg_req *our_req(struct usb_request *req)
> @@ -50,9 +51,9 @@ static inline struct s3c_hsotg_ep *our_ep(struct usb_ep *ep)
>  	return container_of(ep, struct s3c_hsotg_ep, ep);
>  }
> 
> -static inline struct s3c_hsotg *to_hsotg(struct usb_gadget *gadget)
> +static inline struct dwc2_hsotg *to_hsotg(struct usb_gadget *gadget)
>  {
> -	return container_of(gadget, struct s3c_hsotg, gadget);
> +	return container_of(gadget, struct dwc2_hsotg, gadget);
>  }
> 
>  static inline void __orr32(void __iomem *ptr, u32 val)
> @@ -65,9 +66,6 @@ static inline void __bic32(void __iomem *ptr, u32 val)
>  	writel(readl(ptr) & ~val, ptr);
>  }
> 
> -/* forward decleration of functions */
> -static void s3c_hsotg_dump(struct s3c_hsotg *hsotg);
> -
>  /**
>   * using_dma - return the DMA status of the driver.
>   * @hsotg: The driver state.
> @@ -97,16 +95,16 @@ static inline bool using_dma(struct s3c_hsotg *hsotg)
>   * @hsotg: The device state
>   * @ints: A bitmask of the interrupts to enable
>   */
> -static void s3c_hsotg_en_gsint(struct s3c_hsotg *hsotg, u32 ints)
> +static void s3c_hsotg_en_gsint(struct dwc2_hsotg *dwc2, u32 ints)
>  {
> -	u32 gsintmsk = readl(hsotg->regs + GINTMSK);
> +	u32 gsintmsk = readl(dwc2->regs + GINTMSK);
>  	u32 new_gsintmsk;
> 
>  	new_gsintmsk = gsintmsk | ints;
> 
>  	if (new_gsintmsk != gsintmsk) {
> -		dev_dbg(hsotg->dev, "gsintmsk now 0x%08x\n", new_gsintmsk);
> -		writel(new_gsintmsk, hsotg->regs + GINTMSK);
> +		dev_dbg(dwc2->dev, "gsintmsk now 0x%08x\n", new_gsintmsk);
> +		writel(new_gsintmsk, dwc2->regs + GINTMSK);
>  	}
>  }

As a general comment for the entire patch, if you left the function
parameter names as 'hsotg' and just changed their type, it would
simplify this entire patch a *lot*. Care to respin this patch like that?
The renaming could always be done later in a separate patch, but right
now I'm not sure there's a need for it.

-- 
Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux