From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751818AbdBHThO (ORCPT ); Wed, 8 Feb 2017 14:37:14 -0500 Received: from mga03.intel.com ([134.134.136.65]:8852 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750911AbdBHThM (ORCPT ); Wed, 8 Feb 2017 14:37:12 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,348,1484035200"; d="scan'208";a="931638237" Message-ID: <1486581644.2133.409.camel@linux.intel.com> Subject: Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver From: Andy Shevchenko To: Richard Leitner , Richard Leitner , Greg KH Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, robh+dt@kernel.org, mark.rutland@arm.com, stern@rowland.harvard.edu Date: Wed, 08 Feb 2017 21:20:44 +0200 In-Reply-To: <80e533b6-4eb3-0019-fe18-82dd0d7aaa1c@g0hl1n.net> References: <1486543976-28006-1-git-send-email-richard.leitner@skidata.com> <1486560068.2133.395.camel@linux.intel.com> <20170208135957.GA916@kroah.com> <96c8a6f9-3b93-8ba0-22b1-502e22dac31c@skidata.com> <1486572009.2133.406.camel@linux.intel.com> <80e533b6-4eb3-0019-fe18-82dd0d7aaa1c@g0hl1n.net> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2017-02-08 at 19:45 +0100, Richard Leitner wrote: > On 02/08/2017 05:40 PM, Andy Shevchenko wrote: > > On Wed, 2017-02-08 at 16:17 +0100, Richard Leitner wrote: > > > 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: > > > > > 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. > > > > __set_bit(), __clear_bit() -- non-atomic variants, but you are > > right, > > that (unsigned long) exactly the point I didn't propose them. > > So the preferred solution is the BIT() stuff? I think BIT() macro in place. Otherwise you'll need a temporary unsigned long variable. If you already have one, then __*_bit() would work. > + /* 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? > > > > I would do like > > > > /* > >  * The multi-line > >  * comment. > >  */ > > > > Capital letter, period at the end, first empty line (unlike in net > > subsystem). > > So what's the preferred format? Empty line at the beginning or not? Both fine to me. It's not a real code anyway. > > > > > > +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? > > > > __maybe_unused for function, device_property_*() instead of > > of_property_*() calls. > > > > Something like that. But if you are insisting this is *only* OF > > available hardware or we don't care, I'll not object. > > In usb3503.c and usb4604.c we have that #ifdef CONFIG_OF too. IMHO > those  > drivers should use the same solution here. But you guys are the ones  > with tons of kernel coding experience, so just say how it should be > done :-) I'll agree with whatever Greg wants here. -- Andy Shevchenko Intel Finland Oy