Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, 2017-02-08 at 19:45 +0100, Richard Leitner wrote:
> On 02/08/2017 05:40 PM, Andy Shevchenko wrote:
> > On Wed, 2017-02-08 at 16:17 +0100, Richard Leitner wrote:
> > > On 02/08/2017 02:59 PM, Greg KH wrote:
> > > > On Wed, Feb 08, 2017 at 03:21:08PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, 2017-02-08 at 09:52 +0100, Richard Leitner wrote:

> > > > > Above doesn't make much sense. Why not to use
> > > > > 
> > > > > > BIT(bit)
> > > > > 
> > > > > and
> > > > > 
> > > > > & ~BIT(bit)
> > > > > 
> > > > > in place?
> > > > 
> > > > I thought we already had functions to do this for you.  Don't
> > > > write
> > > > new
> > > > ones "by hand" either wya.
> > > 
> > > Which functions do you mean? I only found set_bit() and
> > > clear_bit()
> > > from
> > > atomic_ops. But those operate on "unsigned long" variables. From
> > > the
> > > documentation:
> > > 	Native atomic bit operations are defined to operate
> > > 	on objects aligned to the size of an "unsigned long"
> > > 	C data type, and are least of that size.
> > 
> > __set_bit(), __clear_bit() -- non-atomic variants, but you are
> > right,
> > that (unsigned long) exactly the point I didn't propose them.
> 
> So the preferred solution is the BIT() stuff?

I think BIT() macro in place. Otherwise you'll need a temporary unsigned
long variable. If you already have one, then __*_bit() would work.

> +		/* the first data byte transferred tells
> > > > > > the
> > > > > > hub how
> > > > > > many data
> > > > > > +		 * bytes will follow (byte count)
> > > > > > +		 */
> > > > > 
> > > > > I'm not sure this is good formatted comment for USB subsystem.
> > > > 
> > > > Looks fine to me, why do you think it is incorrect?
> > 
> > I would do like
> > 
> > /*
> >  * The multi-line
> >  * comment.
> >  */
> > 
> > Capital letter, period at the end, first empty line (unlike in net
> > subsystem).
> 
> So what's the preferred format? Empty line at the beginning or not?

Both fine to me. It's not a real code anyway.

> > > > > > +static int usb251xb_get_ofdata(struct usb251xb *hub,
> > > > > > +			       struct usb251xb_data *data)
> > > > > > +{
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +#endif /* CONFIG_OF */
> > > > > 
> > > > > I don't think it's a good idea to have those ugly #ifdef.
> > > > 
> > > > How can it be removed?
> > 
> > __maybe_unused for function, device_property_*() instead of
> > of_property_*() calls.
> > 
> > Something like that. But if you are insisting this is *only* OF
> > available hardware or we don't care, I'll not object.
> 
> In usb3503.c and usb4604.c we have that #ifdef CONFIG_OF too. IMHO
> those 
> drivers should use the same solution here. But you guys are the ones 
> with tons of kernel coding experience, so just say how it should be
> done :-)

I'll agree with whatever Greg wants here.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy