[U-Boot] [PATCH v2 1/2] usb: eth: add ASIX AX88179 DRIVER

[René Griessl]

Am 10.09.2014 17:00, schrieb Marek Vasut:
> On Wednesday, September 10, 2014 at 12:00:29 PM, René Griessl wrote:
>> Am 09.09.2014 16:34, schrieb Marek Vasut:
>>> On Wednesday, September 03, 2014 at 04:31:20 PM, Rene Griessl wrote:
>>>> changes in v2:
>>>> 	-added usb_ether.h to change list
>>>> 	-added 2nd patch to enable driver for arndale board
>>>>
>>>> Signed-off-by: Rene Griessl <rgriessl at cit-ec.uni-bielefeld.de>
>>> I see that in Linux, there is asix_common.c stuff. Can this driver share
>>> any parts with drivers/net/ax88* ?
>> The asix_common.c includes asix.h. There you see that the common driver
>> supports following devices:
>> AX88172
>> AX88772
>> AX88178
>> but it is not supporting AX88179 and AX88178a, they have a separate
>> driver called ax88179_178a.c
>> These two have a different style in accessing MAC registers and PHY
> I didn't look deep enough. The 88179 driver is a completely separate driver, not
> sharing any code what-so-ever with the other ASIX driver, yes ?

The USB read and write cmd functions are similar and the probe function 
is the same. So they are
sharing 6% of code.
What do you have in mind? Do you want to build one driver supporting all 
devices?

> [...]
>
>>>> +		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
>>>> +		}
>>>> +	else {
>>>> +		}
>>> Uh, this check needs some rework, right ? Also, you want to lint your
>>> patches with ./scripts/checkpatch.pl tool before resubmitting.
>> was OK for ./scripts/checkpatch.pl
>> but I can change that
> This is rather surprising, but you're right. Please fix this up, the else can be
> dropped and the trailing } can be indented just under the if () clause.
OK
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +
>>>> +static int asix_read_mac(struct eth_device *eth)
>>>> +{
>>>> +	struct ueth_data *dev = (struct ueth_data *)eth->priv;
>>>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
>>>> +
>>>> +	if (dev->pusb_dev->descriptor.idProduct == 0x1790) {
>>>> +		asix_read_cmd(dev, AX_ACCESS_MAC, 0x10, 6, 6, buf);
>>>> +		debug("asix_read_mac() returning 0x%02x%02x%02x%02x%02x%02x\n",
>>>> +		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
>>>> +		memcpy(eth->enetaddr, buf, ETH_ALEN);
>>>> +		}
> Same here.
>
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +
>>>> +
>>>> +static int asix_basic_reset(struct ueth_data *dev)
>>>> +{
>>>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
>>> Why does the buffer need to be aligned here ? It's just a buffer that is
>>> not used for DMA, no ?
>>>
>>>> +	u16 *tmp16;
>>> Is it because you were getting unaligned accesses , since when the buffer
>>> itself was not aligned and you did cast it to u16, the CPU triggered
>>> unaligned access ?
>> Thats right, if I do not align I get unaligned accesses during USB
>> communication.
>> Is there a smarter solution for that?
> Oh I see. The smart solution would be to add bounce buffer into the USB stack,
> but unless you want to dive into this, this solution would be OK here.
OK, then I will leave it this way.

-- 

--------------------------------------------------------

Dipl.-Ing. René Griessl
Universität Bielefeld
AG Kognitronik & Sensorik
Exzellenzcluster Cognitive Interaction Technology (CITEC)
Inspiration 1 (Zehlendorfer Damm 199)
33615 Bielefeld

Telefon : +49 521-106-67362
Fax     : +49 521-106-12348
eMail   : rgriessl at cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/