[PATCH v3 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[PATCH v3 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[Zixun LI]

Changes in v3:
- Separate code refactor into individual commits
- Extract the controller point from udevice private data in DM functions

Changes in v2:
- Fix null pointer deference in driver unbinding
- Separate code refactor into 2 parts
- Remove dead code

Changes in v1:
- Based on [PATCH RFC] usb: gadget: atmel: Add DM_USB_GADGET support:
  https://lists.denx.de/pipermail/u-boot/2024-July/559503.html
- Addressed comments, moved the refactoring to a preparatory patch.

Zixun LI (7):
  usb: gadget: atmel: Sort includes
  usb: gadget: atmel: Replace printf() and pr_err() by log_err()
  usb: gadget: atmel: Fix typo in usb_gadget_register_driver()
  usb: gadget: atmel: Move usba_udc_pdata() with other static functions
  usb: gadget: atmel: Rename atmel_usba_start()/_stop() to
    usba_udc_enable()/_disable()
  usb: gadget: atmel: Add attach/detach support
  usb: gadget: atmel: Add DM_USB_GADGET support

 drivers/usb/gadget/atmel_usba_udc.c | 250 ++++++++++++++++++++++------
 drivers/usb/gadget/atmel_usba_udc.h |   3 +
 include/linux/usb/atmel_usba_udc.h  |   2 +
 3 files changed, 208 insertions(+), 47 deletions(-)

--
2.45.2

[PATCH v3 1/7] usb: gadget: atmel: Sort includes

[Zixun LI]

Sort includes in alphabetical order.

Signed-off-by: Zixun LI <zli@ogga.fr>
---
 drivers/usb/gadget/atmel_usba_udc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index f99553df8d..5f78251fdb 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -7,16 +7,16 @@
  *			   Bo Shen <voice.shen@atmel.com>
  */
 
-#include <linux/bitops.h>
-#include <linux/errno.h>
+#include <malloc.h>
 #include <asm/gpio.h>
 #include <asm/hardware.h>
+#include <linux/bitops.h>
+#include <linux/errno.h>
 #include <linux/list.h>
 #include <linux/printk.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/atmel_usba_udc.h>
-#include <malloc.h>
 
 #include "atmel_usba_udc.h"
 
-- 
2.45.2

[PATCH v3 2/7] usb: gadget: atmel: Replace printf() and pr_err() by log_err()

[Zixun LI]

To have a uniform printing function, also drop linux/printk.h as no
longer used.

Signed-off-by: Zixun LI <zli@ogga.fr>
---
 drivers/usb/gadget/atmel_usba_udc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 5f78251fdb..83fdc36870 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -7,13 +7,13 @@
  *			   Bo Shen <voice.shen@atmel.com>
  */
 
+#include <log.h>
 #include <malloc.h>
 #include <asm/gpio.h>
 #include <asm/hardware.h>
 #include <linux/bitops.h>
 #include <linux/errno.h>
 #include <linux/list.h>
-#include <linux/printk.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/atmel_usba_udc.h>
@@ -1204,12 +1204,12 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 	int ret;
 
 	if (!driver || !driver->bind || !driver->setup) {
-		printf("bad paramter\n");
+		log_err("bad paramter\n");
 		return -EINVAL;
 	}
 
 	if (udc->driver) {
-		printf("UDC already has a gadget driver\n");
+		log_err("UDC already has a gadget driver\n");
 		return -EBUSY;
 	}
 
@@ -1219,7 +1219,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 
 	ret = driver->bind(&udc->gadget);
 	if (ret) {
-		pr_err("driver->bind() returned %d\n", ret);
+		log_err("driver->bind() returned %d\n", ret);
 		udc->driver = NULL;
 	}
 
@@ -1231,7 +1231,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	struct usba_udc *udc = &controller;
 
 	if (!driver || !driver->unbind || !driver->disconnect) {
-		pr_err("bad paramter\n");
+		log_err("bad parameter\n");
 		return -EINVAL;
 	}
 
@@ -1252,7 +1252,7 @@ static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
 
 	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
 	if (!eps) {
-		pr_err("failed to alloc eps\n");
+		log_err("failed to alloc eps\n");
 		return NULL;
 	}
 
-- 
2.45.2

[PATCH v3 3/7] usb: gadget: atmel: Fix typo in usb_gadget_register_driver()

[Zixun LI]

Replace "paramter" by "parameter".

Signed-off-by: Zixun LI <zli@ogga.fr>
---
 drivers/usb/gadget/atmel_usba_udc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 83fdc36870..2e3d8f36a2 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1204,7 +1204,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 	int ret;
 
 	if (!driver || !driver->bind || !driver->setup) {
-		log_err("bad paramter\n");
+		log_err("bad parameter\n");
 		return -EINVAL;
 	}
 
-- 
2.45.2

[PATCH v3 4/7] usb: gadget: atmel: Move usba_udc_pdata() with other static functions

[Zixun LI]

To make all static functions in the top, no functional change.

Signed-off-by: Zixun LI <zli@ogga.fr>
---
 drivers/usb/gadget/atmel_usba_udc.c | 80 ++++++++++++++---------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 2e3d8f36a2..a3d24501ba 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1179,6 +1179,46 @@ static int atmel_usba_stop(struct usba_udc *udc)
 	return 0;
 }
 
+static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
+				      struct usba_udc *udc)
+{
+	struct usba_ep *eps;
+	int i;
+
+	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
+	if (!eps) {
+		log_err("failed to alloc eps\n");
+		return NULL;
+	}
+
+	udc->gadget.ep0 = &eps[0].ep;
+
+	INIT_LIST_HEAD(&udc->gadget.ep_list);
+	INIT_LIST_HEAD(&eps[0].ep.ep_list);
+
+	for (i = 0; i < pdata->num_ep; i++) {
+		struct usba_ep *ep = &eps[i];
+
+		ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
+		ep->dma_regs = udc->regs + USBA_DMA_BASE(i);
+		ep->fifo = udc->fifo + USBA_FIFO_BASE(i);
+		ep->ep.ops = &usba_ep_ops;
+		ep->ep.name = pdata->ep[i].name;
+		ep->ep.maxpacket = pdata->ep[i].fifo_size;
+		ep->fifo_size = ep->ep.maxpacket;
+		ep->udc = udc;
+		INIT_LIST_HEAD(&ep->queue);
+		ep->nr_banks = pdata->ep[i].nr_banks;
+		ep->index = pdata->ep[i].index;
+		ep->can_dma = pdata->ep[i].can_dma;
+		ep->can_isoc = pdata->ep[i].can_isoc;
+		if (i)
+			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
+	};
+
+	return eps;
+}
+
 static struct usba_udc controller = {
 	.regs = (unsigned *)ATMEL_BASE_UDPHS,
 	.fifo = (unsigned *)ATMEL_BASE_UDPHS_FIFO,
@@ -1244,46 +1284,6 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	return 0;
 }
 
