Re: [PATCH v5] NET: Add ezchip ethernet driver

[Paul Gortmaker]

[Re: [PATCH v5] NET: Add ezchip ethernet driver] On 22/06/2015 (Mon 10:45) Mahesh Bandewar wrote:

> On Tue, Jun 16, 2015 at 7:35 AM, Noam Camus <noamc@xxxxxxxxxx> wrote:
> >
> > From: Noam Camus <noamc@xxxxxxxxxx>
> >
> > Simple LAN device for debug or management purposes.
> > Device supports interrupts for RX and TX(completion).
> > Device does not have DMA ability.
> >
> > Signed-off-by: Noam Camus <noamc@xxxxxxxxxx>
> > Signed-off-by: Tal Zilcer <talz@xxxxxxxxxx>
> > Acked-by: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> > ---
> > Change log for v5:
> > Basically its all based on Florian comments.
> > Main items are:
> > 1) Move all interrupt chore to bottom-half
> > 2) use memcpy_toio/fromio
> > 3) dev_kfree_skb() moved to bottom-half
> > 4) add set_rx_mode callback
> > 5) use platform api toward non-DT platforms
> > ---

[...]

> > diff --git a/drivers/net/ethernet/ezchip/Kconfig b/drivers/net/ethernet/ezchip/Kconfig
> > new file mode 100644
> > index 0000000..d031177
> > --- /dev/null
> > +++ b/drivers/net/ethernet/ezchip/Kconfig
> > @@ -0,0 +1,27 @@
> > +#
> > +# EZchip network device configuration
> > +#
> > +
> > +config NET_VENDOR_EZCHIP
> > +       bool "EZchip devices"
> > +       default y
> >
> Why this has to be included in the default build? Shouldn't it be
> either "m" or "N" by default?
> 

Mahesh, in the future please don't quote 1200 lines of code in its
entirety just to add these two lines of comment, which in this case
happen to also be incorrect.  It is a NET_VENDOR_<name> option which
is just the lead in as it clearly states below if we go look...

> > +       ---help---
> > +         If you have a network (Ethernet) device belonging to this class, say Y
> > +         and read the Ethernet-HOWTO, available from
> > +         <http://www.tldp.org/docs.html#howto>.

Noam, since you are unlikely to get this in final from before net-next
closes (happening any time now), please delete the Howto reference when
you resubmit in two weeks, assuming that my cleanup here:

   https://patchwork.ozlabs.org/patch/487080/

gets applied to this release.

> > +
> > +         Note that the answer to this question doesn't directly affect the
> > +         kernel: saying N will just cause the configurator to skip all
> > +         the questions about EZchip devices. If you say Y, you will be asked for
> > +         your specific device in the following questions.

Mahesh, here above.  So he's not doing a "default y" on an actual driver
(which, if he were, yes that would be wrong.)  What worries me more
though, is that ...

> > +
> > +if NET_VENDOR_EZCHIP
> > +
> > +config EZCHIP_NPS_MANAGEMENT_ENET
> > +       tristate "EZchip NPS management enet support"

...there is no dependency line here.  None.  Noam - this implicitly
means that you expect this to build on all arch for all combinations of
.config files.  At a quick glance I see a dts fragment, and I also see a
call to of_get_mac_address() so I'm thinking there needs to be some
dependency here, on OF or OF_NET, or... ?

Paul.
--

> > +       ---help---
> > +         Simple LAN device for debug or management purposes.
> > +         Device supports interrupts for RX and TX(completion).
> > +         Device does not have DMA ability.
> > +
> > +endif
> > diff --git a/drivers/net/ethernet/ezchip/Makefile b/drivers/net/ethernet/ezchip/Makefile
> > new file mode 100644
> > index 0000000..e490176
> > --- /dev/null
> > +++ b/drivers/net/ethernet/ezchip/Makefile

[snip]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/