Re: [zd1211-devs] Re: [patch] updates to zd1211ng for 2.6(.11)
Status: Beta
Brought to you by:
mayne
Menu
▾
▴
Re: [zd1211-devs] Re: [patch] updates to zd1211ng for 2.6(.11)
|
From: Roman K <rom...@ma...> - 2005-06-21 00:24:50
|
Hi,
> Hi Dmitriy,
>
> I think your mailer is line-wrapping the code lines, maybe you could turn
> that off for emails with code in them?
>
> On Mon, 20 Jun 2005, Dmitriy Korovkin wrote:
>
> >4. I don't see a reason to remove locks. They are reserved for the
> >future implementation. REJECTED.
>
> It doesn't matter to me either way, however I'll just say you can ALWAYS
> add locks in when you DO need them. I don't see the point of having locks
> when you aren't using them.
The code is under development and lock would be needed in the near future.
It is not bad to write a code with one step ahead.
> >5. I also don't see a reason in changes like this:
> >- struct usb_interface_descriptor *iface_desc; //interface
> >descriptor
> > struct usb_endpoint_descriptor *endpoint; //endpoint
> >descriptor
> >+ __u8 bNumEndpoints =
> >CUR_ALTSETTING(interface).bNumEndpoints;
> >Variable you have removed may be used in the future.
> >REJECTED.
>
> I'm pretty sure you'll need to change the method of access the USB
> topology items, as it's different in 2.6. Do you have a different
> suggestion for making the driver compile/work with the 2.6 kernel?
I think this will be accepted in some form.
> >6. I also don't see a reason in changes like this:
> >- if (val == 0x04 || val == 0x07)
> >{
> >- debug("AiroHa
> >RF");
> >-
> >}
> >
> >- else if (val == 0x09)
> >{
> >- debug("GCT
> >RF");
> >-
> >}
> >
> >- else if (val == RFMD_RF)
> >{
> >- debug("RFMD
> >RF");
> >-
> >}
> >
> >- else
> >{
> >
> >- debug("Unknown RF - 0x%X",
> >val);
> >+ switch (priv->ieee.rfMode)
> >{
> >+ case RF_AIROHA: debug("Airoha RF");
> >break;
> >+ case RF_MAXIM: debug("Maxim RF");
> >break;
> >+ case RF_RFMD: debug("RFMD RF");
> >break;
> >+ case 0x9: debug("GCT RF?");
> >break;
> >+ default: debug("Unknown RF : 0x%x",
> >priv->ieee.rfMode);
> >REJECTED.
>
> :-)
>
> The point is you shouldn't use "magic numbers" in code. It's much better
> to define constants and use those. There are (at least) 2 benefits to it:
> 1) you have a single place to change a constant, if you ever need to
> 2) when reviewing code, it's much easier to understand the code if
> constant variables (with descriptive names) are used instead of mysterious
> "magic numbers".
>
> As far as if vs. switch, I just think the switch statement was easier that
> if, but it doesn't really matter.
This would be also accepted after checking for some lost code.
> >7. I also don't understand, what did you want to do with these changes
> >in callbacks:
> >- if
> >(!priv->usb_dev)
> >
> >-
> >return;
> >
> > priv->macCmdResult =
> >uurb->status;
> >- if (uurb->status != 0 &&
> >waitqueue_active(&priv->regwq))
> >-
> >wake_up_interruptible(&priv->regwq);
> >
> >+ priv->reg_rw_sent =
> >1;
> >+
> >wake_up_all(&priv->regwq);
> >
> >
> >And these:
> >-
> >interruptible_sleep_on(&priv->regwq);
> >
> >+ wait_event_interruptible(priv->regwq, priv->reg_rw_sent &&
> >priv->resp_received);
> >Please, explain why your solution is better, so I'll make a decision.
> >DELAYED.
>
> Ok, I'll explain what you have above, let me know if there are other parts
> you would like more explanation for.
>
> I removed the bailout on "!priv->usb_dev" as just because the device is
> gone is no reason to stop the callback. In fact, it is quite important
> to wake up the code sleeping on the waitqueue, even if the device is
> gone. You could add it before the URB resubmission, as there is no reason
> to resubmit if the device is gone.
>
> The waitqueue change is needed because simply sleeping on a waitqueue is
> deprecated, because it is racy. There is simply no way with a simple
> sleep_on() to ensure you sleep before the event happens (in which case you
> never get woken up!). So I changed it to the correct mechanism, where
> there is a flag (event) you're waiting for. There is no race in this
> case. I used only 2 flags, one for completion on ep3 (register r/w) and
> one for ep4 (responses).
Why wake_up_all() not wake_up_interruptible() or even wake_up_interruptible_all()
if you prefer '_all()'.
> >The main reason I have rejected some changes - big changes of code
> >though all the text confuses other developers. we MUST avoid operations
> >like tab -> space conversions, function names changes, reformatting and
> >constants renaming even if we don't like some of them.
>
> I have no problem following tab vs. space usage, I'll use whatever you
> would like, I actually didn't realize mine was different...I always use
> tabs. I don't think any of my patch converted tabs to spaces, did it?
> Anyway, which is the right one to use? Tabs? Or 4-space, or 8-space?
I believe that this was general remark.
Probably there is some space cleanup/addition, but this is small part of
'useless' changes. Please, do not treate it as main reason.
> As for changing function names or constants, as long as the names or
> constants are meaningful, I don't understand why you are against changing
> them, if the new name is more consistent or meaningful than the old
> name. Are there any examples, maybe that would make it clearer to me...?
There was one, please refer to the original reply by Dmitriy.
> >Let's follow the old army principle "ugly but stable".
>
> Well, I personally have always thought those 2 adjectives are mutually
> exclusive when being used to describe code. :-)
> I think ugly code will always be less stable, and (more importantly) less
> maintainable, than clean code. Especially when people who didn't write
> the code try to understand and update the code. But, this is much more
> your project than mine so you make the call ;-)
> >But anyway, thank you very much for your work. It's very usefull. But as
> >I mentioned previously, ng driver is in experimental stage, so IMHO it's
> >better for us to concentrate on making this driver working.
> >Hope to see you in the list of the developers.
>
> Sure, I'll get the latest CVS code and keep trying to get it working on
> 2.6.
>
>
> --
> Dan Streetman
> dds...@ie...
> ---------------------
> 186,272 miles per second:
> It isn't just a good idea, it's the law!
|