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. > > <snip> > > > > > 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
