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 <[email protected]>
>
> If you want to fix the above you have to fix your Git configuration.
>
>
> > 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.
> > +#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?
> > +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.
> > + /* 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.
thanks,
greg k-h
