otg/ulpi.c : fix register write

PATCH New Eric Bénard 3 participants 6 messages Jun 23, 2010

Subsystems (4)

Arm (32-bit) USB Arm sub-architectures Arm i.MX and Layerscape

lore

Share & tools

lore

Eric Bénard

author

2010-06-23 14:50 UTC #0 lore raw

PATCH 1/1 otg/ulpi.c : fix register write

* 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 <[email protected]>

Cc Daniel Mack <[email protected]>

Cc linux-usb at vger.kernel.org

Cc linux-arm-kernel at lists.infradead.org

Cc Sascha Hauer <[email protected]>

---
 arch/arm/plat-mxc/ulpi.c |    2 +-
 drivers/usb/otg/ulpi.c   |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

arch/arm/plat-mxc/ulpi.c +1 -1

index 582c6df..84eb5c7 100644
@@ -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;

drivers/usb/otg/ulpi.c +2 -2

index b1b3469..f6048ca 100644
@@ -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 *

Signature · 1 line

1.6.3.3

Daniel Mack

2010-06-23 14:58 UTC #1 lore raw

PATCH 1/1 otg/ulpi.c : fix register write

Quoted 28 lines · Eric B?nard

* 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 <[email protected]>
Cc Daniel Mack <[email protected]>
Cc linux-usb at vger.kernel.org
Cc linux-arm-kernel at lists.infradead.org
Cc Sascha Hauer <[email protected]>
---
 arch/arm/plat-mxc/ulpi.c |    2 +-
 drivers/usb/otg/ulpi.c   |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
arch/arm/plat-mxc/ulpi.c +0 -0
index 582c6df..84eb5c7 100644

arch/arm/plat-mxc/ulpi.c static int ulpi_read(struct otg_transceiver *otg, u32 reg) · line 86

Quoted 5 lines

arch/arm/plat-mxc/ulpi.c +1 -1

@@ -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

Eric Bénard

author

2010-06-23 15:07 UTC #2 lore raw

PATCH 1/1 otg/ulpi.c : fix register write

Le 23/06/2010 16:58, Daniel Mack a ?crit :

Quoted 12 lines · Eric B?nard

arch/arm/plat-mxc/ulpi.c static int ulpi_read(struct otg_transceiver *otg, u32 reg) · line 86
Quoted 2 lines
arch/arm/plat-mxc/ulpi.c +1 -1
-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.

Quoted 6 lines

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

Igor Grinberg

2010-06-23 15:16 UTC #3 lore raw

PATCH 1/1 otg/ulpi.c : fix register write

In reply to Eric Bénard

Quoted 11 lines · Eric B?nard

* 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

Quoted 49 lines

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 <[email protected]>

Cc Daniel Mack <[email protected]>

Cc linux-usb at vger.kernel.org

Cc linux-arm-kernel at lists.infradead.org

Cc Sascha Hauer <[email protected]>

---
 arch/arm/plat-mxc/ulpi.c |    2 +-
 drivers/usb/otg/ulpi.c   |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

arch/arm/plat-mxc/ulpi.c +1 -1

index 582c6df..84eb5c7 100644
@@ -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;

drivers/usb/otg/ulpi.c +2 -2

index b1b3469..f6048ca 100644
@@ -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 *

Signature · 2 lines

Regards,
Igor.

Igor Grinberg

2010-06-23 15:20 UTC #4 lore raw

PATCH 1/1 otg/ulpi.c : fix register write

In reply to Eric Bénard

Sorry for that link inside the code, should be:

Quoted 11 lines · Eric B?nard

* 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

Quoted 49 lines

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 <[email protected]>

Cc Daniel Mack <[email protected]>

Cc linux-usb at vger.kernel.org

Cc linux-arm-kernel at lists.infradead.org

Cc Sascha Hauer <[email protected]>

---
 arch/arm/plat-mxc/ulpi.c |    2 +-
 drivers/usb/otg/ulpi.c   |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

arch/arm/plat-mxc/ulpi.c +1 -1

index 582c6df..84eb5c7 100644
@@ -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;

drivers/usb/otg/ulpi.c +2 -2

index b1b3469..f6048ca 100644
@@ -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 *

Signature · 2 lines

Regards,
Igor.

Eric Bénard

author

2010-06-23 15:22 UTC #5 lore raw

PATCH 1/1 otg/ulpi.c : fix register write

Hi Igor,

Le 23/06/2010 17:20, Igor Grinberg a ?crit :

Quoted 10 lines

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