Re: [PATCH v1 05/11] drivers: usb: twl6030: Add dt support for twl6030 usb

2012-07-10 Thread ABRAHAM, KISHON VIJAY
Hi,

On Tue, Jul 10, 2012 at 12:14 PM, Rajendra Nayak  wrote:
> On Tuesday 10 July 2012 11:58 AM, ABRAHAM, KISHON VIJAY wrote:
>>
>> Hi,
>>
>> On Tue, Jul 10, 2012 at 11:28 AM, Rajendra Nayak  wrote:
>>>
>>> On Thursday 28 June 2012 05:21 PM, Kishon Vijay Abraham I wrote:


 Add device tree support for twl6030 usb driver.
 Update the Documentation with device tree binding information.

 Signed-off-by: Kishon Vijay Abraham I
 ---
.../devicetree/bindings/usb/twl-usb.txt|   18 
drivers/usb/otg/twl6030-usb.c  |   45
 ++--
2 files changed, 50 insertions(+), 13 deletions(-)
create mode 100644
 Documentation/devicetree/bindings/usb/twl-usb.txt

 diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt
 b/Documentation/devicetree/bindings/usb/twl-usb.txt
 new file mode 100644
 index 000..f293271
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
 @@ -0,0 +1,18 @@
 +USB COMPARATOR OF TWL CHIPS
 +
 +TWL6030 USB COMPARATOR
 + - compatible : Should be "ti,twl6030-usb"
 + - interrupts : Two interrupt numbers to the cpu should be specified.
 First
 +   interrupt number is the otg interrupt number that raises ID
 interrupts
 when
 +   the controller has to act as host and the second interrupt number is
 the
 +   usb interrupt number that raises VBUS interrupts when the controller
 has to
 +   act as device
 + - regulator :   can be "vusb" or "ldousb"
 + --supply : phandle to the regulator device tree node
 +
 +twl6030-usb {
 +   compatible = "ti,twl6030-usb";
 +   interrupts =<   4 10>;
 +   regulator = "vusb";
 +   vusb-supply =<>;
>>>
>>>
>>>
>>> This doesn't seem right. Why do you ned a 'regulator' string along
>>> with the phandle?
>>
>>
>> The original code was something like
>> if (twl->features&  TWL6025_SUBCLASS)
>>
>> regulator_name = "ldousb";
>> else
>> regulator_name = "vusb";
>>
>> I wasn't sure how to handle this *TWL6025_SUBCLASS* stuff.
>>
>>>
 +};
 diff --git a/drivers/usb/otg/twl6030-usb.c
 b/drivers/usb/otg/twl6030-usb.c
 index 6a361d2..20b7abe 100644
 --- a/drivers/usb/otg/twl6030-usb.c
 +++ b/drivers/usb/otg/twl6030-usb.c
 @@ -105,7 +105,7 @@ struct twl6030_usb {
  u8  asleep;
  boolirq_enabled;
  boolvbus_enable;
 -   unsigned long   features;
 +   const char  *regulator;
};

#define   comparator_to_twl(x) container_of((x), struct
 twl6030_usb,
 comparator)
 @@ -153,13 +153,6 @@ static int twl6030_start_srp(struct phy_companion
 *comparator)