-static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
-				      struct usba_udc *udc)
-{
-	struct usba_ep *eps;
-	int i;
-
-	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
-	if (!eps) {
-		log_err("failed to alloc eps\n");
-		return NULL;
-	}
-
-	udc->gadget.ep0 = &eps[0].ep;
-
-	INIT_LIST_HEAD(&udc->gadget.ep_list);
-	INIT_LIST_HEAD(&eps[0].ep.ep_list);
-
-	for (i = 0; i < pdata->num_ep; i++) {
-		struct usba_ep *ep = &eps[i];
-
-		ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
-		ep->dma_regs = udc->regs + USBA_DMA_BASE(i);
-		ep->fifo = udc->fifo + USBA_FIFO_BASE(i);
-		ep->ep.ops = &usba_ep_ops;
-		ep->ep.name = pdata->ep[i].name;
-		ep->ep.maxpacket = pdata->ep[i].fifo_size;
-		ep->fifo_size = ep->ep.maxpacket;
-		ep->udc = udc;
-		INIT_LIST_HEAD(&ep->queue);
-		ep->nr_banks = pdata->ep[i].nr_banks;
-		ep->index = pdata->ep[i].index;
-		ep->can_dma = pdata->ep[i].can_dma;
-		ep->can_isoc = pdata->ep[i].can_isoc;
-		if (i)
-			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
-	};
-
-	return eps;
-}
-
 int usba_udc_probe(struct usba_platform_data *pdata)
 {
 	struct usba_udc *udc;
-- 
2.45.2

[PATCH v3 5/7] usb: gadget: atmel: Rename atmel_usba_start()/_stop() to usba_udc_enable()/_disable()

[Zixun LI]

Rename atmel_usba_start() / atmel_usba_stop() to usba_udc_enable()
/ usba_udc_disable(), remove atmel_ prefix to be inline with other
functions. Also avoid confusion with DM start() / stop() functions.

Signed-off-by: Zixun LI <zli@ogga.fr>
---
 drivers/usb/gadget/atmel_usba_udc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index a3d24501ba..ea9ad7585e 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1153,7 +1153,7 @@ static int usba_udc_irq(struct usba_udc *udc)
 	return 0;
 }
 
-static int atmel_usba_start(struct usba_udc *udc)
+static int usba_udc_enable(struct usba_udc *udc)
 {
 	udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
 
@@ -1168,7 +1168,7 @@ static int atmel_usba_start(struct usba_udc *udc)
 	return 0;
 }
 
-static int atmel_usba_stop(struct usba_udc *udc)
+static int usba_udc_disable(struct usba_udc *udc)
 {
 	udc->gadget.speed = USB_SPEED_UNKNOWN;
 	reset_all_endpoints(udc);
@@ -1253,7 +1253,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 		return -EBUSY;
 	}
 
-	atmel_usba_start(udc);
+	usba_udc_enable(udc);
 
 	udc->driver = driver;
 
@@ -1279,7 +1279,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	driver->unbind(&udc->gadget);
 	udc->driver = NULL;
 
-	atmel_usba_stop(udc);
+	usba_udc_disable(udc);
 
 	return 0;
 }
-- 
2.45.2

[PATCH v3 6/7] usb: gadget: atmel: Add attach/detach support

[Zixun LI]

Add controller attach/detach support by using
usb_gadget_ops.pullup() method.

Signed-off-by: Zixun LI <zli@ogga.fr>
---
 drivers/usb/gadget/atmel_usba_udc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index ea9ad7585e..a7b96449f8 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -506,10 +506,28 @@ usba_udc_set_selfpowered(struct usb_gadget *gadget, int is_selfpowered)
 	return 0;
 }
 
