| Message ID | 1348612532-24419-2-git-send-email-dev@lynxeye.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
On 09/26/12 00:35, 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> > Acked-by: Igor Grinberg <grinberg@compulab.co.il> > --- > v3: Only touch each register once. Now checkpatch clean. > --- > drivers/usb/ulpi/ulpi.c | 32 ++++++++++++++++++++++++++++---- > include/usb/ulpi.h | 13 +++++++++++-- > 2 Dateien geändert, 39 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-) > > diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c > index dde2585..98dd23c 100644 > --- a/drivers/usb/ulpi/ulpi.c > +++ b/drivers/usb/ulpi/ulpi.c > @@ -106,20 +106,44 @@ 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) > +{ > + u32 flags; > + int ret; > + > + ret = ulpi_write(ulpi_vp, > + external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear, > + ULPI_OTG_EXTVBUSIND); I think the below would be clearer and also will look as the rest of the file does: reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear; ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND); > + if (ret) > + return ret; > + > + flags = passthu ? ULPI_IFACE_PASSTHRU : 0; > + flags |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0; > + ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_set, flags); > + if (ret) > + return ret; > + > + flags = passthu ? 0 : ULPI_IFACE_PASSTHRU; > + flags |= complement ? 0 : ULPI_IFACE_EXTVBUS_COMPLEMENT; > + ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_clear, flags); Errr..., that is not what I meant... sorry for confusion. What I meant is something like: u32 pthrough = passthu ? ULPI_IFACE_PASSTHRU : 0; u32 extcompl |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0; val = ulpi_read(ulpi_vp, &ulpi->iface_ctrl); if (val == ULPI_ERROR) return val; val = (val & ~ULPI_IFACE_PASSTHRU) | pthrough; val = (val & ~ULPI_IFACE_EXTVBUS_COMPLEMENT) | extcompl; ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl, val); That way you write only once to each register and the code also look uniform. > + if (ret) > + 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.
Am Freitag, den 28.09.2012, 10:15 +0200 schrieb Igor Grinberg: > On 09/26/12 00:35, 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> > > Acked-by: Igor Grinberg <grinberg@compulab.co.il> > > --- > > v3: Only touch each register once. Now checkpatch clean. > > --- > > drivers/usb/ulpi/ulpi.c | 32 ++++++++++++++++++++++++++++---- > > include/usb/ulpi.h | 13 +++++++++++-- > > 2 Dateien geändert, 39 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-) > > > > diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c > > index dde2585..98dd23c 100644 > > --- a/drivers/usb/ulpi/ulpi.c > > +++ b/drivers/usb/ulpi/ulpi.c > > @@ -106,20 +106,44 @@ 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) > > +{ > > + u32 flags; > > + int ret; > > + > > + ret = ulpi_write(ulpi_vp, > > + external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear, > > + ULPI_OTG_EXTVBUSIND); > > I think the below would be clearer and also will look as the rest of the file does: > > reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear; > ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND); > > > + if (ret) > > + return ret; > > + > > + flags = passthu ? ULPI_IFACE_PASSTHRU : 0; > > + flags |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0; > > + ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_set, flags); > > + if (ret) > > + return ret; > > + > > + flags = passthu ? 0 : ULPI_IFACE_PASSTHRU; > > + flags |= complement ? 0 : ULPI_IFACE_EXTVBUS_COMPLEMENT; > > + ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_clear, flags); > > Errr..., that is not what I meant... sorry for confusion. > What I meant is something like: > > u32 pthrough = passthu ? ULPI_IFACE_PASSTHRU : 0; > u32 extcompl |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0; > > val = ulpi_read(ulpi_vp, &ulpi->iface_ctrl); > if (val == ULPI_ERROR) > return val; > > val = (val & ~ULPI_IFACE_PASSTHRU) | pthrough; > val = (val & ~ULPI_IFACE_EXTVBUS_COMPLEMENT) | extcompl; > ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl, val); > > That way you write only once to each register and the code also look uniform. > I tend to disagree. The ULPI PHY register set was specifically designed to not need this use-modify-write dance. Why would we like to ignore this? Yes we are possible doing one unneeded register access here, but what would it buy us to ignore the set/clear registers just to avoid one register access? > > + if (ret) > > + 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. >
Hi Lucas, On 09/28/12 16:46, Lucas Stach wrote: > Am Freitag, den 28.09.2012, 10:15 +0200 schrieb Igor Grinberg: >> On 09/26/12 00:35, 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> >>> Acked-by: Igor Grinberg <grinberg@compulab.co.il> >>> --- >>> v3: Only touch each register once. Now checkpatch clean. >>> --- >>> drivers/usb/ulpi/ulpi.c | 32 ++++++++++++++++++++++++++++---- >>> include/usb/ulpi.h | 13 +++++++++++-- >>> 2 Dateien geändert, 39 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-) [...] >>> >>> diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c >>> index dde2585..98dd23c 100644 >>> --- a/drivers/usb/ulpi/ulpi.c >>> +++ b/drivers/usb/ulpi/ulpi.c [...] >>> +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external, >>> + int passthu, int complement) >>> +{ >>> + u32 flags; >>> + int ret; >>> + >>> + ret = ulpi_write(ulpi_vp, >>> + external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear, >>> + ULPI_OTG_EXTVBUSIND); >> >> I think the below would be clearer and also will look as the rest of the file does: >> >> reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear; >> ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND); >> >>> + if (ret) >>> + return ret; >>> + >>> + flags = passthu ? ULPI_IFACE_PASSTHRU : 0; >>> + flags |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0; >>> + ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_set, flags); >>> + if (ret) >>> + return ret; >>> + >>> + flags = passthu ? 0 : ULPI_IFACE_PASSTHRU; >>> + flags |= complement ? 0 : ULPI_IFACE_EXTVBUS_COMPLEMENT; >>> + ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_clear, flags); >> >> Errr..., that is not what I meant... sorry for confusion. >> What I meant is something like: >> >> u32 pthrough = passthu ? ULPI_IFACE_PASSTHRU : 0; >> u32 extcompl |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0; >> >> val = ulpi_read(ulpi_vp, &ulpi->iface_ctrl); >> if (val == ULPI_ERROR) >> return val; >> >> val = (val & ~ULPI_IFACE_PASSTHRU) | pthrough; >> val = (val & ~ULPI_IFACE_EXTVBUS_COMPLEMENT) | extcompl; >> ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl, val); >> >> That way you write only once to each register and the code also look uniform. >> > I tend to disagree. The ULPI PHY register set was specifically designed > to not need this use-modify-write dance. Why would we like to ignore > this? We don't... I mean, we use this in cases when only one bit needs to be modified. When you need to modify more then one bit (or more precisely >2 bits), it is better be done in r-m-w manner, so you do as few ULPI bus accesses as possible. Each ULPI transaction takes much more time than playing with bits in the function. > > Yes we are possible doing one unneeded register access here, but what > would it buy us to ignore the set/clear registers just to avoid one > register access? In this function only two bits are changed, so it is still two transactions on the ULPI bus, but if for some reason, in the future, we will need to change more bits in the same function and register, we will get patches adding more transactions instead of updating the local variable. Also, the ulpi.c is done having this in mind, so for it to be uniform, we'd better do this in the same fashion. Also, I've understood from your patch, that what you have understood from my comments was not exactly what meant, so I wanted to make my comments clear and open for discussion. It does not meter now, as Marek already applied the new version. Anyway, thanks for the patches and the effort! [...]
diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c index dde2585..98dd23c 100644 --- a/drivers/usb/ulpi/ulpi.c +++ b/drivers/usb/ulpi/ulpi.c @@ -106,20 +106,44 @@ 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) +{ + u32 flags; + int ret; + + ret = ulpi_write(ulpi_vp, + external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear, + ULPI_OTG_EXTVBUSIND); + if (ret) + return ret; + + flags = passthu ? ULPI_IFACE_PASSTHRU : 0; + flags |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0; + ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_set, flags); + if (ret) + return ret; + + flags = passthu ? 0 : ULPI_IFACE_PASSTHRU; + flags |= complement ? 0 : ULPI_IFACE_EXTVBUS_COMPLEMENT; + ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_clear, flags); + if (ret) + 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.