[RFC PATCH 1/3] arcnet: com20020: Add memory map of com20020

[RFC PATCH 1/3] arcnet: com20020: Add memory map of com20020

[Andrea Greco]

From: Andrea Greco <a.greco@4sigma.it>

Add support for com20022I/com20020, memory mapped chip version.
Support bus: Intel 80xx and Motorola 68xx.
Bus size: Only 8 bit bus size is supported.
Added related device tree bindings

Signed-off-by: Andrea Greco <a.greco@4sigma.it>
---
 .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
 drivers/net/arcnet/Kconfig                         |  12 +-
 drivers/net/arcnet/Makefile                        |   1 +
 drivers/net/arcnet/arcdevice.h                     |  27 ++-
 drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
 drivers/net/arcnet/com20020.c                      |   9 +-
 6 files changed, 253 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
 create mode 100644 drivers/net/arcnet/com20020-membus.c

diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
new file mode 100644
index 000000000000..39c5b19c55af
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
@@ -0,0 +1,23 @@
+SMSC com20020, com20022I
+
+timeout: Arcnet timeout, checkout datashet
+clockp: Clock Prescaler, checkout datashet
+clockm: Clock multiplier, checkout datasheet
+
+phy-reset-gpios: Chip reset ppin
+phy-irq-gpios: Chip irq pin
+
+com20020_A@0 {
+    compatible = "smsc,com20020";
+
+	timeout	= <0x3>;
+	backplane = <0x0>;
+
+	clockp = <0x0>;
+	clockm = <0x3>;
+
+	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
+	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
+
+	status = "okay";
+};
diff --git a/drivers/net/arcnet/Kconfig b/drivers/net/arcnet/Kconfig
index 39bd16f3f86d..d39faf45be1e 100644
--- a/drivers/net/arcnet/Kconfig
+++ b/drivers/net/arcnet/Kconfig
@@ -3,7 +3,7 @@
 #
 
 menuconfig ARCNET
-	depends on NETDEVICES && (ISA || PCI || PCMCIA)
+	depends on NETDEVICES
 	tristate "ARCnet support"
 	---help---
 	  If you have a network card of this type, say Y and check out the
@@ -129,5 +129,15 @@ config ARCNET_COM20020_CS
 
 	  To compile this driver as a module, choose M here: the module will be
 	  called com20020_cs.  If unsure, say N.
+config ARCNET_COM20020_MEMORY_BUS
+	bool "Support for COM20020 on external memory"
+	depends on ARCNET_COM20020 && !(ARCNET_COM20020_PCI || ARCNET_COM20020_ISA || ARCNET_COM20020_CS)
+	help
+	  Say Y here if on your custom board mount com20020 or friends.
+
+	  Com20022I support arcnet bus 10Mbitps.
+	  This driver support only 8bit, and DMA is not supported is attached on your board at external interface bus.
+	  Supported bus Intel80xx / Motorola 68xx.
+	  This driver not work with other com20020 module: PCI or PCMCIA compiled as [M].
 
 endif # ARCNET
diff --git a/drivers/net/arcnet/Makefile b/drivers/net/arcnet/Makefile
index 53525e8ea130..19425c1e06f4 100644
--- a/drivers/net/arcnet/Makefile
+++ b/drivers/net/arcnet/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_ARCNET_COM20020) += com20020.o
 obj-$(CONFIG_ARCNET_COM20020_ISA) += com20020-isa.o
 obj-$(CONFIG_ARCNET_COM20020_PCI) += com20020-pci.o
 obj-$(CONFIG_ARCNET_COM20020_CS) += com20020_cs.o
+obj-$(CONFIG_ARCNET_COM20020_MEMORY_BUS) += com20020-membus.o
diff --git a/drivers/net/arcnet/arcdevice.h b/drivers/net/arcnet/arcdevice.h
index d09b2b46ab63..16c608269cca 100644
--- a/drivers/net/arcnet/arcdevice.h
+++ b/drivers/net/arcnet/arcdevice.h
@@ -201,7 +201,7 @@ struct ArcProto {
 	void (*rx)(struct net_device *dev, int bufnum,
 		   struct archdr *pkthdr, int length);
 	int (*build_header)(struct sk_buff *skb, struct net_device *dev,
-			    unsigned short ethproto, uint8_t daddr);
+				unsigned short ethproto, uint8_t daddr);
 
 	/* these functions return '1' if the skb can now be freed */
 	int (*prepare_tx)(struct net_device *dev, struct archdr *pkt,
@@ -326,9 +326,9 @@ struct arcnet_local {
 		void (*recontrigger) (struct net_device * dev, int enable);
 
 		void (*copy_to_card)(struct net_device *dev, int bufnum,
-				     int offset, void *buf, int count);
+					 int offset, void *buf, int count);
 		void (*copy_from_card)(struct net_device *dev, int bufnum,
-				       int offset, void *buf, int count);
+					   int offset, void *buf, int count);
 	} hw;
 
 	void __iomem *mem_start;	/* pointer to ioremap'ed MMIO */
@@ -360,7 +360,7 @@ struct net_device *alloc_arcdev(const char *name);
 int arcnet_open(struct net_device *dev);
 int arcnet_close(struct net_device *dev);
 netdev_tx_t arcnet_send_packet(struct sk_buff *skb,
-			       struct net_device *dev);
+				   struct net_device *dev);
 void arcnet_timeout(struct net_device *dev);
 
 /* I/O equivalents */
@@ -371,7 +371,23 @@ void arcnet_timeout(struct net_device *dev);
 #define BUS_ALIGN  1
 #endif
 
-/* addr and offset allow register like names to define the actual IO  address.
+#ifdef CONFIG_ARCNET_COM20020_MEMORY_BUS
+#define arcnet_inb(addr, offset)					\
+	ioread8((void __iomem *)(addr) + BUS_ALIGN * (offset))
+
+#define arcnet_outb(value, addr, offset)				\
+	iowrite8(value, (void __iomem *)(addr) + BUS_ALIGN * (offset))
+
+#define arcnet_insb(addr, offset, buffer, count)			\
+	ioread8_rep((void __iomem *)					\
+	(addr) + BUS_ALIGN * (offset), buffer, count)
+
+#define arcnet_outsb(addr, offset, buffer, count)			\
+	iowrite8_rep((void __iomem *)					\
+	(addr) + BUS_ALIGN * (offset), buffer, count)
+#else
+/**
+ * addr and offset allow register like names to define the actual IO  address.
  * A configuration option multiplies the offset for alignment.
  */
 #define arcnet_inb(addr, offset)					\
@@ -388,6 +404,7 @@ void arcnet_timeout(struct net_device *dev);
 	readb((addr) + (offset))
 #define arcnet_writeb(value, addr, offset)				\
 	writeb(value, (addr) + (offset))
+#endif
 
 #endif				/* __KERNEL__ */
 #endif				/* _LINUX_ARCDEVICE_H */
