> > +/* USB 2.0 hardware LMP capability*/
> > +#define USBSSP_HLC (1 << 19)
> > +#define USBSSP_BLC (1 << 20)
>
> Again, BIT() please.
>
> > +int usbssp_handshake(void __iomem *ptr, u32 mask, u32 done, int usec)
> > +{
> > + u32 result;
>
> Some places you use tabs for the variable declarations, and some you do
> not. Pick a single style and stick to it please.
>
> > +
> > + do {
> > + result = readl(ptr);
> > + if (result == ~(u32)0) /* card removed */
> > + return -ENODEV;
> > + result &= mask;
> > + if (result == done)
> > + return 0;
> > + udelay(1);
> > + usec--;
> > + } while (usec > 0);
> > + return -ETIMEDOUT;
>
> We don't have a built-in kernel function to do this type of thing already?
> That's sad. Oh well...
>
> > +int usbssp_init(struct usbssp_udc *usbssp_data) {
> > + int retval = 0;
> > +
> > + usbssp_dbg_trace(usbssp_data, trace_usbssp_dbg_init,
> "usbssp_init");
> > +
> > + spin_lock_init(&usbssp_data->lock);
> > + spin_lock_init(&usbssp_data->irq_thread_lock);
> > +
> > + //TODO: memory initialization
> > + //retval = usbssp_mem_init(usbssp_data, GFP_KERNEL);
> > +
> > + usbssp_dbg_trace(usbssp_data, trace_usbssp_dbg_init,
> > + "Finished usbssp_init");
>
> When your trace functions do nothing but say "entered a function", and
> "exited a function", why even have them? ftrace can provide that for you
> already, no need to overload that on the tracing framework, right?
Do you suggest to use only:
trace_usbssp_dbg_init("Finished usbssp_init");
instead:
usbssp_dbg(usbssp_data, "%pV\n", "Finished usbssp_init");
trace_usbssp_dbg_init("Finished usbssp_init");
?
I'm simple re-used the code from XHCI driver. It's really redundant,
but I don't know the intention of author 😊.
> > +/*
> > + * gadget-if.c file is part of gadget.c file and implements interface
> > + * for gadget driver
> > + */
> > +#include "gadget-if.c"
>
> Ugh, I know USB hcd drivers love to include .c files in the middle of them, but
> do we have to continue that crazy practice in newer drivers?
> Is it really necessary?
It's not necessary I wanted to leave this as two separate file, because gadget.c
Is based on xhci.c and gadget-if.c define interface between gadget.c and
USB gadget core driver. I didn't want to combine these two files.
I will add some additional function declaration to gadget.h and remove it
from gadget.c file.
> > + /*
> > + * Check the compiler generated sizes of structures that must be laid
> > + * out in specific ways for hardware access.
> > + */
> > + BUILD_BUG_ON(sizeof(struct usbssp_doorbell_array) != 2*32/8);
> > + BUILD_BUG_ON(sizeof(struct usbssp_slot_ctx) != 8*32/8);
> > + BUILD_BUG_ON(sizeof(struct usbssp_ep_ctx) != 8*32/8);
> > + /* usbssp_device has eight fields, and also
> > + * embeds one usbssp_slot_ctx and 31 usbssp_ep_ctx
> > + */
> > + BUILD_BUG_ON(sizeof(struct usbssp_stream_ctx) != 4*32/8);
> > + BUILD_BUG_ON(sizeof(union usbssp_trb) != 4*32/8);
> > + BUILD_BUG_ON(sizeof(struct usbssp_erst_entry) != 4*32/8);
> > + BUILD_BUG_ON(sizeof(struct usbssp_cap_regs) != 8*32/8);
> > + BUILD_BUG_ON(sizeof(struct usbssp_intr_reg) != 8*32/8);
> > + /* usbssp_run_regs has eight fields and embeds 128
> usbssp_intr_regs */
> > + BUILD_BUG_ON(sizeof(struct usbssp_run_regs) != (8+8*128)*32/8);
>
> I love hard-coded numbers as much as the next person, but really? Is this
> necessary now that you have the code up and working properly?
Code is still being developed, and is still tested. It's better to leave this code at
this moment. I will remove it in the feature.
thanks,
Pawel Laszczak
��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
