Re: [RFC PATCH v2 6/7] typec: tcpm: Represent source supply through power_supply class

[Heikki Krogerus]

On Mon, Nov 27, 2017 at 04:54:08PM +0000, Adam Thomson wrote:
> On 27 November 2017 14:12, Heikki Krogerus wrote:
> 
> > Hi Adam,
> > 
> > On Fri, Nov 24, 2017 at 02:05:27PM +0000, Adam Thomson wrote:
> > > On 24 November 2017 12:19, Heikki Krogerus wrote:
> > > > Is it OK to everybody that the type of the psy is changed like that?
> > > > Hans?!
> > > >
> > > > We do have drivers that already change the type, for example
> > > > drivers/power/supply/isp1704_charger.c, but what does the user space
> > > > expect? The ABI for the power supply class was never documented I
> > > > guess.
> > > >
> > >
> > > Hi Heikki,
> > >
> > > Appreciate your time in reviewing this.
> > >
> > > Yes, I actually saw that as an example when I considered this approach. I didn't
> > > see anything obvious for this in the ABI documentation. Any ideas Sebastian?
> > > What is user-space expectation for the 'type' property of a power_supply? I
> > > assume having this dynamic is ok given existing drivers can already do something
> > > like this, but would be good to have clarification.
> > >
> > > > I'm not against changing the type, but I think that we should have an
> > > > attribute file listing all supported types a psy can have if we go
> > > > forward with this. Ideally the type file would just list them as space
> > > > separated values, and show the current one with asterisk in front of
> > > > it. The output would be similar we have with some of the other files
> > > > under /sys/power, at least /sys/power/state, but that would break the
> > > > ABI.
> > > >
> > >
> > > I added this as I wanted the user to know what was connected rather than
> > > blindly trying to set the 'online' property to enable PPS, even if the attached
> > > source partner didn't support this. As you say, am not sure we could change the
> > > 'TYPE' property as that to my knowledge has always been a single string.
> > >
> > > Maybe the addition of a 'SUPPORTED_TYPES' property or something similar could
> > > close this gap (as you eluded to), at least by providing a RO list of all
> > > supported types? Another option would be to add a type which indicates the
> > > supply supports multiple types, and then based on this we can read another
> > > property which does as you suggest with multiple strings and one being
> > > highlighted? Am certainly open to discussion on this.
> > 
> > It looks like this is USB specific problem. I think the type should
> > actually be just USB, and there should be an other USB only attribute
> > file which should list the current and supported connection types.
> > Well, maybe it does not even need to be USB only.
> 
> I believe in Android (BatteryMonitor) right now it maps the various USB
> types that can be reported by a power_supply driver to an internal Android 'AC'
> type (or 'USB' for 'PD_DRP') for use in upper level software, so they handle the
> various USB reported types but simplify reporting. That code seems like it would
> also handle changes in type as well because the type is re-read when dealing
> with updates. So there is a wide spread user which already 'copes' with current
> driver implementations. That's not saying it's necessarily the correct way to go
> in the future but wanted to highlight current usage.
> 
> Personally what you're suggesting seems reasonable to me. I would however like
> to get a general consensus on this as I guess this could potentially change
> future power_supply driver implementations.

Agreed. What ever the final solution will be, the power supply ABI
should be documented at the same time.

> Sebastian, do you have any thoughts on this topic?


Cheers,

-- 
heikki