| Message ID | 1345580337-9377-5-git-send-email-dev@lynxeye.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Marek Vasut |
| Headers | show |
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. 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! 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: 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. 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.
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. _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 > 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: > 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.
Hi Tom, Am Mittwoch, den 05.09.2012, 09:25 -0700 schrieb Tom Warren: > 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... > > > > > > 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. > I thought about how we could merge all this without loosing our sanity. I've already wrote this a bit hidden in a reply to the multi controller thread: I think it's best to handle all USB related changes through the u-boot-usb tree, as all this stuff should really be under drivers/usb. This means: I'll split out the clock output related changes, so they can go in the Tegra tree. Everything touching USB goes into the u-boot-usb tree and I'll rebase my changes accordingly. This also means commit "dm: Tegra: Staticize local functions" should be removed from the Tegra tree and move over to the USB tree. This way we won't get any build breakages and there should be no merge conflicts. It also opens the possibility to move the Tegra USB implementation to the right location in the source tree a bit later in this cycle, without messing up the merge. Thanks, Lucas
Dear Lucas Stach, > Hi Tom, > > Am Mittwoch, den 05.09.2012, 09:25 -0700 schrieb Tom Warren: > > 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... > > > > > > > > 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. > > I thought about how we could merge all this without loosing our sanity. > I've already wrote this a bit hidden in a reply to the multi controller > thread: I think it's best to handle all USB related changes through the > u-boot-usb tree, as all this stuff should really be under drivers/usb. Let's extend this a bit. Since I'm really under heavy load now, why don't you prepare a patchset (that includes all the tegra goo, stuff that Tom will drop etc) that I can pick, submit it (and Cc me) and I'll just pick it all ? That way we'll have a proper ordering and nothing will be lost and all will be tested. > This means: I'll split out the clock output related changes, so they can > go in the Tegra tree. Everything touching USB goes into the u-boot-usb > tree and I'll rebase my changes accordingly. This also means commit "dm: > Tegra: Staticize local functions" should be removed from the Tegra tree > and move over to the USB tree. Drop the dm: from it along the way. > This way we won't get any build breakages and there should be no merge > conflicts. It also opens the possibility to move the Tegra USB > implementation to the right location in the source tree a bit later in > this cycle, without messing up the merge. > > Thanks, > Lucas Best regards, Marek Vasut
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; + + 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.
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> --- drivers/usb/ulpi/ulpi.c | 26 ++++++++++++++++++++++---- include/usb/ulpi.h | 13 +++++++++++-- 2 Dateien geändert, 33 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)