Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver
From: Richard Leitner
Date: Wed Feb 08 2017 - 10:32:17 EST
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
>>> USB 2.0 hub controller series with USB 2.0 upstream connectivity,
>>> 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 . 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.
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)
>> & ~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
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 =
>>> + dev);
>>> + int err;
>>> + dev_info(dev, DRIVER_DESC " " DRIVER_NAME "\n");
Ok, I will remove it in v5!
Thanks & regards,