RE: [PATCH v4 6/7] typec: tcpm: Represent source supply through power_supply class
On 08 February 2018 10:45, Heikki Krogerus wrote: Hi Heikki, Comments in-line as usual. Bit verbose, and may have stated the obvious in places, but trying to build a picture and aim for something sensible. > Hi Adam, > > On Tue, Feb 06, 2018 at 03:51:26PM +, Adam Thomson wrote: > > Right now there is no documentation for the generic psy class. The stuff in > > sysfs-class-power is device specific property information, and the same > > goes for > > sysfs-class-power-twl4030. The property usage can vary depending on driver > > implementation, an example being the 'online' property which can differ > > between > > drivers, so the usage I have here is very much tcpm related. Also, the > > ability > > to write to certain properties varies depending on the driver and HW, so > > here > > where we configure 'voltage_now' and 'current_now', the likelihood is that > > most > > other psy driver implementations won't allow for this. > > The power supply class is missing documentation, YES! That is what I > have been saying! The fact that even an attribute like "online" can > mean different things depending on the driver is absolutely horrible. > The ABI documentation FOR THE POWER SUPPLY CLASS needs to provide > clear meaning for the attributes. It needs to also point out which > attributes can be hidden, and it should also give some hints for > things like which attributes can be expected to be visible for example > in case of USB type of psy and so on. There is some coverage under Documenation/power/power_supply_class.txt but it's not necessarily extensive, at least not to the level you're suggesting. With regards to the psy class though, I believe this is meant to cover a range of supply types so I guess the original intention was to make it flexible to cover those. Maybe along the way that's helped muddy the waters somewhat though. As an example, a 'Battery' type supply would represent say voltage_now and current_now, and those values would relate to VBAT and IBAT. However a Charger IC reporting the USB side information (probably of type 'USB', but could be AC, Mains) would report voltage_now and current_now, and those values would represent VIN/VBUS and IIN/IBUS. This example assumes that both sets of HW would be able to provide instantaneous readings. Some only give averaged readings. Just from this you can already start to see why that framework has evolved in the way it has. Covering the 'online' property, for Charger ICs this would indicate whether they were externally powered or not (i.e. external charger attached/detached), so a positive value would indicate external charger presence and 0 would mean not present. In drivers/power/supply/abx500_chargalg.c it goes further with this and the positive online value can differ based on what kind of charger was attached (i.e. Mains, USB). Not sure if anything in user-space would behave differently based on different values though and suspect a positive or 0 value will be the only differentiator. For the Battery type scenario 'online', if provided, is usually a repeat of battery presence information. Examples of this exist although it's really just redundant information, so a guideline would help to avoid this. So right now the only sensible method seems to be to describe property usage based on the main type, which brings me round to what you're suggesting I think. I can start something, at least for common USB property usage scenarios, and maybe Battery as well, but I do think this needs additional attention as there are so far 66 (including my new addition) properties to document, some of which may require multiple definitions based on type specific usage. With regards to presence of attributes, that's going to be solely related to HW and what is implemented for that device, I think. We can give preferred, sunny day examples, but I think you'll be hard pressed to make one size fit all. Or do you disagree with that sentiment? > > We are talking about user space ABI for power supplies here. The user > space does not know that its dealing with tcpm in this case, or some > other driver in an other case, AND, the user space _must_ not be > expected to know that kind of details. The behaviour and meaning of an > individual attribute file quite simply has to be the same, always, > regardless of the platform, HW, driver or whatever. Otherwise this > whole ABI is completely useless. Not technically true as decisions could be based on psy naming. Generic TCPM usage of the power_supply framework would mean the naming of the psy sysfs entry (or entries) are well known, just like using /dev. I believe 'type' though is specifically used in Android to determine which properties it uses for specific purposes. Personally I go along with this and think 'type' should be the major determining factor on expectation/usage. User-space needs to know the type of supply it's dealing with to be able to act accordingly. > To summarize: We can't just accept chaos. Instead
Re: [PATCH v4 6/7] typec: tcpm: Represent source supply through power_supply class
Hi Adam, On Tue, Feb 06, 2018 at 03:51:26PM +, Adam Thomson wrote: > Right now there is no documentation for the generic psy class. The stuff in > sysfs-class-power is device specific property information, and the same goes > for > sysfs-class-power-twl4030. The property usage can vary depending on driver > implementation, an example being the 'online' property which can differ > between > drivers, so the usage I have here is very much tcpm related. Also, the ability > to write to certain properties varies depending on the driver and HW, so here > where we configure 'voltage_now' and 'current_now', the likelihood is that > most > other psy driver implementations won't allow for this. The power supply class is missing documentation, YES! That is what I have been saying! The fact that even an attribute like "online" can mean different things depending on the driver is absolutely horrible. The ABI documentation FOR THE POWER SUPPLY CLASS needs to provide clear meaning for the attributes. It needs to also point out which attributes can be hidden, and it should also give some hints for things like which attributes can be expected to be visible for example in case of USB type of psy and so on. We are talking about user space ABI for power supplies here. The user space does not know that its dealing with tcpm in this case, or some other driver in an other case, AND, the user space _must_ not be expected to know that kind of details. The behaviour and meaning of an individual attribute file quite simply has to be the same, always, regardless of the platform, HW, driver or whatever. Otherwise this whole ABI is completely useless. Working around the issue of missing guidelines and documentation for subsystem ABI by providing it for the device drivers instead is not acceptable. If you don't want to propose documentation for the class, don't propose any documentation at all is better answer then that. And using arguments like "well, twl4030 did it" is really starting to annoy me. We are not lemmings here. We can make this right instead of following others blindly over the cliff edge. To summarize: We can't just accept chaos. Instead we should organize the places without structure, in this case the user space ABI for power supplies. On top of ABI documentation, we will need driver API documentation as well. I'm not expecting that you would also propose something for the API too, but I just wanted to bring that up here. I would like to have some guidelines on how the power supplies should be used also in kernel. Right now it is possible for one driver to create the power supply and an other to take over the control of it. It is super easy to gain access to a power supply. You can request it with just the name without any control, and after gaining access, you have full control over it. That makes it really easy to have race condition where both the psy device driver and some other driver try to control the same things of the same psy. I guess the whole design of the psy class could use a little bit of re-designing. So IMO, access to the psy should be more strict then it is now, and also, even after gaining access to a psy handle, drivers that are not the actual psy device drivers should have more controlled access to it. So possibly separate API for them... OK, this is definitely a separate topic. Sorry, I'll stop here. Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 6/7] typec: tcpm: Represent source supply through power_supply class
On 30 January 2018 13:12, Heikki Krogerus wrote: > Hi Adam, > > On Tue, Jan 02, 2018 at 03:50:54PM +, Adam Thomson wrote: > > This commit adds a power_supply class instance to represent a > > PD source's voltage and current properties. This provides an > > interface for reading these properties from user-space or other > > drivers. > > > > For PPS enabled Sources, this also provides write access to set > > the current and voltage and allows for swapping between standard > > PDO and PPS APDO. > > > > As this represents a superset of the information provided in the > > fusb302 driver, the power_supply instance in that code is removed > > as part of this change, so reverting the commit titled > > 'typec: tcpm: Represent source supply through power_supply class' > > > > > Signed-off-by: Adam Thomson > > --- > > .../ABI/testing/sysfs-class-power-tcpm-source-psy | 92 > > drivers/usb/typec/Kconfig | 1 + > > drivers/usb/typec/fusb302/Kconfig | 2 +- > > drivers/usb/typec/fusb302/fusb302.c| 63 +- > > drivers/usb/typec/tcpm.c | 233 > > - > > 5 files changed, 328 insertions(+), 63 deletions(-) > > create mode 100644 Documentation/ABI/testing/sysfs-class-power-tcpm-source- > psy > > > > diff --git a/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy > b/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy > > new file mode 100644 > > index 000..4986cba > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy > > @@ -0,0 +1,92 @@ > > +What: /sys/class/power_supply/tcpm-source-psy/type > > +Date: December 2017 > > +Contact: Adam Thomson > > +Description: > > + This read-only property describes the main type of source supply. > > + Type-C is a USB standard so this property always returns "USB". > > + > > +What: /sys/class/power_supply/tcpm-source-psy/connected_type > > +Date: December 2017 > > +Contact: Adam Thomson > > +Description: > > + This read-only property describes the type of source supply that is > > + connected, if the supply is online. The value is always Type C > > + unless a source has been attached which is identified as USB-PD capable. > > + > > + Valid values: > > + - "USB_TYPE_C" : Type C connected supply, not UBS-PD capable > > + (default value) > > + - "USB_PD" : USB-PD capable source supply connected > > + - "USB_PD_PPS" : USB-PD PPS capable source supply connected > > + > > +What: /sys/class/power_supply/tcpm-source-psy/online > > +Date: December 2017 > > +Contact: Adam Thomson > > +Description: > > + This read-write property describes the online state of the source > > + supply. When the value of this property is not 0, and the supply allows > > + it, then it's possible to switch between online states (i.e. 1 -> 2, > > + 2 -> 1) > > + > > + Valid values: > > + - 0 : Offline, no source supply attached > > + - 1 : Fixed Online, Type-C or USB-PD capable supply > > + attached, non-configurable current and voltage > > + properties in this state. > > + - 2 : PPS Online, USB-PD PPS feature enabled, 'current_now' > > + and 'voltage_now' properties can be modified in this > > + state. Re-writing of this value again, once already > > + set, will re-request the same configured voltage and > > + current values. This can be used as a keep-alive for > > + the PPS connection. > > + [NOTE: This is value only selectable if > > + 'connected_type' reports a value of "USB_PD_PPS"] > > + > > +What: /sys/class/power_supply/tcpm-source-psy/voltage_min > > +Date: December 2017 > > +Contact: Adam Thomson > > +Description: > > + This read-only property describes the minimum voltage the source supply > > + can provide. > > + > > + Value in microvolts. > > + > > +What: /sys/class/power_supply/tcpm-source-psy/voltage_max > > +Date: December 2017 > > +Contact: Adam Thomson > > +Description: > > + This read-only property describes the maximum voltage the source supply > > + can provide. > > + > > + Value in microvolts. > > + > > +What: /sys/class/power_supply/tcpm-source-psy/voltage_now > > +Date: December 2017 > > +Contact: Adam Thomson > > +Description: > > + This read-write property describes the voltage the source supply is > > + providing now. This property can only be written to if the source supply > > + is in online state '2' (PPS enabled), otherwise it's read-only > > + information. > > + > > + Value in microvolts. > > + > > +What:
Re: [PATCH v4 6/7] typec: tcpm: Represent source supply through power_supply class
Hi Adam, On Tue, Jan 02, 2018 at 03:50:54PM +, Adam Thomson wrote: > This commit adds a power_supply class instance to represent a > PD source's voltage and current properties. This provides an > interface for reading these properties from user-space or other > drivers. > > For PPS enabled Sources, this also provides write access to set > the current and voltage and allows for swapping between standard > PDO and PPS APDO. > > As this represents a superset of the information provided in the > fusb302 driver, the power_supply instance in that code is removed > as part of this change, so reverting the commit titled > 'typec: tcpm: Represent source supply through power_supply class' > Signed-off-by: Adam Thomson > --- > .../ABI/testing/sysfs-class-power-tcpm-source-psy | 92 > drivers/usb/typec/Kconfig | 1 + > drivers/usb/typec/fusb302/Kconfig | 2 +- > drivers/usb/typec/fusb302/fusb302.c| 63 +- > drivers/usb/typec/tcpm.c | 233 > - > 5 files changed, 328 insertions(+), 63 deletions(-) > create mode 100644 > Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy > > diff --git a/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy > b/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy > new file mode 100644 > index 000..4986cba > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy > @@ -0,0 +1,92 @@ > +What:/sys/class/power_supply/tcpm-source-psy/type > +Date:December 2017 > +Contact: Adam Thomson > +Description: > + This read-only property describes the main type of source supply. > + Type-C is a USB standard so this property always returns "USB". > + > +What:/sys/class/power_supply/tcpm-source-psy/connected_type > +Date:December 2017 > +Contact: Adam Thomson > +Description: > + This read-only property describes the type of source supply that is > + connected, if the supply is online. The value is always Type C > + unless a source has been attached which is identified as USB-PD capable. > + > + Valid values: > + - "USB_TYPE_C" : Type C connected supply, not UBS-PD capable > + (default value) > + - "USB_PD" : USB-PD capable source supply connected > + - "USB_PD_PPS" : USB-PD PPS capable source supply connected > + > +What:/sys/class/power_supply/tcpm-source-psy/online > +Date:December 2017 > +Contact: Adam Thomson > +Description: > + This read-write property describes the online state of the source > + supply. When the value of this property is not 0, and the supply allows > + it, then it's possible to switch between online states (i.e. 1 -> 2, > + 2 -> 1) > + > + Valid values: > + - 0 : Offline, no source supply attached > + - 1 : Fixed Online, Type-C or USB-PD capable supply > + attached, non-configurable current and voltage > + properties in this state. > + - 2 : PPS Online, USB-PD PPS feature enabled, 'current_now' > + and 'voltage_now' properties can be modified in this > + state. Re-writing of this value again, once already > + set, will re-request the same configured voltage and > + current values. This can be used as a keep-alive for > + the PPS connection. > + [NOTE: This is value only selectable if > +'connected_type' reports a value of "USB_PD_PPS"] > + > +What:/sys/class/power_supply/tcpm-source-psy/voltage_min > +Date:December 2017 > +Contact: Adam Thomson > +Description: > + This read-only property describes the minimum voltage the source supply > + can provide. > + > + Value in microvolts. > + > +What:/sys/class/power_supply/tcpm-source-psy/voltage_max > +Date:December 2017 > +Contact: Adam Thomson > +Description: > + This read-only property describes the maximum voltage the source supply > + can provide. > + > + Value in microvolts. > + > +What:/sys/class/power_supply/tcpm-source-psy/voltage_now > +Date:December 2017 > +Contact: Adam Thomson > +Description: > + This read-write property describes the voltage the source supply is > + providing now. This property can only be written to if the source supply > + is in online state '2' (PPS enabled), otherwise it's read-only > + information. > + > + Value in microvolts. > + > +What:/sys/class/power_supply/tcpm-source-psy/current_max > +Date:December 2017 > +Contact: Adam Thomson > +Description: > +
