[PATCH] usb driver for intellon based PLC like devolo dlan duo

[PATCH] usb driver for intellon based PLC like devolo dlan duo

[Peter Holik]

Signed-off-by: Peter Holik <peter@holik.at>
---
 drivers/net/usb/Kconfig    |    7 +
 drivers/net/usb/Makefile   |    2 +-
 drivers/net/usb/intellon.c |  273 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 281 insertions(+), 1 deletions(-)
 create mode 100644 drivers/net/usb/intellon.c

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 8ee2103..068faa5 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -345,4 +345,11 @@ config USB_HSO
 	  To compile this driver as a module, choose M here: the
 	  module will be called hso.

+config USB_NET_INTELLON
+	tristate "Intellon PLC based usb adapter"
+	depends on USB_USBNET
+	help
+	  Choose this option if you're using a PLC (Powerline Communications)
+	  solution with an Intellon chip, like the "devolo dLan duo".
+
 endmenu
diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
index 88a87ee..0fccfe9 100644
--- a/drivers/net/usb/Makefile
+++ b/drivers/net/usb/Makefile
@@ -19,4 +19,4 @@ obj-$(CONFIG_USB_NET_CDC_SUBSET)	+= cdc_subset.o
 obj-$(CONFIG_USB_NET_ZAURUS)	+= zaurus.o
 obj-$(CONFIG_USB_NET_MCS7830)	+= mcs7830.o
 obj-$(CONFIG_USB_USBNET)	+= usbnet.o
