On Fri, Jul 03, 2020 at 02:26:45PM +0800, Peter Chen wrote:
> Since we have both USB2 and USB3 PHYs for cdns3 controller, it is
> better we have a unity API to handle both USB2 and USB3's power, it
> could simplify code for error handling and further power management
> implementation.
>
> Reviewed-by: Jun Li <jun.li@xxxxxxx>
> Signed-off-by: Peter Chen <peter.chen@xxxxxxx>
> ---
> drivers/usb/cdns3/core.c | 44 ++++++++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 19bbb5b7e6b6..bfd09aa98c12 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
> return ret;
> }
>
> +static int cdns3_set_phy_power(struct cdns3 *cdns, bool on)
Please just make function calls self-describing, instead of having to
try to remember what a specific flag means. This isn't as bad as some
functions, but the general idea remains, this should be 2 functions:
set_phy_power_off()
set_phy_power_on()
no need for the cdns3_ prefix, it's a static function.
Then have those two functions call a helper if you really want to, but
that means that reading where those functions are called is obvious and
no need to backtrack to where this was declared to determine that the
second parameter meant on/off and not is_disabled/not_disabled or
something crazy like that.
> +{
> + int ret = 0;
> +
> + if (on) {
> + ret = phy_power_on(cdns->usb2_phy);
See, phy gets it right.
> + if (ret)
> + return ret;
> +
> + ret = phy_power_on(cdns->usb3_phy);
> + if (ret) {
> + phy_power_off(cdns->usb2_phy);
> + return ret;
> + }
> + } else {
> + phy_power_off(cdns->usb3_phy);
> + phy_power_off(cdns->usb2_phy);
Ick, even worse, this needs to be 2 separate functions as there is NO
common code path here at all.
thanks,
greg k-h