diff --git a/drivers/net/arcnet/com20020-membus.c b/drivers/net/arcnet/com20020-membus.c
new file mode 100644
index 000000000000..6e4a2f3a84f7
--- /dev/null
+++ b/drivers/net/arcnet/com20020-membus.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/* Linux ARCnet driver for com 20020.
+ *
+ * This datasheet:
+ * http://ww1.microchip.com/downloads/en/DeviceDoc/200223vrevc.pdf
+ * http://ww1.microchip.com/downloads/en/DeviceDoc/20020.pdf
+ *
+ * This driver support:
+ * - com20020,
+ * - com20022
+ * - com20022I-3v3
+ *
+ * This driver support only, 8bit read and write.
+ * DMA is not supported by this driver.
+ */
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/platform_device.h>
+#include <linux/netdevice.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/sizes.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/random.h>
+
+#include <linux/delay.h>
+#include "arcdevice.h"
+#include "com20020.h"
+
+#define VERSION "arcnet: COM20020 MEMORY BUS support loaded.\n"
+
+static int com20020_probe(struct platform_device *pdev)
+{
+	struct device_node *np;
+	struct net_device *dev;
+	struct arcnet_local *lp;
+	struct resource res, *iores;
+	int ret, phy_reset, err;
+	u32 timeout, backplane, clockp, clockm;
+	void __iomem *ioaddr;
+
+	np = pdev->dev.of_node;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (of_address_to_resource(np, 0, &res))
+		return -EINVAL;
+
+	ret = of_property_read_u32(np, "timeout", &timeout);
+	if (ret) {
+		dev_err(&pdev->dev, "timeout is required param");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "backplane", &backplane);
+	if (ret) {
+		dev_err(&pdev->dev, "backplane is required param");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "clockp", &clockp);
+	if (ret) {
+		dev_err(&pdev->dev, "clockp is required param");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "clockm", &clockm);
+	if (ret) {
+		dev_err(&pdev->dev, "clockm is required param");
+		return ret;
+	}
+
+	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+	if (phy_reset == -EPROBE_DEFER) {
+		return phy_reset;
+	} else if (!gpio_is_valid(phy_reset)) {
+		dev_err(&pdev->dev, "phy-reset-gpios not valid !");
+		return 0;
+	}
+
+	err = devm_gpio_request_one(&pdev->dev, phy_reset, GPIOF_OUT_INIT_LOW,
+				    "arcnet-phy-reset");
+	if (err) {
+		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
+		return err;
+	}
+
+	dev = alloc_arcdev(NULL);// Let autoassign name arc%d
+	dev->netdev_ops = &com20020_netdev_ops;
+	lp = netdev_priv(dev);
+
+	lp->card_flags = ARC_CAN_10MBIT;/* pretend all of them can 10Mbit */
+
+	// Force address to 0
+	// Will be set by user with `ip set dev arc0 address YOUR_NODE_ID`
+	dev->dev_addr[0] = 0;
+
+	// request to system this memory region
+	if (!devm_request_mem_region(&pdev->dev, res.start, resource_size(&res),
+				     lp->card_name))
+		return -EBUSY;
+
+	ioaddr = devm_ioremap(&pdev->dev, iores->start, resource_size(iores));
+	if (!ioaddr) {
+		dev_err(&pdev->dev, "ioremap fallied\n");
+		return -ENOMEM;
+	}
+
+	// Reset time is 5 * xTalFreq, minimal xtal is 10Mhz
+	// (5 * 1000) / 10Mhz = 500ns
+
+	gpio_set_value_cansleep(phy_reset, 0);
+	ndelay(500);
+	gpio_set_value_cansleep(phy_reset, 1);
+	ndelay(500);
+
+	/* Dummy access after Reset
+	 * ARCNET controller needs
+	 * this access to detect bustype
+	 */
+	arcnet_outb(0x00, ioaddr, COM20020_REG_W_COMMAND);
+	arcnet_inb(ioaddr, COM20020_REG_R_DIAGSTAT);
+
+	dev->base_addr = (unsigned long)ioaddr;
+	get_random_bytes(dev->dev_addr, sizeof(u8));
+
+	dev->irq = of_get_named_gpio(np, "phy-irq-gpios", 0);
+	if (dev->irq == -EPROBE_DEFER) {
+		return dev->irq;
+	} else if (!gpio_is_valid(dev->irq)) {
+		dev_err(&pdev->dev, "phy-irq-gpios not valid !");
+		return 0;
+	}
+	dev->irq = gpio_to_irq(dev->irq);
+
+	lp->backplane = backplane;
+	lp->clockp = clockp & 7;
+	lp->clockm = clockm & 3;
+	lp->timeout = timeout;
+	lp->hw.owner = THIS_MODULE;
+
+	if (arcnet_inb(ioaddr, COM20020_REG_R_STATUS) == 0xFF) {
+		ret = -EIO;
+		goto err_release_mem;
+	}
+
+	if (com20020_check(dev)) {
+		ret = -EIO;
+		goto err_release_mem;
+	}
+
+	ret = com20020_found(dev, IRQF_TRIGGER_FALLING);
+	if (ret)
+		goto err_release_mem;
+
+	dev_dbg(&pdev->dev, "probe Done\n");
+	return 0;
+
+err_release_mem:
+	devm_iounmap(&pdev->dev, (void __iomem *)ioaddr);
+	devm_release_mem_region(&pdev->dev, res.start, resource_size(&res));
+	dev_err(&pdev->dev, "probe failed!\n");
+	return ret;
+}
+
+static const struct of_device_id of_com20020_match[] = {
+	{ .compatible = "smsc,com20020",	},
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, of_com20020_match);
+
+static struct platform_driver of_com20020_driver = {
+	.driver			= {
+		.name		= "com20020-memory-bus",
+		.of_match_table = of_com20020_match,
+	},
+	.probe			= com20020_probe,
+};
+
+static int com20020_init(void)
+{
+	return platform_driver_register(&of_com20020_driver);
+}
+late_initcall(com20020_init);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/arcnet/com20020.c b/drivers/net/arcnet/com20020.c
index 78043a9c5981..f09ea77dd6a8 100644
--- a/drivers/net/arcnet/com20020.c
+++ b/drivers/net/arcnet/com20020.c
@@ -43,7 +43,7 @@
 #include "com20020.h"
 
 static const char * const clockrates[] = {
-	"XXXXXXX", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
+	"10 Mb/s", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
 	"1.25Mb/s", "625 Kb/s", "312.5 Kb/s", "156.25 Kb/s",
 	"Reserved", "Reserved", "Reserved"
 };
@@ -391,9 +391,10 @@ static void com20020_set_mc_list(struct net_device *dev)
 	}
 }
 
-#if defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
-    defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
-    defined(CONFIG_ARCNET_COM20020_CS_MODULE)
+#if	defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
+	defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
+	defined(CONFIG_ARCNET_COM20020_CS_MODULE)  || \
+	defined(CONFIG_ARCNET_COM20020_MEMORY_BUS)
 EXPORT_SYMBOL(com20020_check);
 EXPORT_SYMBOL(com20020_found);
 EXPORT_SYMBOL(com20020_netdev_ops);
-- 
2.14.3

Re: [RFC PATCH 1/3] arcnet: com20020: Add memory map of com20020

[Tobin C. Harding]

On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
> From: Andrea Greco <a.greco@4sigma.it>

Hi Andrea,

Here are some (mostly stylistic) suggestions to help you get your driver merged.

> Add support for com20022I/com20020, memory mapped chip version.
> Support bus: Intel 80xx and Motorola 68xx.
> Bus size: Only 8 bit bus size is supported.
> Added related device tree bindings
> 
> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
> ---
>  .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
>  drivers/net/arcnet/Kconfig                         |  12 +-
>  drivers/net/arcnet/Makefile                        |   1 +
>  drivers/net/arcnet/arcdevice.h                     |  27 ++-
>  drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>  drivers/net/arcnet/com20020.c                      |   9 +-
>  6 files changed, 253 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>  create mode 100644 drivers/net/arcnet/com20020-membus.c
> 
> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> new file mode 100644
> index 000000000000..39c5b19c55af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> @@ -0,0 +1,23 @@
> +SMSC com20020, com20022I
> +
> +timeout: Arcnet timeout, checkout datashet
> +clockp: Clock Prescaler, checkout datashet
> +clockm: Clock multiplier, checkout datasheet
> +
> +phy-reset-gpios: Chip reset ppin
> +phy-irq-gpios: Chip irq pin
> +
> +com20020_A@0 {
> +    compatible = "smsc,com20020";
> +
> +	timeout	= <0x3>;
> +	backplane = <0x0>;
> +
> +	clockp = <0x0>;
> +	clockm = <0x3>;
> +
> +	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
> +	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
> +
> +	status = "okay";
> +};
> diff --git a/drivers/net/arcnet/Kconfig b/drivers/net/arcnet/Kconfig
> index 39bd16f3f86d..d39faf45be1e 100644
> --- a/drivers/net/arcnet/Kconfig
> +++ b/drivers/net/arcnet/Kconfig
> @@ -3,7 +3,7 @@
>  #
>  
>  menuconfig ARCNET
> -	depends on NETDEVICES && (ISA || PCI || PCMCIA)
> +	depends on NETDEVICES
>  	tristate "ARCnet support"
>  	---help---
>  	  If you have a network card of this type, say Y and check out the
> @@ -129,5 +129,15 @@ config ARCNET_COM20020_CS
>  
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called com20020_cs.  If unsure, say N.
> +config ARCNET_COM20020_MEMORY_BUS
> +	bool "Support for COM20020 on external memory"
> +	depends on ARCNET_COM20020 && !(ARCNET_COM20020_PCI || ARCNET_COM20020_ISA || ARCNET_COM20020_CS)
> +	help
> +	  Say Y here if on your custom board mount com20020 or friends.
> +
> +	  Com20022I support arcnet bus 10Mbitps.
> +	  This driver support only 8bit

This driver only supports 8bit bus size.

>         	    	 	       , and DMA is not supported is attached on your board at external interface bus.

This bit does not make sense, sorry.

> +	  Supported bus Intel80xx / Motorola 68xx.
> +	  This driver not work with other com20020 module: PCI or PCMCIA compiled as [M].

I'm not sure exactly what you want to say here, perhaps:

  	  This driver does not work with other com20020 modules compiled
	  as PCI or PCMCIA [M].