static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
{
 -   char *regulator_name;
 -
 -   if (twl->features&   TWL6025_SUBCLASS)

 -   regulator_name = "ldousb";
 -   else
 -   regulator_name = "vusb";
 -
  /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
  twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1,
 TWL6030_BACKUP_REG);

 @@ -169,7 +162,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb
 *twl)
  /* Program MISC2 register and set bit VUSB_IN_VBAT */
  twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);

 -   twl->usb3v3 = regulator_get(twl->dev, regulator_name);
 +   twl->usb3v3 = regulator_get(twl->dev, twl->regulator);
  if (IS_ERR(twl->usb3v3))
  return -ENODEV;

 @@ -324,9 +317,9 @@ static int __devinit twl6030_usb_probe(struct
 platform_device *pdev)
{
  struct twl6030_usb  *twl;
  int status, err;
 -   struct twl4030_usb_data *pdata;
 -   struct device *dev =>dev;

 -   pdata = dev->platform_data;
 +   struct device_node  *np = pdev->dev.of_node;
 +   struct device   *dev =>dev;

 +   struct twl4030_usb_data *pdata = dev->platform_data;

  twl = devm_kzalloc(dev, sizeof *twl, GFP_KERNEL);
  if (!twl)
 @@ -335,13 +328,28 @@ static int __devinit twl6030_usb_probe(struct
 platform_device *pdev)
  twl->dev=>dev;

  twl->irq1   = platform_get_irq(pdev, 0);
  twl->irq2   = platform_get_irq(pdev, 1);
 -   twl->features   = pdata->features;
  twl->linkstat   = OMAP_MUSB_UNKNOWN;

  twl->comparator.set_vbus= twl6030_set_vbus;
  twl->comparator.start_srp   = 

Re: [PATCH v1 05/11] drivers: usb: twl6030: Add dt support for twl6030 usb

2012-07-10 Thread Rajendra Nayak

On Tuesday 10 July 2012 11:58 AM, ABRAHAM, KISHON VIJAY wrote:

Hi,

On Tue, Jul 10, 2012 at 11:28 AM, Rajendra Nayak  wrote:

On Thursday 28 June 2012 05:21 PM, Kishon Vijay Abraham I wrote:


Add device tree support for twl6030 usb driver.
Update the Documentation with device tree binding information.

Signed-off-by: Kishon Vijay Abraham I
---
   .../devicetree/bindings/usb/twl-usb.txt|   18 
   drivers/usb/otg/twl6030-usb.c  |   45
++--
   2 files changed, 50 insertions(+), 13 deletions(-)
   create mode 100644 Documentation/devicetree/bindings/usb/twl-usb.txt

diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt
b/Documentation/devicetree/bindings/usb/twl-usb.txt
new file mode 100644
index 000..f293271
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
@@ -0,0 +1,18 @@
+USB COMPARATOR OF TWL CHIPS
+
+TWL6030 USB COMPARATOR
+ - compatible : Should be "ti,twl6030-usb"
+ - interrupts : Two interrupt numbers to the cpu should be specified.
First
+   interrupt number is the otg interrupt number that raises ID interrupts
when
+   the controller has to act as host and the second interrupt number is
the
+   usb interrupt number that raises VBUS interrupts when the controller
has to
+   act as device
+ - regulator :   can be "vusb" or "ldousb"
+ --supply : phandle to the regulator device tree node
+
+twl6030-usb {
+   compatible = "ti,twl6030-usb";
+   interrupts =<   4 10>;
+   regulator = "vusb";
+   vusb-supply =<>;



This doesn't seem right. Why do you ned a 'regulator' string along
with the phandle?


The original code was something like
if (twl->features&  TWL6025_SUBCLASS)
regulator_name = "ldousb";
else
regulator_name = "vusb";

I wasn't sure how to handle this *TWL6025_SUBCLASS* stuff.




+};
diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
index 6a361d2..20b7abe 100644
--- a/drivers/usb/otg/twl6030-usb.c
+++ b/drivers/usb/otg/twl6030-usb.c
@@ -105,7 +105,7 @@ struct twl6030_usb {
 u8  asleep;
 boolirq_enabled;
 boolvbus_enable;
-   unsigned long   features;
+   const char  *regulator;
   };

   #define   comparator_to_twl(x) container_of((x), struct twl6030_usb,
comparator)
@@ -153,13 +153,6 @@ static int twl6030_start_srp(struct phy_companion
*comparator)

   static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
   {
-   char *regulator_name;
-
-   if (twl->features&   TWL6025_SUBCLASS)

-   regulator_name = "ldousb";
-   else
-   regulator_name = "vusb";
-
 /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
 twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG);

@@ -169,7 +162,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb
*twl)
 /* Program MISC2 register and set bit VUSB_IN_VBAT */
 twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);

-   twl->usb3v3 = regulator_get(twl->dev, regulator_name);
+   twl->usb3v3 = regulator_get(twl->dev, twl->regulator);
 if (IS_ERR(twl->usb3v3))
 return -ENODEV;

