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

[Florian Fainelli <florian@openwrt.org>]

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
-------------------------------