>  
>  endif # ARCNET
> diff --git a/drivers/net/arcnet/Makefile b/drivers/net/arcnet/Makefile
> index 53525e8ea130..19425c1e06f4 100644
> --- a/drivers/net/arcnet/Makefile
> +++ b/drivers/net/arcnet/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_ARCNET_COM20020) += com20020.o
>  obj-$(CONFIG_ARCNET_COM20020_ISA) += com20020-isa.o
>  obj-$(CONFIG_ARCNET_COM20020_PCI) += com20020-pci.o
>  obj-$(CONFIG_ARCNET_COM20020_CS) += com20020_cs.o
> +obj-$(CONFIG_ARCNET_COM20020_MEMORY_BUS) += com20020-membus.o
> diff --git a/drivers/net/arcnet/arcdevice.h b/drivers/net/arcnet/arcdevice.h
> index d09b2b46ab63..16c608269cca 100644
> --- a/drivers/net/arcnet/arcdevice.h
> +++ b/drivers/net/arcnet/arcdevice.h
> @@ -201,7 +201,7 @@ struct ArcProto {
>  	void (*rx)(struct net_device *dev, int bufnum,
>  		   struct archdr *pkthdr, int length);
>  	int (*build_header)(struct sk_buff *skb, struct net_device *dev,
> -			    unsigned short ethproto, uint8_t daddr);
> +				unsigned short ethproto, uint8_t daddr);

  +			    unsigned short ethproto, uint8_t daddr);

Please use Linux coding style style, parameters continuing on separate
line are aligned with opening parenthesis.

>  	/* these functions return '1' if the skb can now be freed */
>  	int (*prepare_tx)(struct net_device *dev, struct archdr *pkt,
> @@ -326,9 +326,9 @@ struct arcnet_local {
>  		void (*recontrigger) (struct net_device * dev, int enable);
>  
>  		void (*copy_to_card)(struct net_device *dev, int bufnum,
> -				     int offset, void *buf, int count);
> +					 int offset, void *buf, int count);
>  		void (*copy_from_card)(struct net_device *dev, int bufnum,
> -				       int offset, void *buf, int count);
> +					   int offset, void *buf, int count);
>  	} hw;
>  
>  	void __iomem *mem_start;	/* pointer to ioremap'ed MMIO */
> @@ -360,7 +360,7 @@ struct net_device *alloc_arcdev(const char *name);
>  int arcnet_open(struct net_device *dev);
>  int arcnet_close(struct net_device *dev);
>  netdev_tx_t arcnet_send_packet(struct sk_buff *skb,
> -			       struct net_device *dev);
> +				   struct net_device *dev);
>  void arcnet_timeout(struct net_device *dev);
>  
>  /* I/O equivalents */
> @@ -371,7 +371,23 @@ void arcnet_timeout(struct net_device *dev);
>  #define BUS_ALIGN  1
>  #endif
>  
> -/* addr and offset allow register like names to define the actual IO  address.
> +#ifdef CONFIG_ARCNET_COM20020_MEMORY_BUS
> +#define arcnet_inb(addr, offset)					\
> +	ioread8((void __iomem *)(addr) + BUS_ALIGN * (offset))
> +
> +#define arcnet_outb(value, addr, offset)				\
> +	iowrite8(value, (void __iomem *)(addr) + BUS_ALIGN * (offset))
> +
> +#define arcnet_insb(addr, offset, buffer, count)			\
> +	ioread8_rep((void __iomem *)					\
> +	(addr) + BUS_ALIGN * (offset), buffer, count)
> +
> +#define arcnet_outsb(addr, offset, buffer, count)			\
> +	iowrite8_rep((void __iomem *)					\
> +	(addr) + BUS_ALIGN * (offset), buffer, count)
> +#else
> +/**
> + * addr and offset allow register like names to define the actual IO  address.
>   * A configuration option multiplies the offset for alignment.
>   */
>  #define arcnet_inb(addr, offset)					\
> @@ -388,6 +404,7 @@ void arcnet_timeout(struct net_device *dev);
>  	readb((addr) + (offset))
>  #define arcnet_writeb(value, addr, offset)				\
>  	writeb(value, (addr) + (offset))
> +#endif
>  
>  #endif				/* __KERNEL__ */
>  #endif				/* _LINUX_ARCDEVICE_H */
> diff --git a/drivers/net/arcnet/com20020-membus.c b/drivers/net/arcnet/com20020-membus.c
> new file mode 100644
> index 000000000000..6e4a2f3a84f7
> --- /dev/null
> +++ b/drivers/net/arcnet/com20020-membus.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/* Linux ARCnet driver for com 20020.
> + *
> + * This datasheet:
> + * http://ww1.microchip.com/downloads/en/DeviceDoc/200223vrevc.pdf
> + * http://ww1.microchip.com/downloads/en/DeviceDoc/20020.pdf
> + *
> + * This driver support:

    * This driver supports:

> + * - com20020,
> + * - com20022
> + * - com20022I-3v3
> + *
> + * This driver support only, 8bit read and write.

    * This driver supports only 8bit read and write.

> + * DMA is not supported by this driver.
> + */
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/platform_device.h>
> +#include <linux/netdevice.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/sizes.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioport.h>
> +#include <linux/random.h>
> +
> +#include <linux/delay.h>
> +#include "arcdevice.h"
> +#include "com20020.h"

White space line is not needed here, you might have meant to have it one
line down?

> +
> +#define VERSION "arcnet: COM20020 MEMORY BUS support loaded.\n"
> +
> +static int com20020_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np;
> +	struct net_device *dev;
> +	struct arcnet_local *lp;
> +	struct resource res, *iores;
> +	int ret, phy_reset, err;
> +	u32 timeout, backplane, clockp, clockm;
> +	void __iomem *ioaddr;
> +
> +	np = pdev->dev.of_node;
> +
> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	if (of_address_to_resource(np, 0, &res))
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32(np, "timeout", &timeout);
> +	if (ret) {
> +		dev_err(&pdev->dev, "timeout is required param");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "backplane", &backplane);
> +	if (ret) {
> +		dev_err(&pdev->dev, "backplane is required param");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "clockp", &clockp);
> +	if (ret) {
> +		dev_err(&pdev->dev, "clockp is required param");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "clockm", &clockm);
> +	if (ret) {
> +		dev_err(&pdev->dev, "clockm is required param");
> +		return ret;
> +	}
> +
> +	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> +	if (phy_reset == -EPROBE_DEFER) {
> +		return phy_reset;
> +	} else if (!gpio_is_valid(phy_reset)) {
> +		dev_err(&pdev->dev, "phy-reset-gpios not valid !");
> +		return 0;
> +	}
> +
> +	err = devm_gpio_request_one(&pdev->dev, phy_reset, GPIOF_OUT_INIT_LOW,
> +				    "arcnet-phy-reset");
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
> +		return err;
> +	}
> +
> +	dev = alloc_arcdev(NULL);// Let autoassign name arc%d

/* C89 style comments please */

> +	dev->netdev_ops = &com20020_netdev_ops;
> +	lp = netdev_priv(dev);
> +
> +	lp->card_flags = ARC_CAN_10MBIT;/* pretend all of them can 10Mbit */
> +
> +	// Force address to 0

Unnecessary, we can see this in the code :)  Please don't comment 'what'
the code does (unless it is obfuscated or difficult to read).  You may
still like to comment 'why' the code does what it does though.

> +	// Will be set by user with `ip set dev arc0 address YOUR_NODE_ID`
> +	dev->dev_addr[0] = 0;
> +
> +	// request to system this memory region

Same as above

> +	if (!devm_request_mem_region(&pdev->dev, res.start, resource_size(&res),
> +				     lp->card_name))
> +		return -EBUSY;
> +
> +	ioaddr = devm_ioremap(&pdev->dev, iores->start, resource_size(iores));
> +	if (!ioaddr) {
> +		dev_err(&pdev->dev, "ioremap fallied\n");
> +		return -ENOMEM;
> +	}
> +
> +	// Reset time is 5 * xTalFreq, minimal xtal is 10Mhz
> +	// (5 * 1000) / 10Mhz = 500ns

perhaps a macro definition
#define MAX_XTAL_RESET_TIME ??

> +
> +	gpio_set_value_cansleep(phy_reset, 0);
> +	ndelay(500);
> +	gpio_set_value_cansleep(phy_reset, 1);
> +	ndelay(500);
> +
> +	/* Dummy access after Reset
> +	 * ARCNET controller needs
> +	 * this access to detect bustype
> +	 */

nit: Upto 72 characters wide is fine for comments

	/* Dummy access after Reset ARCNET controller needs
	 * this access to detect bustype
	 */