@@ -324,9 +317,9 @@ static int __devinit twl6030_usb_probe(struct
platform_device *pdev)
   {
 struct twl6030_usb  *twl;
 int status, err;
-   struct twl4030_usb_data *pdata;
-   struct device *dev =>dev;

-   pdata = dev->platform_data;
+   struct device_node  *np = pdev->dev.of_node;
+   struct device   *dev =>dev;

+   struct twl4030_usb_data *pdata = dev->platform_data;

 twl = devm_kzalloc(dev, sizeof *twl, GFP_KERNEL);
 if (!twl)
@@ -335,13 +328,28 @@ static int __devinit twl6030_usb_probe(struct
platform_device *pdev)
 twl->dev=>dev;

 twl->irq1   = platform_get_irq(pdev, 0);
 twl->irq2   = platform_get_irq(pdev, 1);
-   twl->features   = pdata->features;
 twl->linkstat   = OMAP_MUSB_UNKNOWN;

 twl->comparator.set_vbus= twl6030_set_vbus;
 twl->comparator.start_srp   = twl6030_start_srp;
 omap_usb2_set_comparator(>comparator);

+   if (np) {
+   err = of_property_read_string(np,
"regulator",>regulator);

+   if (err<   0) {
+   dev_err(>dev, "unable to get regulator\n");
+   return err;
+   }



Isn't there a better way for the driver to know which supply to use instead
of DT passing the supply name?


The problem I see is this same driver is used for twl6030 and twl6025
and the regulator used is different for these two chips (And I think


hmm, so based on what chip is used on a board, shouldn't the board dts
file just map the right regulator with a supply 

Re: [PATCH v1 05/11] drivers: usb: twl6030: Add dt support for twl6030 usb

2012-07-10 Thread ABRAHAM, KISHON VIJAY
Hi,

On Tue, Jul 10, 2012 at 11:28 AM, Rajendra Nayak  wrote:
> On Thursday 28 June 2012 05:21 PM, Kishon Vijay Abraham I wrote:
>>
>> Add device tree support for twl6030 usb driver.
>> Update the Documentation with device tree binding information.
>>
>> Signed-off-by: Kishon Vijay Abraham I
>> ---
>>   .../devicetree/bindings/usb/twl-usb.txt|   18 
>>   drivers/usb/otg/twl6030-usb.c  |   45
>> ++--
>>   2 files changed, 50 insertions(+), 13 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/usb/twl-usb.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt
>> b/Documentation/devicetree/bindings/usb/twl-usb.txt
>> new file mode 100644
>> index 000..f293271
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
>> @@ -0,0 +1,18 @@
>> +USB COMPARATOR OF TWL CHIPS
>> +
>> +TWL6030 USB COMPARATOR
>> + - compatible : Should be "ti,twl6030-usb"
>> + - interrupts : Two interrupt numbers to the cpu should be specified.
>> First
>> +   interrupt number is the otg interrupt number that raises ID interrupts
>> when
>> +   the controller has to act as host and the second interrupt number is
>> the
>> +   usb interrupt number that raises VBUS interrupts when the controller
>> has to
>> +   act as device
>> + - regulator :  can be "vusb" or "ldousb"
>> + --supply : phandle to the regulator device tree node
>> +
>> +twl6030-usb {
>> +   compatible = "ti,twl6030-usb";
>> +   interrupts =<  4 10>;
>> +   regulator = "vusb";
>> +   vusb-supply =<>;
>
>
> This doesn't seem right. Why do you ned a 'regulator' string along
> with the phandle?

The original code was something like
if (twl->features & TWL6025_SUBCLASS)
regulator_name = "ldousb";
else
regulator_name = "vusb";

I wasn't sure how to handle this *TWL6025_SUBCLASS* stuff.

>
>> +};
>> diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
>> index 6a361d2..20b7abe 100644
>> --- a/drivers/usb/otg/twl6030-usb.c
>> +++ b/drivers/usb/otg/twl6030-usb.c
>> @@ -105,7 +105,7 @@ struct twl6030_usb {
>> u8  asleep;
>> boolirq_enabled;
>> boolvbus_enable;
>> -   unsigned long   features;
>> +   const char  *regulator;
>>   };
>>
>>   #define   comparator_to_twl(x) container_of((x), struct twl6030_usb,
>> comparator)
>> @@ -153,13 +153,6 @@ static int twl6030_start_srp(struct phy_companion
>> *comparator)
>>
>>   static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
>>   {
>> -   char *regulator_name;
>> -
>> -   if (twl->features&  TWL6025_SUBCLASS)
>>
>> -   regulator_name = "ldousb";
>> -   else
>> -   regulator_name = "vusb";
>> -
>> /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
>> twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG);
>>
>> @@ -169,7 +162,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb
>> *twl)
>> /* Program MISC2 register and set bit VUSB_IN_VBAT */
>> twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);
>>
>> -   twl->usb3v3 = regulator_get(twl->dev, regulator_name);
>> +   twl->usb3v3 = regulator_get(twl->dev, twl->regulator);
>> if (IS_ERR(twl->usb3v3))
>> return -ENODEV;
>>
>> @@ -324,9 +317,9 @@ static int __devinit twl6030_usb_probe(struct
>> platform_device *pdev)
>>   {
>> struct twl6030_usb  *twl;
>> int status, err;
>> -   struct twl4030_usb_data *pdata;
>> -   struct device *dev =>dev;
>>
>> -   pdata = dev->platform_data;
>> +   struct device_node  *np = pdev->dev.of_node;
>> +   struct device   *dev =>dev;
>>
>> +   struct twl4030_usb_data *pdata = dev->platform_data;
>>
>> twl = devm_kzalloc(dev, sizeof *twl, GFP_KERNEL);
>> if (!twl)
>> @@ -335,13 +328,28 @@ static int __devinit twl6030_usb_probe(struct
>> platform_device *pdev)
>> twl->dev=>dev;
>>
>> twl->irq1   = platform_get_irq(pdev, 0);
>> twl->irq2   = platform_get_irq(pdev, 1);
>> -   twl->features   = pdata->features;
>> twl->linkstat   = OMAP_MUSB_UNKNOWN;
>>
>> twl->comparator.set_vbus= twl6030_set_vbus;
>> twl->comparator.start_srp   = twl6030_start_srp;
>> omap_usb2_set_comparator(>comparator);
>>
>> +   if (np) {
>> +   err = of_property_read_string(np,
>> "regulator",>regulator);
>>
>> +   if (err<  0) {
>> +   dev_err(>dev, "unable to get regulator\n");
>> +   return err;
>> +   }
>
>
> Isn't there a better way for the driver to know which supply to use instead
> of DT passing the supply name?

The problem I see 

Re: [PATCH v1 05/11] drivers: usb: twl6030: Add dt support for twl6030 usb

2012-07-10 Thread ABRAHAM, KISHON VIJAY
Hi,

On Tue, Jul 10, 2012 at 11:28 AM, Rajendra Nayak rna...@ti.com wrote:
 On Thursday 28 June 2012 05:21 PM, Kishon Vijay Abraham I wrote:

 Add device tree support for twl6030 usb driver.
 Update the Documentation with device tree binding information.

 Signed-off-by: Kishon Vijay Abraham Ikis...@ti.com
 ---
   .../devicetree/bindings/usb/twl-usb.txt|   18 
   drivers/usb/otg/twl6030-usb.c  |   45
 ++--
   2 files changed, 50 insertions(+), 13 deletions(-)
   create mode 100644 Documentation/devicetree/bindings/usb/twl-usb.txt

 diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt
 b/Documentation/devicetree/bindings/usb/twl-usb.txt
 new file mode 100644
 index 000..f293271
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
 @@ -0,0 +1,18 @@
 +USB COMPARATOR OF TWL CHIPS
 +
 +TWL6030 USB COMPARATOR
 + - compatible : Should be ti,twl6030-usb
 + - interrupts : Two interrupt numbers to the cpu should be specified.
 First
 +   interrupt number is the otg interrupt number that raises ID interrupts
 when
 +   the controller has to act as host and the second interrupt number is
 the
 +   usb interrupt number that raises VBUS interrupts when the controller
 has to
 +   act as device
 + - regulator :supply-name  can be vusb or ldousb
 + -supply-name-supply : phandle to the regulator device tree node
 +
 +twl6030-usb {
 +   compatible = ti,twl6030-usb;
 +   interrupts =  4 10;
 +   regulator = vusb;
 +   vusb-supply =vusb;


 This doesn't seem right. Why do you ned a 'regulator' string along
 with the phandle?

The original code was something like
if (twl-features  TWL6025_SUBCLASS)
regulator_name = ldousb;
else
regulator_name = vusb;

I wasn't sure how to handle this *TWL6025_SUBCLASS* stuff.


 +};
 diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
 index 6a361d2..20b7abe 100644
 --- a/drivers/usb/otg/twl6030-usb.c
 +++ b/drivers/usb/otg/twl6030-usb.c
 @@ -105,7 +105,7 @@ struct twl6030_usb {
 u8  asleep;
 boolirq_enabled;
 boolvbus_enable;
 -   unsigned long   features;
 +   const char  *regulator;
   };

   #define   comparator_to_twl(x) container_of((x), struct twl6030_usb,
 comparator)
 @@ -153,13 +153,6 @@ static int twl6030_start_srp(struct phy_companion
 *comparator)

   static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
   {
 -   char *regulator_name;
 -
 -   if (twl-features  TWL6025_SUBCLASS)

 -   regulator_name = ldousb;
 -   else
 -   regulator_name = vusb;
 -
 /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
 twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG);

 @@ -169,7 +162,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb
 *twl)
 /* Program MISC2 register and set bit VUSB_IN_VBAT */
 twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);

 -   twl-usb3v3 = regulator_get(twl-dev, regulator_name);
 +   twl-usb3v3 = regulator_get(twl-dev, twl-regulator);
 if (IS_ERR(twl-usb3v3))
 return -ENODEV;

 @@ -324,9 +317,9 @@ static int __devinit twl6030_usb_probe(struct
 platform_device *pdev)
   {
 struct twl6030_usb  *twl;
 int status, err;
 -   struct twl4030_usb_data *pdata;
 -   struct device *dev =pdev-dev;

 -   pdata = dev-platform_data;
 +   struct device_node  *np = pdev-dev.of_node;
 +   struct device   *dev =pdev-dev;

 +   struct twl4030_usb_data *pdata = dev-platform_data;

 twl = devm_kzalloc(dev, sizeof *twl, GFP_KERNEL);
 if (!twl)
 @@ -335,13 +328,28 @@ static int __devinit twl6030_usb_probe(struct
 platform_device *pdev)
 twl-dev=pdev-dev;

 twl-irq1   = platform_get_irq(pdev, 0);
 twl-irq2   = platform_get_irq(pdev, 1);
 -   twl-features   = pdata-features;
 twl-linkstat   = OMAP_MUSB_UNKNOWN;

 twl-comparator.set_vbus= twl6030_set_vbus;
 twl-comparator.start_srp   = twl6030_start_srp;
 omap_usb2_set_comparator(twl-comparator);

 +   if (np) {
 +   err = of_property_read_string(np,
 regulator,twl-regulator);

 +   if (err  0) {
 +   dev_err(pdev-dev, unable to get regulator\n);
 +   return err;
 +   }


 Isn't there a better way for the driver to know which supply to use instead
 of DT passing the supply name?

The problem I see is this same driver is used for twl6030 and twl6025
and the regulator used is different for these two chips (And I think
twl6025 will also use the same dt file as twl6030 as I don't see a
different file for 6025).


Re: [PATCH v1 05/11] drivers: usb: twl6030: Add dt support for twl6030 usb

2012-07-10 Thread Rajendra Nayak

On Tuesday 10 July 2012 11:58 AM, ABRAHAM, KISHON VIJAY wrote:

Hi,

On Tue, Jul 10, 2012 at 11:28 AM, Rajendra Nayakrna...@ti.com  wrote:

On Thursday 28 June 2012 05:21 PM, Kishon Vijay Abraham I wrote:


Add device tree support for twl6030 usb driver.
Update the Documentation with device tree binding information.

Signed-off-by: Kishon Vijay Abraham Ikis...@ti.com
---
   .../devicetree/bindings/usb/twl-usb.txt|   18 
   drivers/usb/otg/twl6030-usb.c  |   45
++--
   2 files changed, 50 insertions(+), 13 deletions(-)
   create mode 100644 Documentation/devicetree/bindings/usb/twl-usb.txt

diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt
b/Documentation/devicetree/bindings/usb/twl-usb.txt
new file mode 100644
index 000..f293271
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
@@ -0,0 +1,18 @@
+USB COMPARATOR OF TWL CHIPS
+
+TWL6030 USB COMPARATOR
+ - compatible : Should be ti,twl6030-usb
+ - interrupts : Two interrupt numbers to the cpu should be specified.
First
+   interrupt number is the otg interrupt number that raises ID interrupts
when
+   the controller has to act as host and the second interrupt number is
the
+   usb interrupt number that raises VBUS interrupts when the controller
has to
+   act as device
+ - regulator :supply-name   can be vusb or ldousb
+ -supply-name-supply : phandle to the regulator device tree node
+
+twl6030-usb {
+   compatible = ti,twl6030-usb;
+   interrupts =   4 10;
+   regulator = vusb;
+   vusb-supply =vusb;



This doesn't seem right. Why do you ned a 'regulator' string along
with the phandle?


The original code was something like
if (twl-features  TWL6025_SUBCLASS)
regulator_name = ldousb;
else
regulator_name = vusb;

I wasn't sure how to handle this *TWL6025_SUBCLASS* stuff.




+};
diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
index 6a361d2..20b7abe 100644
--- a/drivers/usb/otg/twl6030-usb.c
+++ b/drivers/usb/otg/twl6030-usb.c
@@ -105,7 +105,7 @@ struct twl6030_usb {
 u8  asleep;
 boolirq_enabled;
 boolvbus_enable;
-   unsigned long   features;
+   const char  *regulator;
   };

   #define   comparator_to_twl(x) container_of((x), struct twl6030_usb,
comparator)
@@ -153,13 +153,6 @@ static int twl6030_start_srp(struct phy_companion
*comparator)

   static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
   {
-   char *regulator_name;
-
-   if (twl-features   TWL6025_SUBCLASS)

-   regulator_name = ldousb;
-   else
-   regulator_name = vusb;
-
 /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
 twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG);

@@ -169,7 +162,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb
*twl)
 /* Program MISC2 register and set bit VUSB_IN_VBAT */
 twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);

