Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

2016-11-22 Thread Axel Haslam 
On Mon, Nov 21, 2016 at 5:29 PM, David Lechner  wrote:
> On 11/21/2016 04:22 AM, Axel Haslam wrote:
>>
>> Hi David,
>>
>> Thanks for the review,
>>
>
> You're welcome.
>

 @@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct
 da8xx_ohci_root_hub *hub,
 hub->set_power(port, 0);
  }

 +static int ohci_da8xx_regulator_event(struct notifier_block *nb,
 +   unsigned long event, void *data)
 +{
 +   struct da8xx_ohci_hcd *da8xx_ohci =
 +   container_of(nb, struct da8xx_ohci_hcd,
 nb);
 +   struct device *dev = da8xx_ohci->hcd->self.controller;
 +
 +   if (event & REGULATOR_EVENT_OVER_CURRENT) {
 +   dev_warn(dev, "over current event\n");
>>>
>>>
>>>
>>> Won't this result in duplicate overcurrent warnings in the kernel log? It
>>> seems like in previous version of this patch series, we would get an
>>> overcurrent error from the core usb driver.
>>
>>
>> you mean in the regulator driver? i did not make changes to core usb.
>> but, no,  i did not add a print in the fixed regulator driver itself.
>> Since
>> the regulator is  a separate driver, and could be implemented with or
>> without
>> a trace, i think its better to leave this print. It shows that the usb
>> driver
>> has well received the notification.
>>
>
> No, I mean in drivers/usb/core/hub.c. There is
>
> if (status & USB_PORT_STAT_OVERCURRENT)
> dev_err(&port_dev->dev, "over-current condition\n");
>
> and
>
> if (status & HUB_STATUS_OVERCURRENT)
> dev_err(hub_dev, "over-current condition\n");
>
> In ohci_da8xx_hub_control(), we are setting RH_PS_POCI and RH_PS_OCIC, so
> these messages will be printed via the core hub driver. We don't need to
> print another message from the same event.
>
>

Hi David

I did a couple of tests, and i don't get those prints do you get them?.
What i understand is that they happen when we get a hub event that is
reporting the over current. Which is what the root hub of the davinci system
does not have, and why we use gpios instead).

Regards,
Axel.

>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

2016-11-21 Thread David Lechner 

On 11/21/2016 04:22 AM, Axel Haslam wrote:

Hi David,

Thanks for the review,



You're welcome.



@@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct
da8xx_ohci_root_hub *hub,
hub->set_power(port, 0);
 }

+static int ohci_da8xx_regulator_event(struct notifier_block *nb,
+   unsigned long event, void *data)
+{
+   struct da8xx_ohci_hcd *da8xx_ohci =
+   container_of(nb, struct da8xx_ohci_hcd,
nb);
+   struct device *dev = da8xx_ohci->hcd->self.controller;
+
+   if (event & REGULATOR_EVENT_OVER_CURRENT) {
+   dev_warn(dev, "over current event\n");



Won't this result in duplicate overcurrent warnings in the kernel log? It
seems like in previous version of this patch series, we would get an
overcurrent error from the core usb driver.


you mean in the regulator driver? i did not make changes to core usb.
but, no,  i did not add a print in the fixed regulator driver itself. Since
the regulator is  a separate driver, and could be implemented with or without
a trace, i think its better to leave this print. It shows that the usb driver
has well received the notification.



No, I mean in drivers/usb/core/hub.c. There is

if (status & USB_PORT_STAT_OVERCURRENT)
dev_err(&port_dev->dev, "over-current condition\n");

and

if (status & HUB_STATUS_OVERCURRENT)
dev_err(hub_dev, "over-current condition\n");

In ohci_da8xx_hub_control(), we are setting RH_PS_POCI and RH_PS_OCIC, 
so these messages will be printed via the core hub driver. We don't need 
to print another message from the same event.




--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

2016-11-21 Thread Axel Haslam 
Hi David,

Thanks for the review,


On Sun, Nov 20, 2016 at 4:31 AM, David Lechner  wrote:
> On 11/14/2016 08:41 AM, ahas...@baylibre.com wrote:
>>
>> Using a regulator to handle VBUS will eliminate the need for
>> platform data and callbacks, and make the driver more generic
>> allowing different types of regulators to handle VBUS.
>>
>> The regulator equivalents to the platform callbacks are:
>> set_power   ->  regulator_enable/regulator_disable
>> get_power   ->  regulator_is_enabled
>> get_oci ->  regulator_get_error_flags
>> ocic_notify ->  regulator event notification
>>
>> Signed-off-by: Axel Haslam 
>> ---
>>  drivers/usb/host/ohci-da8xx.c | 95
>> ++-
>>  1 file changed, 93 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>> index 83e3c98..42eaeb9 100644
>> --- a/drivers/usb/host/ohci-da8xx.c
>> +++ b/drivers/usb/host/ohci-da8xx.c
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd
>> *hcd, u16 typeReq,
>>  static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
>>
>>  struct da8xx_ohci_hcd {
>> +   struct usb_hcd *hcd;
>> struct clk *usb11_clk;
>> struct phy *usb11_phy;
>> +   struct regulator *vbus_reg;
>> +   struct notifier_block nb;
>> +   unsigned int is_powered;
>
>
> Since is_powered is only used to indicate if we have called
> regulator_enable(), I think it would make more sense to name it reg_enabled
> instead.

ok.

>
>>  };
>>
>>  #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd
>> *)(hcd_to_ohci(hcd)->priv)
>> @@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)
>>
>>  static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> +   int ret;
>>
>> if (hub && hub->set_power)
>> return hub->set_power(1, on);
>>
>> +   if (!da8xx_ohci->vbus_reg)
>> +   return 0;
>> +
>> +   if (on && !da8xx_ohci->is_powered) {
>> +   ret = regulator_enable(da8xx_ohci->vbus_reg);
>> +   if (ret) {
>> +   dev_err(dev, "Fail to enable regulator: %d\n",
>> ret);
>
>
> s/Fail/Failed/

will fix in both places.

>
>> +   return ret;
>> +   }
>> +   da8xx_ohci->is_powered = 1;
>> +
>> +   } else if (!on && da8xx_ohci->is_powered) {
>> +   ret = regulator_disable(da8xx_ohci->vbus_reg);
>> +   if (ret) {
>> +   dev_err(dev, "Fail to disable regulator: %d\n",
>> ret);
>
>
> s/Fail/Failed/
>
>> +   return ret;
>> +   }
>> +   da8xx_ohci->is_powered = 0;
>> +   }
>> +
>> return 0;
>>  }
>>
>>  static int ohci_da8xx_get_power(struct usb_hcd *hcd)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>
>> if (hub && hub->get_power)
>> return hub->get_power(1);
>>
>> +   if (da8xx_ohci->vbus_reg)
>> +   return regulator_is_enabled(da8xx_ohci->vbus_reg);
>> +
>> return 1;
>>  }
>>
>>  static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> +   unsigned int flags;
>> +   int ret;
>>
>> if (hub && hub->get_oci)
>> return hub->get_oci(1);
>>
>> +   if (!da8xx_ohci->vbus_reg)
>> +   return 0;
>> +
>> +   ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, &flags);
>> +   if (ret)
>> +   return ret;
>> +
>> +   if (flags && REGULATOR_ERROR_OVER_CURRENT)
>
>
> Is this supposed to be...