> +	arcnet_outb(0x00, ioaddr, COM20020_REG_W_COMMAND);
> +	arcnet_inb(ioaddr, COM20020_REG_R_DIAGSTAT);
> +
> +	dev->base_addr = (unsigned long)ioaddr;
> +	get_random_bytes(dev->dev_addr, sizeof(u8));
> +
> +	dev->irq = of_get_named_gpio(np, "phy-irq-gpios", 0);
> +	if (dev->irq == -EPROBE_DEFER) {
> +		return dev->irq;
> +	} else if (!gpio_is_valid(dev->irq)) {
> +		dev_err(&pdev->dev, "phy-irq-gpios not valid !");
> +		return 0;
> +	}
> +	dev->irq = gpio_to_irq(dev->irq);
> +
> +	lp->backplane = backplane;
> +	lp->clockp = clockp & 7;
> +	lp->clockm = clockm & 3;
> +	lp->timeout = timeout;
> +	lp->hw.owner = THIS_MODULE;
> +
> +	if (arcnet_inb(ioaddr, COM20020_REG_R_STATUS) == 0xFF) {
> +		ret = -EIO;
> +		goto err_release_mem;
> +	}
> +
> +	if (com20020_check(dev)) {
> +		ret = -EIO;
> +		goto err_release_mem;
> +	}
> +
> +	ret = com20020_found(dev, IRQF_TRIGGER_FALLING);
> +	if (ret)
> +		goto err_release_mem;
> +
> +	dev_dbg(&pdev->dev, "probe Done\n");
> +	return 0;
> +
> +err_release_mem:
> +	devm_iounmap(&pdev->dev, (void __iomem *)ioaddr);
> +	devm_release_mem_region(&pdev->dev, res.start, resource_size(&res));
> +	dev_err(&pdev->dev, "probe failed!\n");
> +	return ret;
> +}
> +
> +static const struct of_device_id of_com20020_match[] = {
> +	{ .compatible = "smsc,com20020",	},
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, of_com20020_match);
> +
> +static struct platform_driver of_com20020_driver = {
> +	.driver			= {
> +		.name		= "com20020-memory-bus",
> +		.of_match_table = of_com20020_match,
> +	},
> +	.probe			= com20020_probe,
> +};
> +
> +static int com20020_init(void)
> +{
> +	return platform_driver_register(&of_com20020_driver);
> +}
> +late_initcall(com20020_init);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/arcnet/com20020.c b/drivers/net/arcnet/com20020.c
> index 78043a9c5981..f09ea77dd6a8 100644
> --- a/drivers/net/arcnet/com20020.c
> +++ b/drivers/net/arcnet/com20020.c
> @@ -43,7 +43,7 @@
>  #include "com20020.h"
>  
>  static const char * const clockrates[] = {
> -	"XXXXXXX", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
> +	"10 Mb/s", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
>  	"1.25Mb/s", "625 Kb/s", "312.5 Kb/s", "156.25 Kb/s",
>  	"Reserved", "Reserved", "Reserved"
>  };
> @@ -391,9 +391,10 @@ static void com20020_set_mc_list(struct net_device *dev)
>  	}
>  }
>  
> -#if defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
> -    defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
> -    defined(CONFIG_ARCNET_COM20020_CS_MODULE)
> +#if	defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
> +	defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
> +	defined(CONFIG_ARCNET_COM20020_CS_MODULE)  || \
> +	defined(CONFIG_ARCNET_COM20020_MEMORY_BUS)

Why the whitespace change?

Hope this helps,
Tobin.

Re: [RFC PATCH 1/3] arcnet: com20020: Add memory map of com20020

[Andrea Greco]

On 05/07/2018 04:55 AM, Tobin C. Harding wrote:
> On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
>> From: Andrea Greco <a.greco@4sigma.it>
> 
> Hi Andrea,
> 
> Here are some (mostly stylistic) suggestions to help you get your driver merged.
> 
>> Add support for com20022I/com20020, memory mapped chip version.
>> Support bus: Intel 80xx and Motorola 68xx.
>> Bus size: Only 8 bit bus size is supported.
>> Added related device tree bindings
>>
>> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
>> ---
>>   .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
>>   drivers/net/arcnet/Kconfig                         |  12 +-
>>   drivers/net/arcnet/Makefile                        |   1 +
>>   drivers/net/arcnet/arcdevice.h                     |  27 ++-
>>   drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>>   drivers/net/arcnet/com20020.c                      |   9 +-
>>   6 files changed, 253 insertions(+), 10 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>>   create mode 100644 drivers/net/arcnet/com20020-membus.c
>>
>> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>> new file mode 100644
>> index 000000000000..39c5b19c55af
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>> @@ -0,0 +1,23 @@
>> +SMSC com20020, com20022I
>> +
>> +timeout: Arcnet timeout, checkout datashet
>> +clockp: Clock Prescaler, checkout datashet
>> +clockm: Clock multiplier, checkout datasheet
>> +
>> +phy-reset-gpios: Chip reset ppin
>> +phy-irq-gpios: Chip irq pin

ppin Corrected by my-self.

>> +
>> +com20020_A@0 {
>> +    compatible = "smsc,com20020";
>> +
>> +	timeout	= <0x3>;
>> +	backplane = <0x0>;
>> +
>> +	clockp = <0x0>;
>> +	clockm = <0x3>;
>> +
>> +	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
>> +	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
>> +
>> +	status = "okay";
>> +};
>> diff --git a/drivers/net/arcnet/Kconfig b/drivers/net/arcnet/Kconfig
>> index 39bd16f3f86d..d39faf45be1e 100644
>> --- a/drivers/net/arcnet/Kconfig
>> +++ b/drivers/net/arcnet/Kconfig
>> @@ -3,7 +3,7 @@
>>   #
>>   
>>   menuconfig ARCNET
>> -	depends on NETDEVICES && (ISA || PCI || PCMCIA)
>> +	depends on NETDEVICES
>>   	tristate "ARCnet support"
>>   	---help---
>>   	  If you have a network card of this type, say Y and check out the
>> @@ -129,5 +129,15 @@ config ARCNET_COM20020_CS
>>   
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called com20020_cs.  If unsure, say N.
>> +config ARCNET_COM20020_MEMORY_BUS
>> +	bool "Support for COM20020 on external memory"
>> +	depends on ARCNET_COM20020 && !(ARCNET_COM20020_PCI || ARCNET_COM20020_ISA || ARCNET_COM20020_CS)
>> +	help
>> +	  Say Y here if on your custom board mount com20020 or friends.
>> +
>> +	  Com20022I support arcnet bus 10Mbitps.
>> +	  This driver support only 8bit
> 
> This driver only supports 8bit bus size.
> 
>>          	    	 	       , and DMA is not supported is attached on your board at external interface bus.
> 
> This bit does not make sense, sorry.
Removed description,

Proposal for v2:
Say Y here if your custom board mount com20020 chipset or friends.
Supported Chipset: com20020, com20022, com20022I-3v3.
If unsure, say N.

>> +	  Supported bus Intel80xx / Motorola 68xx.
>> +	  This driver not work with other com20020 module: PCI or PCMCIA compiled as [M].
> 
> I'm not sure exactly what you want to say here, perhaps:
> 
>    	  This driver does not work with other com20020 modules compiled
> 	  as PCI or PCMCIA [M].

About this, all removed from kconfig
For PCI, PCMCIA, checkout downside

