[PATCH net-next 0/2] net: ethernet: enc28j60: small improvements

[PATCH net-next 0/2] net: ethernet: enc28j60: small improvements

[Michael Heimpold]

This series of two patches adds the following improvements to the driver:

1) Rework the central SPI read function so that it is compatible with
   SPI masters which only support half duplex transfers.

2) Add a device tree binding for the driver.

Michael Heimpold (2):
  net: ethernet: enc28j60: support half-duplex SPI controllers
  net: ethernet: enc28j60: add device tree support

 .../devicetree/bindings/net/microchip-enc28j60.txt | 49 ++++++++++++++++++++++
 drivers/net/ethernet/microchip/enc28j60.c          | 27 +++++++++---
 2 files changed, 70 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/microchip-enc28j60.txt

-- 
2.5.0

[PATCH net-next 1/2] net: ethernet: enc28j60: support half-duplex SPI controllers

[Michael Heimpold]

The current spi_read_buf function fails on SPI host masters which
are only half-duplex capable. Splitting the Tx and Rx part solves
this issue.

Tested on Raspberry Pi (full duplex) and I2SE Duckbill (half duplex).

Signed-off-by: Michael Heimpold <mhei@heimpold.de>
---
 drivers/net/ethernet/microchip/enc28j60.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c
index 86ea17e..b723622 100644
--- a/drivers/net/ethernet/microchip/enc28j60.c
+++ b/drivers/net/ethernet/microchip/enc28j60.c
@@ -89,22 +89,26 @@ spi_read_buf(struct enc28j60_net *priv, int len, u8 *data)
 {
 	u8 *rx_buf = priv->spi_transfer_buf + 4;
 	u8 *tx_buf = priv->spi_transfer_buf;
-	struct spi_transfer t = {
+	struct spi_transfer tx = {
 		.tx_buf = tx_buf,
+		.len = SPI_OPLEN,
+	};
+	struct spi_transfer rx = {
 		.rx_buf = rx_buf,
-		.len = SPI_OPLEN + len,
+		.len = len,
 	};
 	struct spi_message msg;
 	int ret;
 
 	tx_buf[0] = ENC28J60_READ_BUF_MEM;
-	tx_buf[1] = tx_buf[2] = tx_buf[3] = 0;	/* don't care */
 
 	spi_message_init(&msg);
-	spi_message_add_tail(&t, &msg);
+	spi_message_add_tail(&tx, &msg);
+	spi_message_add_tail(&rx, &msg);
+
 	ret = spi_sync(priv->spi, &msg);
 	if (ret == 0) {
-		memcpy(data, &rx_buf[SPI_OPLEN], len);
+		memcpy(data, rx_buf, len);
 		ret = msg.status;
 	}
 	if (ret && netif_msg_drv(priv))
-- 
2.5.0

[PATCH net-next 2/2] net: ethernet: enc28j60: add device tree support

[Michael Heimpold]

The following patch adds the required match table for device tree support
(and while at, fix the indent).

Also add the corresponding binding documentation file.

Signed-off-by: Michael Heimpold <mhei@heimpold.de>
---
 .../devicetree/bindings/net/microchip-enc28j60.txt | 49 ++++++++++++++++++++++
 drivers/net/ethernet/microchip/enc28j60.c          | 13 +++++-
 2 files changed, 61 insertions(+), 1 deletion(-)
 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..c472427
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/microchip-enc28j60.txt
@@ -0,0 +1,49 @@
+* 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.
+
+
+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..49635d7 100644
--- a/drivers/net/ethernet/microchip/enc28j60.c
+++ b/drivers/net/ethernet/microchip/enc28j60.c
@@ -1634,9 +1634,20 @@ static int enc28j60_remove(struct spi_device *spi)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id enc28j60_dt_ids[] = {
+	{ .compatible = "microchip,enc28j60" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, enc28j60_dt_ids);
+#else
+#define enc28j60_dt_ids NULL
+#endif
+
 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,
-- 
2.5.0

Re: [PATCH net-next 2/2] net: ethernet: enc28j60: add device tree support

[Arnd Bergmann]

On Saturday 23 April 2016 23:43:20 Michael Heimpold wrote:
> diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c
> index b723622..49635d7 100644
> --- a/drivers/net/ethernet/microchip/enc28j60.c
> +++ b/drivers/net/ethernet/microchip/enc28j60.c
> @@ -1634,9 +1634,20 @@ static int enc28j60_remove(struct spi_device *spi)
>         return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id enc28j60_dt_ids[] = {
> +       { .compatible = "microchip,enc28j60" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, enc28j60_dt_ids);
> +#else
> +#define enc28j60_dt_ids NULL
> +#endif
> +
>  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,
> -- 
> 

You probably also want to add support for calling of_get_mac_address()
here, to allow giving the device a fixed address from the boot loader.

I think you can drop the #ifdef and just define the device table
unconditionally.

	Arnd

[PATCH v2 net-next] net: ethernet: enc28j60: add device tree support

[Michael Heimpold]

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)
+		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,
-- 
2.5.0

Re: [PATCH v2 net-next] net: ethernet: enc28j60: add device tree support

[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

>  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>;
> +                };
> +        };
> +

Re: [PATCH v2 net-next] net: ethernet: enc28j60: add device tree support

[Michael Heimpold]

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>;
> > +                };
> > +        };
> > +

Re: [PATCH v2 net-next] net: ethernet: enc28j60: add device tree support

[Andrew F. Davis]

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-Z/Lg1yOAjpkb1SvskN2V4Q@public.gmane.org>
> ---
> 
> 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,
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 net-next] net: ethernet: enc28j60: add device tree support

[Michael Heimpold]

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,

Re: [PATCH v2 net-next] net: ethernet: enc28j60: add device tree support

[Andrew F. Davis]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html