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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
