Igor/Marek,
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, September 05, 2012 1:52 AM To: Igor Grinberg Cc: Lucas Stach; u-boot@lists.denx.de; Stephen Warren; Tom Warren Subject: Re: [PATCH v2 4/5] usb: ulpi: add indicator configuration function
Dear Igor Grinberg,
Hi Lucas, Tom,
I'm sorry for the late reply. I understand, that Tom has already applied this to tegra/next, but as the changes/follow up patches are required, may be we can do this in another fashion...
- Thanks for the patch and working on extending the generic framework!
- This patch has no dependencies on tegra specific patches, so I think, it should go through Marex usb tree, but doing this will require the right merge order, so bisectability will not suffer. So, Marek, Tom, you should decide which way is fine with you both.
I'm not sure how the USB and Tegra repos can coordinate on patches like this, since I don't pull from/rebase against USB, and AFAIK Marek doesn't reference Tegra when he updates his repo. I'm a sub-repo of ARM, which is a sub-repo of TOT (u-boot/master). What I usually do (and have always done) is to take the entire patchset that includes a Tegra component (USB, mmc, SPI, etc.) and hope (pray?) that anyone merging my changes upstream of me will be able to resolve the conflicts/pre-existing patches. So far, I haven't heard from anyone (Albert or Wolfgang) that's had a problem with that, perhaps because it's pretty rare. AFAICT, there's no other procedure outlined in the U-Boot wiki custodian's page. If there's a better procedure I should be following, let's get it documented and I'll be glad to hew to the line. I'm still on the learning curve for git merging, rebasing, etc.
_ALWAYS_ CC the right custodians. That is, me. Seeing this patch bypassed me completely, I'm really unhappy.
Tom, Yesterday, I was wondering if the patch was already applied, and I had no clue what's its status. Also, the patchwork says "New". So, if it is not hard for you in the future, I'd like a short reply to the list, saying something like: "Applied, thanks.", like most custodians do. Thanks!
+1
I _do_ owe the list, and the patch's author(s) an 'applied' message, as Igor points out. I've been viewing u-boot-tegra/next as just a staging area for patches that'll eventually go into master, and I've been copying patch authors and Tegra devs on pull requests, but I'll also send out a notice in the future when I apply a patch to /next.
Thanks,
Tom
On 08/21/12 23:18, Lucas Stach wrote:
Allows for easy configuration of the VBUS indicator related ULPI config bits.
Also move the external indicator setup from ulpi_set_vbus() to the new function.
Signed-off-by: Lucas Stach dev@lynxeye.de
After the below comments are fixed: Acked-by: Igor Grinberg grinberg@compulab.co.il
drivers/usb/ulpi/ulpi.c | 26 ++++++++++++++++++++++---- include/usb/ulpi.h | 13 +++++++++++-- 2 Dateien geändert, 33 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)
diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c index dde2585..f358bde 100644 --- a/drivers/usb/ulpi/ulpi.c +++ b/drivers/usb/ulpi/ulpi.c @@ -106,20 +106,38 @@ int ulpi_select_transceiver(struct ulpi_viewport *ulpi_vp, unsigned speed)
return ulpi_write(ulpi_vp, &ulpi->function_ctrl, val);
}
-int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power,
int ext_ind)+int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int +ext_power)
{
u32 flags = ULPI_OTG_DRVVBUS; u8 *reg = on ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
if (ext_power)
flags |= ULPI_OTG_DRVVBUS_EXT;
if (ext_ind)
flags |= ULPI_OTG_EXTVBUSIND;return ulpi_write(ulpi_vp, reg, flags);
}
+int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int
external,
int passthu, int complement)+{
- u8 *reg;
- int ret;
- reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
- if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
return ret;- reg = passthu ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
- if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
return ret;- reg = complement ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
- if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
return ret;These are fine, two requests though:
- As Tom already pointed in the private email:
ERROR: do not use assignment in if condition #361: FILE: drivers/usb/ulpi/ulpi.c:127:
- if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
ERROR: do not use assignment in if condition #365: FILE: drivers/usb/ulpi/ulpi.c:131:
- if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
ERROR: do not use assignment in if condition #369: FILE: drivers/usb/ulpi/ulpi.c:135:
- if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
those must be fixed.
Agreed, _ALWAYS_ run checkpatch.pl before submitting. It's even better idea to add a git precommit hook for that.
- Can you make only one access for each register? Use flags/val variable (like in other places) and do only one access per register. Can you?
- return 0;
+}
int ulpi_set_pd(struct ulpi_viewport *ulpi_vp, int enable) {
u32 val = ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN;
diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h index 9a75c24..99166c4 100644 --- a/include/usb/ulpi.h +++ b/include/usb/ulpi.h @@ -61,8 +61,17 @@ int ulpi_select_transceiver(struct ulpi_viewport *ulpi_vp, unsigned speed);
- returns 0 on success, ULPI_ERROR on failure.
*/
-int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp,
int on, int ext_power, int ext_ind);+int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int +ext_power);
+/*
- Configure VBUS indicator
- @external - external VBUS over-current indicator is used
- @passthru - disables ANDing of internal VBUS comparator
with external VBUS input
- @complement - inverts the external VBUS input
- */
+int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int
external,
int passthru, int complement);/*
- Enable/disable pull-down resistors on D+ and D- USB lines.