From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933309AbdBHPcJ (ORCPT ); Wed, 8 Feb 2017 10:32:09 -0500 Received: from mail1.skidata.com ([91.230.2.99]:12866 "EHLO mail1.skidata.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752869AbdBHPbX (ORCPT ); Wed, 8 Feb 2017 10:31:23 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2HLCQBWNptY/0oKEKxdGwEBAQMBAQEJA?= =?us-ascii?q?QEBhFxfB59jEJMXhBuGIgKDLBQBAgEBAQEBAQEDgQeEaQEBAQQnCwFGEAsNCAM?= =?us-ascii?q?uVwYBCQMBBQICu246JgKLHwEBAQEBAQEDAQEBAQEBASEJAYInhBuCBYJqgTyDG?= =?us-ascii?q?IVlBZtwgX+IDpJEF4YvkxM2gSAPEA8EHYUSAw0QgWJ0h3KBDAEBAQ?= X-IPAS-Result: =?us-ascii?q?A2HLCQBWNptY/0oKEKxdGwEBAQMBAQEJAQEBhFxfB59jEJM?= =?us-ascii?q?XhBuGIgKDLBQBAgEBAQEBAQEDgQeEaQEBAQQnCwFGEAsNCAMuVwYBCQMBBQICu?= =?us-ascii?q?246JgKLHwEBAQEBAQEDAQEBAQEBASEJAYInhBuCBYJqgTyDGIVlBZtwgX+IDpJ?= =?us-ascii?q?EF4YvkxM2gSAPEA8EHYUSAw0QgWJ0h3KBDAEBAQ?= X-IronPort-AV: E=Sophos;i="5.33,348,1477954800"; d="scan'208";a="2372334" Subject: Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver To: Greg KH , Andy Shevchenko References: <1486543976-28006-1-git-send-email-richard.leitner@skidata.com> <1486560068.2133.395.camel@linux.intel.com> <20170208135957.GA916@kroah.com> CC: , , , , , , From: Richard Leitner Message-ID: <96c8a6f9-3b93-8ba0-22b1-502e22dac31c@skidata.com> Date: Wed, 8 Feb 2017 16:17:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170208135957.GA916@kroah.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [172.16.60.30] X-ClientProxiedBy: sdex1srv.skidata.net (172.16.10.92) To sdex1srv.skidata.net (172.16.10.92) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> 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 >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> 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