Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support

[Michal Simek]

Hi Mark,

On 02/21/2014 01:04 PM, Mark Rutland wrote:
> 
> On Thu, Feb 20, 2014 at 06:23:13PM +0000, Felipe Balbi wrote:
>> Hi,
>>
>> On Wed, Feb 19, 2014 at 03:10:45PM +0530, Subbaraya Sundeep Bhatta wrote:
>>> This patch adds xilinx axi usb2 device driver support
>>>
>>> Signed-off-by: Subbaraya Sundeep Bhatta <sbhatta@xxxxxxxxxx>
>>> ---
>>>  .../devicetree/bindings/usb/xilinx_usb.txt         |   21 +
>>>  drivers/usb/gadget/Kconfig                         |   14 +
>>>  drivers/usb/gadget/Makefile                        |    1 +
>>>  drivers/usb/gadget/xilinx_udc.c                    | 2045 ++++++++++++++++++++
>>>  4 files changed, 2081 insertions(+), 0 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/usb/xilinx_usb.txt
>>>  create mode 100644 drivers/usb/gadget/xilinx_udc.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/xilinx_usb.txt b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
>>> new file mode 100644
>>> index 0000000..acf03ab
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
>>> @@ -0,0 +1,21 @@
>>> +Xilinx AXI USB2 device controller
>>> +
>>> +Required properties:
>>> +- compatible		: Should be "xlnx,axi-usb2-device-4.00.a"
> 
> Is "axi-usb2-device" the official device name?

It is the name of IP which Xilinx have and we are using names in this format.


>>> +- reg			: Physical base address and size of the Axi USB2
>>> +			  device registers map.
>>> +- interrupts		: Property with a value describing the interrupt
>>> +			  number.
> 
> Does the device only have a single interrupt?

I believe so.


>>> +- interrupt-parent	: Must be core interrupt controller
> 
> Is this strictly necessary?

I am not sure what do you mean by that. If you mean that interrupt-parent
can be written to the root of DTS file then we can setup system with more
interrupt controllers that's why it is required.

If we can point to standard interrupt description then please point me to
exact description you would like to see here and we can change it.


>>> +- xlnx,include-dma	: Must be 1 or 0 based on whether DMA is included
>>> +			  in IP or not.
> 
> Perhaps xlnx,has-builtin-dma would better describe this?

No opinion.

>>> +
>>> +Example:
>>> + 		axi-usb2-device@42e00000 {
>>> +                        compatible = "xlnx,axi-usb2-device-4.00.a";
>>> +                        interrupt-parent = <0x1>;
>>> +                        interrupts = <0x0 0x39 0x1>;
>>> +                        reg = <0x42e00000 0x10000>;
>>> +                        xlnx,include-dma = <0x1>;
>>> +                };
>>> +
>>
>> you need to Cc devicetree@xxxxxxxxxxxxxxx for this.
> 
> Cheers Filipe; thanks for adding us to Cc :)
> 

Sundeep with CC devicetree list in next patch version.

>>> +	/* Map the registers */
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	udc->base_address = devm_ioremap_nocache(&pdev->dev, res->start,
>>> +						 resource_size(res));
>>
>> use devm_ioremap_resource() instead.
> 
> Also, res might be NULL. You should check that before dereferencing it.

yes it is necessary for both cases with devm_ioremap_nocache
or with devm_ioremap_resource.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Attachment: signature.asc
Description: OpenPGP digital signature