On Thu, Sep 9, 2010 at 15:49, Steve deRosier <steve@xxxxxxxxxxx> wrote:
> On Wed, Sep 8, 2010 at 7:06 PM, Julian Calaby <julian.calaby@xxxxxxxxx> wrote:
>> More nits:
>>
>>
>> Are these changes really necessary here? - It'd probably be cleaner to
>> change the initial functions to just take the card struct instead? (as
>> it's always available as priv->card)
>>
>> I'm guessing you're going to do something with the _* versions of
>> these functions later, so should this change be there instead?
>>
>
> Yes, these changes are necessary. The enable and disable interrupt
> calls are called both from main.c as well as sdio.c. main.c has
> access to and needs to call with a priv, while at least one of the
> calls from sdio.c happens before priv exists. I could have changed
> the stuff in main.c to call with priv->card, but alas at the time I
> was still trying to minimize structural changes main.c and avoiding
> changes to the USB driver.
I'm no maintainer, but my opinion is that it's better to make
(sensible) sweeping changes to a code base, then add the changes you
want to make, rather than adding extra functions to access a
particular piece of functionality in different ways.
>>> @@ -187,7 +201,6 @@ static int if_sdio_handle_cmd(struct if_sdio_card *card,
>>> struct lbtf_private *priv = card->priv;
>>> int ret;
>>> unsigned long flags;
>>> - u8 i;
>>>
>>> lbtf_deb_enter(LBTF_DEB_SDIO);
>>>
>>
>> Shouldn't this be elsewhere (maybe rolled into patch #1?)
>>
>
> It could be, but I didn't do this cleanup till now, some 20 commits
> later. Not that easy to pick and choose which lines from an atomic
> commit go to which patch some weeks later.
I know it isn't - I'm doing exactly that for a project I'm working on,
and my MO is to split out such changes, then roll them into more
appropriate patches later. (e.g. if I'm making the foo() function do
bar work, and, in the process of doing this, fix a whole stack of
stupid code in it, I'll commit the latter first, then the former, then
roll the changes into more appropriate patches - even if that means
more work for me. - But then I am pedantic about that.) These changes
are, in a way, why I've run my particular fine tooth comb over your
patches - I'm bored =)
>>> diff --git a/drivers/net/wireless/libertas_tf/main.c b/drivers/net/wireless/libertas_tf/main.c
>>> index 119d625..14ce1bc 100644
>>> --- a/drivers/net/wireless/libertas_tf/main.c
>>> +++ b/drivers/net/wireless/libertas_tf/main.c
>>> @@ -150,14 +150,15 @@ static int lbtf_setup_firmware(struct lbtf_private *priv)
>>> int ret = -1;
>>>
>>> lbtf_deb_enter(LBTF_DEB_FW);
>>> +
>>> /*
>>> * Read priv address from HW
>>> */
>>> memset(priv->current_addr, 0xff, ETH_ALEN);
>>> ret = lbtf_update_hw_spec(priv);
>>> if (ret) {
>>> - ret = -1;
>>> - goto done;
>>> + ret = -1;
>>> + goto done;
>>> }
>>>
>>> lbtf_set_mac_control(priv);
>>
>> Whitespace change noise.
>
> Well, perhaps. On the other hand, it made it more readable to me (the
> first one, the newline. The indentation change is actually incorrect,
> no doubt about it). Have you ever worked with a codebase for 3 some
> months and not incidentally or accidentally changed some whitespace?
> If this patch was just to change whitespace, then I'd agree with you
> and wouldn't have wasted time bothering to submit it, but the patch as
> a whole adds necessary functionality.
Yes, I have. (and it was over 3 years in my case)
>From what I understand, it's expected that white space changes and
other trivial cleanups get their own patches, mainly so that it's
easier to review separate patches. E.g. If I change the parameters
passed to bar(), and in the process lean up the whitespace in one of
the functions that calls it, those extra changes, whilst not that
significant, can make the patch harder to read. Some of the SCSI
maintainers get particularly pedantic about this, (with occasional
discussions on whether a piece of whitespace should or should not be
changed) but I expect the wireless folks are a little more relaxed. =)
I'm not meaning to lean on you over all of this, just running my eyes
over these patches.
Thanks,
--
Julian Calaby
Email: julian.calaby@xxxxxxxxx
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
