From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH] net: dm9000: Allow instantiation using device tree Date: Sun, 19 May 2013 13:04:59 +0200 Message-ID: <1433733.7VTOAEOlx2@flatron> References: <1368918194-24030-1-git-send-email-tomasz.figa@gmail.com> <2763249.7aFBh8NOHl@flatron> <20130519103544.GZ32299@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Return-path: In-Reply-To: <20130519103544.GZ32299@pengutronix.de> Sender: netdev-owner@vger.kernel.org To: Sascha Hauer Cc: netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree-discuss@lists.ozlabs.org, Grant Likely , Rob Herring List-Id: devicetree@vger.kernel.org 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 > > > > --- > > > > > > > > .../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