+static int usba_udc_pullup(struct usb_gadget *gadget, int is_on)
+{
+	struct usba_udc *udc = to_usba_udc(gadget);
+	u32 ctrl;
+
+	ctrl = usba_readl(udc, CTRL);
+
+	if (is_on)
+		ctrl &= ~USBA_DETACH;
+	else
+		ctrl |= USBA_DETACH;
+
+	usba_writel(udc, CTRL, ctrl);
+
+	return 0;
+}
+
 static const struct usb_gadget_ops usba_udc_ops = {
 	.get_frame		= usba_udc_get_frame,
 	.wakeup			= usba_udc_wakeup,
 	.set_selfpowered	= usba_udc_set_selfpowered,
+	.pullup			= usba_udc_pullup,
 };
 
 static struct usb_endpoint_descriptor usba_ep0_desc = {
-- 
2.45.2

[PATCH v3 7/7] usb: gadget: atmel: Add DM_USB_GADGET support

[Zixun LI]

Add driver model support by using the uclass UCLASS_USB_GADGET_GENERIC.

Disable local usb_gadget_register_driver()/usb_gadget_unregister_driver()
implementation which is implemented in udc-core.c when DM_USB_GADGET
is enabled.

Replace dm_usb_gadget_handle_interrupts() with handle_interrupts ops
when DM_USB_GADGET is enabled.

Disable legacy struct usba_udc controller as controller point is extracted
from udevice private data with DM.

Disable legacy usba_udc_probe() to avoid conflict with DM when it's
enabled.

Compared to Linux driver only supported devices' DT bindings are included
(sorted as Linux driver)

Signed-off-by: Zixun LI <zli@ogga.fr>
---
 drivers/usb/gadget/atmel_usba_udc.c | 138 ++++++++++++++++++++++++++++
 drivers/usb/gadget/atmel_usba_udc.h |   3 +
 include/linux/usb/atmel_usba_udc.h  |   2 +
 3 files changed, 143 insertions(+)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index a7b96449f8..b7b2e5196b 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -7,10 +7,14 @@
  *			   Bo Shen <voice.shen@atmel.com>
  */
 
+#include <clk.h>
+#include <dm.h>
 #include <log.h>
 #include <malloc.h>
 #include <asm/gpio.h>
 #include <asm/hardware.h>
+#include <dm/device_compat.h>
+#include <dm/devres.h>
 #include <linux/bitops.h>
 #include <linux/errno.h>
 #include <linux/list.h>
@@ -18,6 +22,14 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/atmel_usba_udc.h>
 
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+#include <mach/atmel_usba_udc.h>
+
+static int usba_udc_start(struct usb_gadget *gadget,
+			  struct usb_gadget_driver *driver);
+static int usba_udc_stop(struct usb_gadget *gadget);
+#endif /* CONFIG_IS_ENABLED(DM_USB_GADGET) */
+
 #include "atmel_usba_udc.h"
 
 static int vbus_is_present(struct usba_udc *udc)
@@ -528,6 +540,10 @@ static const struct usb_gadget_ops usba_udc_ops = {
 	.wakeup			= usba_udc_wakeup,
 	.set_selfpowered	= usba_udc_set_selfpowered,
 	.pullup			= usba_udc_pullup,
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+	.udc_start		= usba_udc_start,
+	.udc_stop		= usba_udc_stop,
+#endif
 };
 
 static struct usb_endpoint_descriptor usba_ep0_desc = {
@@ -1237,6 +1253,7 @@ static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
 	return eps;
 }
 
+#if !CONFIG_IS_ENABLED(DM_USB_GADGET)
 static struct usba_udc controller = {
 	.regs = (unsigned *)ATMEL_BASE_UDPHS,
 	.fifo = (unsigned *)ATMEL_BASE_UDPHS_FIFO,
@@ -1312,3 +1329,124 @@ int usba_udc_probe(struct usba_platform_data *pdata)
 
 	return 0;
 }
+
+#else /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */
+struct usba_priv_data {
+	struct clk_bulk		clks;
+	struct usba_udc		udc;
+};
+
+static int usba_udc_start(struct usb_gadget *gadget,
+			  struct usb_gadget_driver *driver)
+{
+	struct usba_udc *udc = to_usba_udc(gadget);
+
+	usba_udc_enable(udc);
+
+	udc->driver = driver;
+
+	return 0;
+}
+
+static int usba_udc_stop(struct usb_gadget *gadget)
+{
+	struct usba_udc *udc = to_usba_udc(gadget);
+
+	udc->driver = NULL;
+
+	usba_udc_disable(udc);
+
+	return 0;
+}
+
+static int usba_udc_clk_init(struct udevice *dev, struct clk_bulk *clks)
+{
+	int ret;
+
+	ret = clk_get_bulk(dev, clks);
+	if (ret == -ENOSYS)
+		return 0;
+
+	if (ret)
+		return ret;
+
+	ret = clk_enable_bulk(clks);
+	if (ret) {
+		clk_release_bulk(clks);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int usba_udc_probe(struct udevice *dev)
+{
+	struct usba_priv_data *priv = dev_get_priv(dev);
+	struct usba_udc *udc = &priv->udc;
+	int ret;
+
+	ret = usba_udc_clk_init(dev, &priv->clks);
+	if (ret)
+		return ret;
+
+	udc->fifo = (void __iomem *)dev_remap_addr_index(dev, FIFO_IOMEM_ID);
+	if (!udc->fifo)
+		return -EINVAL;
+
+	udc->regs = (void __iomem *)dev_remap_addr_index(dev, CTRL_IOMEM_ID);
+	if (!udc->regs)
+		return -EINVAL;
+
+	udc->gadget.ops = &usba_udc_ops;
+	udc->gadget.speed = USB_SPEED_HIGH,
+	udc->gadget.is_dualspeed = 1,
+	udc->gadget.name = "atmel_usba_udc",
+
+	udc->usba_ep = usba_udc_pdata(&pdata, udc);
+
+	udc->driver = NULL;
+
+	ret = usb_add_gadget_udc((struct device *)dev, &udc->gadget);
+
+	return ret;
+}
+
+static int usba_udc_remove(struct udevice *dev)
+{
+	struct usba_priv_data *priv = dev_get_priv(dev);
+
+	usb_del_gadget_udc(&priv->udc.gadget);
+
+	clk_release_bulk(&priv->clks);
+
+	return dm_scan_fdt_dev(dev);
+}
+
+static int usba_udc_handle_interrupts(struct udevice *dev)
+{
+	struct usba_priv_data *priv = dev_get_priv(dev);
+
+	return usba_udc_irq(&priv->udc);
+}
+
+static const struct usb_gadget_generic_ops usba_udc_gadget_ops = {
+	.handle_interrupts	= usba_udc_handle_interrupts,
+};
+
+static const struct udevice_id usba_udc_ids[] = {
+	{ .compatible = "atmel,at91sam9rl-udc" },
+	{ .compatible = "atmel,at91sam9g45-udc" },
+	{ .compatible = "atmel,sama5d3-udc" },
+	{}
+};
+
+U_BOOT_DRIVER(atmel_usba_udc) = {
+	.name	= "atmel_usba_udc",
+	.id	= UCLASS_USB_GADGET_GENERIC,
+	.of_match = usba_udc_ids,
+	.ops = &usba_udc_gadget_ops,
+	.probe = usba_udc_probe,
+	.remove = usba_udc_remove,
+	.priv_auto = sizeof(struct usba_priv_data),
+};
+#endif /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */
diff --git a/drivers/usb/gadget/atmel_usba_udc.h b/drivers/usb/gadget/atmel_usba_udc.h
index f6cb48c1cf..7f5e98f6c4 100644
--- a/drivers/usb/gadget/atmel_usba_udc.h
+++ b/drivers/usb/gadget/atmel_usba_udc.h
@@ -211,6 +211,9 @@
 #define EP0_EPT_SIZE		USBA_EPT_SIZE_64
 #define EP0_NR_BANKS		1
 
+#define FIFO_IOMEM_ID	0
+#define CTRL_IOMEM_ID	1
+
 #define DBG_ERR		0x0001	/* report all error returns */
 #define DBG_HW		0x0002	/* debug hardware initialization */
 #define DBG_GADGET	0x0004	/* calls to/from gadget driver */
diff --git a/include/linux/usb/atmel_usba_udc.h b/include/linux/usb/atmel_usba_udc.h
index c1c810759c..37c4f21849 100644
--- a/include/linux/usb/atmel_usba_udc.h
+++ b/include/linux/usb/atmel_usba_udc.h
@@ -20,6 +20,8 @@ struct usba_platform_data {
 	struct usba_ep_data	*ep;
 };
 
+#if !CONFIG_IS_ENABLED(DM_USB_GADGET)
 extern int usba_udc_probe(struct usba_platform_data *pdata);
+#endif
 
 #endif /* __LINUX_USB_USBA_H */
-- 
2.45.2

Re: [PATCH v3 1/7] usb: gadget: atmel: Sort includes

[Marek Vasut]

On 7/23/24 3:18 PM, Zixun LI wrote:
> Sort includes in alphabetical order.

Reviewed-by: Marek Vasut <marex@denx.de>

Thanks !

Re: [PATCH v3 2/7] usb: gadget: atmel: Replace printf() and pr_err() by log_err()

[Marek Vasut]

On 7/23/24 3:18 PM, Zixun LI wrote:
> To have a uniform printing function, also drop linux/printk.h as no
> longer used.

Reviewed-by: Marek Vasut <marex@denx.de>

Thanks !

Re: [PATCH v3 3/7] usb: gadget: atmel: Fix typo in usb_gadget_register_driver()

[Marek Vasut]

On 7/23/24 3:18 PM, Zixun LI wrote:
> Replace "paramter" by "parameter".
> 
> Signed-off-by: Zixun LI <zli@ogga.fr>

Reviewed-by: Marek Vasut <marex@denx.de>

This is really MUCH better and MUCH easier to review, thank you!

Re: [PATCH v3 4/7] usb: gadget: atmel: Move usba_udc_pdata() with other static functions

[Marek Vasut]

On 7/23/24 3:18 PM, Zixun LI wrote:
> To make all static functions in the top, no functional change.

Reviewed-by: Marek Vasut <marex@denx.de>

Thanks !

Re: [PATCH v3 5/7] usb: gadget: atmel: Rename atmel_usba_start()/_stop() to usba_udc_enable()/_disable()

[Marek Vasut]

On 7/23/24 3:18 PM, Zixun LI wrote:
> Rename atmel_usba_start() / atmel_usba_stop() to usba_udc_enable()
> / usba_udc_disable(), remove atmel_ prefix to be inline with other
> functions. Also avoid confusion with DM start() / stop() functions.

Reviewed-by: Marek Vasut <marex@denx.de>

Thanks !

Re: [PATCH v3 6/7] usb: gadget: atmel: Add attach/detach support

[Marek Vasut]

On 7/23/24 3:18 PM, Zixun LI wrote:
> Add controller attach/detach support by using
> usb_gadget_ops.pullup() method.

It is called 'function' in C, not 'method'. I think 'method' is C++ or 
maybe Java ?

In any case:

Reviewed-by: Marek Vasut <marex@denx.de>

Thanks !

Re: [PATCH v3 1/7] usb: gadget: atmel: Sort includes

[Mattijs Korpershoek]

Hi Zixun,

Thank you for the patch.

On mar., juil. 23, 2024 at 15:18, Zixun LI <admin@hifiphile.com> wrote:

> Sort includes in alphabetical order.
>
> Signed-off-by: Zixun LI <zli@ogga.fr>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/gadget/atmel_usba_udc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
> index f99553df8d..5f78251fdb 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/atmel_usba_udc.c
> @@ -7,16 +7,16 @@
>   *			   Bo Shen <voice.shen@atmel.com>
>   */
>  
> -#include <linux/bitops.h>
> -#include <linux/errno.h>
> +#include <malloc.h>
>  #include <asm/gpio.h>
>  #include <asm/hardware.h>
> +#include <linux/bitops.h>
> +#include <linux/errno.h>
>  #include <linux/list.h>
>  #include <linux/printk.h>
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/atmel_usba_udc.h>
> -#include <malloc.h>
>  
>  #include "atmel_usba_udc.h"
>  
> -- 
> 2.45.2

Re: [PATCH v3 2/7] usb: gadget: atmel: Replace printf() and pr_err() by log_err()

[Mattijs Korpershoek]

Hi Zixun,

Thank you for the patch.

On mar., juil. 23, 2024 at 15:18, Zixun LI <admin@hifiphile.com> wrote:

> To have a uniform printing function, also drop linux/printk.h as no
> longer used.
>
> Signed-off-by: Zixun LI <zli@ogga.fr>

Checkpatch complains here:

checkpatch.pl: 164: WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI <admin@hifiphile.com>' != 'Signed-off-by: Zixun LI <zli@ogga.fr>'

Please make sure that the commiter matches the Signed-off-by.
This can be done with git commit --reset-author or --author="name <email>"

> ---
>  drivers/usb/gadget/atmel_usba_udc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
> index 5f78251fdb..83fdc36870 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/atmel_usba_udc.c
> @@ -7,13 +7,13 @@
>   *			   Bo Shen <voice.shen@atmel.com>
>   */
>  
> +#include <log.h>
>  #include <malloc.h>
>  #include <asm/gpio.h>
>  #include <asm/hardware.h>
>  #include <linux/bitops.h>
>  #include <linux/errno.h>
>  #include <linux/list.h>
> -#include <linux/printk.h>
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/atmel_usba_udc.h>
> @@ -1204,12 +1204,12 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>  	int ret;
>  
>  	if (!driver || !driver->bind || !driver->setup) {
> -		printf("bad paramter\n");
> +		log_err("bad paramter\n");
>  		return -EINVAL;
>  	}
>  
>  	if (udc->driver) {
> -		printf("UDC already has a gadget driver\n");
> +		log_err("UDC already has a gadget driver\n");
>  		return -EBUSY;
>  	}
>  
> @@ -1219,7 +1219,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>  
>  	ret = driver->bind(&udc->gadget);
>  	if (ret) {
> -		pr_err("driver->bind() returned %d\n", ret);
> +		log_err("driver->bind() returned %d\n", ret);
>  		udc->driver = NULL;
>  	}
>  
> @@ -1231,7 +1231,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  	struct usba_udc *udc = &controller;
>  
>  	if (!driver || !driver->unbind || !driver->disconnect) {
> -		pr_err("bad paramter\n");
> +		log_err("bad parameter\n");

Here we change both:
1. pr_err() to log_err()
2. fix paramter -> parameter

2. should be done in patch 3/7 instead of here.

With the above fixed:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

>  		return -EINVAL;
>  	}
>  
> @@ -1252,7 +1252,7 @@ static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
>  
>  	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
>  	if (!eps) {
> -		pr_err("failed to alloc eps\n");
> +		log_err("failed to alloc eps\n");
>  		return NULL;
>  	}
>  
> -- 
> 2.45.2

Re: [PATCH v3 3/7] usb: gadget: atmel: Fix typo in usb_gadget_register_driver()

[Mattijs Korpershoek]

Hi Zixun,

Thank you for the patch.

On mar., juil. 23, 2024 at 15:18, Zixun LI <admin@hifiphile.com> wrote:

> Replace "paramter" by "parameter".
>
> Signed-off-by: Zixun LI <zli@ogga.fr>

Same remark on checkpatch warning:

WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI <admin@hifiphile.com>' != 'Signed-off-by: Zixun LI <zli@ogga.fr>'


> ---
>  drivers/usb/gadget/atmel_usba_udc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
> index 83fdc36870..2e3d8f36a2 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/atmel_usba_udc.c
> @@ -1204,7 +1204,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>  	int ret;
>  
>  	if (!driver || !driver->bind || !driver->setup) {
> -		log_err("bad paramter\n");
> +		log_err("bad parameter\n");
>  		return -EINVAL;
>  	}
>  
> -- 
> 2.45.2

Re: [PATCH v3 4/7] usb: gadget: atmel: Move usba_udc_pdata() with other static functions

[Mattijs Korpershoek]

Hi Zixun,

Thank you for the patch.

On mar., juil. 23, 2024 at 15:18, Zixun LI <admin@hifiphile.com> wrote:

> To make all static functions in the top, no functional change.
>
> Signed-off-by: Zixun LI <zli@ogga.fr>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/gadget/atmel_usba_udc.c | 80 ++++++++++++++---------------
>  1 file changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
> index 2e3d8f36a2..a3d24501ba 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/atmel_usba_udc.c
> @@ -1179,6 +1179,46 @@ static int atmel_usba_stop(struct usba_udc *udc)
>  	return 0;
>  }
>  
> +static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
> +				      struct usba_udc *udc)
> +{
> +	struct usba_ep *eps;
> +	int i;
> +
> +	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
> +	if (!eps) {
> +		log_err("failed to alloc eps\n");
> +		return NULL;
> +	}
> +
> +	udc->gadget.ep0 = &eps[0].ep;
> +
> +	INIT_LIST_HEAD(&udc->gadget.ep_list);
> +	INIT_LIST_HEAD(&eps[0].ep.ep_list);
> +
> +	for (i = 0; i < pdata->num_ep; i++) {
> +		struct usba_ep *ep = &eps[i];
> +
> +		ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
> +		ep->dma_regs = udc->regs + USBA_DMA_BASE(i);
> +		ep->fifo = udc->fifo + USBA_FIFO_BASE(i);
> +		ep->ep.ops = &usba_ep_ops;
> +		ep->ep.name = pdata->ep[i].name;
> +		ep->ep.maxpacket = pdata->ep[i].fifo_size;
> +		ep->fifo_size = ep->ep.maxpacket;
> +		ep->udc = udc;
> +		INIT_LIST_HEAD(&ep->queue);
> +		ep->nr_banks = pdata->ep[i].nr_banks;
> +		ep->index = pdata->ep[i].index;
> +		ep->can_dma = pdata->ep[i].can_dma;
> +		ep->can_isoc = pdata->ep[i].can_isoc;
> +		if (i)
> +			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
> +	};
> +
> +	return eps;
> +}
> +
>  static struct usba_udc controller = {
>  	.regs = (unsigned *)ATMEL_BASE_UDPHS,
>  	.fifo = (unsigned *)ATMEL_BASE_UDPHS_FIFO,
> @@ -1244,46 +1284,6 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  	return 0;
>  }
>  
> -static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
> -				      struct usba_udc *udc)
> -{
> -	struct usba_ep *eps;
> -	int i;
> -
> -	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
> -	if (!eps) {
> -		log_err("failed to alloc eps\n");
> -		return NULL;
> -	}
> -
> -	udc->gadget.ep0 = &eps[0].ep;
> -
> -	INIT_LIST_HEAD(&udc->gadget.ep_list);
> -	INIT_LIST_HEAD(&eps[0].ep.ep_list);
> -
> -	for (i = 0; i < pdata->num_ep; i++) {
> -		struct usba_ep *ep = &eps[i];
> -
> -		ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
> -		ep->dma_regs = udc->regs + USBA_DMA_BASE(i);
> -		ep->fifo = udc->fifo + USBA_FIFO_BASE(i);
> -		ep->ep.ops = &usba_ep_ops;
> -		ep->ep.name = pdata->ep[i].name;
> -		ep->ep.maxpacket = pdata->ep[i].fifo_size;
> -		ep->fifo_size = ep->ep.maxpacket;
> -		ep->udc = udc;
> -		INIT_LIST_HEAD(&ep->queue);
> -		ep->nr_banks = pdata->ep[i].nr_banks;
> -		ep->index = pdata->ep[i].index;
> -		ep->can_dma = pdata->ep[i].can_dma;
> -		ep->can_isoc = pdata->ep[i].can_isoc;
> -		if (i)
> -			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
> -	};
> -
> -	return eps;
> -}
> -
>  int usba_udc_probe(struct usba_platform_data *pdata)
>  {
>  	struct usba_udc *udc;
> -- 
> 2.45.2

Re: [PATCH v3 5/7] usb: gadget: atmel: Rename atmel_usba_start()/_stop() to usba_udc_enable()/_disable()

[Mattijs Korpershoek]

Hi Zixun,

Thank you for the patch.

On mar., juil. 23, 2024 at 15:18, Zixun LI <admin@hifiphile.com> wrote:

> Rename atmel_usba_start() / atmel_usba_stop() to usba_udc_enable()
> / usba_udc_disable(), remove atmel_ prefix to be inline with other
> functions. Also avoid confusion with DM start() / stop() functions.
>
> Signed-off-by: Zixun LI <zli@ogga.fr>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/gadget/atmel_usba_udc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
> index a3d24501ba..ea9ad7585e 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/atmel_usba_udc.c
> @@ -1153,7 +1153,7 @@ static int usba_udc_irq(struct usba_udc *udc)
>  	return 0;
>  }
>  
> -static int atmel_usba_start(struct usba_udc *udc)
> +static int usba_udc_enable(struct usba_udc *udc)
>  {
>  	udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
>  
> @@ -1168,7 +1168,7 @@ static int atmel_usba_start(struct usba_udc *udc)
>  	return 0;
>  }
>  
> -static int atmel_usba_stop(struct usba_udc *udc)
> +static int usba_udc_disable(struct usba_udc *udc)
>  {
>  	udc->gadget.speed = USB_SPEED_UNKNOWN;
>  	reset_all_endpoints(udc);
> @@ -1253,7 +1253,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>  		return -EBUSY;
>  	}
>  
> -	atmel_usba_start(udc);
> +	usba_udc_enable(udc);
>  
>  	udc->driver = driver;
>  
> @@ -1279,7 +1279,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
>  	driver->unbind(&udc->gadget);
>  	udc->driver = NULL;
>  
> -	atmel_usba_stop(udc);
> +	usba_udc_disable(udc);
>  
>  	return 0;
>  }
> -- 
> 2.45.2

