lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 12 Sep 2012 22:34:44 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Venu Byravarasu <vbyravarasu@...dia.com>
CC:	"balbi@...com" <balbi@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH] USB: phy: re-organize tegra phy driver

On 09/12/2012 10:16 PM, Venu Byravarasu wrote:
> Forgot to address some of the comments made by stephen, in my previous update.
> Hence addressing them now.
> Thanks a lot Stephen, for detailed review.

OK, so since this patch is basically just splitting the file into
multiple parts, you can ignore most of my review comments for this patch
and consider them as input for things in future cleanup patches.

One comment below:

> Stephen Warren wrote at Thursday, September 13, 2012 12:06 AM:
>> On 09/12/2012 04:58 AM, Venu Byravarasu wrote:
...
>>> +static int	tegra2_usb_phy_open(struct tegra_usb_phy *phy)
>>> +{
>>> +	struct tegra_ulpi_config *ulpi_config;
>>> +	int err;
>>> +
>>> +	if (phy_is_ulpi(phy)) {
>>
>> Similarly, this seems like it'd be better as two separate functions,
>> since there's already an op defined for open.
> 
> tegra2_usb_phy_open is called via open of ops only. 
> Plz let me know if you still have any concern here.

What I meant was the body of this function is:

tegra2_usb_phy_open:
    if (ulpi)
        do a bunch of ULPI stuff
    else
        do a bunch of UTMI stuff

It's seems it'd be simpler to split this into two functions:

tegra2_usb_ulpi_phy_open:
    do a bunch of ULPI stuff

tegra2_usb_utmi_phy_open:
    do a bunch of UTMI stuff

... and have the code that initializes the ops assign the appropriate
one of those two functions into the open op, just like it does for
all/most other ops.

Still, this may come under the same argument as above; fuel for future
cleanup since the existing code already works this way?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists