R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Messaggio originale-----
> Da: [email protected]
> [mailto:[email protected]]Per conto di
> Francois Romieu
> Inviato: domenica 4 settembre 2005 14.01
> A: Giampaolo Tomassoni
> Cc: [email protected];
> [email protected]
> Oggetto: Re: [Linux-ATM-General] [ATMSAR] Request for review - update #1
> 
> 
> Giampaolo Tomassoni <[email protected]> :
> [...]
> > However, I'm still hearing for your comments about the usefulness of an
> > ATMSAR layer.
> 
> Afaik all but one pci ADSL modems are out of tree drivers and 
> include various
> level of proprietary code. If Duncan is not interested in 
> changing its code,
> the usefulness remains to be proven.

Well, the idea is that more pci devices may appear, as adsl-enabled embedded systems will begin to appear in the market.

Also, I believe that adsl will carry much more services then just AAL5 for internet connection in the future. Even if the ATMSAR actually lacks of AAL1 and AAL2/3 capabilities, adding them in a single, specialized module is much easier than swimming in a usb+atm middle layer.

Finally, the fact that ATMSAR is device-unspecific makes it easier to maintain, I guess.


> The codingstyle is broken. Please read again Documentation/CodingStyle,

That's a matter of taste: even Linus burned the GNU coding style book...

However, if it is needed by the linux community, I shurely will fix it whenever the ATMSAR idea will get passed: I'm just gathering feedbacks like the previous one you expressed.


> remove the redundant typedef

Oh, you mean the "typedef enum _HECSTS ..." ?

You're right, thanks.


> and the silly comments ("Reserve 
> header space",
> Encode packet into cells", ...).

I would prefer to explain better what the ATMSAR is doing there. So, I'll get your as a "clarify silly comments". Ok?


> - &page[strlen(page)] in atmProcRead sucks.

Why? It is preceded by an strcpy(page,...). A constant would be worse if someone changes the prefix string...

Or is a page (a pointer) + strlen(page) (an integer) preferred over a closed syntax?


> - "return" is not a function.

Not even for() or while(). But doesn't they look cute this way?


> - consider 'goto' to handle the errors instead of deep nesting

I prefer not using goto when not required to. Nesting is far more readable to my opinion. Compilers do work fine with both.

Anyway, which are the functions you are objecting?


> - +const atmsar_aalops_t opsAALR = {
>   +       ATM_AAL0,
>   +       "raw",
>   -> use .foo = baz instead.

atmasr_aalops_t is not an exported structure (you'll find just an opaque definition in include/linux/atmsar.h), so it is not meant to be statically declared by device drivers. But I guess that the problem is readability, right?

Ok, I'm going to consider your hint in the next patch version.


> drivers/net/*c may give some hint.
> 
> --
> Ueimor

Thank you for your help.

May I ask if this is just your own contribution or if you are in charge of something in the linux and/or linux-atm projects?

Regards,

-----------------------------------
Giampaolo Tomassoni - IT Consultant
Piazza VIII Aprile 1948, 4
I-53044 Chiusi (SI) - Italy
Ph: +39-0578-21100

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]
  Powered by Linux