Re: [PATCHv1 2/2] usb: gadget: add udc driver for max3420

[Jassi Brar]

Hi Felipe,

On Tue, Dec 10, 2019 at 1:13 AM Felipe Balbi <balbi@xxxxxxxxxx> wrote:

> > +#define MAX3420_MAX_EPS              4
> > +#define EP_MAX_PACKET                64  /* Same for all Endpoints */
> > +#define EPNAME_SIZE          16  /* Buffer size for endpoint name */
> > +
> > +#define ACKSTAT              BIT(0)
>
> Let's prepend everything with MAX3420_.
>
OK

> > +
> > +#define MAX3420_REG_EPSTALLS 9
> > +     #define bACKSTAT        BIT(6)
>
> let's avoid CaMeLcAsE :-)
>
ok

> > +#define field(val, bit)      ((val) << (bit))
>
> The kernel has a bunch of helpers for this. Look at BIT() and GENMASK()
> for example.
>
ok

> > +struct max3420_req {
> > +     struct usb_request usb_req;
> > +     struct list_head queue;
> > +     struct max3420_ep *ep;
> > +};
> > +
> > +struct max3420_ep {
> > +     struct max3420_udc *udc;
> > +     struct list_head queue;
> > +     char name[EPNAME_SIZE];
> > +     unsigned int maxpacket;
> > +     struct usb_ep ep_usb;
>
> considering you'll run container_of() on this ep_usb field, it's wise to
> put it as the first field in the struct. That way, compiler can optimize
> container_of() into a simple type cast.
>
ok

> > +struct max3420_udc {
> > +     struct max3420_ep ep[MAX3420_MAX_EPS];
> > +     struct usb_gadget_driver *driver;
> > +     struct task_struct *thread_task;
> > +     int remote_wkp, is_selfpowered;
> > +     bool vbus_active, softconnect;
> > +     struct usb_ctrlrequest setup;
> > +     struct mutex spi_bus_mutex;
> > +     struct max3420_req ep0req;
> > +     struct usb_gadget gadget;
>
> likewise with gadget field.
>
ok

>> +     spi_message_add_tail(&transfer, &msg);
>> +     spi_sync(spi, &msg);
> Not checking return code?
ok.

> > +     if (todo == ENABLE) {
> > +             epdis &= ~BIT(ep->id + 4);
> > +             epien |= BIT(ep->id + 1);
> > +     } else {
> > +             epdis |= BIT(ep->id + 4);
> > +             epien &= ~BIT(ep->id + 1);
> > +     }
> > +
> > +     spi_wr8(udc, MAX3420_REG_CLRTOGS, epdis);
> > +     spi_wr8(udc, MAX3420_REG_EPIEN, epien);
> > +
> > +     return 1;
>
> Usually we return 0 on success and a negative errno on failure. What do
> you mean here by return 1?
>
ok


> > +             ep->halted = 0;
> > +             epstalls &= ~BIT(ep->id + 1);
> > +             clrtogs = spi_rd8(udc, MAX3420_REG_CLRTOGS);
> > +             clrtogs |= BIT(ep->id + 1);
> > +             spi_wr8(udc, MAX3420_REG_CLRTOGS, clrtogs);
> > +     }
> > +     spi_wr8(udc, MAX3420_REG_EPSTALLS, epstalls | bACKSTAT);
> > +
> > +     return 1;
>
> and here?
>
ok


> > +
> > +     /* Clear Remote-WkUp Signal*/
> > +     usbctl = spi_rd8(udc, MAX3420_REG_USBCTL);
> > +     usbctl &= ~bSIGRWU;
> > +     spi_wr8(udc, MAX3420_REG_USBCTL, usbctl);
> > +
> > +     udc->suspended = false;
> > +
> > +     return 1;
>
> here?
>
ok


> > +
> > +static void __max3420_start(struct max3420_udc *udc)
> > +{
> > +     u8 val;
> > +
> > +     /* Need this delay if bus-powered */
> > +     msleep_interruptible(250);
>
> should you check if you're bus powered?
>
for some reason, even for self-powered, it helped reliability.


> > +
> > +static int do_data(struct max3420_udc *udc, int ep_id, int in)
>
> add a max3420_ prefix like all other functions
>
ok


> > +
> > +static int max3420_thread(void *dev_id)
>
> Why do you need this thread? Sure you can't live without it?
>
All the slow spi-bus transfers are handled at one place here without
blocking any api call. IMO it is cleaner and easier to manage.


> > +static struct usb_request *max3420_alloc_request(struct usb_ep *_ep,
> > +                                                 gfp_t gfp_flags)
> > +{
> > +     struct max3420_ep *ep = to_max3420_ep(_ep);
> > +     struct max3420_req *req;
> > +
> > +     req = kzalloc(sizeof(*req), gfp_flags);
> > +     if (!req)
> > +             return NULL;
> > +
> > +     req->ep = ep;
> > +     INIT_LIST_HEAD(&req->queue);
>
> unnecessary list initialization
>
ok.

Thank you!