>>   
>>   endif # ARCNET
>> diff --git a/drivers/net/arcnet/Makefile b/drivers/net/arcnet/Makefile
>> index 53525e8ea130..19425c1e06f4 100644
>> --- a/drivers/net/arcnet/Makefile
>> +++ b/drivers/net/arcnet/Makefile
>> @@ -14,3 +14,4 @@ obj-$(CONFIG_ARCNET_COM20020) += com20020.o
>>   obj-$(CONFIG_ARCNET_COM20020_ISA) += com20020-isa.o
>>   obj-$(CONFIG_ARCNET_COM20020_PCI) += com20020-pci.o
>>   obj-$(CONFIG_ARCNET_COM20020_CS) += com20020_cs.o
>> +obj-$(CONFIG_ARCNET_COM20020_MEMORY_BUS) += com20020-membus.o
>> diff --git a/drivers/net/arcnet/arcdevice.h b/drivers/net/arcnet/arcdevice.h
>> index d09b2b46ab63..16c608269cca 100644
>> --- a/drivers/net/arcnet/arcdevice.h
>> +++ b/drivers/net/arcnet/arcdevice.h
>> @@ -201,7 +201,7 @@ struct ArcProto {
>>   	void (*rx)(struct net_device *dev, int bufnum,
>>   		   struct archdr *pkthdr, int length);
>>   	int (*build_header)(struct sk_buff *skb, struct net_device *dev,
>> -			    unsigned short ethproto, uint8_t daddr);
>> +				unsigned short ethproto, uint8_t daddr);
> 
>    +			    unsigned short ethproto, uint8_t daddr);
> 
> Please use Linux coding style style, parameters continuing on separate
> line are aligned with opening parenthesis.
> 
>>   	/* these functions return '1' if the skb can now be freed */
>>   	int (*prepare_tx)(struct net_device *dev, struct archdr *pkt,
>> @@ -326,9 +326,9 @@ struct arcnet_local {
>>   		void (*recontrigger) (struct net_device * dev, int enable);
>>   
>>   		void (*copy_to_card)(struct net_device *dev, int bufnum,
>> -				     int offset, void *buf, int count);
>> +					 int offset, void *buf, int count);
>>   		void (*copy_from_card)(struct net_device *dev, int bufnum,
>> -				       int offset, void *buf, int count);
>> +					   int offset, void *buf, int count);
>>   	} hw;
>>   
>>   	void __iomem *mem_start;	/* pointer to ioremap'ed MMIO */
>> @@ -360,7 +360,7 @@ struct net_device *alloc_arcdev(const char *name);
>>   int arcnet_open(struct net_device *dev);
>>   int arcnet_close(struct net_device *dev);
>>   netdev_tx_t arcnet_send_packet(struct sk_buff *skb,
>> -			       struct net_device *dev);
>> +				   struct net_device *dev);
>>   void arcnet_timeout(struct net_device *dev);

Not required modification then all removed.

>>   
>>   /* I/O equivalents */
>> @@ -371,7 +371,23 @@ void arcnet_timeout(struct net_device *dev);
>>   #define BUS_ALIGN  1
>>   #endif
>>   
>> -/* addr and offset allow register like names to define the actual IO  address.
>> +#ifdef CONFIG_ARCNET_COM20020_MEMORY_BUS
>> +#define arcnet_inb(addr, offset)					\
>> +	ioread8((void __iomem *)(addr) + BUS_ALIGN * (offset))
>> +
>> +#define arcnet_outb(value, addr, offset)				\
>> +	iowrite8(value, (void __iomem *)(addr) + BUS_ALIGN * (offset))
>> +
>> +#define arcnet_insb(addr, offset, buffer, count)			\
>> +	ioread8_rep((void __iomem *)					\
>> +	(addr) + BUS_ALIGN * (offset), buffer, count)
>> +
>> +#define arcnet_outsb(addr, offset, buffer, count)			\
>> +	iowrite8_rep((void __iomem *)					\
>> +	(addr) + BUS_ALIGN * (offset), buffer, count)
>> +#else
>> +/**
>> + * addr and offset allow register like names to define the actual IO  address.
>>    * A configuration option multiplies the offset for alignment.
>>    */
>>   #define arcnet_inb(addr, offset)					\
>> @@ -388,6 +404,7 @@ void arcnet_timeout(struct net_device *dev);
>>   	readb((addr) + (offset))
>>   #define arcnet_writeb(value, addr, offset)				\
>>   	writeb(value, (addr) + (offset))
>> +#endif
>>   
>>   #endif				/* __KERNEL__ */
>>   #endif				/* _LINUX_ARCDEVICE_H */


This is most important part where a suggestion will be very appreciated.
This part define how arcnet drivers read and write over physical.
The old driver can always use readb/writeb and friends, this driver rise 
big kernel panic.

This driver make a ioreamp with: devm_ioremap.
Then, for r/w operation i use ioread8/iowrite8 and friends.

My quickly solution was make a ugly #ifdef.

With #ifndef all other driver implementation could not be used together
this driver, because break, how driver write over physical.
A proposal could be add a read/write callback to arcdevice.h struct hw.

PROS:
Every driver fill this callback, this is a solution.

CONS:
This solution require a small change for all com20020 driver
implementations. I don't dispose of all hardware for make a accurate
test. I could only test memory bus version.

Any other ideas, will be very appreciated.

>> diff --git a/drivers/net/arcnet/com20020-membus.c b/drivers/net/arcnet/com20020-membus.c
>> new file mode 100644
>> index 000000000000..6e4a2f3a84f7
>> --- /dev/null
>> +++ b/drivers/net/arcnet/com20020-membus.c
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/* Linux ARCnet driver for com 20020.
>> + *
>> + * This datasheet:
>> + * http://ww1.microchip.com/downloads/en/DeviceDoc/200223vrevc.pdf
>> + * http://ww1.microchip.com/downloads/en/DeviceDoc/20020.pdf
>> + *
>> + * This driver support:
> 
>      * This driver supports:
> 
>> + * - com20020,
>> + * - com20022
>> + * - com20022I-3v3
>> + *
>> + * This driver support only, 8bit read and write.
> 
>      * This driver supports only 8bit read and write.
> 
>> + * DMA is not supported by this driver.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/types.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/sizes.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/ioport.h>
>> +#include <linux/random.h>
>> +
>> +#include <linux/delay.h>
>> +#include "arcdevice.h"
>> +#include "com20020.h"
> 
> White space line is not needed here, you might have meant to have it one
> line down?
>
Removed

>> +
>> +#define VERSION "arcnet: COM20020 MEMORY BUS support loaded.\n"
>> +
>> +static int com20020_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np;
>> +	struct net_device *dev;
>> +	struct arcnet_local *lp;
>> +	struct resource res, *iores;
>> +	int ret, phy_reset, err;
>> +	u32 timeout, backplane, clockp, clockm;
>> +	void __iomem *ioaddr;
>> +
>> +	np = pdev->dev.of_node;
>> +
>> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +	if (of_address_to_resource(np, 0, &res))
>> +		return -EINVAL;
>> +
>> +	ret = of_property_read_u32(np, "timeout", &timeout);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "timeout is required param");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "backplane", &backplane);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "backplane is required param");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "clockp", &clockp);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "clockp is required param");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "clockm", &clockm);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "clockm is required param");
>> +		return ret;
>> +	}
>> +
>> +	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>> +	if (phy_reset == -EPROBE_DEFER) {
>> +		return phy_reset;
>> +	} else if (!gpio_is_valid(phy_reset)) {
>> +		dev_err(&pdev->dev, "phy-reset-gpios not valid !");
>> +		return 0;
>> +	}
>> +
>> +	err = devm_gpio_request_one(&pdev->dev, phy_reset, GPIOF_OUT_INIT_LOW,
>> +				    "arcnet-phy-reset");
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	dev = alloc_arcdev(NULL);// Let autoassign name arc%d
> 
> /* C89 style comments please */

All comment bring to C89

> 
>> +	dev->netdev_ops = &com20020_netdev_ops;
>> +	lp = netdev_priv(dev);
>> +
>> +	lp->card_flags = ARC_CAN_10MBIT;/* pretend all of them can 10Mbit */
>> +
>> +	// Force address to 0
> 
Removed
> Unnecessary, we can see this in the code :)  Please don't comment 'what'
> the code does (unless it is obfuscated or difficult to read).  You may
> still like to comment 'why' the code does what it does though.
> 
>> +	// Will be set by user with `ip set dev arc0 address YOUR_NODE_ID`
>> +	dev->dev_addr[0] = 0;
>> +

All this become
/* Will be set by userspace during if setup */

>> +	// request to system this memory region
> 
> Same as above
> 
Removed