Re: [PATCH v3 6/7] usb: gadget: atmel: Add attach/detach support

[Mattijs Korpershoek]

Hi Zixun,

Thank you for the patch.

On mar., juil. 23, 2024 at 15:18, Zixun LI <admin@hifiphile.com> wrote:

> Add controller attach/detach support by using
> usb_gadget_ops.pullup() method.
>
> Signed-off-by: Zixun LI <zli@ogga.fr>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/gadget/atmel_usba_udc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
> index ea9ad7585e..a7b96449f8 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/atmel_usba_udc.c
> @@ -506,10 +506,28 @@ usba_udc_set_selfpowered(struct usb_gadget *gadget, int is_selfpowered)
>  	return 0;
>  }
>  
> +static int usba_udc_pullup(struct usb_gadget *gadget, int is_on)
> +{
> +	struct usba_udc *udc = to_usba_udc(gadget);
> +	u32 ctrl;
> +
> +	ctrl = usba_readl(udc, CTRL);
> +
> +	if (is_on)
> +		ctrl &= ~USBA_DETACH;
> +	else
> +		ctrl |= USBA_DETACH;
> +
> +	usba_writel(udc, CTRL, ctrl);
> +
> +	return 0;
> +}
> +
>  static const struct usb_gadget_ops usba_udc_ops = {
>  	.get_frame		= usba_udc_get_frame,
>  	.wakeup			= usba_udc_wakeup,
>  	.set_selfpowered	= usba_udc_set_selfpowered,
> +	.pullup			= usba_udc_pullup,
>  };
>  
>  static struct usb_endpoint_descriptor usba_ep0_desc = {
> -- 
> 2.45.2

Re: [PATCH v3 6/7] usb: gadget: atmel: Add attach/detach support

[Mattijs Korpershoek]

On mar., juil. 23, 2024 at 15:36, Marek Vasut <marex@denx.de> wrote:

> On 7/23/24 3:18 PM, Zixun LI wrote:
>> Add controller attach/detach support by using
>> usb_gadget_ops.pullup() method.
>
> It is called 'function' in C, not 'method'. I think 'method' is C++ or 
> maybe Java ?

Agreed, 'function' is more appropriate wording.

Zixun, if you need to respin a v4 please consider using 'function'.

Otherwise I can fix it up while applying !

>
> In any case:
>
> Reviewed-by: Marek Vasut <marex@denx.de>
>
> Thanks !

Re: [PATCH v3 7/7] usb: gadget: atmel: Add DM_USB_GADGET support

[Mattijs Korpershoek]

Hi Zixun,

Thank you for the patch and doing this very welcome DM_USB_GADGET
addition :)

