Igor/Marek, > -----Original Message----- > From: Marek Vasut [mailto:ma...@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... > > > > 1) Thanks for the patch and working on extending the generic framework! > > 2) 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 <d...@lynxeye.de> > > > > After the below comments are fixed: > > Acked-by: Igor Grinberg <grinb...@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: > > 1) 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. > > > 2) 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. -- nvpublic _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot