[PATCH] net: dm9000: Allow instantiation using device tree

[tomasz.figa@gmail.com (Tomasz Figa)]

On Sunday 19 of May 2013 12:35:44 Sascha Hauer wrote:
> On Sun, May 19, 2013 at 10:54:53AM +0200, Tomasz Figa wrote:
> > Hi Sascha,
> > 
> > On Sunday 19 of May 2013 09:05:38 Sascha Hauer wrote:
> > > Hi Tomasz,
> > > 
> > > On Sun, May 19, 2013 at 01:03:14AM +0200, Tomasz Figa wrote:
> > > > This patch adds Device Tree support to dm9000 driver.
> > > > 
> > > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > > ---
> > > > 
> > > >  .../devicetree/bindings/net/davicom-dm9000.txt     | 27
> > > >  ++++++++++
> > > >  .../devicetree/bindings/vendor-prefixes.txt        |  1 +
> > > >  drivers/net/ethernet/davicom/dm9000.c              | 60
> > > >  ++++++++++++++++++++++ 3 files changed, 88 insertions(+)
> > > >  create mode 100644
> > > >  Documentation/devicetree/bindings/net/davicom-dm9000.txt>
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/net/davicom-dm9000.txt
> > > > b/Documentation/devicetree/bindings/net/davicom-dm9000.txt new
> > > > file
> > > > mode 100644
> > > > index 0000000..d2902db
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/davicom-dm9000.txt
> > > > @@ -0,0 +1,27 @@
> > > > +Davicom DM9000 Fast Ethernet controller
> > > > +
> > > > +Required properties:
> > > > +- compatible = "davicom,dm9000";
> > > > +- reg : physical addresses and sizes of registers, must contain 2
> > > > entries: +    first entry : address register,
> > > > +    second entry : address register.
> > > > +- interrupt-parent : interrupt controller to which the device is
> > > > connected +- interrupts : interrupt specifier specific to
> > > > interrupt
> > > > controller +
> > > > +Optional properties:
> > > > +- local-mac-address : A bytestring of 6 bytes specifying Ethernet
> > > > MAC
> > > > address +    to use (from firmware or bootloader)
> > > > +- davicom,no-eeprom : Configuration EEPROM is not available
> > > > +- davicom,ext-phy : Use external PHY
> > > > +- davicom,simple-phy : Use NSR to find LinkStatus
> > > 
> > > Do we really need to expose this simple-phy property? Looking at the
> > > drvier code this more looks like a work around shortcomings of the
> > > driver code rather than something really necessary.
> > 
> > Well, depending on this property it can use two different methods of
> > checking carrier status and it seems to depend on hardware, which one
> > should be used. I don't see any driver shortcomings here, but maybe
> > I'm
> > missing something.
> 
> It's not hardware dependent, at least not from the original commit log.
> 
> The driver doesn't use mdiobus_register which you normally would do
> today. If someone ports the driver over to mdiobus we would still have
> to support this legacy flag as a special case.
> 
> Since no mainline user makes use of this flag I suggest to skip it from
> the devicetree, at least until someone really asks for this specific
> flag (and has good reasons to use it)

Well, I don't need this flag on my platform, so I'm not strongly against 
removing it. If you really don't like it, I will remove it in v2.

> > > > +#ifdef CONFIG_OF
> > > > +static struct dm9000_plat_data *dm9000_parse_dt(struct device
> > > > *dev)
> > > > +{
> > > > +	struct dm9000_plat_data *pdata;
> > > > +	struct device_node *np = dev->of_node;
> > > > +	const void *prop;
> > > > +	int len;
> > > > +
> > > > +	if (!np)
> > > > +		return NULL;
> > > 
> > > You should be able to kill the ifdef around this function by doing
> > 
> > Basically this would be a kill with a double-edged sword.
> > 
> > I could completely drop the #ifdef without any additional check, as it
> > is not possible that np is not NULL with !CONFIG_OF,
> 
> Yes, but the compiler doesn't know this.
> 
> > but I decided to keep
> > the code conditional to reduce driver code a bit on platforms that
> > don't need OF.
> 
> IS_ENABLED(CONFIG_OF) will expand to 0 without OF and the compiler will
> through the unused code away. You won't find differences in the binary
> size.

Hmm, so IS_ENABLED returns a constant, OK. I have never bothered to look 
that up and ended with a strange belief that it is a runtime check (which 
doesn't make sense...). Always good to learn something, thanks.

OK, I'm fine with this, will change it in v2.

Best regards,
Tomasz