From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1756649AbdESUMW (ORCPT <rfc822;w@1wt.eu>);
        Fri, 19 May 2017 16:12:22 -0400
Received: from mx1.redhat.com ([209.132.183.28]:51034 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1756193AbdESUMT (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 19 May 2017 16:12:19 -0400
DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D5D6E7D4E1
Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=hdegoede@redhat.com
DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D5D6E7D4E1
Subject: Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH
 2/5] extcon: Add FUSB302 USB TYPE-C controller support)
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Sebastian Reichel <sre@kernel.org>
References: <20170421130119.6187-1-hdegoede@redhat.com>
 <20170421130119.6187-2-hdegoede@redhat.com>
 <CAHp75VciTKO_PXGQKs_a=S2qwK7eUG38yGQEidRtAcoOi1YPKA@mail.gmail.com>
 <20170424110204.GA14268@kuha.fi.intel.com>
 <d36522ae-2d08-8995-87ca-49ffdc3b2e71@roeck-us.net>
 <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>
From: Hans de Goede <hdegoede@redhat.com>
Message-ID: <78734081-2d60-8658-d506-56ce3cce32a5@redhat.com>
Date: Fri, 19 May 2017 22:12:14 +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: <20170518083756.GA14626@kuha.fi.intel.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.28]); Fri, 19 May 2017 20:12:19 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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:

<snip>

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

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