Message-ID: <dcbddc43-e581-505e-915b-3bf16b477422@redhat.com>
Date:   Mon, 7 Aug 2017 16:41:18 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Guenter Roeck <linux@...ck-us.net>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Wolfram Sang <wsa@...-dreams.de>,
        Sebastian Reichel <sre@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-i2c@...r.kernel.org, Liam Breck <liam@...workimprov.net>,
        Tony Lindgren <tony@...mide.com>, linux-pm@...r.kernel.org,
        devel@...verdev.osuosl.org
Subject: Re: [PATCH 10/18] staging: typec: fusb302: Add support for
 fcs,vbus-regulator-name device-property

Hi Mark,

On 07-08-17 13:10, Mark Brown wrote:
> On Sun, Aug 06, 2017 at 05:44:36PM +0200, Hans de Goede wrote:
>> On 06-08-17 16:30, Guenter Roeck wrote:
>>> On 08/06/2017 05:35 AM, Hans de Goede wrote:
> 
>>>> On ACPI platforms, there are no phandles and we need to get the vbus by a
>>>> system wide unique name. Add support for a new "fcs,vbus-regulator-name"
>>>> device-property which ACPI platform code can set to pass the name.
> 
>>> Another property to be documented and approved.
> 
>> Again this is for kernel internal use on non-dt platforms only, so documenting
>> it in the devicetree bindings is not necessary.
> 
> However it *is* for use on ACPI platforms and is impacting power
> management (which is something ACPI definitely models) so should be
> being documented in an ASWG spec.  We don't want Linux systems to start
> breaking the ACPI power management model with uncontrolled extensions,
> it's fine to add new bindings for things where there's just no ACPI
> specification at all but power management isn't one of those areas.

This regulator is used to enable/disable driving vbus on the Type-C connector
from a 5V boost converter or not depending on the power direction (sink
or source) negotiated by the Type-C port-controller. As such this is never
under firmware/ACPI control it always gets controlled by the Type-C
port-manager, so there is no need for ACPI to control it. The problem is
that the Type-C setup on these boards consist of a bunch of ICs chained
together / driving different pins of the Type-C connector. So we need to
somehow tell the bq24292i charger-IC to turn on/off its 5V boost converter
from the Type-C port-controller driver. This discussion (and this patch)
is about getting a handle to the regulator-device for the 5V boost converter
from the Type-C port-controller driver.

For added fun the bq24292i charger-IC is not described in ACPI at all,
but we know that the Whiskey Cove PMIC used is always paired with it.

The fusb302 Type-c port-controller itself is enumerated to the weird
INT33FE ACPI device node (which describes 3 different i2c ICs, including
the fusb302)

>> TL;DR: It seems that on x86, at least for existing devices where we cannot
>> control the ACPI tables that getting things by name is the thing to do.
> 
> The idiomatic thing to do on an ACPI system at present appears to be to
> have a big DMI quirk table somewhere that instantiates the regulators
> and mappings required for them based on the machine's DMI data.  Or if
> it's a self contained PCI device or something with both regulator and
> consumer do it as part of the subfunction instantiation there.

Thanks for your input. I've taken a look at the possibility to specify
a mapping via regualtor_init_data, rather then falling back to finding the
regulator by name. I've found 2 problems with this:

Problem 1)

The regulator in question is part of the bq24292i charger-IC attached to
a private i2c bus between the PMIC and the charger. The driver for the i2c
controller inside the PMIC which drivers this bus currently also instantiates
the i2c-client for the charger:

drivers/i2c/busses/i2c-cht-wc.c:

static const char * const bq24190_suppliers[] = { "fusb302-typec-source" };

static const struct property_entry bq24190_props[] = {
         PROPERTY_ENTRY_STRING_ARRAY("supplied-from", bq24190_suppliers),
         PROPERTY_ENTRY_BOOL("input-current-limit-from-supplier"),
         PROPERTY_ENTRY_BOOL("omit-battery-class"),
         PROPERTY_ENTRY_BOOL("disable-reset"),
         { }
};

static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
{
         struct i2c_board_info board_info = {
                 .type = "bq24190",
                 .addr = 0x6b,
                 .properties = bq24190_props,
         };
	...
	adap->client_irq = irq_create_mapping(adap->irq_domain, 0);
	ret = i2c_add_adapter(&adap->adapter);

	board_info.irq = adap->client_irq;
	adap->client = i2c_new_device(&adap->adapter, &board_info);
	...
}

Note that the bq24190 driver is a generic driver, so to pass the
board specific regulator_init_data to it I would need to somehow
pass it here, but I don't see how, except by storing a pointer to
it in an u64 device-property which seems like a bad idea


Problem 2)

Even if I could add the mapping through regulator_init_data
then it may well be too late, if the regulator_get happens
before the bq24190 driver registers its regulator (and thus
the mapping) the regulator_get for it may have already
happened and returned a dummy-regulator, or another regulator
with the rather generic vbus name.


TL;DR: It is a mess and I cannot come up with anything better then
just using a globally-unique name, suggestions for a better
solution are welcome.

Regards,

Hans

Powered by blists - more mailing lists