[RFC PATCH 07/14] drivers/smsc911x: add DT support
Grant Likely
grant.likely at secretlab.ca
Sat Aug 21 07:14:42 EST 2010
On Fri, Aug 20, 2010 at 4:24 AM, Lorenzo Pieralisi
<Lorenzo.Pieralisi at arm.com> wrote:
>> -----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,
Heh, I happen to think it looks *better* when all that needs to be
done is return, or at least easier to parse. :-) There's no
accounting for some tastes.
> 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.
There is also Mitch's good suggestion to have a helper function for
reading simple single-cell properties. That would simplify the code
too. Eventually I'll get around to implementing that (unless someone
beats me to it).
>
>>
>> > +
>> > + 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 ?
Usually there is a separate node for the MDIO bus, and then a node on
the MDIO bus for each PHY. A phy-device property is used to link the
MAC to the PHY node. Search for mdio in arch/powerpc/boot/dts/* for
examples. For the internal PHY, that might not use MDIO at all, so it
may be appropriate to have a device-specific property in that use
case.
>> > + 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.
The of_device_id match table already need to be wrapped with #ifdef
CONFIG_OF, so I think it is no worse to also include the decode
function inside that #ifdef block. As for the setting .of_node, I
have been wrapping it with #ifdef, just like CONFIG_PM is often done,
but I agree it is stinky. I'll probably make a macro that can be used
in the static initializer.
>
>>
>> > 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.
Heh; typo. sorry.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
More information about the devicetree-discuss
mailing list