[RFC PATCH 07/14] drivers/smsc911x: add DT support

[Lorenzo Pieralisi]

> -----Original Message-----
> From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On Behalf Of
> Grant Likely
> Sent: 18 August 2010 22:17
> To: Lorenzo Pieralisi
> Cc: devicetree-discuss at lists.ozlabs.org; Philippe Robin;
> nico at fluxnic.net; linux at arm.linux.org.uk; Catalin Marinas;
> jeremy.kerr at canonical.com; David Miller
> Subject: Re: [RFC PATCH 07/14] drivers/smsc911x: add DT support
> 
> [cc'ing networking maintainer David Miller to keep him in the loop]
> 
> Hi Lorenzo,
> 
> A number of comments below and some rework required, but in general
> the approach looks good and impact on the core driver is limited to
> the start of the .probe() routine.
> 
> On Wed, Aug 18, 2010 at 12:59 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi at arm.com> wrote:
> > When OF is enabled the device driver should initialize the match
> table
> > in order to be probable from the device tree. Furthermore, HW
> properties
> > should be retrieved from the device tree node, so a device tree probe
> > function is required to parse node properties at run-time. To avoid
> > preprocessor macros, open firmware functions are defined in a
> separate
> > C file that is optionally compiled-in.
> >
> > This patch adds the infrastructure needed to enable device tree
> parsing
> > to smsc911 platform driver, inclusive of Makefile and device tree
> > parsing functions. Driver properties should be defined and retrieved
> > from the device tree if OF is enabled, otherwise the driver would
> fall
> > back to platform data static initialization.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi at arm.com>
> > ---
> >  drivers/net/Makefile      |    3 +-
> >  drivers/net/smsc911x-of.c |   53
> +++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/smsc911x.c    |   28 ++++++++++++++++-------
> >  drivers/net/smsc911x.h    |    8 ++++++
> >  4 files changed, 82 insertions(+), 10 deletions(-)
> >  create mode 100644 drivers/net/smsc911x-of.c
> >
> > diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> > index 0a0512a..0d2faf9 100644
> > --- a/drivers/net/Makefile
> > +++ b/drivers/net/Makefile
> > @@ -243,7 +243,8 @@ obj-$(CONFIG_VXGE) += vxge/
> >  obj-$(CONFIG_MYRI10GE) += myri10ge/
> >  obj-$(CONFIG_SMC91X) += smc91x.o
> >  obj-$(CONFIG_SMC911X) += smc911x.o
> > -obj-$(CONFIG_SMSC911X) += smsc911x.o
> > +smsc911x_of-$(CONFIG_OF) := smsc911x-of.o
> > +obj-$(CONFIG_SMSC911X) += smsc911x.o $(smsc911x_of-y)
> >  obj-$(CONFIG_BFIN_MAC) += bfin_mac.o
> >  obj-$(CONFIG_DM9000) += dm9000.o
> >  obj-$(CONFIG_PASEMI_MAC) += pasemi_mac_driver.o
> > diff --git a/drivers/net/smsc911x-of.c b/drivers/net/smsc911x-of.c
> > new file mode 100644
> > index 0000000..b8751cf
> > --- /dev/null
> > +++ b/drivers/net/smsc911x-of.c
> > @@ -0,0 +1,53 @@
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of.h>
> > +#include <linux/smsc911x.h>
> > +#include <linux/kernel.h>
> > +
> > +#include "smsc911x.h"
> > +
> > +
> > +int smsc911x_probe_dt(struct smsc911x_platform_config *cfg,
> > +               struct platform_device *pdev)
> > +{
> > +       struct device_node *node = pdev->dev.of_node;
> > +       int ret = -ENODEV;
> > +       const u32 *prop;
> > +
> > +       if (!node)
> > +               goto err;
> 
> Nothing is allocated or needs to be undone in this routine in the
> event of failure.  Instead of "goto err", you can just "return
> -ENODEV" in each error case.

Right, it is just because all those "return -ENODEV;" do not look nice 
either, it is clear there is no point in jumping to just pop out.
But I agree, I will stick to the return approach.
It may be worthwhile to code a loop with a struct containing the set 
of string properties, that's just cosmetic changes.

> 
> > +
> > +       prop = of_get_property(node, "flags", NULL);
> > +       if (!prop)
> > +               goto err;
> > +
> > +       cfg->flags = of_read_number(prop, 1);
> 
> Looks like Linux-specific implementation details are being encoded
> into the device tree.  What is the intended meaning for the "flags"
> property?
> 

Well, looks like you are right. It is to define if a 16-bit bus or 
32-bit bus (to force atomicity of burst of write/read) is used to 
access the device, plus flags to force external or internal phy and
an option to save the MAC address.

What's the approach to be followed in this case ?

> > +
> > +       prop = of_get_property(node, "irq-polarity", NULL);
> > +       if (!prop)
> > +               goto err;
> > +
> > +       cfg->irq_polarity = of_read_number(prop, 1);
> > +
> > +       prop = of_get_property(node, "irq-type", NULL);
> > +       if (!prop)
> > +               goto err;
> > +
> > +       cfg->irq_type = of_read_number(prop, 1);
> 
> Most interrupt controller bindings include an extra cell in the
> interrupt specifier to encode flags like irq polarity and type.
> Encoding these outside of the interrupt specifier makes things less
> clear.
> 

Point taken.

> > +
> > +       prop = of_get_property(node, "phy-interface", NULL);
> > +       if (!prop)
> > +               goto err;
> > +
> > +       cfg->phy_interface = of_read_number(prop, 1);
> 
> There is a binding phy devices with support routines.  This network
> device should use those instead of inventing something new.
> 

Ditto.

> > +
> > +       pr_info("%s: Parsed FDT info\n", node->name);
> > +       pr_info("FLAGS %d\n", cfg->flags);
> > +       pr_info("IRQ-POLARITY %d\n", cfg->irq_polarity);
> > +       pr_info("IRQ-TYPE %d\n", cfg->irq_type);
> > +       pr_info("PHY IF %d\n", cfg->phy_interface);
> 
> I prefer to see the dev_info() macro used for per-device-instance
> messages.
>

 
Ok.

> > +       return 0;
> > +err:
> > +       return ret;
> > +}
> 
> I don't think it is necessary to have a separate .c file for this one
> function.  I don't see any problem with it living in
> drivers/net/smsc911x.c.  I'd rather have everything contained in a
> single place.

The point Grant is that the dt probe function requires access to CONFIG_OF 
dependent members (of_node). If the need for #ifdef CONFIG_OF will go 
I am more than happy to just merge it all in one file. 
This is valid for all the drivers and the match table init macro as well.
 
> 
> > diff --git a/drivers/net/smsc911x.c b/drivers/net/smsc911x.c
> > index cc55974..c56b8f7 100644
> > --- a/drivers/net/smsc911x.c
> > +++ b/drivers/net/smsc911x.c
> > @@ -50,6 +50,7 @@
> >  #include <linux/phy.h>
> >  #include <linux/smsc911x.h>
> >  #include <linux/device.h>
> > +
> >  #include "smsc911x.h"
> 
> Unrelated whitespace change.
> 

Ok.

> >
> {
> >  /* Entry point for loading the module */
> >  static int __init smsc911x_init_module(void)
> >  {
> > +       platform_init_match(&smsc911x_driver, smsc911x_matches);
> 
> the match table should be statically initialized into the driver
> structure.  Right now that does require wrapping the line with #ifdef
> CONFIG_OF which is a little stinky, but I'm working on a solution for
> that.  Regardless, I don't like having to use a macro to set the
> pointer at probe time when it can be done statically.
> 

Point taken, but to be precise it is not set at probe time, it is set at 
init time (insmod or initcalls). Again, it is just to avoid ifdefs,
if the #ifdefs go away, I will do it statically with nary a problem.

> >        return platform_driver_register(&smsc911x_driver);
> >  }
> >
> > diff --git a/drivers/net/smsc911x.h b/drivers/net/smsc911x.h
> > index 016360c..5ec9a8b 100644
> > --- a/drivers/net/smsc911x.h
> > +++ b/drivers/net/smsc911x.h
> > @@ -394,4 +394,12 @@
> >  #define LPA_PAUSE_ALL                  (LPA_PAUSE_CAP | \
> >                                         LPA_PAUSE_ASYM)
> >
> > +#ifndef CONFIG_OF
> > +static inline int smsc911x_probe_dt(struct smsc911x_platform_config
> *cfg,
> > +               struct platform_device *pdev) { return -ENODEV; }
> > +#else
> > +extern int smsc911x_probe_dt(struct smsc911x_platform_config *cfg,
> > +               struct platform_device *pdev);
> > +#endif
> > +
> 
> This hunk will of course also get merged into the main .c file if the
> OF helper function is rolled into the main driver.

Ok.

> 
> Cheers,
> g.

Many thanks, that helps a lot.
Lorenzo

> 
> >  #endif                         /* __SMSC911X_H__ */
> > --
> > 1.6.3.3
> >
> >
> 
> 
> 
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.