[U-Boot] [PATCH v2 6/6] drivers: usb: gadget: ether/rndis: convert driver to adopt device driver model

[Marek Vasut <marex@denx.de>]

On 05/12/2016 07:43 AM, Mugunthan V N wrote:
> On Tuesday 10 May 2016 05:57 PM, Marek Vasut wrote:
>> On 05/10/2016 01:44 PM, Mugunthan V N wrote:
>>> Adopt usb ether gadget and rndis driver to adopt driver model
>>>
>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>> ---
>>>  drivers/usb/gadget/ether.c | 153 ++++++++++++++++++++++++++++++++++++++++++---
>>>  drivers/usb/gadget/rndis.c |  13 +++-
>>>  drivers/usb/gadget/rndis.h |  19 ++++--
>>>  include/net.h              |   7 +++
>>>  4 files changed, 177 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
>>> index 2f70ebf..c436f75 100644
>>> --- a/drivers/usb/gadget/ether.c
>>> +++ b/drivers/usb/gadget/ether.c
>>> @@ -25,6 +25,7 @@
>>>  #include "rndis.h"
>>>  
>>>  #include <dm.h>
>>> +#include <dm/lists.h>
>>>  #include <dm/uclass-internal.h>
>>>  #include <dm/device-internal.h>
>>>  
>>> @@ -116,7 +117,11 @@ struct eth_dev {
>>>  
>>>  	struct usb_request	*tx_req, *rx_req;
>>>  
>>> +#ifndef CONFIG_DM_ETH
>>>  	struct eth_device	*net;
>>> +#else
>>> +	struct udevice		*net;
>>> +#endif
>>>  	struct net_device_stats	stats;
>>>  	unsigned int		tx_qlen;
>>>  
>>> @@ -143,7 +148,11 @@ struct eth_dev {
>>>  /*-------------------------------------------------------------------------*/
>>>  struct ether_priv {
>>>  	struct eth_dev ethdev;
>>> +#ifndef CONFIG_DM_ETH
>>>  	struct eth_device netdev;
>>> +#else
>>> +	struct udevice *netdev;
>>> +#endif
>>>  	struct usb_gadget_driver eth_driver;
>>>  };
>>>  
>>> @@ -1851,7 +1860,11 @@ static void rndis_control_ack_complete(struct usb_ep *ep,
>>>  
>>>  static char rndis_resp_buf[8] __attribute__((aligned(sizeof(__le32))));
>>>  
>>> +#ifndef CONFIG_DM_ETH
>>>  static int rndis_control_ack(struct eth_device *net)
>>> +#else
>>> +static int rndis_control_ack(struct udevice *net)
>>> +#endif
>>>  {
>>>  	struct ether_priv	*priv = (struct ether_priv *)net->priv;
>>>  	struct eth_dev		*dev = &priv->ethdev;
>>> @@ -2001,6 +2014,9 @@ static int eth_bind(struct usb_gadget *gadget)
>>>  	int			status = -ENOMEM;
>>>  	int			gcnum;
>>>  	u8			tmp[7];
>>> +#ifdef CONFIG_DM_ETH
>>> +	struct eth_pdata	*pdata = dev_get_platdata(l_priv->netdev);
>>> +#endif
>>>  
>>>  	/* these flags are only ever cleared; compiler take note */
>>>  #ifndef	CONFIG_USB_ETH_CDC
>>> @@ -2188,7 +2204,11 @@ autoconf_fail:
>>>  
>>>  
>>>  	/* network device setup */
>>> +#ifndef CONFIG_DM_ETH
>>>  	dev->net = &l_priv->netdev;
>>> +#else
>>> +	dev->net = l_priv->netdev;
>>> +#endif
>>>  
>>>  	dev->cdc = cdc;
>>>  	dev->zlp = zlp;
>>> @@ -2197,6 +2217,7 @@ autoconf_fail:
>>>  	dev->out_ep = out_ep;
>>>  	dev->status_ep = status_ep;
>>>  
>>> +	memset(tmp, 0, sizeof(tmp));
>>>  	/*
>>>  	 * Module params for these addresses should come from ID proms.
>>>  	 * The host side address is used with CDC and RNDIS, and commonly
>>> @@ -2204,10 +2225,13 @@ autoconf_fail:
>>>  	 * host side code for the SAFE thing cares -- its original BLAN
>>>  	 * thing didn't, Sharp never assigned those addresses on Zaurii.
>>>  	 */
>>> +#ifndef CONFIG_DM_ETH
>>>  	get_ether_addr(dev_addr, dev->net->enetaddr);
>>> -
>>> -	memset(tmp, 0, sizeof(tmp));
>>>  	memcpy(tmp, dev->net->enetaddr, sizeof(dev->net->enetaddr));
>>> +#else
>>> +	get_ether_addr(dev_addr, pdata->enetaddr);
>>> +	memcpy(tmp, pdata->enetaddr, sizeof(pdata->enetaddr));
>>> +#endif
>>>  
>>>  	get_ether_addr(host_addr, dev->host_mac);
>>>  
>>> @@ -2268,10 +2292,11 @@ autoconf_fail:
>>>  		status_ep ? " STATUS " : "",
>>>  		status_ep ? status_ep->name : ""
>>>  		);
>>> -	printf("MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> -		dev->net->enetaddr[0], dev->net->enetaddr[1],
>>> -		dev->net->enetaddr[2], dev->net->enetaddr[3],
>>> -		dev->net->enetaddr[4], dev->net->enetaddr[5]);
>>> +#ifndef CONFIG_DM_ETH
>>> +	printf("MAC %pM\n", dev->net->enetaddr);
>>> +#else
>>> +	printf("MAC %pM\n", pdata->enetaddr);
>>> +#endif
>>>  
>>>  	if (cdc || rndis)
>>>  		printf("HOST MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> @@ -2520,13 +2545,12 @@ void _usb_eth_halt(struct ether_priv *priv)
>>>  	}
>>>  
>>>  	usb_gadget_unregister_driver(&priv->eth_driver);
>>> -#ifdef CONFIG_DM_USB
>>> -	device_remove(dev->usb_udev);
>>> -#else
>>> +#ifndef CONFIG_DM_USB
>>>  	board_usb_cleanup(0, USB_INIT_DEVICE);
>>>  #endif
>>>  }
>>>  
>>> +#ifndef CONFIG_DM_ETH
>>>  static int usb_eth_init(struct eth_device *netdev, bd_t *bd)
>>>  {
>>>  	struct ether_priv *priv = (struct ether_priv *)netdev->priv;
>>> @@ -2592,3 +2616,114 @@ int usb_eth_initialize(bd_t *bi)
>>>  	eth_register(netdev);
>>>  	return 0;
>>>  }
>>> +#else
>>> +static int usb_eth_start(struct udevice *dev)
>>> +{
>>> +	struct ether_priv *priv = dev_get_priv(dev);
>>> +
>>> +	return _usb_eth_init(priv);
>>> +}
>>> +
>>> +static int usb_eth_send(struct udevice *dev, void *packet, int length)
>>> +{
>>> +	struct ether_priv *priv = dev_get_priv(dev);
>>> +
>>> +	return _usb_eth_send(priv, packet, length);
>>> +}
>>> +
>>> +static int usb_eth_recv(struct udevice *dev, int flags, uchar **packetp)
>>> +{
>>> +	struct ether_priv *priv = dev_get_priv(dev);
>>> +	struct eth_dev *ethdev = &priv->ethdev;
>>> +	int ret;
>>> +
>>> +	ret = _usb_eth_recv(priv);
>>> +	if (ret) {
>>> +		error("error packet receive\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	if (packet_received) {
>>> +		if (ethdev->rx_req) {
>>> +			*packetp = (uchar *)net_rx_packets[0];
>>> +			return ethdev->rx_req->length;
>>> +		} else {
>>> +			error("dev->rx_req invalid");
>>
>> Is this useful information ?
> 
> It is just a carry forward from non DM code.
> I do feel that without a req, packet_receviced will never be true. If
> you are okay, i will remove this in error log in next version.

If this is just carry forward, then subsequent patch is fine too.

>>
>>> +			return -EFAULT;
>>> +		}
>>> +	}
>>> +
>>> +	return -EAGAIN;
>>> +}
>>> +
>>> +static int usb_eth_free_pkt(struct udevice *dev, uchar *packet,
>>> +				   int length)
>>> +{
>>> +	struct ether_priv *priv = dev_get_priv(dev);
>>> +	struct eth_dev *ethdev = &priv->ethdev;
>>> +
>>> +	packet_received = 0;
>>> +
>>> +	return rx_submit(ethdev, ethdev->rx_req, 0);
>>> +}
>>> +
>>> +static void usb_eth_stop(struct udevice *dev)
>>> +{
>>> +	struct ether_priv *priv = dev_get_priv(dev);
>>> +
>>> +	_usb_eth_halt(priv);
>>> +}
>>> +
>>> +static int usb_eth_probe(struct udevice *dev)
>>> +{
>>> +	struct ether_priv *priv = dev_get_priv(dev);
>>> +	struct eth_pdata *pdata = dev_get_platdata(dev);
>>> +
>>> +	priv->netdev = dev;
>>> +	l_priv = priv;
>>> +
>>> +	get_ether_addr("de:ad:be:ef:00:01", pdata->enetaddr);
>>
>> Can we avoid hard-coding this default MAC ?
> 
> Yeah, will fix in next version.
> 
>>
>>> +	eth_setenv_enetaddr("usbnet_devaddr", pdata->enetaddr);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct eth_ops usb_eth_ops = {
>>> +	.start		= usb_eth_start,
>>> +	.send		= usb_eth_send,
>>> +	.recv		= usb_eth_recv,
>>> +	.free_pkt	= usb_eth_free_pkt,
>>> +	.stop		= usb_eth_stop,
>>> +};
>>> +
>>> +int usb_ether_init(void)
>>> +{
>>> +	struct udevice *dev;
>>> +	struct udevice *usb_dev;
>>> +	int ret;
>>> +
>>> +	ret = uclass_first_device(UCLASS_USB_DEV_GENERIC, &usb_dev);
>>> +	if (!usb_dev || ret) {
>>> +		error("No USB device found\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = device_bind_driver(usb_dev, "usb_ether", "usb_ether", &dev);
>>> +	if (!dev || ret) {
>>> +		error("usb - not able to bind usb_ether device\n");
>>
>> Can you keep the messages consistent in some way ? Some start with
>> capital letter, some don't . I would much rather see something like
>> "%s: unable to bind usb_ether device\n", __func__
> 
> Yeah, will fix in next version.

Thanks

[...]
-- 
Best regards,
Marek Vasut