> From: [email protected] [mailto:[email protected]]
> Sent: Thursday, February 13, 2014 1:11 PM
>
> The dwc2_hsotg will be the main data structure for both the dwc2 and
> s3c-hsotg drivers. This patch adds the appropriate data structures into the
> dwc2_hsotg data structure in order for it to support both host and gadget
> functionality.
>
> This commit still keeps the dwc2 host and s3c_hsotg gadget driver separate.
> The only commonality is a single data structure, clock, register base, and
> irq.
>
> The majority of changes in this commit are edits to the s3c-hsotg.c file. The
> edits are dominated by changes in how the s3c_hsotg data structure are
> accessed
> directly that now have to be accessed through a pointer in the dwc2_hsotg
> structure.
>
> The gadget data structure is now also part of the dwc2_hsotg structure. The
> dwc2_hsotg data structure now has a pointer to the clk structure. The "otg"
> clock that was needed for the s3c_hsotg driver is now needed for both the
> dwc2 and s3c_hsotg drivers.
< snip >
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 1efd10c..1dae260 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -84,6 +84,8 @@ static const char * const s3c_hsotg_supply_names[] = {
> */
> #define EP0_MPS_LIMIT 64
>
> +struct dwc2_hsotg;
> +struct dwc2_host_chan;
> struct s3c_hsotg;
> struct s3c_hsotg_req;
>
> @@ -130,7 +132,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;
Whitespace damage here. In fact, it looks like a lot (all?) of the
s3c-hsotg code here got whitespace damaged when you moved it to this file
in earlier patches. That all needs to be fixed (redo the earlier patches
to fix the damage).
> struct s3c_hsotg_req *req;
> struct dentry *debugfs;
>
> @@ -162,7 +164,6 @@ struct s3c_hsotg_ep {
> * @plat: The platform specific configuration data. This can be removed once
> * all SoCs support usb transceiver.
> * @regs: The memory area mapped for accessing registers.
> - * @irq: The IRQ number we are using
> * @supplies: Definition of USB power supplies
> * @phyif: PHY interface width
> * @dedicated_fifos: Set if the hardware has dedicated IN-EP fifos.
> @@ -179,18 +180,10 @@ struct s3c_hsotg_ep {
> * @eps: The endpoints being supplied to the gadget framework
> */
> struct s3c_hsotg {
> - struct device *dev;
> - struct usb_gadget_driver *driver;
> struct phy *phy;
> struct usb_phy *uphy;
> struct s3c_hsotg_plat *plat;
>
> - spinlock_t lock;
> -
> - void __iomem *regs;
> - int irq;
> - struct clk *clk;
> -
> struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
>
> u32 phyif;
> @@ -206,10 +199,8 @@ struct s3c_hsotg {
> u8 ep0_buff[8];
> u8 ctrl_buff[8];
>
> - struct usb_gadget gadget;
> unsigned int setup;
> unsigned long last_rst;
> - struct s3c_hsotg_ep *eps;
> };
>
> /**
> @@ -236,9 +227,6 @@ do { \
> } \
> } while (0)
>
> -struct dwc2_hsotg;
> -struct dwc2_host_chan;
> -
> /* Device States */
> enum dwc2_lx_state {
> DWC2_L0, /* On state */
> @@ -587,6 +575,8 @@ struct dwc2_hw_params {
> struct dwc2_hsotg {
> struct device *dev;
> void __iomem *regs;
> + struct clk *clk;
> +
Please keep the formatting consistent when modifying existing code.
> /** Params detected from hardware */
> struct dwc2_hw_params hw_params;
> /** Params to actually use */
> @@ -601,6 +591,11 @@ struct dwc2_hsotg {
> struct timer_list wkp_timer;
> enum dwc2_lx_state lx_state;
>
> + struct s3c_hsotg *s3c_hsotg;
I don't think it's a good idea to include a pointer to the s3c_hsotg
struct inside of the dwc2_hsotg struct. Then you get things like
dwc2->s3c_hsotg->dedicated_fifos later on in this patch. How about just
moving all the members of s3c_hsotg into dwc2_hsotg?
> + struct usb_gadget gadget;
> + struct usb_gadget_driver *driver;
> + struct s3c_hsotg_ep *eps;
> +
Formatting here is a mess.
< snip >
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index d01d0d3..c6fb4f2 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -34,6 +34,7 @@
> * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> */
>
> +#include <linux/clk.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> @@ -90,12 +91,16 @@ static int dwc2_driver_remove(struct platform_device *dev)
> {
> struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
>
> - dwc2_hcd_remove(hsotg);
> + if (IS_ENABLED(CONFIG_USB_DWC2_HOST))
> + dwc2_hcd_remove(hsotg);
> + else /* s3c-hsotg gadget */
> + s3c_hsotg_remove(hsotg);
>
> return 0;
> }
>
> static const struct of_device_id dwc2_of_match_table[] = {
> + { .compatible = "samsung,s3c6400-hsotg", },
> { .compatible = "brcm,bcm2835-usb", .data = ¶ms_bcm2835 },
> { .compatible = "snps,dwc2", .data = NULL },
> {},
> @@ -129,7 +134,8 @@ static int dwc2_driver_probe(struct platform_device *dev)
> params = match->data;
> } else {
> /* Default all params to autodetect */
> - dwc2_set_all_params(&defparams, -1);
> + if (IS_ENABLED(CONFIG_USB_DWC2_HOST))
> + dwc2_set_all_params(&defparams, -1);
> params = &defparams;
> }
>
> @@ -162,9 +168,23 @@ static int dwc2_driver_probe(struct platform_device *dev)
> dev_dbg(&dev->dev, "mapped PA %08lx to VA %p\n",
> (unsigned long)res->start, hsotg->regs);
>
> - retval = dwc2_hcd_init(hsotg, irq, params);
> - if (retval)
> - return retval;
> + /* Get clock */
> + hsotg->clk = devm_clk_get(hsotg->dev, "otg");
> + if (IS_ERR(hsotg->clk)) {
> + dev_err(hsotg->dev, "cannot get otg clock\n");
> + return PTR_ERR(hsotg->clk);
> + }
> + clk_prepare_enable(hsotg->clk);
Doesn't the addition of this get clock code break any existing dwc2
platforms that don't have a clock named "otg"?
The rest of the patch looks like mostly mechanical replacement of struct
s3c_hsotg with struct dwc2_hsotg, which should be fine, modulo moving
the members of s3c_hsotg into dwc2_hsotg like I recommended.
--
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
