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!
