From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753023AbdBUO5k (ORCPT ); Tue, 21 Feb 2017 09:57:40 -0500 Received: from mail2.skidata.com ([91.230.2.91]:46818 "EHLO mail2.skidata.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752529AbdBUO5c (ORCPT ); Tue, 21 Feb 2017 09:57:32 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2FVAgAMVaxY/0oKEKxeGQEBAQEBAQEBA?= =?us-ascii?q?QEBBwEBAQEBhDIqXweDVIoIkhYQkxWCD4FKQyqFeAKDLRgBAgEBAQEBAQEDgQe?= =?us-ascii?q?CYj0KMgEBAQEBAQEBAQEBAQEBARoCCEgCGAEBAQIBASMPAUYQCw0LAgImAgJHE?= =?us-ascii?q?AYNAQUCAoliFq40giYmAoswAQEBAQEBAQMBAQEBASMJAYEBgSaEG4IFgmqBPIJ?= =?us-ascii?q?+gyCCXwWJGYYwhheGKIIChHKNJIUbgy0XhjSTJB+BORAQEAQfU4RXgXJ0AYoCg?= =?us-ascii?q?Q0BAQE?= X-IPAS-Result: =?us-ascii?q?A2FVAgAMVaxY/0oKEKxeGQEBAQEBAQEBAQEBBwEBAQEBhDI?= =?us-ascii?q?qXweDVIoIkhYQkxWCD4FKQyqFeAKDLRgBAgEBAQEBAQEDgQeCYj0KMgEBAQEBA?= =?us-ascii?q?QEBAQEBAQEBARoCCEgCGAEBAQIBASMPAUYQCw0LAgImAgJHEAYNAQUCAoliFq4?= =?us-ascii?q?0giYmAoswAQEBAQEBAQMBAQEBASMJAYEBgSaEG4IFgmqBPIJ+gyCCXwWJGYYwh?= =?us-ascii?q?heGKIIChHKNJIUbgy0XhjSTJB+BORAQEAQfU4RXgXJ0AYoCgQ0BAQE?= X-IronPort-AV: E=Sophos;i="5.35,190,1484002800"; d="scan'208";a="405740" Subject: Re: [PATCH v5] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver To: Rob Herring References: <1486714767-25930-1-git-send-email-richard.leitner@skidata.com> <20170216023004.25dwzy7ppxdoasfz@rob-hp-laptop> <31fd459f-aed3-9d4a-2a63-651131216542@g0hl1n.net> CC: Linux USB List , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Greg Kroah-Hartman , Mark Rutland , Andy Shevchenko , From: Richard Leitner Message-ID: Date: Tue, 21 Feb 2017 15:57:23 +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: Content-Type: text/plain; charset="utf-8" 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/21/2017 03:37 PM, Rob Herring wrote: > On Thu, Feb 16, 2017 at 12:36 AM, Richard Leitner wrote: >> On 02/16/2017 03:30 AM, Rob Herring wrote: >>> >>> On Fri, Feb 10, 2017 at 09:19:27AM +0100, Richard Leitner wrote: >>>> >>>> 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. >>>> >>>> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/00001692C.pdf >>>> >>>> Signed-off-by: Richard Leitner >>>> --- >>>> CHANGES v5: >>> >>> >>> V5 and the first I see it? >> >> >> Just double-checked it, you (robh+dt@kernel.org) were on CC since v1. > > Indeed. You just sent new versions faster than I get to them. New > versions puts you at the back of my queue. Sending out 5 versions in a > week is a lot. Ooh. I'm sorry. It was just that I got feedback quite fast and had the time to implement the changes. BTW: I already sent a patch fixing most of your issues. Please just ignore that, as not everything is fixed in there already. I'll send an improved version end of this week. > > [...] > >>>> + >>>> +Optional properties : >>>> + - reg : I2C address on the selected bus (default is <0x2C>) >>> >>> >>> Why is this optional? >> >> >> Due to the fact the address is hardcoded insinde the chip I thought we could >> set it to 0x2C by default if reg is not given. > > That's true for pretty much every I2C chip. > >> Should it be required? > > Yes. The only time it would not be present is the I2C bus is not > physically connected. In that case though, this should be a child of > the USB host node. Ok. [...] >>> >>>> + - compound-device : indicated the hub is part of a compound device >>>> + - port-mapping-mode : enable port mapping mode >>>> + - string-support : enable string descriptor support (required for >>>> manufacturer, >>>> + product and serial string configuration) >>>> + - non-removable-ports : Should specify the ports which have a >>>> non-removable >>>> + device connected. >>>> + - sp-disabled-ports : Specifies the ports which will be self-power >>>> disabled >>>> + - bp-disabled-ports : Specifies the ports which will be bus-power >>>> disabled >>>> + - max-sp-power : Specifies the maximum current the hub consumes from an >>>> + upstream port when operating as self-powered hub including the >>>> power >>>> + consumption of a permanently attached peripheral if the hub is >>>> + configured as a compound device. The value is given in mA in a 0 >>>> - 500 >>>> + range (default is 2). >>>> + - max-bp-power : Specifies the maximum current the hub consumes from an >>>> + upstream port when operating as bus-powered hub including the >>>> power >>>> + consumption of a permanently attached peripheral if the hub is >>>> + configured as a compound device. The value is given in mA in a 0 >>>> - 500 >>>> + range (default is 100). >>>> + - max-sp-current : Specifies the maximum current the hub consumes from >>>> an >>>> + upstream port when operating as self-powered hub EXCLUDING the >>>> power >>>> + consumption of a permanently attached peripheral if the hub is >>>> + configured as a compound device. The value is given in mA in a 0 >>>> - 500 >>>> + range (default is 2). >>>> + - max-bp-current : Specifies the maximum current the hub consumes from >>>> an >>>> + upstream port when operating as bus-powered hub EXCLUDING the >>>> power >>>> + consumption of a permanently attached peripheral if the hub is >>>> + configured as a compound device. The value is given in mA in a 0 >>>> - 500 >>>> + range (default is 100). >>>> + - power-on-time : Specifies the time it takes from the time the host >>>> initiates >>>> + the power-on sequence to a port until the port has adequate >>>> power. The >>>> + value is given in ms in a 0 - 510 range (default is 100ms). >>> >>> >>> Various properties need unit suffixes (see property-units.txt) and >>> either be common properties or need vendor prefixes. >> >> >> Ok. What exactly do you mean with common properties? I don't think it's the >> endianness settings described in common-properties.txt, is it? > > No, not common-properties.txt (maybe that needs another name). Simply > documented in a common location that multiple device bindings can > share. So things that would be common to USB devices or USB hubs in > particular. > > Looking through the list again, probably just the ones corresponding > to USB descriptors should be common. Ok. But aren't for example "power-on-time" or "disabled-ports" also properties (most) hubs share and should therefore be common? > >> Is "microchip," fine as vendor prefix? > > Yes, if that's the correct string for Microchip. Ok. > >>> This is a lot of properties. Are you really finding a need for all of >>> them? Is this to handle h/w designers too cheap to put down the EEPROM? >>> Maybe better to just define an eeprom property in the format the h/w >>> expects. >> >> >> I need about 15 of these properties. I just exposed them all to dt because I >> thought they could be useful for somebody. >> >> Yes, these are a subset of the settings which can be configured via an >> external EEPROM (By strapping some pins of the hub you can select if it >> loads its configuration from an EEPROM or receives it via SMBus). >> >> My first thought was also to define only a byte array in dt, but IMHO these >> options are much more readable and convenient for everybody. I'd also be >> fine with removing the properties I don't need from dt. But that may lead to >> future patches where somebody needs some of the options not already exposed >> to dt. > > Okay. It's really a judgement call. If this is most of the settings, > then it's fine. If it was only a fraction of them, then maybe we'd > want to do just a byte array. Sounds like it is the former. In fact there are 6 more parameters available according to the datasheet. So how should I proceed here? Remove the one's I'm not using at the moment, leave them as they are or add the missing 6 too? Thank you for your feedback! regards, Richard L