| Message ID | 1461533283-24852-1-git-send-email-mhei@heimpold.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, Apr 24, 2016 at 11:28:03PM +0200, Michael Heimpold wrote: > The following patch adds the required match table for device tree support > (and while at, fix the indent). It's also possible to specify the > MAC address in the DT blob. > > Also add the corresponding binding documentation file. > > Signed-off-by: Michael Heimpold <mhei@heimpold.de> > --- > > v2: * took care of Arnd Bergmann's review comments > - allow to specify MAC address via DT > - unconditionally define DT id table > * increased the driver version minor number > * driver author's email address bounces, removed from address list > > .../devicetree/bindings/net/microchip-enc28j60.txt | 50 ++++++++++++++++++++++ Matching the compatible string is preferred here: microchip,enc28j60.txt > drivers/net/ethernet/microchip/enc28j60.c | 20 +++++++-- > 2 files changed, 67 insertions(+), 3 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/microchip-enc28j60.txt > > diff --git a/Documentation/devicetree/bindings/net/microchip-enc28j60.txt b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt > new file mode 100644 > index 0000000..847a97b > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt > @@ -0,0 +1,50 @@ > +* Microchip ENC28J60 > + > +This is a standalone 10 MBit ethernet controller with SPI interface. > + > +For each device connected to a SPI bus, define a child node within > +the SPI master node. > + > +Required properties: > +- compatible: Should be "microchip,enc28j60" > +- reg: Specify the SPI chip select the ENC28J60 is wired to > +- interrupts: Specify the interrupt and interrupt type (usually falling edge) > + > +Optional properties: > +- interrupt-parent: Specify the pHandle of the source interrupt This is required in the sense that either the node or a parent node must define it. In this case, since the SPI controller likely has a different parent, you will pretty much always need it defined in this node. > +- spi-max-frequency: Maximum frequency of the SPI bus when accessing the ENC28J60. > + According to the ENC28J80 datasheet, the chip allows a maximum of 20 MHz, however, > + board designs may need to limit this value. > +- local-mac-address: See ethernet.txt in the same directory. > + > + > +Example (for NXP i.MX28 with pin control stuff for GPIO irq): > + > + ssp2: ssp@80014000 { > + compatible = "fsl,imx28-spi"; > + pinctrl-names = "default"; > + pinctrl-0 = <&spi2_pins_b &spi2_sck_cfg>; > + status = "okay"; > + > + enc28j60: ethernet@0 { > + compatible = "microchip,enc28j60"; > + pinctrl-names = "default"; > + pinctrl-0 = <&enc28j60_pins>; Need to document using the pinctrl binding. > + reg = <0>; > + interrupt-parent = <&gpio3>; > + interrupts = <3 IRQ_TYPE_EDGE_FALLING>; > + spi-max-frequency = <12000000>; > + }; > + }; > +
On 04/24/2016 04:28 PM, Michael Heimpold wrote: > The following patch adds the required match table for device tree support > (and while at, fix the indent). It's also possible to specify the > MAC address in the DT blob. > > Also add the corresponding binding documentation file. > > Signed-off-by: Michael Heimpold <mhei@heimpold.de> > --- > > v2: * took care of Arnd Bergmann's review comments > - allow to specify MAC address via DT > - unconditionally define DT id table > * increased the driver version minor number > * driver author's email address bounces, removed from address list > > .../devicetree/bindings/net/microchip-enc28j60.txt | 50 ++++++++++++++++++++++ > drivers/net/ethernet/microchip/enc28j60.c | 20 +++++++-- > 2 files changed, 67 insertions(+), 3 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/microchip-enc28j60.txt > > diff --git a/Documentation/devicetree/bindings/net/microchip-enc28j60.txt b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt > new file mode 100644 > index 0000000..847a97b > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt > @@ -0,0 +1,50 @@ > +* Microchip ENC28J60 > + > +This is a standalone 10 MBit ethernet controller with SPI interface. > + > +For each device connected to a SPI bus, define a child node within > +the SPI master node. > + > +Required properties: > +- compatible: Should be "microchip,enc28j60" > +- reg: Specify the SPI chip select the ENC28J60 is wired to > +- interrupts: Specify the interrupt and interrupt type (usually falling edge) > + > +Optional properties: > +- interrupt-parent: Specify the pHandle of the source interrupt > +- spi-max-frequency: Maximum frequency of the SPI bus when accessing the ENC28J60. > + According to the ENC28J80 datasheet, the chip allows a maximum of 20 MHz, however, > + board designs may need to limit this value. > +- local-mac-address: See ethernet.txt in the same directory. > + > + > +Example (for NXP i.MX28 with pin control stuff for GPIO irq): > + > + ssp2: ssp@80014000 { > + compatible = "fsl,imx28-spi"; > + pinctrl-names = "default"; > + pinctrl-0 = <&spi2_pins_b &spi2_sck_cfg>; > + status = "okay"; > + > + enc28j60: ethernet@0 { > + compatible = "microchip,enc28j60"; > + pinctrl-names = "default"; > + pinctrl-0 = <&enc28j60_pins>; > + reg = <0>; > + interrupt-parent = <&gpio3>; > + interrupts = <3 IRQ_TYPE_EDGE_FALLING>; > + spi-max-frequency = <12000000>; > + }; > + }; > + > + pinctrl@80018000 { > + enc28j60_pins: enc28j60_pins@0 { > + reg = <0>; > + fsl,pinmux-ids = < > + MX28_PAD_AUART0_RTS__GPIO_3_3 /* Interrupt */ > + >; > + fsl,drive-strength = <MXS_DRIVE_4mA>; > + fsl,voltage = <MXS_VOLTAGE_HIGH>; > + fsl,pull-up = <MXS_PULL_DISABLE>; > + }; > + }; > diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c > index b723622..7066954 100644 > --- a/drivers/net/ethernet/microchip/enc28j60.c > +++ b/drivers/net/ethernet/microchip/enc28j60.c > @@ -28,11 +28,12 @@ > #include <linux/skbuff.h> > #include <linux/delay.h> > #include <linux/spi/spi.h> > +#include <linux/of_net.h> > > #include "enc28j60_hw.h" > > #define DRV_NAME "enc28j60" > -#define DRV_VERSION "1.01" > +#define DRV_VERSION "1.02" > > #define SPI_OPLEN 1 > > @@ -1548,6 +1549,7 @@ static int enc28j60_probe(struct spi_device *spi) > { > struct net_device *dev; > struct enc28j60_net *priv; > + const void *macaddr; > int ret = 0; > > if (netif_msg_drv(&debug)) > @@ -1579,7 +1581,12 @@ static int enc28j60_probe(struct spi_device *spi) > ret = -EIO; > goto error_irq; > } > - eth_hw_addr_random(dev); > + > + macaddr = of_get_mac_address(spi->dev.of_node); > + if (macaddr) You should also check if it is a valid MAC for Ethernet, recommend: if (macaddr && is_valid_ether_addr(macaddr)) > + ether_addr_copy(dev->dev_addr, macaddr); > + else > + eth_hw_addr_random(dev); > enc28j60_set_hw_macaddr(dev); > > /* Board setup must set the relevant edge trigger type; > @@ -1634,9 +1641,16 @@ static int enc28j60_remove(struct spi_device *spi) > return 0; > } > > +static const struct of_device_id enc28j60_dt_ids[] = { > + { .compatible = "microchip,enc28j60" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, enc28j60_dt_ids); > + > static struct spi_driver enc28j60_driver = { > .driver = { > - .name = DRV_NAME, > + .name = DRV_NAME, > + .of_match_table = enc28j60_dt_ids, > }, > .probe = enc28j60_probe, > .remove = enc28j60_remove, >
Hi, Am Monday 25 April 2016, 10:39:41 schrieben Sie: > On 04/24/2016 04:28 PM, Michael Heimpold wrote: > > The following patch adds the required match table for device tree support > > (and while at, fix the indent). It's also possible to specify the > > MAC address in the DT blob. > > > > Also add the corresponding binding documentation file. > > > > Signed-off-by: Michael Heimpold <mhei@heimpold.de> > > --- > > > > v2: * took care of Arnd Bergmann's review comments > > > > - allow to specify MAC address via DT > > - unconditionally define DT id table > > > > * increased the driver version minor number > > * driver author's email address bounces, removed from address list > > > > .../devicetree/bindings/net/microchip-enc28j60.txt | 50 > > ++++++++++++++++++++++ drivers/net/ethernet/microchip/enc28j60.c > > | 20 +++++++-- > > 2 files changed, 67 insertions(+), 3 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/net/microchip-enc28j60.txt> > > diff --git a/Documentation/devicetree/bindings/net/microchip-enc28j60.txt > > b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt new file > > mode 100644 > > index 0000000..847a97b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt > > @@ -0,0 +1,50 @@ > > +* Microchip ENC28J60 > > + > > +This is a standalone 10 MBit ethernet controller with SPI interface. > > + > > +For each device connected to a SPI bus, define a child node within > > +the SPI master node. > > + > > +Required properties: > > +- compatible: Should be "microchip,enc28j60" > > +- reg: Specify the SPI chip select the ENC28J60 is wired to > > +- interrupts: Specify the interrupt and interrupt type (usually falling > > edge) + > > +Optional properties: > > +- interrupt-parent: Specify the pHandle of the source interrupt > > +- spi-max-frequency: Maximum frequency of the SPI bus when accessing the > > ENC28J60. + According to the ENC28J80 datasheet, the chip allows a > > maximum of 20 MHz, however, + board designs may need to limit this > > value. > > +- local-mac-address: See ethernet.txt in the same directory. > > + > > + > > +Example (for NXP i.MX28 with pin control stuff for GPIO irq): > > + > > + ssp2: ssp@80014000 { > > + compatible = "fsl,imx28-spi"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&spi2_pins_b &spi2_sck_cfg>; > > + status = "okay"; > > + > > + enc28j60: ethernet@0 { > > + compatible = "microchip,enc28j60"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&enc28j60_pins>; > > + reg = <0>; > > + interrupt-parent = <&gpio3>; > > + interrupts = <3 IRQ_TYPE_EDGE_FALLING>; > > + spi-max-frequency = <12000000>; > > + }; > > + }; > > + > > + pinctrl@80018000 { > > + enc28j60_pins: enc28j60_pins@0 { > > + reg = <0>; > > + fsl,pinmux-ids = < > > + MX28_PAD_AUART0_RTS__GPIO_3_3 /* > > Interrupt */ + >; > > + fsl,drive-strength = <MXS_DRIVE_4mA>; > > + fsl,voltage = <MXS_VOLTAGE_HIGH>; > > + fsl,pull-up = <MXS_PULL_DISABLE>; > > + }; > > + }; > > diff --git a/drivers/net/ethernet/microchip/enc28j60.c > > b/drivers/net/ethernet/microchip/enc28j60.c index b723622..7066954 100644 > > --- a/drivers/net/ethernet/microchip/enc28j60.c > > +++ b/drivers/net/ethernet/microchip/enc28j60.c > > @@ -28,11 +28,12 @@ > > > > #include <linux/skbuff.h> > > #include <linux/delay.h> > > #include <linux/spi/spi.h> > > > > +#include <linux/of_net.h> > > > > #include "enc28j60_hw.h" > > > > #define DRV_NAME "enc28j60" > > > > -#define DRV_VERSION "1.01" > > +#define DRV_VERSION "1.02" > > > > #define SPI_OPLEN 1 > > > > @@ -1548,6 +1549,7 @@ static int enc28j60_probe(struct spi_device *spi) > > > > { > > > > struct net_device *dev; > > struct enc28j60_net *priv; > > > > + const void *macaddr; > > > > int ret = 0; > > > > if (netif_msg_drv(&debug)) > > > > @@ -1579,7 +1581,12 @@ static int enc28j60_probe(struct spi_device *spi) > > > > ret = -EIO; > > goto error_irq; > > > > } > > > > - eth_hw_addr_random(dev); > > + > > + macaddr = of_get_mac_address(spi->dev.of_node); > > + if (macaddr) > > You should also check if it is a valid MAC for Ethernet, recommend: > > if (macaddr && is_valid_ether_addr(macaddr)) > But of_get_mac_address already takes care of this, see http://lxr.free-electrons.com/source/drivers/of/of_net.c#L45 Also it already checks whether spi->dev.of_node is populated at all. It returns NULL in both error cases. So I prefered to omit both test here. Regards, Michael > > + ether_addr_copy(dev->dev_addr, macaddr); > > + else > > + eth_hw_addr_random(dev); > > > > enc28j60_set_hw_macaddr(dev); > > > > /* Board setup must set the relevant edge trigger type; > > > > @@ -1634,9 +1641,16 @@ static int enc28j60_remove(struct spi_device *spi) > > > > return 0; > > > > } > > > > +static const struct of_device_id enc28j60_dt_ids[] = { > > + { .compatible = "microchip,enc28j60" }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, enc28j60_dt_ids); > > + > > > > static struct spi_driver enc28j60_driver = { > > > > .driver = { > > > > - .name = DRV_NAME, > > + .name = DRV_NAME, > > + .of_match_table = enc28j60_dt_ids, > > > > }, > > > > .probe = enc28j60_probe, > > .remove = enc28j60_remove,
On 04/25/2016 12:46 PM, Michael Heimpold wrote: > Hi, > > Am Monday 25 April 2016, 10:39:41 schrieben Sie: >> On 04/24/2016 04:28 PM, Michael Heimpold wrote: >>> - eth_hw_addr_random(dev); >>> + >>> + macaddr = of_get_mac_address(spi->dev.of_node); >>> + if (macaddr) >> >> You should also check if it is a valid MAC for Ethernet, recommend: >> >> if (macaddr && is_valid_ether_addr(macaddr)) >> > > But of_get_mac_address already takes care of this, see > http://lxr.free-electrons.com/source/drivers/of/of_net.c#L45 > Also it already checks whether spi->dev.of_node is populated at all. > It returns NULL in both error cases. > So I prefered to omit both test here. > Ah, missed that, no problem here then. Hmm, I wonder how many other drivers then do this check needlessly.. Time to fire-up Coccinelle :) Andrew
Hi, Am Monday 25 April 2016, 08:23:26 schrieb Rob Herring: > On Sun, Apr 24, 2016 at 11:28:03PM +0200, Michael Heimpold wrote: > > The following patch adds the required match table for device tree support > > (and while at, fix the indent). It's also possible to specify the > > MAC address in the DT blob. > > > > Also add the corresponding binding documentation file. > > > > Signed-off-by: Michael Heimpold <mhei@heimpold.de> > > --- > > > > v2: * took care of Arnd Bergmann's review comments > > > > - allow to specify MAC address via DT > > - unconditionally define DT id table > > > > * increased the driver version minor number > > * driver author's email address bounces, removed from address list > > > > .../devicetree/bindings/net/microchip-enc28j60.txt | 50 > > ++++++++++++++++++++++ > Matching the compatible string is preferred here: microchip,enc28j60.txt > Ok, will change in next round. > > drivers/net/ethernet/microchip/enc28j60.c | 20 +++++++-- > > 2 files changed, 67 insertions(+), 3 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/net/microchip-enc28j60.txt> > > diff --git a/Documentation/devicetree/bindings/net/microchip-enc28j60.txt > > b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt new file > > mode 100644 > > index 0000000..847a97b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt > > @@ -0,0 +1,50 @@ > > +* Microchip ENC28J60 > > + > > +This is a standalone 10 MBit ethernet controller with SPI interface. > > + > > +For each device connected to a SPI bus, define a child node within > > +the SPI master node. > > + > > +Required properties: > > +- compatible: Should be "microchip,enc28j60" > > +- reg: Specify the SPI chip select the ENC28J60 is wired to > > +- interrupts: Specify the interrupt and interrupt type (usually falling > > edge) + > > +Optional properties: > > +- interrupt-parent: Specify the pHandle of the source interrupt > > This is required in the sense that either the node or a parent node must > define it. In this case, since the SPI controller likely has a different > parent, you will pretty much always need it defined in this node. Understand. I think I should then move it to the required properties? BTW: I orientated on the qca-qca7000-spi.txt as this is a SPI ethernet device, too. Maybe we should also clarify this topic for this binding... > > +- spi-max-frequency: Maximum frequency of the SPI bus when accessing the > > ENC28J60. + According to the ENC28J80 datasheet, the chip allows a > > maximum of 20 MHz, however, + board designs may need to limit this > > value. > > +- local-mac-address: See ethernet.txt in the same directory. > > + > > + > > +Example (for NXP i.MX28 with pin control stuff for GPIO irq): > > + > > + ssp2: ssp@80014000 { > > + compatible = "fsl,imx28-spi"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&spi2_pins_b &spi2_sck_cfg>; > > + status = "okay"; > > + > > + enc28j60: ethernet@0 { > > + compatible = "microchip,enc28j60"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&enc28j60_pins>; > > Need to document using the pinctrl binding. Would it be enough, when I list both properties above and refer to the pinctrl binding? I wondered whether there are other possibilities to wire the irq, but I guess that using GPIO line is the usual way to do, so I'd add this to required properties, too. Thanks for your feedback, regards Michael > > > + reg = <0>; > > + interrupt-parent = <&gpio3>; > > + interrupts = <3 IRQ_TYPE_EDGE_FALLING>; > > + spi-max-frequency = <12000000>; > > + }; > > + }; > > +
diff --git a/Documentation/devicetree/bindings/net/microchip-enc28j60.txt b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt new file mode 100644 index 0000000..847a97b --- /dev/null +++ b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt @@ -0,0 +1,50 @@ +* Microchip ENC28J60 + +This is a standalone 10 MBit ethernet controller with SPI interface. + +For each device connected to a SPI bus, define a child node within +the SPI master node. + +Required properties: +- compatible: Should be "microchip,enc28j60" +- reg: Specify the SPI chip select the ENC28J60 is wired to +- interrupts: Specify the interrupt and interrupt type (usually falling edge) + +Optional properties: +- interrupt-parent: Specify the pHandle of the source interrupt +- spi-max-frequency: Maximum frequency of the SPI bus when accessing the ENC28J60. + According to the ENC28J80 datasheet, the chip allows a maximum of 20 MHz, however, + board designs may need to limit this value. +- local-mac-address: See ethernet.txt in the same directory. + + +Example (for NXP i.MX28 with pin control stuff for GPIO irq): + + ssp2: ssp@80014000 { + compatible = "fsl,imx28-spi"; + pinctrl-names = "default"; + pinctrl-0 = <&spi2_pins_b &spi2_sck_cfg>; + status = "okay"; + + enc28j60: ethernet@0 { + compatible = "microchip,enc28j60"; + pinctrl-names = "default"; + pinctrl-0 = <&enc28j60_pins>; + reg = <0>; + interrupt-parent = <&gpio3>; + interrupts = <3 IRQ_TYPE_EDGE_FALLING>; + spi-max-frequency = <12000000>; + }; + }; + + pinctrl@80018000 { + enc28j60_pins: enc28j60_pins@0 { + reg = <0>; + fsl,pinmux-ids = < + MX28_PAD_AUART0_RTS__GPIO_3_3 /* Interrupt */ + >; + fsl,drive-strength = <MXS_DRIVE_4mA>; + fsl,voltage = <MXS_VOLTAGE_HIGH>; + fsl,pull-up = <MXS_PULL_DISABLE>; + }; + }; diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c index b723622..7066954 100644 --- a/drivers/net/ethernet/microchip/enc28j60.c +++ b/drivers/net/ethernet/microchip/enc28j60.c @@ -28,11 +28,12 @@ #include <linux/skbuff.h> #include <linux/delay.h> #include <linux/spi/spi.h> +#include <linux/of_net.h> #include "enc28j60_hw.h" #define DRV_NAME "enc28j60" -#define DRV_VERSION "1.01" +#define DRV_VERSION "1.02" #define SPI_OPLEN 1 @@ -1548,6 +1549,7 @@ static int enc28j60_probe(struct spi_device *spi) { struct net_device *dev; struct enc28j60_net *priv; + const void *macaddr; int ret = 0; if (netif_msg_drv(&debug)) @@ -1579,7 +1581,12 @@ static int enc28j60_probe(struct spi_device *spi) ret = -EIO; goto error_irq; } - eth_hw_addr_random(dev); + + macaddr = of_get_mac_address(spi->dev.of_node); + if (macaddr) + ether_addr_copy(dev->dev_addr, macaddr); + else + eth_hw_addr_random(dev); enc28j60_set_hw_macaddr(dev); /* Board setup must set the relevant edge trigger type; @@ -1634,9 +1641,16 @@ static int enc28j60_remove(struct spi_device *spi) return 0; } +static const struct of_device_id enc28j60_dt_ids[] = { + { .compatible = "microchip,enc28j60" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, enc28j60_dt_ids); + static struct spi_driver enc28j60_driver = { .driver = { - .name = DRV_NAME, + .name = DRV_NAME, + .of_match_table = enc28j60_dt_ids, }, .probe = enc28j60_probe, .remove = enc28j60_remove,
The following patch adds the required match table for device tree support (and while at, fix the indent). It's also possible to specify the MAC address in the DT blob. Also add the corresponding binding documentation file. Signed-off-by: Michael Heimpold <mhei@heimpold.de> --- v2: * took care of Arnd Bergmann's review comments - allow to specify MAC address via DT - unconditionally define DT id table * increased the driver version minor number * driver author's email address bounces, removed from address list .../devicetree/bindings/net/microchip-enc28j60.txt | 50 ++++++++++++++++++++++ drivers/net/ethernet/microchip/enc28j60.c | 20 +++++++-- 2 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/microchip-enc28j60.txt