On mar., juil. 23, 2024 at 15:18, Zixun LI <admin@hifiphile.com> wrote:

> Add driver model support by using the uclass UCLASS_USB_GADGET_GENERIC.
>
> Disable local usb_gadget_register_driver()/usb_gadget_unregister_driver()
> implementation which is implemented in udc-core.c when DM_USB_GADGET
> is enabled.
>
> Replace dm_usb_gadget_handle_interrupts() with handle_interrupts ops
> when DM_USB_GADGET is enabled.
>
> Disable legacy struct usba_udc controller as controller point is extracted
> from udevice private data with DM.
>
> Disable legacy usba_udc_probe() to avoid conflict with DM when it's
> enabled.
>
> Compared to Linux driver only supported devices' DT bindings are included
> (sorted as Linux driver)
>
> Signed-off-by: Zixun LI <zli@ogga.fr>

checkpatch.pl seems to complain about Signed-off-by vs commiter here as
well:

checkpatch.pl: 324: WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI <admin@hifiphile.com>' != 'Signed-off-by: Zixun LI <zli@ogga.fr>'

> ---
>  drivers/usb/gadget/atmel_usba_udc.c | 138 ++++++++++++++++++++++++++++
>  drivers/usb/gadget/atmel_usba_udc.h |   3 +
>  include/linux/usb/atmel_usba_udc.h  |   2 +
>  3 files changed, 143 insertions(+)
>
> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
> index a7b96449f8..b7b2e5196b 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/atmel_usba_udc.c
> @@ -7,10 +7,14 @@
>   *			   Bo Shen <voice.shen@atmel.com>
>   */
>  
> +#include <clk.h>
> +#include <dm.h>
>  #include <log.h>
>  #include <malloc.h>
>  #include <asm/gpio.h>
>  #include <asm/hardware.h>
> +#include <dm/device_compat.h>
> +#include <dm/devres.h>
>  #include <linux/bitops.h>
>  #include <linux/errno.h>
>  #include <linux/list.h>
> @@ -18,6 +22,14 @@
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/atmel_usba_udc.h>
>  
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +#include <mach/atmel_usba_udc.h>
> +
> +static int usba_udc_start(struct usb_gadget *gadget,
> +			  struct usb_gadget_driver *driver);
> +static int usba_udc_stop(struct usb_gadget *gadget);
> +#endif /* CONFIG_IS_ENABLED(DM_USB_GADGET) */
> +
>  #include "atmel_usba_udc.h"
>  
>  static int vbus_is_present(struct usba_udc *udc)
> @@ -528,6 +540,10 @@ static const struct usb_gadget_ops usba_udc_ops = {
>  	.wakeup			= usba_udc_wakeup,
>  	.set_selfpowered	= usba_udc_set_selfpowered,
>  	.pullup			= usba_udc_pullup,
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +	.udc_start		= usba_udc_start,
> +	.udc_stop		= usba_udc_stop,
> +#endif
>  };
>  
>  static struct usb_endpoint_descriptor usba_ep0_desc = {
> @@ -1237,6 +1253,7 @@ static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
>  	return eps;
>  }
>  
> +#if !CONFIG_IS_ENABLED(DM_USB_GADGET)
>  static struct usba_udc controller = {
>  	.regs = (unsigned *)ATMEL_BASE_UDPHS,
>  	.fifo = (unsigned *)ATMEL_BASE_UDPHS_FIFO,
> @@ -1312,3 +1329,124 @@ int usba_udc_probe(struct usba_platform_data *pdata)
>  
>  	return 0;
>  }
> +
> +#else /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */
> +struct usba_priv_data {
> +	struct clk_bulk		clks;
> +	struct usba_udc		udc;
> +};
> +
> +static int usba_udc_start(struct usb_gadget *gadget,
> +			  struct usb_gadget_driver *driver)
> +{
> +	struct usba_udc *udc = to_usba_udc(gadget);
> +
> +	usba_udc_enable(udc);
> +
> +	udc->driver = driver;

Nitpick: no need for extra blank line here:

	usba_udc_enable(udc);
	udc->driver = driver;

> +
> +	return 0;
> +}
> +
> +static int usba_udc_stop(struct usb_gadget *gadget)
> +{
> +	struct usba_udc *udc = to_usba_udc(gadget);
> +
> +	udc->driver = NULL;
> +
> +	usba_udc_disable(udc);

Same

> +
> +	return 0;
> +}
> +
> +static int usba_udc_clk_init(struct udevice *dev, struct clk_bulk *clks)
> +{
> +	int ret;
> +
> +	ret = clk_get_bulk(dev, clks);
> +	if (ret == -ENOSYS)
> +		return 0;
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable_bulk(clks);
> +	if (ret) {
> +		clk_release_bulk(clks);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int usba_udc_probe(struct udevice *dev)
> +{
> +	struct usba_priv_data *priv = dev_get_priv(dev);
> +	struct usba_udc *udc = &priv->udc;
> +	int ret;
> +
> +	ret = usba_udc_clk_init(dev, &priv->clks);
> +	if (ret)
> +		return ret;
> +
> +	udc->fifo = (void __iomem *)dev_remap_addr_index(dev, FIFO_IOMEM_ID);
> +	if (!udc->fifo)

I think that at this points &priv->clks are valid and enabled.
Therefore, we should disable/release them (as being done in the remove)

> +		return -EINVAL;
> +
> +	udc->regs = (void __iomem *)dev_remap_addr_index(dev, CTRL_IOMEM_ID);
> +	if (!udc->regs)

Same here

> +		return -EINVAL;
> +
> +	udc->gadget.ops = &usba_udc_ops;
> +	udc->gadget.speed = USB_SPEED_HIGH,
> +	udc->gadget.is_dualspeed = 1,
> +	udc->gadget.name = "atmel_usba_udc",
> +
> +	udc->usba_ep = usba_udc_pdata(&pdata, udc);
> +
> +	udc->driver = NULL;

Why do we assign the driver to NULL ? Maybe this deserves a comment in
the code?

> +
> +	ret = usb_add_gadget_udc((struct device *)dev, &udc->gadget);
> +
> +	return ret;
> +}
> +
> +static int usba_udc_remove(struct udevice *dev)
> +{
> +	struct usba_priv_data *priv = dev_get_priv(dev);
> +
> +	usb_del_gadget_udc(&priv->udc.gadget);
> +
> +	clk_release_bulk(&priv->clks);
> +
> +	return dm_scan_fdt_dev(dev);
> +}
> +
> +static int usba_udc_handle_interrupts(struct udevice *dev)
> +{
> +	struct usba_priv_data *priv = dev_get_priv(dev);
> +
> +	return usba_udc_irq(&priv->udc);
> +}
> +
> +static const struct usb_gadget_generic_ops usba_udc_gadget_ops = {
> +	.handle_interrupts	= usba_udc_handle_interrupts,
> +};
> +
> +static const struct udevice_id usba_udc_ids[] = {
> +	{ .compatible = "atmel,at91sam9rl-udc" },
> +	{ .compatible = "atmel,at91sam9g45-udc" },
> +	{ .compatible = "atmel,sama5d3-udc" },
> +	{}
> +};
> +
> +U_BOOT_DRIVER(atmel_usba_udc) = {
> +	.name	= "atmel_usba_udc",
> +	.id	= UCLASS_USB_GADGET_GENERIC,
> +	.of_match = usba_udc_ids,
> +	.ops = &usba_udc_gadget_ops,
> +	.probe = usba_udc_probe,
> +	.remove = usba_udc_remove,
> +	.priv_auto = sizeof(struct usba_priv_data),
> +};
> +#endif /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */
> diff --git a/drivers/usb/gadget/atmel_usba_udc.h b/drivers/usb/gadget/atmel_usba_udc.h
> index f6cb48c1cf..7f5e98f6c4 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/atmel_usba_udc.h
> @@ -211,6 +211,9 @@
>  #define EP0_EPT_SIZE		USBA_EPT_SIZE_64
>  #define EP0_NR_BANKS		1
>  
> +#define FIFO_IOMEM_ID	0
> +#define CTRL_IOMEM_ID	1
> +
>  #define DBG_ERR		0x0001	/* report all error returns */
>  #define DBG_HW		0x0002	/* debug hardware initialization */
>  #define DBG_GADGET	0x0004	/* calls to/from gadget driver */
> diff --git a/include/linux/usb/atmel_usba_udc.h b/include/linux/usb/atmel_usba_udc.h
> index c1c810759c..37c4f21849 100644
> --- a/include/linux/usb/atmel_usba_udc.h
> +++ b/include/linux/usb/atmel_usba_udc.h
> @@ -20,6 +20,8 @@ struct usba_platform_data {
>  	struct usba_ep_data	*ep;
>  };
>  
> +#if !CONFIG_IS_ENABLED(DM_USB_GADGET)
>  extern int usba_udc_probe(struct usba_platform_data *pdata);
> +#endif
>  
>  #endif /* __LINUX_USB_USBA_H */
> -- 
> 2.45.2

Re: [PATCH v3 7/7] usb: gadget: atmel: Add DM_USB_GADGET support

[admin LI]

Hi Mattijs,

On Wed, Jul 24, 2024 at 12:12 PM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Checkpatch complains here:
>
> checkpatch.pl: 164: WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI <admin@hifiphile.com>' != 'Signed-off-by: Zixun LI <zli@ogga.fr>'
>
> Please make sure that the commiter matches the Signed-off-by.
> This can be done with git commit --reset-author or --author="name <email>"
>

Will fix all Signed-off-by in v4.

>
> Here we change both:
> 1. pr_err() to log_err()
> 2. fix paramter -> parameter
>
> 2. should be done in patch 3/7 instead of here.
>
> With the above fixed:
>

Will fix in v4.

> Agreed, 'function' is more appropriate wording.
> Zixun, if you need to respin a v4 please consider using 'function'.
> Otherwise I can fix it up while applying !

Will fix in v4.

> > +static int usba_udc_probe(struct udevice *dev)
> > +{
> > +     struct usba_priv_data *priv = dev_get_priv(dev);
> > +     struct usba_udc *udc = &priv->udc;
> > +     int ret;
> > +
> > +     ret = usba_udc_clk_init(dev, &priv->clks);
> > +     if (ret)
> > +             return ret;
> > +
> > +     udc->fifo = (void __iomem *)dev_remap_addr_index(dev, FIFO_IOMEM_ID);
> > +     if (!udc->fifo)
>
> I think that at this points &priv->clks are valid and enabled.
> Therefore, we should disable/release them (as being done in the remove)
>
> > +             return -EINVAL;
> > +
> > +     udc->regs = (void __iomem *)dev_remap_addr_index(dev, CTRL_IOMEM_ID);
> > +     if (!udc->regs)
>
> Same here

Good catch, I copied the dwc2 driver who has the same issue, will fix in v4.

Also spotted udc->usba_ep allocated in usba_udc_pdata() is not freed.

I'll change probe() and remove() functions as follow:

static int usba_udc_probe(struct udevice *dev)
{
    struct usba_priv_data *priv = dev_get_priv(dev);
    struct usba_udc *udc = &priv->udc;
    int ret;

    udc->fifo = (void __iomem *)dev_remap_addr_index(dev, FIFO_IOMEM_ID);
    if (!udc->fifo)
        return -EINVAL;

    udc->regs = (void __iomem *)dev_remap_addr_index(dev, CTRL_IOMEM_ID);
    if (!udc->regs)
        return -EINVAL;

    ret = usba_udc_clk_init(dev, &priv->clks);
    if (ret)
        return ret;

    udc->usba_ep = usba_udc_pdata(&pdata, udc);

    udc->gadget.ops = &usba_udc_ops;
    udc->gadget.speed = USB_SPEED_HIGH,
    udc->gadget.is_dualspeed = 1,
    udc->gadget.name = "atmel_usba_udc",

    ret = usb_add_gadget_udc((struct device *)dev, &udc->gadget);
    if (ret)
        goto err;

    return 0;
err:
    free(priv->udc.usba_ep);

    clk_release_bulk(&priv->clks);

    return ret;
}

static int usba_udc_remove(struct udevice *dev)
{
    struct usba_priv_data *priv = dev_get_priv(dev);

    usb_del_gadget_udc(&priv->udc.gadget);

    free(priv->udc.usba_ep);

    clk_release_bulk(&priv->clks);

    return dm_scan_fdt_dev(dev);
}