Re: [PATCH 05/31] usb: usbssp: Added first part of initialization sequence.

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Jul 12, 2018 at 06:47:02AM +0100, Pawel Laszczak wrote:
> +/* 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?

> +/*
> + * 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?

> +	/*
> +	 * 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?


I've stopped reviewing this series here.

thanks,

greg k-h