| Message ID | 1368918194-24030-1-git-send-email-tomasz.figa@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. > +#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 if (!IS_ENABLED(CONFIG_OF) || !np) return NULL; > + > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) { > + dev_err(dev, "failed to allocate platform data struct\n"); > + return ERR_PTR(-ENOMEM); > + } > + > + if (of_find_property(np, "davicom,ext-phy", NULL)) > + pdata->flags |= DM9000_PLATF_EXT_PHY; > + if (of_find_property(np, "davicom,no-eeprom", NULL)) > + pdata->flags |= DM9000_PLATF_NO_EEPROM; > + if (of_find_property(np, "davicom,simple-phy", NULL)) > + pdata->flags |= DM9000_PLATF_SIMPLE_PHY; > + > + prop = of_get_property(np, "local-mac-address", &len); > + if (!prop) > + return pdata; I think you should use of_get_mac_address() here. Sascha
Sascha Hauer <s.hauer@pengutronix.de> : [...] > > +#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 > > if (!IS_ENABLED(CONFIG_OF) || !np) > return NULL; It will be the first such use of IS_ENABLED in net land. David, is it ok ?
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. > > +#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, but I decided to keep the code conditional to reduce driver code a bit on platforms that don't need OF. Maybe this single driver wouldn't give any significant difference, but if you make all the drivers used on the platform this way, you will save some kilobytes. > if (!IS_ENABLED(CONFIG_OF) || !np) > return NULL; > > > + > > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > > + if (!pdata) { > > + dev_err(dev, "failed to allocate platform data struct\n"); > > + return ERR_PTR(-ENOMEM); > > + } > > + > > + if (of_find_property(np, "davicom,ext-phy", NULL)) > > + pdata->flags |= DM9000_PLATF_EXT_PHY; > > + if (of_find_property(np, "davicom,no-eeprom", NULL)) > > + pdata->flags |= DM9000_PLATF_NO_EEPROM; > > + if (of_find_property(np, "davicom,simple-phy", NULL)) > > + pdata->flags |= DM9000_PLATF_SIMPLE_PHY; > > + > > + prop = of_get_property(np, "local-mac-address", &len); > > + if (!prop) > > + return pdata; > > I think you should use of_get_mac_address() here. Oops. Right, this is what I was looking for, but somehow I failed. Will change this in next version. Thanks. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) > > > > +#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. Sascha
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 + +Example: + + ethernet@18000000 { + compatible = "davicom,dm9000"; + reg = <0x18000000 0x2 0x18000004 0x2>; + interrupt-parent = <&gpn>; + interrupts = <7 4>; + local-mac-address = [00 00 de ad be ef]; + davicom,no-eeprom; + }; diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 6931c43..2fe74e6 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -18,6 +18,7 @@ chrp Common Hardware Reference Platform cirrus Cirrus Logic, Inc. cortina Cortina Systems, Inc. dallas Maxim Integrated Products (formerly Dallas Semiconductor) +davicom DAVICOM Semiconductor, Inc. denx Denx Software Engineering emmicro EM Microelectronic epson Seiko Epson Corp. diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c index 9105465..6c91708 100644 --- a/drivers/net/ethernet/davicom/dm9000.c +++ b/drivers/net/ethernet/davicom/dm9000.c @@ -29,6 +29,7 @@ #include <linux/spinlock.h> #include <linux/crc32.h> #include <linux/mii.h> +#include <linux/of.h> #include <linux/ethtool.h> #include <linux/dm9000.h> #include <linux/delay.h> @@ -1358,6 +1359,50 @@ static const struct net_device_ops dm9000_netdev_ops = { #endif }; +#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; + + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) { + dev_err(dev, "failed to allocate platform data struct\n"); + return ERR_PTR(-ENOMEM); + } + + if (of_find_property(np, "davicom,ext-phy", NULL)) + pdata->flags |= DM9000_PLATF_EXT_PHY; + if (of_find_property(np, "davicom,no-eeprom", NULL)) + pdata->flags |= DM9000_PLATF_NO_EEPROM; + if (of_find_property(np, "davicom,simple-phy", NULL)) + pdata->flags |= DM9000_PLATF_SIMPLE_PHY; + + prop = of_get_property(np, "local-mac-address", &len); + if (!prop) + return pdata; + + if (len < sizeof(pdata->dev_addr)) { + dev_err(dev, "invalid length of local-mac-address property\n"); + return ERR_PTR(-EINVAL); + } + + memcpy(pdata->dev_addr, prop, sizeof(pdata->dev_addr)); + + return pdata; +} +#else +static struct dm9000_plat_data *dm9000_parse_dt(struct device *unused) +{ + return NULL; +} +#endif + /* * Search DM9000 board, allocate space and register it */ @@ -1373,6 +1418,12 @@ dm9000_probe(struct platform_device *pdev) int i; u32 id_val; + if (!pdata) { + pdata = dm9000_parse_dt(&pdev->dev); + if (IS_ERR(pdata)) + return PTR_ERR(pdata); + } + /* Init network device */ ndev = alloc_etherdev(sizeof(struct board_info)); if (!ndev) @@ -1683,11 +1734,20 @@ dm9000_drv_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_OF +static const struct of_device_id dm9000_of_matches[] = { + { .compatible = "davicom,dm9000", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, dm9000_of_matches); +#endif + static struct platform_driver dm9000_driver = { .driver = { .name = "dm9000", .owner = THIS_MODULE, .pm = &dm9000_drv_pm_ops, + .of_match_table = of_match_ptr(dm9000_of_matches), }, .probe = dm9000_probe, .remove = dm9000_drv_remove,
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