[PATCH v2] otg/ulpi.c : fix register write

[PATCH v2] otg/ulpi.c : fix register write

[Eric Bénard]

ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write
to the PHY's registers, which means we can only set bits in the PHY's
register and not clear them.
By directly using the address of the register without any offset, we
now get the expected behaviour for these functions.

Signed-off-by: Eric B?nard <eric@eukrea.com>
Cc: Daniel Mack <daniel@caiaq.de>
Cc: linux-usb at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: Sascha Hauer <kernel@pengutronix.de>
---
v2 :
drop reg/val swap which is handled in a better way by Igor's patch here :
http://comments.gmane.org/gmane.linux.usb.general/32092 
 
 drivers/usb/otg/ulpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
index b1b3469..086853b 100644
--- a/drivers/usb/otg/ulpi.c
+++ b/drivers/usb/otg/ulpi.c
@@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
 		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 static int ulpi_init(struct otg_transceiver *otg)
@@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
 	}
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 struct otg_transceiver *
-- 
1.6.3.3

[PATCH v2] otg/ulpi.c : fix register write

[Igor Grinberg]

On 06/23/10 19:43, Eric B?nard wrote:
> ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write
> to the PHY's registers, which means we can only set bits in the PHY's
> register and not clear them.
> By directly using the address of the register without any offset, we
> now get the expected behaviour for these functions.
>
> Signed-off-by: Eric B?nard <eric@eukrea.com>
> Cc: Daniel Mack <daniel@caiaq.de>
> Cc: linux-usb at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> ---
> v2 :
> drop reg/val swap which is handled in a better way by Igor's patch here :
> http://comments.gmane.org/gmane.linux.usb.general/32092 
>  
>  drivers/usb/otg/ulpi.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
> index b1b3469..086853b 100644
> --- a/drivers/usb/otg/ulpi.c
> +++ b/drivers/usb/otg/ulpi.c
> @@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
>  	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
>  		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
>  
> -	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>  }
>   

ulpi_set_flags is used to set the OTG flags and not to clear them,
also it is a static function and is not a part of struct otg_transceiver,
so it cannot be called from outside the ulpi.c, thus after the ulpi_create,
it is never called.
I think, currently, this should stay as it is.


>  static int ulpi_init(struct otg_transceiver *otg)
> @@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
>  			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
>  	}
>  
> -	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>  }
>   

This one seems ok.

>  
>  struct otg_transceiver *
>   

-- 
Regards,
Igor.

[PATCH v2] otg/ulpi.c : fix register write

[Eric Bénard]

Le 24/06/2010 16:04, Igor Grinberg a ?crit :
> ulpi_set_flags is used to set the OTG flags and not to clear them,
> also it is a static function and is not a part of struct otg_transceiver,
> so it cannot be called from outside the ulpi.c, thus after the ulpi_create,
> it is never called.
> I think, currently, this should stay as it is.
>
OK, the problem that when one flag is set, you can't clear it without 
resetting the PHY.

Eric

[PATCH v2] otg/ulpi.c : fix register write

[Igor Grinberg]

On 06/24/10 17:37, Eric B?nard wrote:
> Le 24/06/2010 16:04, Igor Grinberg a ?crit :
>> ulpi_set_flags is used to set the OTG flags and not to clear them,
>> also it is a static function and is not a part of struct
>> otg_transceiver,
>> so it cannot be called from outside the ulpi.c, thus after the
>> ulpi_create,
>> it is never called.
>> I think, currently, this should stay as it is.
>>
> OK, the problem that when one flag is set, you can't clear it without
> resetting the PHY.

This is correct, but current ulpi driver is not designed for writing
multiple times
into the ULPI_OTG_CTRL register, only once when ulpi_init function is
called.
Do you call the ulpi_init function for a couple of times? Looks like a hack.

>
> Eric
>

-- 
Regards,
Igor.

[PATCH v2] otg/ulpi.c : fix register write

[Eric Bénard]

Le 27/06/2010 09:42, Igor Grinberg a ?crit :
>
> On 06/24/10 17:37, Eric B?nard wrote:
>> Le 24/06/2010 16:04, Igor Grinberg a ?crit :
>>> ulpi_set_flags is used to set the OTG flags and not to clear them,
>>> also it is a static function and is not a part of struct
>>> otg_transceiver,
>>> so it cannot be called from outside the ulpi.c, thus after the
>>> ulpi_create,
>>> it is never called.
>>> I think, currently, this should stay as it is.
>>>
>> OK, the problem that when one flag is set, you can't clear it without
>> resetting the PHY.
>
> This is correct, but current ulpi driver is not designed for writing
> multiple times
> into the ULPI_OTG_CTRL register, only once when ulpi_init function is
> called.
> Do you call the ulpi_init function for a couple of times? Looks like a hack.
>
no hack, as I was fixing the other function, I though this one could 
also be fixed :)
I'll send a patch with only the other fix.

Eric

[PATCH v2] otg/ulpi.c : fix register write

[Igor Grinberg]

