[U-Boot] [PATCH v2 4/5] usb: ulpi: add indicator configuration function
Tom Warren
TWarren at nvidia.com
Wed Sep 5 18:25:15 CEST 2012
Igor/Marek,
> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Wednesday, September 05, 2012 1:52 AM
> To: Igor Grinberg
> Cc: Lucas Stach; u-boot at 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 <dev at lynxeye.de>
> >
> > After the below comments are fixed:
> > Acked-by: Igor Grinberg <grinberg at 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
More information about the U-Boot
mailing list