[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 at baylibre.com> wrote:
>
> Checkpatch complains here:
>
> checkpatch.pl: 164: WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI <admin at hifiphile.com>' != 'Signed-off-by: Zixun LI <zli at 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);
}