-
+obj-$(CONFIG_USB_NET_INTELLON)	+= intellon.o
diff --git a/drivers/net/usb/intellon.c b/drivers/net/usb/intellon.c
new file mode 100644
index 0000000..c9fcc38
--- /dev/null
+++ b/drivers/net/usb/intellon.c
@@ -0,0 +1,273 @@
+/*
+ * Copyright (c) 2009 Peter Holik
+ *
+ * Intellon PLC (Powerline Communications) usb net driver
+ *
+ * http://www.tandel.be/downloads/INT51X1_Datasheet.pdf
+ *
+ * Based on the work of Jan 'RedBully' Seiffert
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or.
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Should you need to contact me, the author, you can do so either by
+ * e-mail - mail your message to <vojtech@suse.cz>, or by paper mail:
+ * Vojtech Pavlik, Simunkova 1594, Prague 8, 182 00 Czech Republic
+ */
+
+#include <linux/module.h>
+#include <linux/ctype.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/usbnet.h>
+
+#define INTELLON_VENDOR_ID	0x09e1
+#define INTELLON_PRODUCT_ID	0x5121
+
+#define INTELLON_HEADER_SIZE	2	/* 2 byte header */
+
+#define PACKET_TYPE_PROMISCUOUS		(1 << 0)
+#define PACKET_TYPE_ALL_MULTICAST	(1 << 1) /* no filter */
+#define PACKET_TYPE_DIRECTED		(1 << 2)
+#define PACKET_TYPE_BROADCAST		(1 << 3)
+#define PACKET_TYPE_MULTICAST		(1 << 4) /* filtered */
+
+#define SET_ETHERNET_PACKET_FILTER	0x43
+
+static u8 nibble(unsigned char c)
+{
+	if (likely(isdigit(c)))
+		return c - '0';
+	c = toupper(c);
+	if (likely(isxdigit(c)))
+		return 10 + c - 'A';
+	return 0;
+}
+
+static inline int get_ethernet_addr(struct usbnet *dev)
+{
+	int             tmp, i;
+	unsigned char   buf [13];
+
+	tmp = usb_string(dev->udev, 3, buf, sizeof buf);
+	if (tmp != 12) {
+		devdbg(dev, "bad MAC string fetch, %d\n", tmp);
+		if (tmp >= 0)
+			tmp = -EINVAL;
+		return tmp;
+	}
+	for (i = tmp = 0; i < 6; i++, tmp += 2)
+		dev->net->dev_addr [i] =
+			(nibble(buf [tmp]) << 4) + nibble(buf [tmp + 1]);
+	return 0;
+}
+
+
+static int intellon_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+{
+	int len;
+
+	if (unlikely(skb->len < INTELLON_HEADER_SIZE)) {
+		deverr(dev, "unexpected tiny rx frame");
+		return 0;
+	}
+
+	len = (skb->data[skb->len - 2] | (skb->data[skb->len - 1] << 8));
+
+	skb_trim(skb, len);
+
+	return 1;
+}
+
+static struct sk_buff *intellon_tx_fixup(struct usbnet *dev,
+		struct sk_buff *skb, gfp_t flags)
+{
+	int pack_len = skb->len;
+	int headroom = skb_headroom(skb);
+	int tailroom = skb_tailroom(skb);
+	int need_tail = 0;
+
+	/*
+	 * usbnet would send a ZLP if packetlength mod urbsize == 0 for us,
+	 * but we need to know ourself, because this would add to the length
+	 * we send down to the device...
+	 */
+	if (!((pack_len + INTELLON_HEADER_SIZE) % dev->maxpacket))
+		need_tail = 1;
+
+	/* if packet and our header is smaler than 64 pad to 64 (+ ZLP) */
+	if ((pack_len + INTELLON_HEADER_SIZE + need_tail) < dev->maxpacket + 1)
+		need_tail = dev->maxpacket + 1 - pack_len - INTELLON_HEADER_SIZE;
+
+	if (!skb_cloned(skb) &&
+			(headroom + tailroom >= need_tail + INTELLON_HEADER_SIZE)) {
+		if (headroom < INTELLON_HEADER_SIZE || tailroom < need_tail) {
+			skb->data = memmove(skb->head + INTELLON_HEADER_SIZE,
+					skb->data, skb->len);
+			skb_set_tail_pointer(skb, skb->len);
+		}
+	} else {
+		struct sk_buff *skb2;
+
+		skb2 = skb_copy_expand(skb,
+				INTELLON_HEADER_SIZE,
+				need_tail,
+				flags);
+		dev_kfree_skb_any(skb);
+		if (!skb2)
+			return skb2;
+		skb = skb2;
+	}
+
+	pack_len += need_tail;
+
+	__skb_push(skb, INTELLON_HEADER_SIZE);
+
+	skb->data[0] = pack_len & 0xFF;
+	skb->data[1] = (pack_len & 0x700) >> 8;
+
+	if(need_tail)
+		memset(__skb_put(skb, need_tail), 0, need_tail);
+
+	return skb;
+}
+
+static void intellon_async_cmd_callback(struct urb *urb)
+{
+	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
+	int status = urb->status;
+
+	if (status < 0)
+		dev_warn(&urb->dev->dev, "async callback failed with %d\n", status);
+
+	kfree(req);
+	usb_free_urb(urb);
+}
+
+static void intellon_set_multicast(struct net_device *netdev)
+{
+	struct usb_ctrlrequest *req;
+	int status;
+	struct urb *urb;
+	struct usbnet *dev = netdev_priv(netdev);
+	u16 filter = PACKET_TYPE_DIRECTED |
+		     PACKET_TYPE_BROADCAST;
+
+	if (netdev->flags & IFF_PROMISC) {
+		/* do not expect to see traffic of other PLCs */
+		filter |= PACKET_TYPE_PROMISCUOUS;
+		devinfo(dev, "promiscuous mode enabled");
+	} else if (netdev->mc_count ||
+		  (netdev->flags & IFF_ALLMULTI)) {
+		filter |= PACKET_TYPE_ALL_MULTICAST;
+		devdbg(dev, "receive all multicast enabled");
+	} else {
+		/* ~PROMISCUOUS, ~MULTICAST */
+		devdbg(dev, "receive own packets only");
+	}
+
+	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	if (!urb) {
+		devwarn(dev, "Error allocating URB");
+		return;
+	}
+
+	req = kmalloc(sizeof *req, GFP_ATOMIC);
+	if (!req) {
+		devwarn(dev, "Error allocating control msg");
+		usb_free_urb(urb);
+		return;
+	}
+
+	req->bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE;
+	req->bRequest = SET_ETHERNET_PACKET_FILTER;
+	req->wValue = cpu_to_le16(filter);
+	req->wIndex = 0;
+	req->wLength = 0;
+
+	usb_fill_control_urb(urb, dev->udev, usb_sndctrlpipe(dev->udev, 0),
+		(void *)req, NULL, 0,
+		intellon_async_cmd_callback,
+		(void *)req);
+
+	status = usb_submit_urb(urb, GFP_ATOMIC);
+	if (status < 0) {
+		devwarn(dev, "Error submitting control msg, sts=%d", status);
+		kfree(req);
+		usb_free_urb(urb);
+	}
+}
+
+static int intellon_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int status = get_ethernet_addr(dev);
+
+	if (status)
+		return status;
+
+	dev->net->hard_header_len += INTELLON_HEADER_SIZE;
+	dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
+	dev->net->set_multicast_list = intellon_set_multicast;
+
+	return usbnet_get_endpoints(dev, intf);
+}
+
+static const struct driver_info intellon_info = {
+	.description = "Intellon powerline adapter",
+	.bind        = intellon_bind,
+	.rx_fixup    = intellon_rx_fixup,
+	.tx_fixup    = intellon_tx_fixup,
+	.in          = 1,
+	.out         = 2,
+	.flags      = FLAG_ETHER,
+};
+
+static const struct usb_device_id products[] = {
+	{
+	USB_DEVICE(INTELLON_VENDOR_ID, INTELLON_PRODUCT_ID),
+		.driver_info = (unsigned long) &intellon_info,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(usb, products);
+
+static struct usb_driver intellon_driver = {
+	.name       = "intellon",
+	.id_table   = products,
+	.probe      = usbnet_probe,
+	.disconnect = usbnet_disconnect,
+	.suspend    = usbnet_suspend,
+	.resume     = usbnet_resume,
+};
+
+static int __init intellon_init(void)
+{
+	return usb_register(&intellon_driver);
+}
+module_init(intellon_init);
+
+static void __exit intellon_exit(void)
+{
+	usb_deregister(&intellon_driver);
+}
+module_exit(intellon_exit);
+
+MODULE_AUTHOR("Peter Holik");
+MODULE_DESCRIPTION("Intellon powerline adapter");
+MODULE_LICENSE("GPL");
-- 
1.6.2.1

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Florian Fainelli]

Hi Peter,

Nice to see such a driver coming up!

Le Friday 17 April 2009 16:10:24 Peter Holik, vous avez écrit :
> Signed-off-by: Peter Holik <peter@holik.at>
> ---
>  drivers/net/usb/Kconfig    |    7 +
>  drivers/net/usb/Makefile   |    2 +-
>  drivers/net/usb/intellon.c |  273
> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 281
> insertions(+), 1 deletions(-)
>  create mode 100644 drivers/net/usb/intellon.c
>
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index 8ee2103..068faa5 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -345,4 +345,11 @@ config USB_HSO
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called hso.
>
> +config USB_NET_INTELLON
> +	tristate "Intellon PLC based usb adapter"
> +	depends on USB_USBNET
> +	help
> +	  Choose this option if you're using a PLC (Powerline Communications)
> +	  solution with an Intellon chip, like the "devolo dLan duo".
> +

Please be more specific, i.e: using a USB-based PLC (...) solution. There 
might be support for PLC PHYs connected to a MII-bus in a near future, even 
though they will not reside in drivers/net/usb/.

>  endmenu
> diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
> index 88a87ee..0fccfe9 100644
> --- a/drivers/net/usb/Makefile
> +++ b/drivers/net/usb/Makefile
> @@ -19,4 +19,4 @@ obj-$(CONFIG_USB_NET_CDC_SUBSET)	+= cdc_subset.o
>  obj-$(CONFIG_USB_NET_ZAURUS)	+= zaurus.o
>  obj-$(CONFIG_USB_NET_MCS7830)	+= mcs7830.o
>  obj-$(CONFIG_USB_USBNET)	+= usbnet.o
> -
> +obj-$(CONFIG_USB_NET_INTELLON)	+= intellon.o

I would not name this intellon for the same reasons as explained below, but 
rather int51x1.c since this driver will for instance not work with HomePlug 
AV designs which use different Intellon integrated chips like the 6000 and 
6300 series.

> diff --git a/drivers/net/usb/intellon.c b/drivers/net/usb/intellon.c
> new file mode 100644
> index 0000000..c9fcc38
> --- /dev/null
> +++ b/drivers/net/usb/intellon.c
> @@ -0,0 +1,273 @@
> +/*
> + * Copyright (c) 2009 Peter Holik
> + *
> + * Intellon PLC (Powerline Communications) usb net driver

Intellon INT51x1 PLC ...
[snip]

> +
> +static u8 nibble(unsigned char c)
> +{
> +	if (likely(isdigit(c)))
> +		return c - '0';
> +	c = toupper(c);
> +	if (likely(isxdigit(c)))
> +		return 10 + c - 'A';
> +	return 0;
> +}

Please prefix this with intellon_ (or int51x1_) for instance to avoid any 
possible namespace clash.

> +
> +static inline int get_ethernet_addr(struct usbnet *dev)
> +{
> +	int             tmp, i;
> +	unsigned char   buf [13];
> +
> +	tmp = usb_string(dev->udev, 3, buf, sizeof buf);
> +	if (tmp != 12) {
> +		devdbg(dev, "bad MAC string fetch, %d\n", tmp);
> +		if (tmp >= 0)
> +			tmp = -EINVAL;
> +		return tmp;
> +	}
> +	for (i = tmp = 0; i < 6; i++, tmp += 2)
> +		dev->net->dev_addr [i] =
> +			(nibble(buf [tmp]) << 4) + nibble(buf [tmp + 1]);
> +	return 0;
> +}

Same here.

Please fix the intellon prefixing with something more specific to the driver 
like int51x1_ and I am ok with that driver.

Acked-by: Florian Fainelli <florian@openwrt.org>
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Oliver Neukum]

Am Freitag 17 April 2009 16:10:24 schrieb Peter Holik:

+static int intellon_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+{
+       int len;
+
+       if (unlikely(skb->len < INTELLON_HEADER_SIZE)) {
+               deverr(dev, "unexpected tiny rx frame");
+               return 0;
+       }
+
+       len = (skb->data[skb->len - 2] | (skb->data[skb->len - 1] << 8));

Please use a conversion function.

+       } else {
+               struct sk_buff *skb2;
+
+               skb2 = skb_copy_expand(skb,
+                               INTELLON_HEADER_SIZE,
+                               need_tail,
+                               flags);
+               dev_kfree_skb_any(skb);
+               if (!skb2)
+                       return skb2;

If you return NULL in an error case, write it so explicitely.

+       __skb_push(skb, INTELLON_HEADER_SIZE);
+
+       skb->data[0] = pack_len & 0xFF;
+       skb->data[1] = (pack_len & 0x700) >> 8;

Again, a conversion function is called for.

	Regards
		Oliver

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Guennadi Liakhovetski]

On Fri, 17 Apr 2009, Florian Fainelli wrote:

> > diff --git a/drivers/net/usb/intellon.c b/drivers/net/usb/intellon.c
> > new file mode 100644
> > index 0000000..c9fcc38
> > --- /dev/null
> > +++ b/drivers/net/usb/intellon.c
> > @@ -0,0 +1,273 @@
> > +/*
> > + * Copyright (c) 2009 Peter Holik
> > + *
> > + * Intellon PLC (Powerline Communications) usb net driver
> 
> Intellon INT51x1 PLC ...
> [snip]
> 
> > +
> > +static u8 nibble(unsigned char c)
> > +{
> > +	if (likely(isdigit(c)))
> > +		return c - '0';
> > +	c = toupper(c);
> > +	if (likely(isxdigit(c)))
> > +		return 10 + c - 'A';
> > +	return 0;
> > +}
> 
> Please prefix this with intellon_ (or int51x1_) for instance to avoid any 
> possible namespace clash.
> 
> > +
> > +static inline int get_ethernet_addr(struct usbnet *dev)
> > +{
> > +	int             tmp, i;
> > +	unsigned char   buf [13];
> > +
> > +	tmp = usb_string(dev->udev, 3, buf, sizeof buf);
> > +	if (tmp != 12) {
> > +		devdbg(dev, "bad MAC string fetch, %d\n", tmp);
> > +		if (tmp >= 0)
> > +			tmp = -EINVAL;
> > +		return tmp;
> > +	}
> > +	for (i = tmp = 0; i < 6; i++, tmp += 2)
> > +		dev->net->dev_addr [i] =
> > +			(nibble(buf [tmp]) << 4) + nibble(buf [tmp + 1]);
> > +	return 0;
> > +}
> 
> Same here.

FWIW, I think it's pretty common to name static functions in a .c file, 
which perform auxiliary function, not really specific to the context 
without the respective context-prefix. Of course, it might indeed happen 
that a global symbol "nibble" or "get_ethernet_addr" gets introduced at 
some point thus leading to a conflict, but this seems to be unlikely. And 
in any case the one who introduces such a symbol will have to grep the 
entire kernel anyway.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Peter Holik]

> Hi Peter,
>
> Nice to see such a driver coming up!

thanks

> Le Friday 17 April 2009 16:10:24 Peter Holik, vous avez écrit :
>> Signed-off-by: Peter Holik <peter@holik.at>
>> ---
>>  drivers/net/usb/Kconfig    |    7 +
>>  drivers/net/usb/Makefile   |    2 +-
>>  drivers/net/usb/intellon.c |  273
>> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 281
>> insertions(+), 1 deletions(-)
>>  create mode 100644 drivers/net/usb/intellon.c
>>
>> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
>> index 8ee2103..068faa5 100644
>> --- a/drivers/net/usb/Kconfig
>> +++ b/drivers/net/usb/Kconfig
>> @@ -345,4 +345,11 @@ config USB_HSO
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called hso.
>>
>> +config USB_NET_INTELLON
>> +	tristate "Intellon PLC based usb adapter"
>> +	depends on USB_USBNET
>> +	help
>> +	  Choose this option if you're using a PLC (Powerline Communications)
>> +	  solution with an Intellon chip, like the "devolo dLan duo".
>> +
>
> Please be more specific, i.e: using a USB-based PLC (...) solution.

> There might be support for PLC PHYs connected to a MII-bus in a near future, even
> though they will not reside in drivers/net/usb/.

What do you mean with the last sentence?

>>  endmenu
>> diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
>> index 88a87ee..0fccfe9 100644
>> --- a/drivers/net/usb/Makefile
>> +++ b/drivers/net/usb/Makefile
>> @@ -19,4 +19,4 @@ obj-$(CONFIG_USB_NET_CDC_SUBSET)	+= cdc_subset.o
>>  obj-$(CONFIG_USB_NET_ZAURUS)	+= zaurus.o
>>  obj-$(CONFIG_USB_NET_MCS7830)	+= mcs7830.o
>>  obj-$(CONFIG_USB_USBNET)	+= usbnet.o
>> -
>> +obj-$(CONFIG_USB_NET_INTELLON)	+= intellon.o
>
> I would not name this intellon for the same reasons as explained below, but
> rather int51x1.c since this driver will for instance not work with HomePlug
> AV designs which use different Intellon integrated chips like the 6000 and
> 6300 series.

work in progress...

>> diff --git a/drivers/net/usb/intellon.c b/drivers/net/usb/intellon.c
>> new file mode 100644
>> index 0000000..c9fcc38
>> --- /dev/null
>> +++ b/drivers/net/usb/intellon.c
>> @@ -0,0 +1,273 @@
>> +/*
>> + * Copyright (c) 2009 Peter Holik
>> + *
>> + * Intellon PLC (Powerline Communications) usb net driver
>
> Intellon INT51x1 PLC ...
> [snip]
>
>> +
>> +static u8 nibble(unsigned char c)
>> +{
>> +	if (likely(isdigit(c)))
>> +		return c - '0';
>> +	c = toupper(c);
>> +	if (likely(isxdigit(c)))
>> +		return 10 + c - 'A';
>> +	return 0;
>> +}
>
> Please prefix this with intellon_ (or int51x1_) for instance to avoid any
> possible namespace clash.
>
>> +
>> +static inline int get_ethernet_addr(struct usbnet *dev)
>> +{
>> +	int             tmp, i;
>> +	unsigned char   buf [13];
>> +
>> +	tmp = usb_string(dev->udev, 3, buf, sizeof buf);
>> +	if (tmp != 12) {
>> +		devdbg(dev, "bad MAC string fetch, %d\n", tmp);
>> +		if (tmp >= 0)
>> +			tmp = -EINVAL;
>> +		return tmp;
>> +	}
>> +	for (i = tmp = 0; i < 6; i++, tmp += 2)
>> +		dev->net->dev_addr [i] =
>> +			(nibble(buf [tmp]) << 4) + nibble(buf [tmp + 1]);
>> +	return 0;
>> +}
>
> Same here.

Disagree, because i've taken "nibble" and "get_ethernet_addr" from cdc_ether.c
to have the same code (the version of Jan was different).

> Please fix the intellon prefixing with something more specific to the driver
> like int51x1_ and I am ok with that driver.

ok

cu Peter

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Peter Holik]

> Am Freitag 17 April 2009 16:10:24 schrieb Peter Holik:
>
> +static int intellon_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> +{
> +       int len;
> +
> +       if (unlikely(skb->len < INTELLON_HEADER_SIZE)) {
> +               deverr(dev, "unexpected tiny rx frame");
> +               return 0;
> +       }
> +
> +       len = (skb->data[skb->len - 2] | (skb->data[skb->len - 1] << 8));
>
> Please use a conversion function.

do you mean

len = le16_to_cpu(skb->data[skb->len - 2] | (skb->data[skb->len - 1] << 8));

> +       } else {
> +               struct sk_buff *skb2;
> +
> +               skb2 = skb_copy_expand(skb,
> +                               INTELLON_HEADER_SIZE,
> +                               need_tail,
> +                               flags);
> +               dev_kfree_skb_any(skb);
> +               if (!skb2)
> +                       return skb2;
>
> If you return NULL in an error case, write it so explicitely.

ok

> +       __skb_push(skb, INTELLON_HEADER_SIZE);
> +
> +       skb->data[0] = pack_len & 0xFF;
> +       skb->data[1] = (pack_len & 0x700) >> 8;
>
> Again, a conversion function is called for.

is this correct

  __le16 *len;
.
.
.
  pack_len &= 0x07ff;

  len = (__le16 *) __skb_push(skb, INT51X1_HEADER_SIZE);
  *len = cpu_to_le16(pack_len);


cu Peter

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Oliver Neukum]

Am Samstag 18 April 2009 09:16:28 schrieb Peter Holik:

> > +       len = (skb->data[skb->len - 2] | (skb->data[skb->len - 1] << 8));
> >
> > Please use a conversion function.
>
> do you mean
>
> len = le16_to_cpu(skb->data[skb->len - 2] | (skb->data[skb->len - 1] <<
> 8));

Without the "|". Just dereference the pointer.

>   pack_len &= 0x07ff;
>
>   len = (__le16 *) __skb_push(skb, INT51X1_HEADER_SIZE);
>   *len = cpu_to_le16(pack_len);

That's correct.

	Regards
		Oliver

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Peter Holik]

> Am Samstag 18 April 2009 09:16:28 schrieb Peter Holik:
>
>> > +       len = (skb->data[skb->len - 2] | (skb->data[skb->len - 1] << 8));
>> >
>> > Please use a conversion function.
>>
>> do you mean
>>
>> len = le16_to_cpu(skb->data[skb->len - 2] | (skb->data[skb->len - 1] <<
>> 8));
>
> Without the "|". Just dereference the pointer.

this way?

len = le16_to_cpu(*(__le16 *)&skb->data[skb->len - 2]);

cu Peter

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Oliver Neukum]

Am Samstag 18 April 2009 10:41:37 schrieb Peter Holik:
> > Without the "|". Just dereference the pointer.
>
> this way?
>
> len = le16_to_cpu(*(__le16 *)&skb->data[skb->len - 2]);

That does the job.

	Regards
		Oliver

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[David Miller]

From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date: Fri, 17 Apr 2009 21:07:52 +0200 (CEST)

> FWIW, I think it's pretty common to name static functions in a .c file, 
> which perform auxiliary function, not really specific to the context 
> without the respective context-prefix.

I'll remember to think of you next time I run grep on the tree.

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Florian Fainelli]

Le Saturday 18 April 2009 08:48:22 Peter Holik, vous avez écrit :
> > Hi Peter,
> >
> > Nice to see such a driver coming up!
>
> thanks
>
> > Le Friday 17 April 2009 16:10:24 Peter Holik, vous avez écrit :
> >> Signed-off-by: Peter Holik <peter@holik.at>
> >> ---
> >>  drivers/net/usb/Kconfig    |    7 +
> >>  drivers/net/usb/Makefile   |    2 +-
> >>  drivers/net/usb/intellon.c |  273
> >> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 281
> >> insertions(+), 1 deletions(-)
> >>  create mode 100644 drivers/net/usb/intellon.c
> >>
> >> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> >> index 8ee2103..068faa5 100644
> >> --- a/drivers/net/usb/Kconfig
> >> +++ b/drivers/net/usb/Kconfig
> >> @@ -345,4 +345,11 @@ config USB_HSO
> >>  	  To compile this driver as a module, choose M here: the
> >>  	  module will be called hso.
> >>
> >> +config USB_NET_INTELLON
> >> +	tristate "Intellon PLC based usb adapter"
> >> +	depends on USB_USBNET
> >> +	help
> >> +	  Choose this option if you're using a PLC (Powerline Communications)
> >> +	  solution with an Intellon chip, like the "devolo dLan duo".
> >> +
> >
> > Please be more specific, i.e: using a USB-based PLC (...) solution.
> >
> > There might be support for PLC PHYs connected to a MII-bus in a near
> > future, even though they will not reside in drivers/net/usb/.
>
> What do you mean with the last sentence?

I mean that one might come up with a design that uses Intellon PLC PHY chips 
only with a custom MAC chip, not integrated MAC+PHY chips only, therefore a 
more specific naming is required, that's purely cosmetic though.

>
> >>  endmenu
> >> diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
> >> index 88a87ee..0fccfe9 100644
> >> --- a/drivers/net/usb/Makefile
> >> +++ b/drivers/net/usb/Makefile
> >> @@ -19,4 +19,4 @@ obj-$(CONFIG_USB_NET_CDC_SUBSET)	+= cdc_subset.o
> >>  obj-$(CONFIG_USB_NET_ZAURUS)	+= zaurus.o
> >>  obj-$(CONFIG_USB_NET_MCS7830)	+= mcs7830.o
> >>  obj-$(CONFIG_USB_USBNET)	+= usbnet.o
> >> -
> >> +obj-$(CONFIG_USB_NET_INTELLON)	+= intellon.o
> >
> > I would not name this intellon for the same reasons as explained below,
> > but rather int51x1.c since this driver will for instance not work with
> > HomePlug AV designs which use different Intellon integrated chips like
> > the 6000 and 6300 series.
>
> work in progress...

Cool :)

> Disagree, because i've taken "nibble" and "get_ethernet_addr" from
> cdc_ether.c to have the same code (the version of Jan was different).

Ok.
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Jon Smirl]

On Sat, Apr 18, 2009 at 6:22 AM, Florian Fainelli <florian@openwrt.org> wrote:
> Le Saturday 18 April 2009 08:48:22 Peter Holik, vous avez écrit :
>> > Hi Peter,
>> >
>> > Nice to see such a driver coming up!
>>
>> thanks
>>
>> > Le Friday 17 April 2009 16:10:24 Peter Holik, vous avez écrit :
>> >> Signed-off-by: Peter Holik <peter@holik.at>
>> >> ---
>> >>  drivers/net/usb/Kconfig    |    7 +
>> >>  drivers/net/usb/Makefile   |    2 +-
>> >>  drivers/net/usb/intellon.c |  273
>> >> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 281
>> >> insertions(+), 1 deletions(-)
>> >>  create mode 100644 drivers/net/usb/intellon.c
>> >>
>> >> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
>> >> index 8ee2103..068faa5 100644
>> >> --- a/drivers/net/usb/Kconfig
>> >> +++ b/drivers/net/usb/Kconfig
>> >> @@ -345,4 +345,11 @@ config USB_HSO
>> >>      To compile this driver as a module, choose M here: the
>> >>      module will be called hso.
>> >>
>> >> +config USB_NET_INTELLON
>> >> +  tristate "Intellon PLC based usb adapter"
>> >> +  depends on USB_USBNET
>> >> +  help
>> >> +    Choose this option if you're using a PLC (Powerline Communications)
>> >> +    solution with an Intellon chip, like the "devolo dLan duo".
>> >> +
>> >
>> > Please be more specific, i.e: using a USB-based PLC (...) solution.
>> >
>> > There might be support for PLC PHYs connected to a MII-bus in a near
>> > future, even though they will not reside in drivers/net/usb/.
>>
>> What do you mean with the last sentence?
>
> I mean that one might come up with a design that uses Intellon PLC PHY chips
> only with a custom MAC chip, not integrated MAC+PHY chips only, therefore a
> more specific naming is required, that's purely cosmetic though.

Those chips that look like PHY chips are just fancy A/D D/A converters.

When connected via Ethernet the chips don't need any drivers. But the
higher speed devices need firmware loaded. Firmware is loaded by
writing normal packets to the device with a specific Intellon owned
Ethernet address. I load the firmware in u-boot so that I can NFS
boot, but you could also load it in Linux. These chips have an
embedded ARM9 core.

More work could be done to integrate Linux commands for changing the
Ethernet address, link encryption key, etc. I just statically set
these in my firmware image. You change them by writing packets to the
special address.

Powerline networking uses technology that is very similar to 802.11g
but with different error recovery encoding. I suspect it is possible
to build a powerline device using 802.11 MAC hardware and different
firmware.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Guennadi Liakhovetski]

On Sat, 18 Apr 2009, David Miller wrote:

> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Date: Fri, 17 Apr 2009 21:07:52 +0200 (CEST)
> 
> > FWIW, I think it's pretty common to name static functions in a .c file, 
> > which perform auxiliary function, not really specific to the context 
> > without the respective context-prefix.
> 
> I'll remember to think of you next time I run grep on the tree.

Thanks David, very nice of you:-)

Once an MMC driver has been submitted for a specific platform, which 
apparently has been derived from an MMC driver for a similar platform. And 
the submitter has preserved the function prefix... So most functions in 
the two drivers were called equally. Now that was bad, and I was the one 
to complain about it, no idea what was the end result with that driver 
though.

What I actually meant is that I don't necessarily consider it very 
inconvenient if different drivers have functions like reg_write() or 
whatever. I cannot think of many situations when this can be confusing. If 
you are working with that file and see a call to reg_write() you know 
where to look for it. If you have an Oops in that function - you just look 
one function up in the backtrace. One of the cases that might be difficult 
is if you have an Oops in such a function without a backtrace and it is in 
a module. Ok, in this case it might be difficult to find out what that 
was. Otherwise - why would you want to grep the sources for such a 
function?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[David Miller]

From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date: Sat, 18 Apr 2009 21:54:11 +0200 (CEST)

> If you are working with that file and see a call to reg_write() you
> know where to look for it.

I would never use such a generic name for a driver local routine.

For example, in the tg3 driver we use "tr32()" and "tw32()" so that
at least some inkling of the driver name, even if it is just one
character, prefixes the name.

This extends to other driver's I've written.  The niu driver thus
uses "tr64()" and "nw64()".

This is just common sense as far as I'm concerned.  It is just
as straight forward as not using variable names like 'foo'.

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sat, 18 Apr 2009 21:15:03 -0700 (PDT)

> This extends to other driver's I've written.  The niu driver thus
> uses "tr64()" and "nw64()".
        ^^^^^

Typo, meant "nr64()" of course :)

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[Guennadi Liakhovetski]

On Sat, 18 Apr 2009, David Miller wrote:

> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Date: Sat, 18 Apr 2009 21:54:11 +0200 (CEST)
> 
> > If you are working with that file and see a call to reg_write() you
> > know where to look for it.
> 
> I would never use such a generic name for a driver local routine.
> 
> For example, in the tg3 driver we use "tr32()" and "tw32()" so that
> at least some inkling of the driver name, even if it is just one
> character, prefixes the name.
> 
> This extends to other driver's I've written.  The niu driver thus
> uses "tr64()" and "nw64()".

Ok, we currently have drivers for mt9m001, mt9m111, mt9t031, and mt9v022. 
Which letter would you take for them?:-) You'd have to take at least two 
characters like "m0", "m1", "t0", and "v0" but that wouldn't be very 
intuitive IMHO either. Take four characters like m001r16() and it looks 
pretty bad compared to all routines being called mt9m001_*.

> This is just common sense as far as I'm concerned.  It is just
> as straight forward as not using variable names like 'foo'.

I'd say that's subjective (not the latter, but the former):-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] usb driver for intellon based PLC like devolo dlan duo

[David Miller]

From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date: Sun, 19 Apr 2009 10:32:24 +0200 (CEST)

> Ok, we currently have drivers for mt9m001, mt9m111, mt9t031, and mt9v022. 
> Which letter would you take for them?:-) You'd have to take at least two 
> characters like "m0", "m1", "t0", and "v0" but that wouldn't be very 
> intuitive IMHO either. Take four characters like m001r16() and it looks 
> pretty bad compared to all routines being called mt9m001_*.

Then mt9m001_FOO() is what you use if you know that such other
similar names exist.