From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756595AbdESUPm (ORCPT ); Fri, 19 May 2017 16:15:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43366 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753208AbdESUPl (ORCPT ); Fri, 19 May 2017 16:15:41 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8CB2A7F415 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=hdegoede@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 8CB2A7F415 Subject: Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) From: Hans de Goede To: Heikki Krogerus Cc: Guenter Roeck , Andy Shevchenko , MyungJoo Ham , Chanwoo Choi , "linux-kernel@vger.kernel.org" , Sebastian Reichel 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> <20170518083756.GA14626@kuha.fi.intel.com> <78734081-2d60-8658-d506-56ce3cce32a5@redhat.com> Message-ID: <6a1b02fe-7d3f-839e-7516-076c91b673f8@redhat.com> Date: Fri, 19 May 2017 22:15:37 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <78734081-2d60-8658-d506-56ce3cce32a5@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 19 May 2017 20:15:40 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 19-05-17 22:12, Hans de Goede wrote: > Hi, > > On 18-05-17 10:37, Heikki Krogerus wrote: >> On Wed, May 17, 2017 at 04:47:14PM +0200, Hans de Goede wrote: >>> Hi, >>> >>> On 17-05-17 13:45, Heikki Krogerus wrote: > > > >>>> 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. > > Ok, so although I'm still not 100% sold on having the fusb302 driver > registering a power-supply is a good idea from an userspace API pov, I > from a design pov otherwise. And in a way it does make sense the fusb302 > driver is a way to query (and in some case modify) settings of the external > power-supply connected to the USB-C port. Ugh I somewhat mangled the first sentence of this paragraph, here is a correct version: Ok, so although I'm still not 100% sold on having the fusb302 driver registering a power-supply being a good idea from an userspace API pov, I do actually like it from a design pov. > So maybe we need a new "External" power-supply type for this ? Either way > this should not be a real issue since userspace (upower) does not seem to > do much if anything with power-supply class devices with a type of USB. > > So lets go with the plan to make the fusb302 driver register a power-supply > for now, which will be a supplier for (for example) the bq24190 charger. > > 2 questions about implementing this: > > 1) You said you want the power-supply code (get_property, etc.) to live > in the fusb302 driver, rather then in the tcpm code. I agree that the > fusb302 device should be the parent-device of the power-supply, but I can > see registering a power-supply and querying things like > POWER_SUPPLY_PROP_VOLTAGE_MAX being something which we will want in other > USB TYPE-C drivers too, so wouldn't it be better to have this code in > the generic tcpm bits ? > > 2) I could modify the bq24190 driver to check and update its > input-current-limit itself from its external_power_changed callback, > but this seems like something which many charger drivers will need > to implement so instead I think that drivers like bq24190 should just > be able to set a "sync_input_current_with_suppliers" flag and then > when a supplier calls power_supply_changed() have the core call > set_property PROP_INPUT_CURRENT_LIMIT? That way all we need to do > to get charger drivers to support this is set that flag rather then > copy and paste code. Or maybe a > power_supply_sync_input_current_with_suppliers() helper which drivers > can call from their external_power_changed callback ? >> Thinking more about this I think I like the helper function idea better Regards, Hans