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 <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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
