On Fri, May 27, 2016 at 03:35:57PM -0400, Vivien Didelot wrote:
> Hi Andrew,
>
> Andrew Lunn <and...@lunn.ch> writes:
>
> > @@ -26,6 +26,7 @@ enum dsa_tag_protocol {
> > DSA_TAG_PROTO_TRAILER,
> > DSA_TAG_PROTO_EDSA,
> > DSA_TAG_PROTO_BRCM,
> > + _DSA_TAG_LAST,
> > };
>
> I would avoid _ prefixed functions or symbols, we've seen in mv88e6xxx
> that it doesn't make the code really readable.
The point is to make is special, so that people look at it, wonder why
it is special, and don't make the stupid mistakes of adding a new
protocol after it.
However, if you don't like the _, we can just add a comment
/* MUST BE LAST */
> I don't see the need to add a dsa_device_ops array. I'd keep the switch
> case on tag_protocol within this new dsa_resolve_tag_protocol function,
> to make it more readable (no value checking necessary).
I don't see one way being better than the other. I see it as a
personal preference. If you don't like it, i won't NACK a patch
submitted after it has been accepted which changes it to your personal
preference.
Andrew
