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

[Richard Leitner <dev@g0hl1n.net>]

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:
>>>>> From: Richard Leitner <dev@g0hl1n.net>

>>>>> +#define DRIVER_NAME "usb251xb"
>>>>> +#define DRIVER_DESC "Microchip USB 2.0 Hi-Speed Hub Controller"
>>>>> +#define DRIVER_VERSION "1.0"
>>>>
>>>> Is it my MUA, or all above indentations are broken?
>>>
>>> What do you mean?
>>
>> Should the strings be aligned, like the following?
>> #define DRIVER_NAME     "usb251xb"
>> #define DRIVER_DESC     "Microchip USB .."
>> #define DRIVER_VERSION	"1.0"
>
> Yep, tab vs. space indentation.

Ok, will do that for v5.

>>>> 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?

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

The captital letter and period looks fine. I'll apply that for v5.

>>>
>>>>> +#else /* CONFIG_OF */
>>>>> +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 :-)

Thanks & regards,
Richard L