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

2016-11-29 Thread Mugunthan V N 
On Wednesday 30 November 2016 05:24 AM, Joe Hershberger wrote:
> On Thu, Nov 17, 2016 at 11:39 PM, Mugunthan V N  wrote:
>> Adopt usb ether gadget and rndis driver to adopt driver model
>>
>> Signed-off-by: Mugunthan V N 
>> ---
>>  drivers/usb/gadget/Kconfig |   4 ++
>>  drivers/usb/gadget/ether.c | 153 
>> ++---
>>  drivers/usb/gadget/rndis.c |  13 +++-
>>  drivers/usb/gadget/rndis.h |  19 --
>>  include/net.h  |   7 +++
>>  5 files changed, 181 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 40839d89e9..261ed128ac 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -112,6 +112,10 @@ config G_DNL_VENDOR_NUM
>>  config G_DNL_PRODUCT_NUM
>> hex "Product ID of USB device"
>>
>> +config USBNET_DEVADDR
>> +   string "USB Gadget Ethernet device mac address"
>> +   default "de:ad:be:ef:00:01"
> 
> Please don't do this. We don't have "default" MAC addresses. They are
> either from the env, from ROM, or randomly generated.
> 

Okay will remove this and use either env MAC or random MAC.

>> +
>>  endif # USB_GADGET_DOWNLOAD
>>
>>  endif # USB_GADGET
>> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
>> index 046ad8ca2b..8c3c3fd9ab 100644
>> --- a/drivers/usb/gadget/ether.c
>> +++ b/drivers/usb/gadget/ether.c
>> @@ -25,6 +25,7 @@
>>  #include "rndis.h"
>>
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>
>> @@ -116,7 +117,11 @@ struct eth_dev {
>>
>> struct usb_request  *tx_req, *rx_req;
>>
>> +#ifndef CONFIG_DM_ETH
> 
> Please use positive logic.

Okay, will fix in next version.

> 
>> struct eth_device   *net;
>> +#else
>> +   struct udevice  *net;
>> +#endif
>> struct net_device_stats stats;
>> unsigned inttx_qlen;
>>
>> @@ -143,7 +148,11 @@ struct eth_dev {
>>  
>> /*-*/
>>  struct ether_priv {
>> struct eth_dev ethdev;
>> +#ifndef CONFIG_DM_ETH
> 
> Please use positive logic
> 
>> struct eth_device netdev;
>> +#else
>> +   struct udevice *netdev;
> 
> Did you really intend to have a pointer here when the other is an
> inline structure?

Yes, udevice is the device allocated by probe and passed in probe, but
in non-dm case netdev has to be allocated by driver, in this case it
declared as inline structure.

> 
>> +#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
> 
> Please use positive logic.
> 
>>  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 = >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 */
>>  #ifndefCONFIG_USB_ETH_CDC
>> @@ -2188,7 +2204,11 @@ autoconf_fail:
>>
>>
>> /* network device setup */
>> +#ifndef CONFIG_DM_ETH
>> dev->net = _priv->netdev;
> 
> You wouldn't need this difference if the priv also used a ptr in the
> non-dm case.
> 
> Also, if you are opposed to cleaning this up (preferably by adding a
> preceding patch to make it a pointer), at least use positive logic
> (#ifdef CONFIG_DM_ETH). Same applies elsewhere.

Okay, will add a patch to convert netdev to pointer

> 
>> +#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,

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

2016-11-29 Thread Joe Hershberger 
On Thu, Nov 17, 2016 at 11:39 PM, Mugunthan V N  wrote:
> Adopt usb ether gadget and rndis driver to adopt driver model
>
> Signed-off-by: Mugunthan V N 
> ---
>  drivers/usb/gadget/Kconfig |   4 ++
>  drivers/usb/gadget/ether.c | 153 
> ++---
>  drivers/usb/gadget/rndis.c |  13 +++-
>  drivers/usb/gadget/rndis.h |  19 --
>  include/net.h  |   7 +++
>  5 files changed, 181 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 40839d89e9..261ed128ac 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -112,6 +112,10 @@ config G_DNL_VENDOR_NUM
>  config G_DNL_PRODUCT_NUM
> hex "Product ID of USB device"
>
> +config USBNET_DEVADDR
> +   string "USB Gadget Ethernet device mac address"
> +   default "de:ad:be:ef:00:01"

Please don't do this. We don't have "default" MAC addresses. They are
either from the env, from ROM, or randomly generated.

> +
>  endif # USB_GADGET_DOWNLOAD
>
>  endif # USB_GADGET
> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
> index 046ad8ca2b..8c3c3fd9ab 100644
> --- a/drivers/usb/gadget/ether.c
> +++ b/drivers/usb/gadget/ether.c
> @@ -25,6 +25,7 @@
>  #include "rndis.h"
>
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -116,7 +117,11 @@ struct eth_dev {
>
> struct usb_request  *tx_req, *rx_req;
>
> +#ifndef CONFIG_DM_ETH

Please use positive logic.

> struct eth_device   *net;
> +#else
> +   struct udevice  *net;
> +#endif
> struct net_device_stats stats;
> unsigned inttx_qlen;
>
> @@ -143,7 +148,11 @@ struct eth_dev {
>  /*-*/
>  struct ether_priv {
> struct eth_dev ethdev;
> +#ifndef CONFIG_DM_ETH

Please use positive logic

> struct eth_device netdev;
> +#else
> +   struct udevice *netdev;

Did you really intend to have a pointer here when the other is an
inline structure?

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

Please use positive logic.

>  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 = >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 */
>  #ifndefCONFIG_USB_ETH_CDC
> @@ -2188,7 +2204,11 @@ autoconf_fail:
>
>
> /* network device setup */
> +#ifndef CONFIG_DM_ETH
> dev->net = _priv->netdev;

You wouldn't need this difference if the priv also used a ptr in the
non-dm case.

Also, if you are opposed to cleaning this up (preferably by adding a
preceding patch to make it a pointer), at least use positive logic
(#ifdef CONFIG_DM_ETH). Same applies elsewhere.

> +#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);
>

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

2016-11-17 Thread Mugunthan V N 
Adopt usb ether gadget and rndis driver to adopt driver model

Signed-off-by: Mugunthan V N 
---
 drivers/usb/gadget/Kconfig |   4 ++
 drivers/usb/gadget/ether.c | 153 ++---
 drivers/usb/gadget/rndis.c |  13 +++-
 drivers/usb/gadget/rndis.h |  19 --
 include/net.h  |   7 +++
 5 files changed, 181 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 40839d89e9..261ed128ac 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -112,6 +112,10 @@ config G_DNL_VENDOR_NUM
 config G_DNL_PRODUCT_NUM
hex "Product ID of USB device"
 
+config USBNET_DEVADDR
+   string "USB Gadget Ethernet device mac address"
+   default "de:ad:be:ef:00:01"
+
 endif # USB_GADGET_DOWNLOAD
 
 endif # USB_GADGET
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index 046ad8ca2b..8c3c3fd9ab 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -25,6 +25,7 @@
 #include "rndis.h"
 
 #include 
+#include 
 #include 
 #include 
 
@@ -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 inttx_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 = >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 */
 #ifndefCONFIG_USB_ETH_CDC
@@ -2188,7 +2204,11 @@ autoconf_fail:
 
 
/* network device setup */
+#ifndef CONFIG_DM_ETH
dev->net = _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(>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;
@@ -2593,3 +2617,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