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 +0000, 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 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. Actually being able to use psy's in other drivers is an approach that's very useful, especially for monitoring & controlling charging using PPS. You are right in that it's very easy to gain access and have control, but have not personally experienced race issues. I guess that's down to the individual driver's own locking implementation right now? Anyway, based on all of this, and assuming I get no major disagreement, I'll try and come up with something as a decent starting point to better document and fix property usage, based on supply type. It won't be every property, but hopefully will be a decent first stab at this. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
