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

[Richard Leitner]

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@xxxxxxxxxx>
>>
>> If you want to fix the above you have to fix your Git configuration.

My git config is fine, just cherry-picked it from a remote and forgot I
committed it from another computer with another git config ;-)
Will fix that in v5 for sure!

>>
>>
>>> This patch adds a driver for configuration of the Microchip
>>> USB251xB/xBi
>>> USB 2.0 hub controller series with USB 2.0 upstream connectivity,
>>> SMBus
>>> configuration interface and two to four USB 2.0 downstream ports.
>>>
>>> Furthermore add myself as a maintainer for this driver.
>>>
>>> The datasheet can be found at the manufacturers website, see [1]. All
>>> device-tree exposed configuration features have been tested on a i.MX6
>>> platform with a USB2512B hub.
>>
>>> +++ b/drivers/usb/misc/usb251xb.c
>>> @@ -0,0 +1,674 @@
>>
>>> +#include <linux/i2c.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/nls.h>
>>
>> Alphabetical order?
> 
> Ick, no, who cares, really.  It's whatever order the author wants, don't
> be so picky.

Ok :-)
But somehow you're right Andy, alphabetical order seems to look better
here (will do that in v5).

> 
>>> +#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"

> 
>>> +static inline void set_bit_in_byte(u8 bit, u8 *val)
>>> +{
>>> +	if (bit < 8)
>>> +		*val |= (1 << bit);
>>> +}
>>> +
>>> +static inline void clr_bit_in_byte(u8 bit, u8 *val)
>>> +{
>>> +	if (bit < 8)
>>> +		*val &= ~(1 << bit);
>>> +}
>>
>> 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.

> 
>>> +		/* 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?
> 
>>> +	/* the following parameters are currently not exposed to
>>> devicetree, but
>>> +	 * may be as soon as needed
>>> +	 */
>>
>> Style of multi-line comment.
> 
> Nope, it's fine.
> 
>>> +#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?
> 
>>> +static int usb251xb_probe(struct usb251xb *hub)
>>> +{
>>> +	struct device *dev = hub->dev;
>>> +	struct device_node *np = dev->of_node;
>>> +	const struct of_device_id *of_id =
>>> of_match_device(usb251xb_of_match,
>>> +							   dev);
>>> +	int err;
>>> +
>>
>>> +	dev_info(dev, DRIVER_DESC " " DRIVER_NAME "\n");
>>
>> Useless.
> 
> Agreed.

Ok, I will remove it in v5!

Thanks & regards,
Richard L