yes!

>
> if (flags & REGULATOR_ERROR_OVER_CURRENT)
>
>
>> +   return 1;
>> +
>> return 0;
>>  }
>>
>>  static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>
>> if (hub && hub->set_power)
>> return 1;
>>
>> +   if (da8xx_ohci->vbus_reg)
>> +   return 1;
>> +
>> return 0;
>>  }
>>
>>  static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> str

Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

2016-11-19 Thread David Lechner 

On 11/19/2016 09:31 PM, David Lechner wrote:

On 11/14/2016 08:41 AM, ahas...@baylibre.com wrote:

+ocic_mask |= 1;


I thought that a previous patch got rid of all globals. Why is ocic_mask
still a global variable?


I suppose if I read the commit message, I will know the answer ;-)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

2016-11-19 Thread David Lechner 

On 11/14/2016 08:41 AM, ahas...@baylibre.com wrote:

Using a regulator to handle VBUS will eliminate the need for
platform data and callbacks, and make the driver more generic
allowing different types of regulators to handle VBUS.

The regulator equivalents to the platform callbacks are:
set_power   ->  regulator_enable/regulator_disable
get_power   ->  regulator_is_enabled
get_oci ->  regulator_get_error_flags
ocic_notify ->  regulator event notification

Signed-off-by: Axel Haslam 
---
 drivers/usb/host/ohci-da8xx.c | 95 ++-
 1 file changed, 93 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 83e3c98..42eaeb9 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, 
u16 typeReq,
 static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);

 struct da8xx_ohci_hcd {
+   struct usb_hcd *hcd;
struct clk *usb11_clk;
struct phy *usb11_phy;
+   struct regulator *vbus_reg;
+   struct notifier_block nb;
+   unsigned int is_powered;


Since is_powered is only used to indicate if we have called 
regulator_enable(), I think it would make more sense to name it 
reg_enabled instead.



 };

 #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
@@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)

 static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
+   int ret;

if (hub && hub->set_power)
return hub->set_power(1, on);

+   if (!da8xx_ohci->vbus_reg)
+   return 0;
+
+   if (on && !da8xx_ohci->is_powered) {
+   ret = regulator_enable(da8xx_ohci->vbus_reg);
+   if (ret) {
+   dev_err(dev, "Fail to enable regulator: %d\n", ret);


s/Fail/Failed/


+   return ret;
+   }
+   da8xx_ohci->is_powered = 1;
+
+   } else if (!on && da8xx_ohci->is_powered) {
+   ret = regulator_disable(da8xx_ohci->vbus_reg);
+   if (ret) {
+   dev_err(dev, "Fail to disable regulator: %d\n", ret);


s/Fail/Failed/


+   return ret;
+   }
+   da8xx_ohci->is_powered = 0;
+   }
+
return 0;
 }

 static int ohci_da8xx_get_power(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->get_power)
return hub->get_power(1);

+   if (da8xx_ohci->vbus_reg)
+   return regulator_is_enabled(da8xx_ohci->vbus_reg);
+
return 1;
 }

 static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
+   unsigned int flags;
+   int ret;

if (hub && hub->get_oci)
return hub->get_oci(1);

+   if (!da8xx_ohci->vbus_reg)
+   return 0;
+
+   ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, &flags);
+   if (ret)
+   return ret;
+
+   if (flags && REGULATOR_ERROR_OVER_CURRENT)


Is this supposed to be...

if (flags & REGULATOR_ERROR_OVER_CURRENT)



+   return 1;
+
return 0;
 }

 static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->set_power)
return 1;

+   if (da8xx_ohci->vbus_reg)
+   return 1;
+
return 0;
 }

 static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->get_oci)
return 1;

+   if (da8xx_ohci->vbus_reg)
+   return 1;
+
return 0;
 }

@@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct 
da8xx_ohci_root_hub *hub,
hub->set_power(port, 0);
 }

+static int ohci_da8xx_regulator_event(struct notifier_block *nb,
+   unsigned long event, void *data)
+{
+   struct