Giampaolo Tomassoni <[EMAIL PROTECTED]> :
[...]
> 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.
I'd be happily surprized to see more documented ADSL PCI/USB device in the
near future. :o(
> 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.
Ok. Your suggestion may have more impact if there is a patch to convert
the sole existing in-kernel driver to use this module.
[...]
> > The codingstyle is broken. Please read again Documentation/CodingStyle,
>
> That's a matter of taste: even Linus burned the GNU coding style book...
An uniform codingstyle is useful when people need to review code. Something
is wrong when a reviewer must uncipher a piece of code. You will find areas
in the kernel whose trends differ but a codingstyle from Mars is usually a
hint. So it is not _only_ a matter of taste.
> 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.
You may have more feedback/review then. I only gave a cursory look at the
code.
[...]
> > remove the redundant typedef
>
> Oh, you mean the "typedef enum _HECSTS ..." ?
Rather the "typedef struct atmsar_dev atmsar_dev_t;" (yes, I know the "It
saves typing" argument). Maybe something could be done at the same time
regarding the need for the forward declarations.
[...]
> > 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?
s/what/why/
And no, documenting a call to skb_reserve is silly.
[...]
> > - &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...
The value returned by sprintf and friends contains the needed offset, i.e.
buf += sprintf(buf, ...);.
[...]
> > - "return" is not a function.
>
> Not even for() or while(). But doesn't they look cute this way?
No.
for (), while (), return rc;
[...]
> > - 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.
OTOH, it makes ugly code to have it fit in a 80 columns console.
[...]
> Anyway, which are the functions you are objecting?
atmSend. Probably others.
If you can make the code look like existing in-kernel code (not fs/cifs
please) say network or ata driver code and you do not need goto, it's fine
too.
[...]
> > - +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?
struct foo zoy {
.bar = barbar,
.baz = bazbaz,
.quuz = ...
};
[...]
> 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?
/me scratches head
http://ww.google.com/search?hl=en&q=romieu+linux+cabal
--
Ueimor
-
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/
