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

[Pawel Laszczak]

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

Ok, I already understand what's going on. 

I didn't use ftrace. During developing driver it is faster and simpler for me to add simple debug message 
then use ftrace.

I agree that it is not needed here.  It will be removed.

thanks,
Pawel Laszczak