Re: [PATCH v4 1/9] usb: cdns3: introduce cdns3_set_phy_power API

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

Greg KH <gregkh@linuxfoundation.org> writes:

> 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@nxp.com>
>> Signed-off-by: Peter Chen <peter.chen@nxp.com>
>> ---
>>  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.

the cdns3_ prefix helps when using function trace or function graph
trace. It's a lot easier to say "trace anything matching cdns3_*" than
it is to remember each function ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]