* [PATCH] otg/ulpi.c : fix register write
@ 2010-06-23 14:50 Eric Bénard
2010-06-23 14:58 ` Daniel Mack
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eric Bénard @ 2010-06-23 14:50 UTC (permalink / raw)
To: linux-arm-kernel
* drivers/usb/otg/ulpi.c :
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.
* this patch also keep usage of otg_io_write & ulpi parameters coherent
as in include/linux/usb/otg.h we have :
otg_io_write(struct otg_transceiver *otg, u32 reg, u32 val)
so keep the same parameters order in drivers/usb/otg/ulpi.c and
arch/arm/plat-mxc/ulpi.c.
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>
---
arch/arm/plat-mxc/ulpi.c | 2 +-
drivers/usb/otg/ulpi.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/plat-mxc/ulpi.c b/arch/arm/plat-mxc/ulpi.c
index 582c6df..84eb5c7 100644
--- a/arch/arm/plat-mxc/ulpi.c
+++ b/arch/arm/plat-mxc/ulpi.c
@@ -83,7 +83,7 @@ static int ulpi_read(struct otg_transceiver *otg, u32 reg)
return (__raw_readl(view) >> ULPIVW_RDATA_SHIFT) & ULPIVW_RDATA_MASK;
}
-static int ulpi_write(struct otg_transceiver *otg, u32 val, u32 reg)
+static int ulpi_write(struct otg_transceiver *otg, u32 reg, u32 val)
{
int ret;
void __iomem *view = otg->io_priv;
diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
index b1b3469..f6048ca 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, ULPI_OTG_CTRL, flags);
}
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, ULPI_OTG_CTRL, flags);
}
struct otg_transceiver *
--
1.6.3.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] otg/ulpi.c : fix register write
2010-06-23 14:50 [PATCH] otg/ulpi.c : fix register write Eric Bénard
@ 2010-06-23 14:58 ` Daniel Mack
2010-06-23 15:07 ` Eric Bénard
2010-06-23 15:16 ` Igor Grinberg
2010-06-23 15:20 ` Igor Grinberg
2 siblings, 1 reply; 6+ messages in thread
From: Daniel Mack @ 2010-06-23 14:58 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 23, 2010 at 04:50:32PM +0200, Eric B?nard wrote:
> * drivers/usb/otg/ulpi.c :
> 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.
>
> * this patch also keep usage of otg_io_write & ulpi parameters coherent
> as in include/linux/usb/otg.h we have :
> otg_io_write(struct otg_transceiver *otg, u32 reg, u32 val)
> so keep the same parameters order in drivers/usb/otg/ulpi.c and
> arch/arm/plat-mxc/ulpi.c.
>
> 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>
> ---
> arch/arm/plat-mxc/ulpi.c | 2 +-
> drivers/usb/otg/ulpi.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/plat-mxc/ulpi.c b/arch/arm/plat-mxc/ulpi.c
> index 582c6df..84eb5c7 100644
> --- a/arch/arm/plat-mxc/ulpi.c
> +++ b/arch/arm/plat-mxc/ulpi.c
> @@ -83,7 +83,7 @@ static int ulpi_read(struct otg_transceiver *otg, u32 reg)
> return (__raw_readl(view) >> ULPIVW_RDATA_SHIFT) & ULPIVW_RDATA_MASK;
> }
>
> -static int ulpi_write(struct otg_transceiver *otg, u32 val, u32 reg)
> +static int ulpi_write(struct otg_transceiver *otg, u32 reg, u32 val)
Urgs. Is this really necessary? It's not that I have a strong opinion
about the order of arguments in such cases (I kept to the convention
of __readl() when I wrote it). But _changing_ it like this is really
confusing. Once in awhile I stumble over such API changes and I always
wonder about the reason. The problem is that not even the compiler will
warn you if you got it wrong, when you copied a sniplet from older
sources etc.
And you really want to break someone knee caps once you find out what
caused the breakage ;)
So - if we can avoid that, we should do. If anyone speaks up with a real
reason for changing it, I'd be fine :)
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] otg/ulpi.c : fix register write
2010-06-23 14:58 ` Daniel Mack
@ 2010-06-23 15:07 ` Eric Bénard
0 siblings, 0 replies; 6+ messages in thread
From: Eric Bénard @ 2010-06-23 15:07 UTC (permalink / raw)
To: linux-arm-kernel
Le 23/06/2010 16:58, Daniel Mack a ?crit :
> On Wed, Jun 23, 2010 at 04:50:32PM +0200, Eric B?nard wrote:
>> -static int ulpi_write(struct otg_transceiver *otg, u32 val, u32 reg)
>> +static int ulpi_write(struct otg_transceiver *otg, u32 reg, u32 val)
>
> Urgs. Is this really necessary? It's not that I have a strong opinion
> about the order of arguments in such cases (I kept to the convention
> of __readl() when I wrote it). But _changing_ it like this is really
> confusing. Once in awhile I stumble over such API changes and I always
> wonder about the reason. The problem is that not even the compiler will
> warn you if you got it wrong, when you copied a sniplet from older
> sources etc.
>
> And you really want to break someone knee caps once you find out what
> caused the breakage ;)
>
> So - if we can avoid that, we should do. If anyone speaks up with a real
> reason for changing it, I'd be fine :)
>
No problem if you prefer to keep as it is (this is "cosmetic" but val
and reg are reversed in include/linux/usb/otg.h which I took as the
reference in the present case).
Do you have any opinion on the other part of the patch which removes
ULPI_SET as this prevents to clear bits in the phy register ? If ok for
you, I send a revised patch containing only this change.
Thanks,
Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] otg/ulpi.c : fix register write
2010-06-23 14:50 [PATCH] otg/ulpi.c : fix register write Eric Bénard
2010-06-23 14:58 ` Daniel Mack
@ 2010-06-23 15:16 ` Igor Grinberg
2010-06-23 15:20 ` Igor Grinberg
2 siblings, 0 replies; 6+ messages in thread
From: Igor Grinberg @ 2010-06-23 15:16 UTC (permalink / raw)
To: linux-arm-kernel
On 06/23/10 17:50, Eric B?nard wrote:
> * drivers/usb/otg/ulpi.c :
> 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.
>
> * this patch also keep usage of otg_io_write & ulpi parameters coherent
> as in include/linux/usb/otg.h we have :
> otg_io_write(struct otg_transceiver *otg, u32 reg, u32 val)
>
and also:
struct otg_io_access_ops {
int (*read)(struct otg_transceiver *otg, u32 reg);
<http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=include/linux/usb/otg.h;h=f8302d036a7674e33109bf8fce16d67325d2d17a;hb=HEAD#l60>
int (*write)(struct otg_transceiver *otg, u32 val, u32 reg);
};
and please, look here:
http://comments.gmane.org/gmane.linux.usb.general/32092
> so keep the same parameters order in drivers/usb/otg/ulpi.c and
> arch/arm/plat-mxc/ulpi.c.
>
> 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>
> ---
> arch/arm/plat-mxc/ulpi.c | 2 +-
> drivers/usb/otg/ulpi.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/plat-mxc/ulpi.c b/arch/arm/plat-mxc/ulpi.c
> index 582c6df..84eb5c7 100644
> --- a/arch/arm/plat-mxc/ulpi.c
> +++ b/arch/arm/plat-mxc/ulpi.c
> @@ -83,7 +83,7 @@ static int ulpi_read(struct otg_transceiver *otg, u32 reg)
> return (__raw_readl(view) >> ULPIVW_RDATA_SHIFT) & ULPIVW_RDATA_MASK;
> }
>
> -static int ulpi_write(struct otg_transceiver *otg, u32 val, u32 reg)
> +static int ulpi_write(struct otg_transceiver *otg, u32 reg, u32 val)
> {
> int ret;
> void __iomem *view = otg->io_priv;
> diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
> index b1b3469..f6048ca 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, ULPI_OTG_CTRL, flags);
> }
>
> 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, ULPI_OTG_CTRL, flags);
> }
>
> struct otg_transceiver *
>
--
Regards,
Igor.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] otg/ulpi.c : fix register write
2010-06-23 14:50 [PATCH] otg/ulpi.c : fix register write Eric Bénard
2010-06-23 14:58 ` Daniel Mack
2010-06-23 15:16 ` Igor Grinberg
@ 2010-06-23 15:20 ` Igor Grinberg
2010-06-23 15:22 ` Eric Bénard
2 siblings, 1 reply; 6+ messages in thread
From: Igor Grinberg @ 2010-06-23 15:20 UTC (permalink / raw)
To: linux-arm-kernel
Sorry for that link inside the code, should be:
On 06/23/10 17:50, Eric B?nard wrote:
> * drivers/usb/otg/ulpi.c :
> 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.
>
> * this patch also keep usage of otg_io_write & ulpi parameters coherent
> as in include/linux/usb/otg.h we have :
> otg_io_write(struct otg_transceiver *otg, u32 reg, u32 val)
>
and also:
struct otg_io_access_ops {
int (*read)(struct otg_transceiver *otg, u32 reg);
int (*write)(struct otg_transceiver *otg, u32 val, u32 reg);
};
and please, look here:
http://comments.gmane.org/gmane.linux.usb.general/32092
> so keep the same parameters order in drivers/usb/otg/ulpi.c and
> arch/arm/plat-mxc/ulpi.c.
>
> 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>
> ---
> arch/arm/plat-mxc/ulpi.c | 2 +-
> drivers/usb/otg/ulpi.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/plat-mxc/ulpi.c b/arch/arm/plat-mxc/ulpi.c
> index 582c6df..84eb5c7 100644
> --- a/arch/arm/plat-mxc/ulpi.c
> +++ b/arch/arm/plat-mxc/ulpi.c
> @@ -83,7 +83,7 @@ static int ulpi_read(struct otg_transceiver *otg, u32 reg)
> return (__raw_readl(view) >> ULPIVW_RDATA_SHIFT) & ULPIVW_RDATA_MASK;
> }
>
> -static int ulpi_write(struct otg_transceiver *otg, u32 val, u32 reg)
> +static int ulpi_write(struct otg_transceiver *otg, u32 reg, u32 val)
> {
> int ret;
> void __iomem *view = otg->io_priv;
> diff --git a/drivers/usb/otg/ulpi.c b/drivers/usb/otg/ulpi.c
> index b1b3469..f6048ca 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, ULPI_OTG_CTRL, flags);
> }
>
> 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, ULPI_OTG_CTRL, flags);
> }
>
> struct otg_transceiver *
>
--
Regards,
Igor.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] otg/ulpi.c : fix register write
2010-06-23 15:20 ` Igor Grinberg
@ 2010-06-23 15:22 ` Eric Bénard
0 siblings, 0 replies; 6+ messages in thread
From: Eric Bénard @ 2010-06-23 15:22 UTC (permalink / raw)
To: linux-arm-kernel
Hi Igor,
Le 23/06/2010 17:20, Igor Grinberg a ?crit :
> and also:
>
> struct otg_io_access_ops {
> int (*read)(struct otg_transceiver *otg, u32 reg);
> int (*write)(struct otg_transceiver *otg, u32 val, u32 reg);
> };
>
> and please, look here:
> http://comments.gmane.org/gmane.linux.usb.general/32092
>
OK, you took the problem from the right side :-)
Thanks
Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-06-23 15:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-23 14:50 [PATCH] otg/ulpi.c : fix register write Eric Bénard
2010-06-23 14:58 ` Daniel Mack
2010-06-23 15:07 ` Eric Bénard
2010-06-23 15:16 ` Igor Grinberg
2010-06-23 15:20 ` Igor Grinberg
2010-06-23 15:22 ` Eric Bénard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).