>> +	if (!devm_request_mem_region(&pdev->dev, res.start, resource_size(&res),
>> +				     lp->card_name))
>> +		return -EBUSY;
>> +
>> +	ioaddr = devm_ioremap(&pdev->dev, iores->start, resource_size(iores));
>> +	if (!ioaddr) {
>> +		dev_err(&pdev->dev, "ioremap fallied\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	// Reset time is 5 * xTalFreq, minimal xtal is 10Mhz
>> +	// (5 * 1000) / 10Mhz = 500ns
> 
> perhaps a macro definition
> #define MAX_XTAL_RESET_TIME ??

Macro made, on head of file.
Rereading Datasheet maby that last delay could be removed.
Then become
	gpio_set_value_cansleep(phy_reset, 0);
	ndelay(RESET_DELAY);
	gpio_set_value_cansleep(phy_reset, 1);
>> +
>> +	gpio_set_value_cansleep(phy_reset, 0);
>> +	ndelay(500);
>> +	gpio_set_value_cansleep(phy_reset, 1);
>> +	ndelay(500);
>> +
>> +	/* Dummy access after Reset
>> +	 * ARCNET controller needs
>> +	 * this access to detect bustype
>> +	 */
> 
> nit: Upto 72 characters wide is fine for comments
> 
> 	/* Dummy access after Reset ARCNET controller needs
> 	 * this access to detect bustype
> 	 */
> 
>> +	arcnet_outb(0x00, ioaddr, COM20020_REG_W_COMMAND);
>> +	arcnet_inb(ioaddr, COM20020_REG_R_DIAGSTAT);
>> +

Changed in:
	/* ARCNET controller needs this access to detect bustype */
	arcnet_outb(0x00, ioaddr, COM20020_REG_W_COMMAND);
	arcnet_inb(ioaddr, COM20020_REG_R_DIAGSTAT);

>> +	dev->base_addr = (unsigned long)ioaddr;
>> +	get_random_bytes(dev->dev_addr, sizeof(u8));
>> +
>> +	dev->irq = of_get_named_gpio(np, "phy-irq-gpios", 0);
>> +	if (dev->irq == -EPROBE_DEFER) {
>> +		return dev->irq;
>> +	} else if (!gpio_is_valid(dev->irq)) {
>> +		dev_err(&pdev->dev, "phy-irq-gpios not valid !");
>> +		return 0;
>> +	}
>> +	dev->irq = gpio_to_irq(dev->irq);
>> +
>> +	lp->backplane = backplane;
>> +	lp->clockp = clockp & 7;
>> +	lp->clockm = clockm & 3;
>> +	lp->timeout = timeout;
>> +	lp->hw.owner = THIS_MODULE;
>> +
>> +	if (arcnet_inb(ioaddr, COM20020_REG_R_STATUS) == 0xFF) {
>> +		ret = -EIO;
>> +		goto err_release_mem;
>> +	}
>> +
>> +	if (com20020_check(dev)) {
>> +		ret = -EIO;
>> +		goto err_release_mem;
>> +	}
>> +
>> +	ret = com20020_found(dev, IRQF_TRIGGER_FALLING);
>> +	if (ret)
>> +		goto err_release_mem;
>> +
>> +	dev_dbg(&pdev->dev, "probe Done\n");
>> +	return 0;
>> +
>> +err_release_mem:
>> +	devm_iounmap(&pdev->dev, (void __iomem *)ioaddr);
>> +	devm_release_mem_region(&pdev->dev, res.start, resource_size(&res));
>> +	dev_err(&pdev->dev, "probe failed!\n");
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id of_com20020_match[] = {
>> +	{ .compatible = "smsc,com20020",	},
>> +	{ },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, of_com20020_match);
>> +
>> +static struct platform_driver of_com20020_driver = {
>> +	.driver			= {
>> +		.name		= "com20020-memory-bus",
>> +		.of_match_table = of_com20020_match,
>> +	},
>> +	.probe			= com20020_probe,
>> +};
>> +
>> +static int com20020_init(void)
>> +{
>> +	return platform_driver_register(&of_com20020_driver);
>> +}
>> +late_initcall(com20020_init);
>> +
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/net/arcnet/com20020.c b/drivers/net/arcnet/com20020.c
>> index 78043a9c5981..f09ea77dd6a8 100644
>> --- a/drivers/net/arcnet/com20020.c
>> +++ b/drivers/net/arcnet/com20020.c
>> @@ -43,7 +43,7 @@
>>   #include "com20020.h"
>>   
>>   static const char * const clockrates[] = {
>> -	"XXXXXXX", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
>> +	"10 Mb/s", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
>>   	"1.25Mb/s", "625 Kb/s", "312.5 Kb/s", "156.25 Kb/s",
>>   	"Reserved", "Reserved", "Reserved"
>>   };
>> @@ -391,9 +391,10 @@ static void com20020_set_mc_list(struct net_device *dev)
>>   	}
>>   }
>>   
>> -#if defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
>> -    defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
>> -    defined(CONFIG_ARCNET_COM20020_CS_MODULE)
>> +#if	defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
>> +	defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
>> +	defined(CONFIG_ARCNET_COM20020_CS_MODULE)  || \
>> +	defined(CONFIG_ARCNET_COM20020_MEMORY_BUS)
> 
> Why the whitespace change?

      defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
-    defined(CONFIG_ARCNET_COM20020_CS_MODULE)
+    defined(CONFIG_ARCNET_COM20020_CS_MODULE)  || \
+    defined(CONFIG_ARCNET_COM20020_MEMORY_BUS)

Removed useless withespace

> 
> Hope this helps,
> Tobin.
> 

Thank you for help,
Andrea

Re: [RFC PATCH 1/3] arcnet: com20020: Add memory map of com20020

[Rob Herring]

On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
> From: Andrea Greco <a.greco@4sigma.it>
> 
> Add support for com20022I/com20020, memory mapped chip version.
> Support bus: Intel 80xx and Motorola 68xx.
> Bus size: Only 8 bit bus size is supported.
> Added related device tree bindings
> 
> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
> ---
>  .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++

Please split bindings to separate patch.

>  drivers/net/arcnet/Kconfig                         |  12 +-
>  drivers/net/arcnet/Makefile                        |   1 +
>  drivers/net/arcnet/arcdevice.h                     |  27 ++-
>  drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>  drivers/net/arcnet/com20020.c                      |   9 +-
>  6 files changed, 253 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>  create mode 100644 drivers/net/arcnet/com20020-membus.c
> 
> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> new file mode 100644
> index 000000000000..39c5b19c55af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> @@ -0,0 +1,23 @@
> +SMSC com20020, com20022I

What does this device do?

> +
> +timeout: Arcnet timeout, checkout datashet
> +clockp: Clock Prescaler, checkout datashet

s/datashet/datasheet/

> +clockm: Clock multiplier, checkout datasheet

Would these 3 properties be common for arcnet devices? If not, then they 
should have a vendor prefix.

> +
> +phy-reset-gpios: Chip reset ppin

Use 'reset-gpios' as that is standard.

> +phy-irq-gpios: Chip irq pin

Use 'interrupts'. Interrupt capable gpio controllers are also interrupt 
controllers.

> +
> +com20020_A@0 {

Node names should be generic based on the class of device. I don't think 
we have one defined, but how about 'arcnet'.

Unit addresses must have a corresponding reg property. How is this 
device accessed?

> +    compatible = "smsc,com20020";

Not documented.

> +
> +	timeout	= <0x3>;
> +	backplane = <0x0>;
> +
> +	clockp = <0x0>;
> +	clockm = <0x3>;
> +
> +	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
> +	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
> +
> +	status = "okay";

Don't should status in examples.

> +};

Re: [RFC PATCH 1/3] arcnet: com20020: Add memory map of com20020

[Tobin C. Harding]

On Tue, May 08, 2018 at 11:36:51AM +0200, Andrea Greco wrote:
> On 05/07/2018 04:55 AM, Tobin C. Harding wrote:
> >On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
> >>From: Andrea Greco <a.greco@4sigma.it>
> >
> >Hi Andrea,
> >
> >Here are some (mostly stylistic) suggestions to help you get your driver merged.
> >
> >>Add support for com20022I/com20020, memory mapped chip version.
> >>Support bus: Intel 80xx and Motorola 68xx.
> >>Bus size: Only 8 bit bus size is supported.
> >>Added related device tree bindings
> >>
> >>Signed-off-by: Andrea Greco <a.greco@4sigma.it>
> >>---
> >>  .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
> >>  drivers/net/arcnet/Kconfig                         |  12 +-
> >>  drivers/net/arcnet/Makefile                        |   1 +
> >>  drivers/net/arcnet/arcdevice.h                     |  27 ++-
> >>  drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
> >>  drivers/net/arcnet/com20020.c                      |   9 +-
> >>  6 files changed, 253 insertions(+), 10 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
> >>  create mode 100644 drivers/net/arcnet/com20020-membus.c
> >>
> >>diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> >>new file mode 100644
> >>index 000000000000..39c5b19c55af
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> >>@@ -0,0 +1,23 @@
> >>+SMSC com20020, com20022I
> >>+
> >>+timeout: Arcnet timeout, checkout datashet
> >>+clockp: Clock Prescaler, checkout datashet
> >>+clockm: Clock multiplier, checkout datasheet
> >>+
> >>+phy-reset-gpios: Chip reset ppin
> >>+phy-irq-gpios: Chip irq pin
> 
> ppin Corrected by my-self.
> 
> >>+
> >>+com20020_A@0 {
> >>+    compatible = "smsc,com20020";
> >>+
> >>+	timeout	= <0x3>;
> >>+	backplane = <0x0>;
> >>+
> >>+	clockp = <0x0>;
> >>+	clockm = <0x3>;
> >>+
> >>+	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
> >>+	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
> >>+
> >>+	status = "okay";
> >>+};
> >>diff --git a/drivers/net/arcnet/Kconfig b/drivers/net/arcnet/Kconfig
> >>index 39bd16f3f86d..d39faf45be1e 100644
> >>--- a/drivers/net/arcnet/Kconfig
> >>+++ b/drivers/net/arcnet/Kconfig
> >>@@ -3,7 +3,7 @@
> >>  #
> >>  menuconfig ARCNET
> >>-	depends on NETDEVICES && (ISA || PCI || PCMCIA)
> >>+	depends on NETDEVICES
> >>  	tristate "ARCnet support"
> >>  	---help---
> >>  	  If you have a network card of this type, say Y and check out the
> >>@@ -129,5 +129,15 @@ config ARCNET_COM20020_CS
> >>  	  To compile this driver as a module, choose M here: the module will be
> >>  	  called com20020_cs.  If unsure, say N.
> >>+config ARCNET_COM20020_MEMORY_BUS
> >>+	bool "Support for COM20020 on external memory"
> >>+	depends on ARCNET_COM20020 && !(ARCNET_COM20020_PCI || ARCNET_COM20020_ISA || ARCNET_COM20020_CS)
> >>+	help
> >>+	  Say Y here if on your custom board mount com20020 or friends.
> >>+
> >>+	  Com20022I support arcnet bus 10Mbitps.
> >>+	  This driver support only 8bit
> >
> >This driver only supports 8bit bus size.
> >
> >>         	    	 	       , and DMA is not supported is attached on your board at external interface bus.
> >
> >This bit does not make sense, sorry.
> Removed description,
> 
> Proposal for v2:
> Say Y here if your custom board mount com20020 chipset or friends.
> Supported Chipset: com20020, com20022, com20022I-3v3.
> If unsure, say N.

