Re: [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property

From: Hans de Goede
Date: Mon Aug 07 2017 - 10:41:29 EST


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