Re: [PATCH] net/smsc911x: add device tree probe support

[Grant Likely <grant.likely@secretlab.ca>]

On Tue, Jul 26, 2011 at 09:30:26AM +0800, Shawn Guo wrote:
> On Mon, Jul 25, 2011 at 09:16:40PM -0400, Nicolas Pitre wrote:
> > On Tue, 26 Jul 2011, Shawn Guo wrote:
> > 
> > > On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> > > > On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > > > > It adds device tree probe support for smsc911x driver.
> > > > > 
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> > > > >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> > > > >  2 files changed, 132 insertions(+), 25 deletions(-)
> > > > >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > new file mode 100644
> > > > > index 0000000..1920695
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > @@ -0,0 +1,34 @@
> > > > > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> > > > 
> > > > Drop "smsc,lan".  That's far too generic.
> > > > 
> > > The following devices are supported by the driver.
> > > 
> > > LAN9115, LAN9116, LAN9117, LAN9118
> > > LAN9215, LAN9216, LAN9217, LAN9218
> > > LAN9210, LAN9211
> > > LAN9220, LAN9221
> > > 
> > > If we only keep specific <model> as the compatible, we will have a
> > > long match table which is actually used nowhere to distinguish the
> > > device.
> > > 
> > > So we need some level generic compatible to save the meaningless
> > > long match table.  What about: 
> > > 
> > > static const struct of_device_id smsc_dt_ids[] = {
> > >         { .compatible = "smsc,lan9", },
> > >         { /* sentinel */ }
> > > };
> > > 
> > > Or:
> > > 
> > > static const struct of_device_id smsc_dt_ids[] = {
> > >         { .compatible = "smsc,lan91", },
> > >         { .compatible = "smsc,lan92", },
> > >         { /* sentinel */ }
> > > };
> > 
> > None of this unambiguously distinguish the devices supported by this 
> > driver and the smc91x driver which supports LAN91C92, LAN91C94, 
> > LAN91C95, LAN91C96, LAN91C100, LAN91C110.
> > 
> So you suggest to make a long list to explicitly tell the device type
> that the driver supports?

If newer devices are 100% backwards compatible with an older device,
then the newer device doesn't need to appear in this list because the
device node can claim compatibility.

If it isn't backwards compatible, then you do need an entry in this
list.

g.