On 06/27/10 17:00, Eric B?nard wrote:
> Le 27/06/2010 09:42, Igor Grinberg a ?crit :
>>
>> On 06/24/10 17:37, Eric B?nard wrote:
>>> Le 24/06/2010 16:04, Igor Grinberg a ?crit :
>>>> ulpi_set_flags is used to set the OTG flags and not to clear them,
>>>> also it is a static function and is not a part of struct
>>>> otg_transceiver,
>>>> so it cannot be called from outside the ulpi.c, thus after the
>>>> ulpi_create,
>>>> it is never called.
>>>> I think, currently, this should stay as it is.
>>>>
>>> OK, the problem that when one flag is set, you can't clear it without
>>> resetting the PHY.
>>
>> This is correct, but current ulpi driver is not designed for writing
>> multiple times
>> into the ULPI_OTG_CTRL register, only once when ulpi_init function is
>> called.
>> Do you call the ulpi_init function for a couple of times? Looks like
>> a hack.
>>
> no hack, as I was fixing the other function, I though this one could
> also be fixed :)
> I'll send a patch with only the other fix.
>
> Eric
>

I've found in the ulpi specification that there are some bits in the
OTG Control Register that are set by default after power-on.
Current ulpi driver can only set those bits and not clear them,
making it impossible for the platform to clear them even at
initialization time.
So, after all v2 of your patch is correct.
Sorry, for not seeing this at first time :( , so

Acked-by: Igor Grinberg <grinberg@compulab.co.il>

-- 
Regards,
Igor.

[PATCH v2] otg/ulpi.c : fix register write

[Igor Grinberg]

Greg,

I have a patch set, I want to submit in the very near future and it
relies on this one.
Will this patch get to your tree?

> ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write
> to the PHY's registers, which means we can only set bits in the PHY's
> register and not clear them.
> By directly using the address of the register without any offset, we
> now get the expected behaviour for these functions.
>
> Signed-off-by: Eric B?nard <eric@eukrea.com>
> Cc: Daniel Mack <daniel@caiaq.de>
> Cc: linux-usb at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> ---
> v2 :
> drop reg/val swap which is handled in a better way by Igor's patch here :
> http://comments.gmane.org/gmane.linux.usb.general/32092 
>  
>  drivers/usb/otg/ulpi.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
> index b1b3469..086853b 100644
> --- a/drivers/usb/otg/ulpi.c
> +++ b/drivers/usb/otg/ulpi.c
> @@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
>  	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
>  		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
>  
> -	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>  }
>  
>  static int ulpi_init(struct otg_transceiver *otg)
> @@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
>  			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
>  	}
>  
> -	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>  }
>  
>  struct otg_transceiver *
>   

-- 
Regards,
Igor.

[PATCH v2] otg/ulpi.c : fix register write

[Greg KH]

On Wed, Jul 14, 2010 at 05:22:59PM +0300, Igor Grinberg wrote:
> Greg,
> 
> I have a patch set, I want to submit in the very near future and it
> relies on this one.
> Will this patch get to your tree?

Odd, I don't see it in my "to-apply" queue.  Did you cc: me on it?  Care
to resend it if you want me to apply it?

thanks,

greg k-h

[PATCH v2] otg/ulpi.c : fix register write

[Igor Grinberg]

>
> On Wed, Jul 14, 2010 at 05:22:59PM +0300, Igor Grinberg wrote:
>   
>> Greg,
>>
>> I have a patch set, I want to submit in the very near future and it
>> relies on this one.
>> Will this patch get to your tree?
>>     
> Odd, I don't see it in my "to-apply" queue.  Did you cc: me on it?  Care
> to resend it if you want me to apply it?
>
>   

Well, you were in "To:", when Eric sent it the first time.
Nevertheless, Eric does not seem to care resending it.
I can send it along with my patch set (with Eric's Signed-off).
Also, this patch will make more sense then.

> thanks,
>
> greg k-h
>
>   

-- 
Regards,
Igor.

[PATCH v2] otg/ulpi.c : fix register write

[Eric Bénard]

ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write
to the PHY's registers, which means we can only set bits in the PHY's
register and not clear them.
By directly using the address of the register without any offset, we
now get the expected behaviour for these functions.

Signed-off-by: Eric B?nard <eric@eukrea.com>
Cc: Daniel Mack <daniel@caiaq.de>
Cc: linux-usb at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: Sascha Hauer <kernel@pengutronix.de>
---
v2 :
drop reg/val swap which is handled in a better way by Igor's patch here :
http://comments.gmane.org/gmane.linux.usb.general/32092 
 
 drivers/usb/otg/ulpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
index b1b3469..086853b 100644
--- a/drivers/usb/otg/ulpi.c
+++ b/drivers/usb/otg/ulpi.c
@@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
 		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 static int ulpi_init(struct otg_transceiver *otg)
@@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
 	}
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 struct otg_transceiver *
-- 
1.6.3.3

[PATCH v2] otg/ulpi.c : fix register write

[Igor Grinberg]

Greg,

This was a resend, you've asked...
Can you please update us on its status?

Thanks

> ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write
> to the PHY's registers, which means we can only set bits in the PHY's
> register and not clear them.
> By directly using the address of the register without any offset, we
> now get the expected behaviour for these functions.
>
> Signed-off-by: Eric B?nard <eric@eukrea.com>
> Cc: Daniel Mack <daniel@caiaq.de>
> Cc: linux-usb at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> ---
> v2 :
> drop reg/val swap which is handled in a better way by Igor's patch here :
> http://comments.gmane.org/gmane.linux.usb.general/32092 
>  
>  drivers/usb/otg/ulpi.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
> index b1b3469..086853b 100644
> --- a/drivers/usb/otg/ulpi.c
> +++ b/drivers/usb/otg/ulpi.c
> @@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
>  	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
>  		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
>  
> -	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>  }
>  
>  static int ulpi_init(struct otg_transceiver *otg)
> @@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
>  			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
>  	}
>  
> -	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>  }
>  
>  struct otg_transceiver *
>   

-- 
Regards,
Igor.