linux-kernel - Re: [PATCH] USB: phy: re-organize tegra phy driver

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]

Message-ID: <50516264.5050303@wwwdotorg.org>
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/