From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755420AbdERIkQ (ORCPT ); Thu, 18 May 2017 04:40:16 -0400 Received: from mga07.intel.com ([134.134.136.100]:60178 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755235AbdERIkK (ORCPT ); Thu, 18 May 2017 04:40:10 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,358,1491289200"; d="scan'208";a="88533605" Date: Thu, 18 May 2017 11:37:56 +0300 From: Heikki Krogerus To: Hans de Goede Cc: Guenter Roeck , Andy Shevchenko , MyungJoo Ham , Chanwoo Choi , "linux-kernel@vger.kernel.org" , Sebastian Reichel Subject: Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) Message-ID: <20170518083756.GA14626@kuha.fi.intel.com> References: <20170421130119.6187-1-hdegoede@redhat.com> <20170421130119.6187-2-hdegoede@redhat.com> <20170424110204.GA14268@kuha.fi.intel.com> <5b09f8ad-be3c-b3c0-9e06-055cd4768700@redhat.com> <20170516120705.GC5322@kuha.fi.intel.com> <8c45819c-1738-dc95-35ad-b6ba37f8f418@redhat.com> <20170517114547.GD5322@kuha.fi.intel.com> <2ad0151a-7332-5de7-2641-ce8aa5983842@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2ad0151a-7332-5de7-2641-ce8aa5983842@redhat.com> User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 17, 2017 at 04:47:14PM +0200, Hans de Goede wrote: > Hi, > > On 17-05-17 13:45, Heikki Krogerus wrote: > > Hi, > > > > On Wed, May 17, 2017 at 12:24:52AM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 05/16/2017 02:07 PM, Heikki Krogerus wrote: > > > But we don't really have multiple sources here, we have only > > > one source, the USB-C cable hooked to an external power-supply. > > > Just because 2 different pieces of hardware may be involved in > > > determining the current limit does not mean that we should > > > model this as 2 different sources. Just as you rightfully > > > pointed out that me using 2 extcon devices for the single > > > Type-C connector is wrong, modelling it as 2 sources is > > > wrong too. > > > > The power supply devices don't represent the port like the extcon > > device would. The power supply devices represent the two types > > of external chargers we support. So BC1.2 and Type-C/PD source. > > Which are both USB chargers, and the TI bq24290 driver itself > also registers itself as a USB charger, continued below ... > > > > > 1. Tell the BC1.2 detection to start from fusb302 driver > > > > 2. Deliver the power limits to the discrete charger ic or battery driver > > > > 3. Tell what ever regulates VBUS to start driving VBUS. > > > > > > > The second problem definitely needs to be handled using psy framework. > > > > The psy framework provides already everything needed for that. > > > > > > So above you're talking about sources to the bq24190, which if > > > I understand you correctly suggest that you want the tcpm code to > > > register a power-supply device per Type-C port ? > > > > No, the underlying device driver (so fusb302) needs to register the > > power supply at this point. We just notify the psy framework if the > > limits we get from tcpm to the set_current_limit hook change. > > > > > I'm not sure that > > > is a good idea, any registered power-supply devices will show up > > > under /sys/class/power_supply, currently on a typical system > > > there will be 2 entries under /sys/class/power_supply one for > > > the "Mains" or USB power input and one for the battery, adding > > > more entries there is likely to confuse userspace. > > > > The userspace uses the type attribute to differentiate the supplies. > > Otherwise it would not be able to tell even which is the Battery and > > which is Mains. Having more power supplies is not a problem. > > I disagree yes power-supply devices have a type, so if we do > as you suggest we end up with 3 power-supplies with type USB under > /sys/class/power_supply suggesting to userspace that the device > may be supplied with power through 3 different USB connectors. > > This is simply just wrong. I understand that you want to decouple > the different drivers from each-other and I agree with that as > a goal. > > But using the power-supply subsys in the way you suggests means > that the way we end up solving a problem which is purely > about different pieces of code in the kernel talking to each > other gets leaked to userspace and once we've done this userspace > may start depending on this and we cannot change things later. > > TL;DR: I'm against the FUSB302 and the BC1.2 drivers each > registering a power-supply because: > > 1) There should be only one power-supply device registered > not 3, since there is only one power-input to the board, not 3. > > 2) Ideally the in kernel interface should not be visible to > userspace at all, we are still figuring all of this out and > we may need to change things later leaking internal kernel > details to userspace in something which will become an ABI > is not a good idea. > > I've added Sebastian Reichel, the power-supply subsys > maintainer to the Cc so that he can weigh in on this. > > > > More in general having 2 power-supply devices for what is > > > in essence one power-input feels wrong. > > > > > > We can still use the power-supply framework, but I would > > > envision this working like this: > > > > > > The platform code which enumerates the type-c controller > > > sets a "input-current-power-supply-name" string device-property > > > on the tcpm's (parent)device. When this is set the tpcm code > > > will do a power_supply_get_by_name and set the input > > > current on the returned power_supply by setting the > > > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property. > > > > The psy framework is probable a bit messy at them moment. It > > definitely does not do much protecting and in many cases one driver > > registers a power supply and an other just takes it over, but that > > should be avoided as it makes things difficult (painful) to maintain. > > It also poses risks IMO. There certainly almost never seems to be a > > real need for that. > > > > In this case the driver for fusb302 registers a power supply that > > provides properties like POWER_SUPPLY_PROP_VOLTAGE_MAX, etc. and > > simply calls power_supply_changed() when ever needed (when we know the > > limits and when a source is attached/de-attached -> changes PRESENT > > & ONLINE properties). The fusb302 driver does not need to know if > > there are any consumers for it or not. The platform driver that > > registers the device for it will simply assign the consumer for it in > > this case, but in the future we want to get that kind of detail from > > the platform of course, so ACPI or DT. > > > > The PMIC charger driver will similarly register a power supply device > > and function pretty much exactly the same way. > > > > The consumer, bq24190, will receive notification from psy framework to > > its external_power_changed hook when ever either of the supplies > > calls power_supply_changed(). It then needs to check the properties of > > the supply (the external power) that are interesting to it in order to > > set the limits accordingly (this btw. is where the psy API and the > > class driver can be improved without much effort to make things easier > > for the consumers). The consumer driver in any case does not need to > > know what the supplies actually are, how many of them it has, or are > > there any at all, just like the drivers for the supplies don't need to > > know the consumers. > > I like parts of this idea, but as said I've trouble with exporting 3 > power-supply devices to userspace for this. > > I must also say that I'm not a fan of making the BC-1.2 charger > a separate power-supply and letting the consumer figure out which > one to use, but lets forget about the BC-1.2 charger for now and > focus on solving this for just setting the TYPE-C determined > input current limit for now. OK, You have a point. I happy to agree that adding an other psy for the BC1.2 charger alone is not the correct thing to do. I'm mainly interested in just considering USB as a power supply with a board like this, because really, that is what it is, but also by using the power supply class properly (and possibly improving it a little), we avoid unnecessary software couplings. > > I hope I was able to explain myself, and make my case. > > Explain yes, but I'm really worried about the consequences of exporting > extra API/ABI to userspace for this, while we are purely trying to > solve in an kernel communication problem. > > > One more thing. I think we could actually build a little bit of > > hierarchy and make the fusb302 the supply of, not bq24190, but the > > PMIC instead. The PMIC would then be the only supply for the bq24190. > > Actually if you look at the schematic (I don't have a schematic for > my device, but I can deduce one) then the bq24190 is supplying power > to the pmic not the other-way around and the fusb302 is not supplying > power in anyway, it purely is negotiating things, but from an electrical > pov it is not supplying anything. Anyways as said lets forgot about > the PMIC doing BC1.2 detection for now and first focus on how we > can get the current-limit info from the fusb302 driver to the > power-supply device registered by the bq24190 driver. I second that. Thanks Hans, -- heikki