lkml.org 
[lkml]   [2017]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver
    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>
    >
    > 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

    \
     
     \ /
      Last update: 2017-02-08 15:01    [W:4.986 / U:0.728 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site