If you don't mind me doing review I'll do so again on v2 and comment then.

> >>+	  Supported bus Intel80xx / Motorola 68xx.
> >>+	  This driver not work with other com20020 module: PCI or PCMCIA compiled as [M].
> >
> >I'm not sure exactly what you want to say here, perhaps:
> >
> >   	  This driver does not work with other com20020 modules compiled
> >	  as PCI or PCMCIA [M].
> 
> About this, all removed from kconfig
> For PCI, PCMCIA, checkout downside
> 
> >>  endif # ARCNET
> >>diff --git a/drivers/net/arcnet/Makefile b/drivers/net/arcnet/Makefile
> >>index 53525e8ea130..19425c1e06f4 100644
> >>--- a/drivers/net/arcnet/Makefile
> >>+++ b/drivers/net/arcnet/Makefile
> >>@@ -14,3 +14,4 @@ obj-$(CONFIG_ARCNET_COM20020) += com20020.o
> >>  obj-$(CONFIG_ARCNET_COM20020_ISA) += com20020-isa.o
> >>  obj-$(CONFIG_ARCNET_COM20020_PCI) += com20020-pci.o
> >>  obj-$(CONFIG_ARCNET_COM20020_CS) += com20020_cs.o
> >>+obj-$(CONFIG_ARCNET_COM20020_MEMORY_BUS) += com20020-membus.o
> >>diff --git a/drivers/net/arcnet/arcdevice.h b/drivers/net/arcnet/arcdevice.h
> >>index d09b2b46ab63..16c608269cca 100644
> >>--- a/drivers/net/arcnet/arcdevice.h
> >>+++ b/drivers/net/arcnet/arcdevice.h
> >>@@ -201,7 +201,7 @@ struct ArcProto {
> >>  	void (*rx)(struct net_device *dev, int bufnum,
> >>  		   struct archdr *pkthdr, int length);
> >>  	int (*build_header)(struct sk_buff *skb, struct net_device *dev,
> >>-			    unsigned short ethproto, uint8_t daddr);
> >>+				unsigned short ethproto, uint8_t daddr);
> >
> >   +			    unsigned short ethproto, uint8_t daddr);
> >
> >Please use Linux coding style style, parameters continuing on separate
> >line are aligned with opening parenthesis.
> >
> >>  	/* these functions return '1' if the skb can now be freed */
> >>  	int (*prepare_tx)(struct net_device *dev, struct archdr *pkt,
> >>@@ -326,9 +326,9 @@ struct arcnet_local {
> >>  		void (*recontrigger) (struct net_device * dev, int enable);
> >>  		void (*copy_to_card)(struct net_device *dev, int bufnum,
> >>-				     int offset, void *buf, int count);
> >>+					 int offset, void *buf, int count);
> >>  		void (*copy_from_card)(struct net_device *dev, int bufnum,
> >>-				       int offset, void *buf, int count);
> >>+					   int offset, void *buf, int count);
> >>  	} hw;
> >>  	void __iomem *mem_start;	/* pointer to ioremap'ed MMIO */
> >>@@ -360,7 +360,7 @@ struct net_device *alloc_arcdev(const char *name);
> >>  int arcnet_open(struct net_device *dev);
> >>  int arcnet_close(struct net_device *dev);
> >>  netdev_tx_t arcnet_send_packet(struct sk_buff *skb,
> >>-			       struct net_device *dev);
> >>+				   struct net_device *dev);
> >>  void arcnet_timeout(struct net_device *dev);
> 
> Not required modification then all removed.
> 
> >>  /* I/O equivalents */
> >>@@ -371,7 +371,23 @@ void arcnet_timeout(struct net_device *dev);
> >>  #define BUS_ALIGN  1
> >>  #endif
> >>-/* addr and offset allow register like names to define the actual IO  address.
> >>+#ifdef CONFIG_ARCNET_COM20020_MEMORY_BUS
> >>+#define arcnet_inb(addr, offset)					\
> >>+	ioread8((void __iomem *)(addr) + BUS_ALIGN * (offset))
> >>+
> >>+#define arcnet_outb(value, addr, offset)				\
> >>+	iowrite8(value, (void __iomem *)(addr) + BUS_ALIGN * (offset))
> >>+
> >>+#define arcnet_insb(addr, offset, buffer, count)			\
> >>+	ioread8_rep((void __iomem *)					\
> >>+	(addr) + BUS_ALIGN * (offset), buffer, count)
> >>+
> >>+#define arcnet_outsb(addr, offset, buffer, count)			\
> >>+	iowrite8_rep((void __iomem *)					\
> >>+	(addr) + BUS_ALIGN * (offset), buffer, count)
> >>+#else
> >>+/**
> >>+ * addr and offset allow register like names to define the actual IO  address.
> >>   * A configuration option multiplies the offset for alignment.
> >>   */
> >>  #define arcnet_inb(addr, offset)					\
> >>@@ -388,6 +404,7 @@ void arcnet_timeout(struct net_device *dev);
> >>  	readb((addr) + (offset))
> >>  #define arcnet_writeb(value, addr, offset)				\
> >>  	writeb(value, (addr) + (offset))
> >>+#endif
> >>  #endif				/* __KERNEL__ */
> >>  #endif				/* _LINUX_ARCDEVICE_H */
> 
> 
> This is most important part where a suggestion will be very appreciated.
> This part define how arcnet drivers read and write over physical.
> The old driver can always use readb/writeb and friends, this driver rise big
> kernel panic.
> 
> This driver make a ioreamp with: devm_ioremap.
> Then, for r/w operation i use ioread8/iowrite8 and friends.
> 
> My quickly solution was make a ugly #ifdef.
> 
> With #ifndef all other driver implementation could not be used together
> this driver, because break, how driver write over physical.
> A proposal could be add a read/write callback to arcdevice.h struct hw.
> 
> PROS:
> Every driver fill this callback, this is a solution.
> 
> CONS:
> This solution require a small change for all com20020 driver
> implementations. I don't dispose of all hardware for make a accurate
> test. I could only test memory bus version.
> 
> Any other ideas, will be very appreciated.

I had a bit of a think about this for you.  Can I start by saying I am
not an overly experienced driver developer (or kernel developer for that
matter) so please take all suggestions with this in mind.

Instead of using ifdef's to define arnet_inb/outb() what if you define a
set of functions arnet_readb(), arnet_writeb() etc and use them after
you have done the memory map.  (I had to look up the difference between
readb() and inb() so if this suggestion is complete garbage please
ignore me.)

Also while reading com20020-membus.c I saw a few more changes that you
can use if you so please.  Here is a patch with comments added

diff --git a/drivers/net/arcnet/com20020-membus.c b/drivers/net/arcnet/com20020-membus.c
index 9eead734a3cf..9f3d9b8d9d1f 100644
--- a/drivers/net/arcnet/com20020-membus.c
+++ b/drivers/net/arcnet/com20020-membus.c
@@ -39,7 +39,7 @@ static int com20020_probe(struct platform_device *pdev)
        struct net_device *dev;
        struct arcnet_local *lp;
        struct resource res, *iores;
