[PATCH v2] usb: gadget: composite: parse dt values

[PATCH v2] usb: gadget: composite: parse dt values

[Alexandre Pereira da Silva]

Grab the devicetree node properties to set VendorId, ProductId,
bcdDevice, Manucacturer, Product and SerialNumber

Signed-off-by: Alexandre Pereira da Silva <aletes.xgr@gmail.com>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
Acked-by: Rob Herring <rob.herring@calxeda.com>
---
Applies to v3.5

Changes since V1:
* Minor patch description and code comments updates

 Documentation/devicetree/bindings/usb/gadget.txt |   20 +++++++++++
 drivers/usb/gadget/composite.c                   |   39 ++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/gadget.txt

diff --git a/Documentation/devicetree/bindings/usb/gadget.txt b/Documentation/devicetree/bindings/usb/gadget.txt
new file mode 100644
index 0000000..93388d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/gadget.txt
@@ -0,0 +1,20 @@
+Usb Gadget DeviceTree bindings
+
+These optional properties inside the usb device controller node are used to
+change some of the gadget drivers configuration:
+- vendor-id: Usb vendor id
+- product-id: Usb product id
+- release: Version of this device
+- vendor: Textual description of the vendor
+- device: Textual description of this device
+- serial: Textual representation of the device's serial number
+
+Binding Example:
+	usbd@31020000 {
+		vendor-id = <0x0525>;
+		product-id = <0xa4a6>;
+		release = <1>;
+		vendor = "Some Corp";
+		device = "Test Device";
+		serial = "12345";
+	};
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 390749b..a02be8c 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/utsname.h>
+#include <linux/of.h>
 
 #include <linux/usb/composite.h>
 #include <asm/unaligned.h>
@@ -1419,10 +1420,44 @@ static u8 override_id(struct usb_composite_dev *cdev, u8 *desc)
 	return *desc;
 }
 