-   twl-usb3v3 = regulator_get(twl-dev, regulator_name);
+   twl-usb3v3 = regulator_get(twl-dev, twl-regulator);
 if (IS_ERR(twl-usb3v3))
 return -ENODEV;

@@ -324,9 +317,9 @@ static int __devinit twl6030_usb_probe(struct
platform_device *pdev)
   {
 struct twl6030_usb  *twl;
 int status, err;
-   struct twl4030_usb_data *pdata;
-   struct device *dev =pdev-dev;

-   pdata = dev-platform_data;
+   struct device_node  *np = pdev-dev.of_node;
+   struct device   *dev =pdev-dev;

+   struct twl4030_usb_data *pdata = dev-platform_data;

 twl = devm_kzalloc(dev, sizeof *twl, GFP_KERNEL);
 if (!twl)
@@ -335,13 +328,28 @@ static int __devinit twl6030_usb_probe(struct
platform_device *pdev)
 twl-dev=pdev-dev;

 twl-irq1   = platform_get_irq(pdev, 0);
 twl-irq2   = platform_get_irq(pdev, 1);
-   twl-features   = pdata-features;
 twl-linkstat   = OMAP_MUSB_UNKNOWN;

 twl-comparator.set_vbus= twl6030_set_vbus;
 twl-comparator.start_srp   = twl6030_start_srp;
 omap_usb2_set_comparator(twl-comparator);

+   if (np) {
+   err = of_property_read_string(np,
regulator,twl-regulator);

+   if (err   0) {
+   dev_err(pdev-dev, unable to get regulator\n);
+   return err;
+   }



Isn't there a better way for the driver to know which supply to use instead
of DT passing the supply name?


The problem I see is this same driver is used for twl6030 and twl6025
and the regulator used is different for these two chips (And I think


hmm, so based on what chip is used on a board, shouldn't the board dts
file just map the right 

Re: [PATCH v1 05/11] drivers: usb: twl6030: Add dt support for twl6030 usb

2012-07-10 Thread ABRAHAM, KISHON VIJAY
Hi,

On Tue, Jul 10, 2012 at 12:14 PM, Rajendra Nayak rna...@ti.com wrote:
 On Tuesday 10 July 2012 11:58 AM, ABRAHAM, KISHON VIJAY wrote:

 Hi,

 On Tue, Jul 10, 2012 at 11:28 AM, Rajendra Nayakrna...@ti.com  wrote:

 On Thursday 28 June 2012 05:21 PM, Kishon Vijay Abraham I wrote:


 Add device tree support for twl6030 usb driver.
 Update the Documentation with device tree binding information.

 Signed-off-by: Kishon Vijay Abraham Ikis...@ti.com
 ---
.../devicetree/bindings/usb/twl-usb.txt|   18 
drivers/usb/otg/twl6030-usb.c  |   45
 ++--
2 files changed, 50 insertions(+), 13 deletions(-)
create mode 100644
 Documentation/devicetree/bindings/usb/twl-usb.txt

 diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt
 b/Documentation/devicetree/bindings/usb/twl-usb.txt
 new file mode 100644
 index 000..f293271
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
 @@ -0,0 +1,18 @@
 +USB COMPARATOR OF TWL CHIPS
 +
 +TWL6030 USB COMPARATOR
 + - compatible : Should be ti,twl6030-usb
 + - interrupts : Two interrupt numbers to the cpu should be specified.
 First
 +   interrupt number is the otg interrupt number that raises ID
 interrupts
 when
 +   the controller has to act as host and the second interrupt number is
 the
 +   usb interrupt number that raises VBUS interrupts when the controller
 has to
 +   act as device
 + - regulator :supply-name   can be vusb or ldousb
 + -supply-name-supply : phandle to the regulator device tree node
 +
 +twl6030-usb {
 +   compatible = ti,twl6030-usb;
 +   interrupts =   4 10;
 +   regulator = vusb;
 +   vusb-supply =vusb;



 This doesn't seem right. Why do you ned a 'regulator' string along
 with the phandle?


 The original code was something like
 if (twl-features  TWL6025_SUBCLASS)

 regulator_name = ldousb;
 else
 regulator_name = vusb;

 I wasn't sure how to handle this *TWL6025_SUBCLASS* stuff.


 +};
 diff --git a/drivers/usb/otg/twl6030-usb.c
 b/drivers/usb/otg/twl6030-usb.c
 index 6a361d2..20b7abe 100644
 --- a/drivers/usb/otg/twl6030-usb.c
 +++ b/drivers/usb/otg/twl6030-usb.c
 @@ -105,7 +105,7 @@ struct twl6030_usb {
  u8  asleep;
  boolirq_enabled;
  boolvbus_enable;
 -   unsigned long   features;
 +   const char  *regulator;
};

#define   comparator_to_twl(x) container_of((x), struct
 twl6030_usb,
 comparator)
 @@ -153,13 +153,6 @@ static int twl6030_start_srp(struct phy_companion
 *comparator)

static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
{
 -   char *regulator_name;
 -
 -   if (twl-features   TWL6025_SUBCLASS)

 -   regulator_name = ldousb;
 -   else
 -   regulator_name = vusb;
 -
  /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
  twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1,
 TWL6030_BACKUP_REG);

 @@ -169,7 +162,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb
 *twl)
  /* Program MISC2 register and set bit VUSB_IN_VBAT */
  twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);

 -   twl-usb3v3 = regulator_get(twl-dev, regulator_name);
 +   twl-usb3v3 = regulator_get(twl-dev, twl-regulator);
  if (IS_ERR(twl-usb3v3))
  return -ENODEV;

 @@ -324,9 +317,9 @@ static int __devinit twl6030_usb_probe(struct
 platform_device *pdev)
{
  struct twl6030_usb  *twl;
  int status, err;
 -   struct twl4030_usb_data *pdata;
 -   struct device *dev =pdev-dev;

 -   pdata = dev-platform_data;
 +   struct device_node  *np = pdev-dev.of_node;
 +   struct device   *dev =pdev-dev;

 +   struct twl4030_usb_data *pdata = dev-platform_data;

  twl = devm_kzalloc(dev, sizeof *twl, GFP_KERNEL);
  if (!twl)
 @@ -335,13 +328,28 @@ static int __devinit twl6030_usb_probe(struct
 platform_device *pdev)
  twl-dev=pdev-dev;

  twl-irq1   = platform_get_irq(pdev, 0);
  twl-irq2   = platform_get_irq(pdev, 1);
 -   twl-features   = pdata-features;
  twl-linkstat   = OMAP_MUSB_UNKNOWN;

  twl-comparator.set_vbus= twl6030_set_vbus;
  twl-comparator.start_srp   = twl6030_start_srp;
  omap_usb2_set_comparator(twl-comparator);

 +   if (np) {
 +   err = of_property_read_string(np,
 regulator,twl-regulator);

 +   if (err   0) {
 +   dev_err(pdev-dev, unable to get
 regulator\n);
 +   return err;
 +   }



 Isn't there a better way for the driver to know which supply to use
 instead
 of DT passing the supply name?


 The problem I see is this same driver is 

Re: [PATCH v1 05/11] drivers: usb: twl6030: Add dt support for twl6030 usb

2012-07-09 Thread Rajendra Nayak

On Thursday 28 June 2012 05:21 PM, Kishon Vijay Abraham I wrote:

Add device tree support for twl6030 usb driver.
Update the Documentation with device tree binding information.

Signed-off-by: Kishon Vijay Abraham I
---
  .../devicetree/bindings/usb/twl-usb.txt|   18 
  drivers/usb/otg/twl6030-usb.c  |   45 ++--
  2 files changed, 50 insertions(+), 13 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/usb/twl-usb.txt

diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt 
b/Documentation/devicetree/bindings/usb/twl-usb.txt
new file mode 100644
index 000..f293271
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
@@ -0,0 +1,18 @@
+USB COMPARATOR OF TWL CHIPS
+
+TWL6030 USB COMPARATOR
+ - compatible : Should be "ti,twl6030-usb"
+ - interrupts : Two interrupt numbers to the cpu should be specified. First
+   interrupt number is the otg interrupt number that raises ID interrupts when
+   the controller has to act as host and the second interrupt number is the
+   usb interrupt number that raises VBUS interrupts when the controller has to
+   act as device
+ - regulator :  can be "vusb" or "ldousb"
+ --supply : phandle to the regulator device tree node
+
+twl6030-usb {
+   compatible = "ti,twl6030-usb";
+   interrupts =<  4 10>;
+   regulator = "vusb";
+   vusb-supply =<>;


This doesn't seem right. Why do you ned a 'regulator' string along
with the phandle?


+};
diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
index 6a361d2..20b7abe 100644
--- a/drivers/usb/otg/twl6030-usb.c
+++ b/drivers/usb/otg/twl6030-usb.c
@@ -105,7 +105,7 @@ struct twl6030_usb {
u8  asleep;
boolirq_enabled;
boolvbus_enable;
-   unsigned long   features;
+   const char  *regulator;
  };

  #define   comparator_to_twl(x) container_of((x), struct twl6030_usb, 
comparator)
@@ -153,13 +153,6 @@ static int twl6030_start_srp(struct phy_companion 
*comparator)

  static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
  {
-   char *regulator_name;
-
-   if (twl->features&  TWL6025_SUBCLASS)
-   regulator_name = "ldousb";
-   else
-   regulator_name = "vusb";
-
/* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG);

@@ -169,7 +162,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
/* Program MISC2 register and set bit VUSB_IN_VBAT */
twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);

-   twl->usb3v3 = regulator_get(twl->dev, regulator_name);
+   twl->usb3v3 = regulator_get(twl->dev, twl->regulator);
if (IS_ERR(twl->usb3v3))
return -ENODEV;

@@ -324,9 +317,9 @@ static int __devinit twl6030_usb_probe(struct 
platform_device *pdev)
  {
struct twl6030_usb  *twl;
int status, err;
-   struct twl4030_usb_data *pdata;
-   struct device *dev =>dev;
-   pdata = dev->platform_data;
+   struct device_node  *np = pdev->dev.of_node;
+   struct device   *dev =>dev;
+   struct twl4030_usb_data *pdata = dev->platform_data;

twl = devm_kzalloc(dev, sizeof *twl, GFP_KERNEL);
if (!twl)
@@ -335,13 +328,28 @@ static int __devinit twl6030_usb_probe(struct 
platform_device *pdev)
twl->dev =>dev;
twl->irq1= platform_get_irq(pdev, 0);
twl->irq2= platform_get_irq(pdev, 1);
-   twl->features= pdata->features;
twl->linkstat= OMAP_MUSB_UNKNOWN;

twl->comparator.set_vbus = twl6030_set_vbus;
twl->comparator.start_srp= twl6030_start_srp;
omap_usb2_set_comparator(>comparator);

+   if (np) {
+   err = of_property_read_string(np, "regulator",>regulator);
+   if (err<  0) {
+   dev_err(>dev, "unable to get regulator\n");
+   return err;
+   }


Isn't there a better way for the driver to know which supply to use 
instead of DT passing the supply name?


regards,
Rajendra


+   } else if (pdata) {
+   if (pdata->features&  TWL6025_SUBCLASS)
+   twl->regulator = "ldousb";
+   else
+   twl->regulator = "vusb";
+   } else {
+   dev_err(>dev, "twl6030 initialized without pdata\n");
+   return -EINVAL;
+   }
+
/* init spinlock for workqueue */
spin_lock_init(>lock);

@@ -403,12 +411,23 @@ static int __exit twl6030_usb_remove(struct 
platform_device *pdev)
return 0;
  }

+#ifdef CONFIG_OF
+static const struct of_device_id twl6030_usb_id_table[] = {
+   { .compatible = "ti,twl6030-usb" },
+   

Re: [PATCH v1 05/11] drivers: usb: twl6030: Add dt support for twl6030 usb

2012-07-09 Thread Rajendra Nayak

On Thursday 28 June 2012 05:21 PM, Kishon Vijay Abraham I wrote:

Add device tree support for twl6030 usb driver.
Update the Documentation with device tree binding information.

Signed-off-by: Kishon Vijay Abraham Ikis...@ti.com
---
  .../devicetree/bindings/usb/twl-usb.txt|   18 
  drivers/usb/otg/twl6030-usb.c  |   45 ++--
  2 files changed, 50 insertions(+), 13 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/usb/twl-usb.txt

diff --git a/Documentation/devicetree/bindings/usb/twl-usb.txt 
b/Documentation/devicetree/bindings/usb/twl-usb.txt
new file mode 100644
index 000..f293271
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/twl-usb.txt
@@ -0,0 +1,18 @@
+USB COMPARATOR OF TWL CHIPS
+
+TWL6030 USB COMPARATOR
+ - compatible : Should be ti,twl6030-usb
+ - interrupts : Two interrupt numbers to the cpu should be specified. First
+   interrupt number is the otg interrupt number that raises ID interrupts when
+   the controller has to act as host and the second interrupt number is the
+   usb interrupt number that raises VBUS interrupts when the controller has to
+   act as device
+ - regulator :supply-name  can be vusb or ldousb
+ -supply-name-supply : phandle to the regulator device tree node
+
+twl6030-usb {
+   compatible = ti,twl6030-usb;
+   interrupts =  4 10;
+   regulator = vusb;
+   vusb-supply =vusb;


This doesn't seem right. Why do you ned a 'regulator' string along
with the phandle?


+};
diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
index 6a361d2..20b7abe 100644
--- a/drivers/usb/otg/twl6030-usb.c
+++ b/drivers/usb/otg/twl6030-usb.c
@@ -105,7 +105,7 @@ struct twl6030_usb {
u8  asleep;
boolirq_enabled;
boolvbus_enable;
-   unsigned long   features;
+   const char  *regulator;
  };

  #define   comparator_to_twl(x) container_of((x), struct twl6030_usb, 
comparator)
@@ -153,13 +153,6 @@ static int twl6030_start_srp(struct phy_companion 
*comparator)

  static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
  {
-   char *regulator_name;
-
-   if (twl-features  TWL6025_SUBCLASS)
-   regulator_name = ldousb;
-   else
-   regulator_name = vusb;
-
/* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG);

@@ -169,7 +162,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
/* Program MISC2 register and set bit VUSB_IN_VBAT */
twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);

-   twl-usb3v3 = regulator_get(twl-dev, regulator_name);
+   twl-usb3v3 = regulator_get(twl-dev, twl-regulator);
if (IS_ERR(twl-usb3v3))
return -ENODEV;

@@ -324,9 +317,9 @@ static int __devinit twl6030_usb_probe(struct 
platform_device *pdev)
  {
struct twl6030_usb  *twl;
int status, err;
-   struct twl4030_usb_data *pdata;
-   struct device *dev =pdev-dev;
-   pdata = dev-platform_data;
+   struct device_node  *np = pdev-dev.of_node;
+   struct device   *dev =pdev-dev;
+   struct twl4030_usb_data *pdata = dev-platform_data;

twl = devm_kzalloc(dev, sizeof *twl, GFP_KERNEL);
if (!twl)
@@ -335,13 +328,28 @@ static int __devinit twl6030_usb_probe(struct 
platform_device *pdev)
twl-dev =pdev-dev;
twl-irq1= platform_get_irq(pdev, 0);
twl-irq2= platform_get_irq(pdev, 1);
-   twl-features= pdata-features;
twl-linkstat= OMAP_MUSB_UNKNOWN;

twl-comparator.set_vbus = twl6030_set_vbus;
twl-comparator.start_srp= twl6030_start_srp;
omap_usb2_set_comparator(twl-comparator);

+   if (np) {
+   err = of_property_read_string(np, regulator,twl-regulator);
+   if (err  0) {
+   dev_err(pdev-dev, unable to get regulator\n);
+   return err;
+   }


Isn't there a better way for the driver to know which supply to use 
instead of DT passing the supply name?


regards,
Rajendra


+   } else if (pdata) {
+   if (pdata-features  TWL6025_SUBCLASS)
+   twl-regulator = ldousb;
+   else
+   twl-regulator = vusb;
+   } else {
+   dev_err(pdev-dev, twl6030 initialized without pdata\n);
+   return -EINVAL;
+   }
+
/* init spinlock for workqueue */
spin_lock_init(twl-lock);

@@ -403,12 +411,23 @@ static int __exit twl6030_usb_remove(struct 
platform_device *pdev)
return 0;
  }

+#ifdef CONFIG_OF
+static const struct of_device_id twl6030_usb_id_table[] = {
+   { .compatible =