-       int ret, phy_reset, err;
+       int ret, phy_reset;

This function uses 'ret', 'err', and 'phy_reset' for return value.  I
see what you are getting at by using 'phy_reset' but I don't see the
value in having both 'err' and 'ret' in the same function.

        u32 timeout, backplane, clockp, clockm;
        void __iomem *ioaddr;
 
@@ -47,8 +47,9 @@ static int com20020_probe(struct platform_device *pdev)
 
        iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
-       if (of_address_to_resource(np, 0, &res))
-               return -EINVAL;
+       ret = of_address_to_resource(np, 0, &res);
+       if (ret)
+               return ret;
 
of_address_to_resource() returns -EINVAL on error so we can return it
and maintain program logic.  I see other places intree however that
call of_address_to_resource() as you have done.

        ret = of_property_read_u32(np, "timeout", &timeout);
        if (ret) {
@@ -77,16 +78,17 @@ static int com20020_probe(struct platform_device *pdev)
        phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
        if (phy_reset == -EPROBE_DEFER) {
                return phy_reset;
-       } else if (!gpio_is_valid(phy_reset)) {
+       }

No need for else statement after 'return' (golang linter complains about
this so I always notice it :)

+       if (!gpio_is_valid(phy_reset)) {
                dev_err(&pdev->dev, "phy-reset-gpios not valid !");
-               return 0;
+               return 0;       /* why return 0 after calling dev_err() */

(Added code comment so it would show up in patch.)  I couldn't
understand why this returns 0 here.  Is it what you meant to do?

        }
 
-       err = devm_gpio_request_one(&pdev->dev, phy_reset, GPIOF_OUT_INIT_LOW,
+       ret = devm_gpio_request_one(&pdev->dev, phy_reset, GPIOF_OUT_INIT_LOW,
                                    "arcnet-phy-reset");
-       if (err) {
-               dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
-               return err;

Already commented on above.


Hope this helps,
Tobin.

Re: [RFC PATCH 1/3] arcnet: com20020: Add memory map of com20020

[Andrea Greco]

On 05/08/2018 06:16 PM, Rob Herring wrote:
> On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
>> From: Andrea Greco <a.greco@4sigma.it>
>>
>> Add support for com20022I/com20020, memory mapped chip version.
>> Support bus: Intel 80xx and Motorola 68xx.
>> Bus size: Only 8 bit bus size is supported.
>> Added related device tree bindings
>>
>> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
>> ---
>>   .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
>
> Please split bindings to separate patch.

Ok
>
>>   drivers/net/arcnet/Kconfig                         |  12 +-
>>   drivers/net/arcnet/Makefile                        |   1 +
>>   drivers/net/arcnet/arcdevice.h                     |  27 ++-
>>   drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>>   drivers/net/arcnet/com20020.c                      |   9 +-
>>   6 files changed, 253 insertions(+), 10 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>>   create mode 100644 drivers/net/arcnet/com20020-membus.c
>>
>> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>> new file mode 100644
>> index 000000000000..39c5b19c55af
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>> @@ -0,0 +1,23 @@
>> +SMSC com20020, com20022I
>
> What does this device do?
>

Changed in:
SMSC com20020 Arcnet network controller

>> +
>> +timeout: Arcnet timeout, checkout datashet
>> +clockp: Clock Prescaler, checkout datashet
>
> s/datashet/datasheet/
>
>> +clockm: Clock multiplier, checkout datasheet
>
> Would these 3 properties be common for arcnet devices? If not, then they
> should have a vendor prefix.
>

Timeout is arcnet propelty:
Other is smsc params, then become:
- timeout: Arcnet timeout
- smsc-clockp: Clock Prescaler
- smsc-clockm: Clock multiplier
- smsc-backplane: Controller use backplane mode inside of transceiver

I forget backplane propelty, but is required

>> +
>> +phy-reset-gpios: Chip reset ppin
>
> Use 'reset-gpios' as that is standard.
>
>> +phy-irq-gpios: Chip irq pin
>
> Use 'interrupts'. Interrupt capable gpio controllers are also interrupt
> controllers.
>

Ok, change to standard

>> +
>> +com20020_A@0 {
>
> Node names should be generic based on the class of device. I don't think
> we have one defined, but how about 'arcnet'.
>
> Unit addresses must have a corresponding reg property. How is this
> device accessed?
>

Then: arcnet@28000000

>> +    compatible = "smsc,com20020";
>
> Not documented.
>
I miss something? Where add this doc?
Is not this file?
Documentation/devicetree/bindings/net/smsc-com20020.txt

>> +
>> + timeout = <0x3>;
>> + backplane = <0x0>;
>> +
>> + clockp = <0x0>;
>> + clockm = <0x3>;
>> +
>> + phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
>> + phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
>> +
>> + status = "okay";
>
> Don't should status in examples.
>
>> +};
Ok

Final result of new Patch, for bindings:

SMSC com20020 Arcnet network controller

Required propelty:
- timeout: Arcnet timeout
- smsc-clockp: Clock Prescaler
- smsc-clockm: Clock multiplier
- smsc-backplane: Controller use backplane mode inside of transceiver

- reset-gpios: Chip reset pin
- interrupts: Should contain controller interrupt

arcnet@28000000 {
     compatible = "smsc,com20020";

timeout = <0x3>;
smsc-backplane = <0x0>;
smsc-clockp = <0x0>;
smsc-clockm = <0x3>;

reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
interrupts = <&gpio2 10 GPIO_ACTIVE_LOW>;
};

Andrea

Re: [RFC PATCH 1/3] arcnet: com20020: Add memory map of com20020

[Rob Herring]

On Fri, May 11, 2018 at 5:50 AM, Andrea Greco
<andrea.greco.gapmilano@gmail.com> wrote:
> On 05/08/2018 06:16 PM, Rob Herring wrote:
>> On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
>>> From: Andrea Greco <a.greco@4sigma.it>
>>>
>>> Add support for com20022I/com20020, memory mapped chip version.
>>> Support bus: Intel 80xx and Motorola 68xx.
>>> Bus size: Only 8 bit bus size is supported.
>>> Added related device tree bindings
>>>
>>> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
>>> ---
>>>   .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
>>
>> Please split bindings to separate patch.
>
> Ok
>>
>>>   drivers/net/arcnet/Kconfig                         |  12 +-
>>>   drivers/net/arcnet/Makefile                        |   1 +
>>>   drivers/net/arcnet/arcdevice.h                     |  27 ++-
>>>   drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>>>   drivers/net/arcnet/com20020.c                      |   9 +-
>>>   6 files changed, 253 insertions(+), 10 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>>>   create mode 100644 drivers/net/arcnet/com20020-membus.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>>> new file mode 100644
>>> index 000000000000..39c5b19c55af
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>>> @@ -0,0 +1,23 @@
>>> +SMSC com20020, com20022I
>>
>> What does this device do?
>>
>
> Changed in:
> SMSC com20020 Arcnet network controller
>
>>> +
>>> +timeout: Arcnet timeout, checkout datashet
>>> +clockp: Clock Prescaler, checkout datashet
>>
>> s/datashet/datasheet/
>>
>>> +clockm: Clock multiplier, checkout datasheet
>>
>> Would these 3 properties be common for arcnet devices? If not, then they
>> should have a vendor prefix.
>>
>
> Timeout is arcnet propelty:
> Other is smsc params, then become:
> - timeout: Arcnet timeout

Needs unit suffix as defined in property-units.txt.

> - smsc-clockp: Clock Prescaler
> - smsc-clockm: Clock multiplier
> - smsc-backplane: Controller use backplane mode inside of transceiver

Vendor properties are <vendor>,<prop-name>.

>
> I forget backplane propelty, but is required
>
>>> +
>>> +phy-reset-gpios: Chip reset ppin
>>
>> Use 'reset-gpios' as that is standard.
>>
>>> +phy-irq-gpios: Chip irq pin
>>
>> Use 'interrupts'. Interrupt capable gpio controllers are also interrupt
>> controllers.
>>
>
> Ok, change to standard
>
>>> +
>>> +com20020_A@0 {
>>
>> Node names should be generic based on the class of device. I don't think
>> we have one defined, but how about 'arcnet'.
>>
>> Unit addresses must have a corresponding reg property. How is this
>> device accessed?
>>
>
> Then: arcnet@28000000
>
>>> +    compatible = "smsc,com20020";
>>
>> Not documented.
>>
> I miss something? Where add this doc?
> Is not this file?

Yes, this file up above with all the other properties. The example is
just an example, not a binding definition.

Rob