+static void composite_parse_dt(struct usb_composite_dev *cdev,
+	struct device_node *np)
+{
+	u32 reg;
+
+	if (!idVendor && of_property_read_u32(np, "vendor-id", &reg) == 0)
+		idVendor = reg;
+
+	if (!idProduct && of_property_read_u32(np, "product-id", &reg) == 0)
+		idProduct = reg;
+
+	if (!bcdDevice && of_property_read_u32(np, "release", &reg) == 0)
+		bcdDevice = reg;
+
+	if (!iManufacturer)
+		if (of_property_read_string(np, "vendor",
+			&composite->iManufacturer) == 0)
+			cdev->manufacturer_override = override_id(cdev,
+				&cdev->desc.iManufacturer);
+
+	if (!iProduct)
+		if (of_property_read_string(np, "device",
+			&composite->iProduct) == 0)
+			cdev->product_override = override_id(cdev,
+				&cdev->desc.iProduct);
+
+	if (!iSerialNumber)
+		if (of_property_read_string(np, "serial",
+			&composite->iSerialNumber) == 0)
+			cdev->serial_override = override_id(cdev,
+				&cdev->desc.iSerialNumber);
+}
+
 static int composite_bind(struct usb_gadget *gadget)
 {
 	struct usb_composite_dev	*cdev;
 	int				status = -ENOMEM;
+	struct device_node		*np = gadget->dev.of_node;
 
 	cdev = kzalloc(sizeof *cdev, GFP_KERNEL);
 	if (!cdev)
@@ -1470,6 +1505,10 @@ static int composite_bind(struct usb_gadget *gadget)
 
 	cdev->desc = *composite->dev;
 
+	/* grab values from devicetree */
+	if (np)
+		composite_parse_dt(cdev, np);
+
 	/* standardized runtime overrides for device ID data */
 	if (idVendor)
 		cdev->desc.idVendor = cpu_to_le16(idVendor);
-- 
1.7.10

Re: [PATCH v2] usb: gadget: composite: parse dt values

[Sebastian Andrzej Siewior]

On Tue, Jul 24, 2012 at 10:15:20AM -0300, Alexandre Pereira da Silva wrote:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/gadget.txt
> @@ -0,0 +1,20 @@
> +Usb Gadget DeviceTree bindings
> +
> +These optional properties inside the usb device controller node are used to
> +change some of the gadget drivers configuration:
> +- vendor-id: Usb vendor id
> +- product-id: Usb product id
> +- release: Version of this device
> +- vendor: Textual description of the vendor
> +- device: Textual description of this device
> +- serial: Textual representation of the device's serial number
> +
> +Binding Example:
> +	usbd@31020000 {
> +		vendor-id = <0x0525>;
> +		product-id = <0xa4a6>;
> +		release = <1>;
> +		vendor = "Some Corp";
> +		device = "Test Device";
> +		serial = "12345";
> +	};

No, I don't think that this is correct. You put this bindings at device level
and map those values directly to the composite layer for each gadget. You _can_
have multiple gadgets compiled and you may want to have load them depending
on your mood and here you force a special serial or vendor-id for each one of
them. If I take a look on N800 which I have here, I see that the vendor-id
changes if I switch from mass storage to modem. How do you want to make this
possible with device tree? Ah I leave this empty. I see.

Further more, you model this binding according to what the composite framework
is doing today instead of what the hardware should look like. Putting the 
current composite limitations aside the complete UDC node for the driver could 
look something like this:

|	usbd@31020000 {
|		compatible = "nxp,lpc3220-udc";
|		reg = <0x31020000 0x300>;
|		interrupts = <0x3d 0>, <0x3e 0>, <0x3c 0>, <0x3a 0>;
|		status = "disabled";
now  this 1:1 copy, nothing new.

|
|		gadget@0 {
The first gadget we want.

|			vendor-id = <0x0525>;
|			product-id = <0xa4a6>;
|			release = <1>;
|			vendor = "Some Corp";
|			device = "Test Device";
|			serial = "12345";

with some presets here

|			config@0 {
|				max_current = <900>;
|
|				function@0 {
|					compatible = "storage,uas";
|				};
|				function@1 {
|					compatible = "network,ncm";
|				};

Two gadgets here. We may could add extra string descriptors here but I would
have check if this makes sense.

|			};
|			config@1 {
|				max_current = <300>;
|
|				function@0 {
|					compatible = "network,ncm";
|				};

Another config if we don't have enough current, just go for the network and
let the storage off.

|			};
|		}
|
|		gadget@1 {
|			vendor-id = <0x0325>;
|			product-id = <0xa3a1>;
|			release = <1>;
|			vendor = "Some Corp";
|			device = "Test Device";
|			serial = "1234511";
|			
|			config@0 {
|				max_current = <900>;
|
|				function@0 {
|					compatible = "serial,cdc-acm";
|				};

a complete different gadget.

|			};
|		}
|	};

So _this_ binding would add two gadgets to your UDC and take, lets say, the
first one as the default one. And *then* you could depending on your mood switch
between network + storage gadget and modem gadget. Now how does this sound?

I understand that this is not yet possible with current gadget framework. If I
allow you to merge this it will make the rework more hard and painfull than it
already will be.

Based on this, I have to say, sorry, no, NAK.

Sebastian

Re: [PATCH v2] usb: gadget: composite: parse dt values

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

Hi,

On Thu, Aug 09, 2012 at 01:42:42PM +0200, Sebastian Andrzej Siewior wrote:
> On Tue, Jul 24, 2012 at 10:15:20AM -0300, Alexandre Pereira da Silva wrote:
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/gadget.txt
> > @@ -0,0 +1,20 @@
> > +Usb Gadget DeviceTree bindings
> > +
> > +These optional properties inside the usb device controller node are used to
> > +change some of the gadget drivers configuration:
> > +- vendor-id: Usb vendor id
> > +- product-id: Usb product id
> > +- release: Version of this device
> > +- vendor: Textual description of the vendor
> > +- device: Textual description of this device
> > +- serial: Textual representation of the device's serial number
> > +
> > +Binding Example:
> > +	usbd@31020000 {
> > +		vendor-id = <0x0525>;
> > +		product-id = <0xa4a6>;
> > +		release = <1>;
> > +		vendor = "Some Corp";
> > +		device = "Test Device";
> > +		serial = "12345";

on top of everything Sebastian said, I think we should stick to the
field names set forth on the USB specification meaning that instead of
vendor-id we should use idVendor, instead of vendor we should use
iManufacturer, and so on.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] usb: gadget: composite: parse dt values

[Alexandre Pereira da Silva]

On Thu, Aug 9, 2012 at 8:49 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Thu, Aug 09, 2012 at 01:42:42PM +0200, Sebastian Andrzej Siewior wrote:
>> On Tue, Jul 24, 2012 at 10:15:20AM -0300, Alexandre Pereira da Silva wrote:
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/usb/gadget.txt
>> > @@ -0,0 +1,20 @@
>> > +Usb Gadget DeviceTree bindings
>> > +
>> > +These optional properties inside the usb device controller node are used to
>> > +change some of the gadget drivers configuration:
>> > +- vendor-id: Usb vendor id
>> > +- product-id: Usb product id
>> > +- release: Version of this device
>> > +- vendor: Textual description of the vendor
>> > +- device: Textual description of this device
>> > +- serial: Textual representation of the device's serial number
>> > +
>> > +Binding Example:
>> > +   usbd@31020000 {
>> > +           vendor-id = <0x0525>;
>> > +           product-id = <0xa4a6>;
>> > +           release = <1>;
>> > +           vendor = "Some Corp";
>> > +           device = "Test Device";
>> > +           serial = "12345";
>
> on top of everything Sebastian said, I think we should stick to the
> field names set forth on the USB specification meaning that instead of
> vendor-id we should use idVendor, instead of vendor we should use
> iManufacturer, and so on.

Thanks for the feedback.

I agree that this should look more like the usb specifications and
allow mapping between usb interfaces and gadget drivers.

I will rework this to provide a new patch that at least don't conflict
with this idea.