RE: [PATCH 3/4] staging: dwc2: add platform device bindings

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

 



> From: Felipe Balbi [mailto:balbi@xxxxxx]
> Sent: Thursday, April 11, 2013 11:36 AM
> 
> On Thu, Apr 11, 2013 at 06:43:48PM +0200, Matthijs Kooijman wrote:
> > This adds a dwc_platform.ko module that can be loaded by using
> > compatible = "snps,dwc2" in a device tree.
> >
> > Signed-off-by: Matthijs Kooijman <matthijs@xxxxxxxx>
> > ---
> >  Documentation/devicetree/bindings/staging/dwc2.txt |  15 +++
> >  drivers/staging/dwc2/Kconfig                       |   6 +-
> >  drivers/staging/dwc2/Makefile                      |   2 +
> >  drivers/staging/dwc2/platform.c                    | 150 +++++++++++++++++++++
> >  4 files changed, 171 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/staging/dwc2.txt
> >  create mode 100644 drivers/staging/dwc2/platform.c
> >
> > diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt b/Documentation/devicetree/bindings/staging/dwc2.txt
> > new file mode 100644
> > index 0000000..3649c88
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/staging/dwc2.txt
> > @@ -0,0 +1,15 @@
> > +Platform DesignWare HS OTG USB 2.0 controller
> > +-----------------------------------------------------
> > +
> > +Required properties:
> > +- compatible : "snps,dwc2"
> 
> please use the company's full name -> synopsys.

Actually, if you grep the tree for '"snps' vs. '"synopsys', you will see
that 'snps' is much more common in dt bindings. So I would suggest that
this is actually more correct, and the few outliers using 'synopsys'
should be converted.

> > +- reg : Should contain 1 register range (address and length)
> > +- interrupts : Should contain 1 interrupt
> > +
> > +Example:
> > +
> > +        otg@101c0000 {
> 
> this should probably be 'usb' instead of 'otg'
> 
> > +static int dwc2_driver_remove(struct platform_device *dev)
> > +{
> > +	struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
> > +
> > +	dev_dbg(&dev->dev, "%s(%p)\n", __func__, dev);
> 
> I don't think you need this dev_dbg() line.
> 
> > +static int dwc2_driver_probe(struct platform_device *dev)
> > +{
> > +	struct dwc2_hsotg *hsotg;
> > +	struct resource *res;
> > +	int retval;
> > +	int irq;
> > +	struct dwc2_core_params params;
> > +
> > +	/* Default all params to autodetect */
> > +	dwc2_set_all_params(&params, -1);
> > +
> > +	dev_dbg(&dev->dev, "%s(%p)\n", __func__, dev);
> 
> neither this one.
> 
> > +	hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
> > +	if (!hsotg)
> > +		return -ENOMEM;
> > +
> > +	hsotg->dev = &dev->dev;
> > +
> > +	irq = platform_get_irq(dev, 0);
> > +	if (irq < 0) {
> > +		dev_err(&dev->dev, "missing IRQ resource\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&dev->dev, "missing memory base resource\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	hsotg->regs = devm_ioremap_resource(&dev->dev, res);
> > +	if (IS_ERR(hsotg->regs))
> > +		return PTR_ERR(hsotg->regs);
> > +
> > +	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;
> > +
> > +	platform_set_drvdata(dev, hsotg);
> > +	dev_dbg(&dev->dev, "hsotg=%p\n", hsotg);
> 
> or this.
> 
> > +MODULE_AUTHOR("Synopsys, Inc.");
> 
> isn't MODULE_AUTHOR("Matthijs Kooijman <matthijs@xxxxxxxx>") more
> appropriate ?

Yes, I agree.

-- 
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