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

[Greg Kroah-Hartman]

On Thu, Jul 12, 2018 at 09:03:30AM +0000, 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?
> 
> 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 ð.  

Why are any of those lines needed?  Doesn't ftrace work properly for
you?

And yeah, if xhci has this it should be removed from there as well.

thanks,

greg k-h