| Message ID | 20170806123555.5124-11-hdegoede@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
On 08/06/2017 05:35 AM, Hans de Goede wrote: > On devicetree platforms the fusb302 dt-node will have a vbus regulator > property with a phandle to the regulator. > > 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. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c > index e1e08f57af99..c3bcc5484ade 100644 > --- a/drivers/staging/typec/fusb302/fusb302.c > +++ b/drivers/staging/typec/fusb302/fusb302.c > @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client, > return -EPROBE_DEFER; > } > > + /* > + * Devicetree platforms should get vbus from their dt-node. > + * On ACPI platforms, we need to get the vbus by a system wide unique > + * name, which is set in a device prop by the platform code. > + */ > + if (device_property_read_string(dev, "fcs,vbus-regulator-name", > + &name) == 0) { Another property to be documented and approved. Also, isn't there a better way to get regulator names for dt- and non-dt systems ? This would apply to every driver supporting both and using regulators, which seems awkward. > + /* > + * Use regulator_get_optional so that we can detect if we need > + * to defer the probe rather then getting the dummy-regulator. > + */ Wouldn't this apply to dt systems as well ? > + chip->vbus = devm_regulator_get_optional(dev, name); > + if (IS_ERR(chip->vbus)) { > + ret = PTR_ERR(chip->vbus); > + return (ret == -ENODEV) ? -EPROBE_DEFER : ret; > + } > + } else { > + chip->vbus = devm_regulator_get(dev, "vbus"); > + if (IS_ERR(chip->vbus)) > + return PTR_ERR(chip->vbus); > + } > + You might also want to explain why you moved this code. > ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev, > "fusb302-typec-source"); > if (ret < 0) > @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client, > INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); > init_tcpc_dev(&chip->tcpc_dev); > > - chip->vbus = devm_regulator_get(chip->dev, "vbus"); > - if (IS_ERR(chip->vbus)) { > - ret = PTR_ERR(chip->vbus); > - goto destroy_workqueue; > - } > - > if (client->irq) { > chip->gpio_int_n_irq = client->irq; > } else { >
Hi, On 06-08-17 16:30, Guenter Roeck wrote: > On 08/06/2017 05:35 AM, Hans de Goede wrote: >> On devicetree platforms the fusb302 dt-node will have a vbus regulator >> property with a phandle to the regulator. >> >> 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. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------ >> 1 file changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c >> index e1e08f57af99..c3bcc5484ade 100644 >> --- a/drivers/staging/typec/fusb302/fusb302.c >> +++ b/drivers/staging/typec/fusb302/fusb302.c >> @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client, >> return -EPROBE_DEFER; >> } >> + /* >> + * Devicetree platforms should get vbus from their dt-node. >> + * On ACPI platforms, we need to get the vbus by a system wide unique >> + * name, which is set in a device prop by the platform code. >> + */ >> + if (device_property_read_string(dev, "fcs,vbus-regulator-name", >> + &name) == 0) { > > 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. > Also, isn't there a better way to get regulator names for dt- and non-dt systems ? > This would apply to every driver supporting both and using regulators, which seems > awkward. While working on this I noticed that it is possible to add a regulator_match table entry when registering a regulator, but that requires describing this in regulator_init_data. Which would mean passing regulator_init_data from the place where it is instantiated to where it gets registered, which would mean passing a pointer through a device-property, given that this is purely kernel internal that is possible, but not really how device-props are supposed to be used. Also since the regulator-core only adds the mapping when registering the regulator, this means that if we try to get the regulator before it has been registered; and there is another regulator with the rather generic "vbus" name then that will be returned instead. Basically regulators are practically almost unused on x86 systems. I had to add CONFIG_REGULATOR=y to my .config which is based on the Fedora 26 kernel .config, so it has pretty much everything under the sun enabled. So it seems that we are covering new ground here. An alternative would be to not use the regulator subsys for this at all, but it does seem the logical thing to use and using get-by-name is no different then what we've doing for setting the the "fusb302-typec-source" psy as supplier for the charger psy class device registered by the bq24190_charger driver. 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. >> + /* >> + * Use regulator_get_optional so that we can detect if we need >> + * to defer the probe rather then getting the dummy-regulator. >> + */ > > Wouldn't this apply to dt systems as well ? No because there will be a property named "vbus-supply" in the fusb302 node containing a phandle to the regulator, if the regulator to which the phandle points has not been registered yet regulator_get will automatically return -EPROBE_DEFER because there is a "vbus-supply" property, only if there is no such property at all will it return a dummy regulator. >> + chip->vbus = devm_regulator_get_optional(dev, name); >> + if (IS_ERR(chip->vbus)) { >> + ret = PTR_ERR(chip->vbus); >> + return (ret == -ENODEV) ? -EPROBE_DEFER : ret; >> + } >> + } else { >> + chip->vbus = devm_regulator_get(dev, "vbus"); >> + if (IS_ERR(chip->vbus)) >> + return PTR_ERR(chip->vbus); >> + } >> + > > You might also want to explain why you moved this code. Right, I did that because it may fail with -EPROBE_DEFER and I wanted to do that before the register_psy. But as I just explained the old code could do that too, so I properly should just put the register_psy later. Regards, Hans >> ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev, >> "fusb302-typec-source"); >> if (ret < 0) >> @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client, >> INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); >> init_tcpc_dev(&chip->tcpc_dev); >> - chip->vbus = devm_regulator_get(chip->dev, "vbus"); >> - if (IS_ERR(chip->vbus)) { >> - ret = PTR_ERR(chip->vbus); >> - goto destroy_workqueue; >> - } >> - >> if (client->irq) { >> chip->gpio_int_n_irq = client->irq; >> } else { >> >
On 08/06/2017 07:52 AM, Hans de Goede wrote: > Hi, > > On 06-08-17 16:30, Guenter Roeck wrote: >> On 08/06/2017 05:35 AM, Hans de Goede wrote: >>> On devicetree platforms the fusb302 dt-node will have a vbus regulator >>> property with a phandle to the regulator. >>> >>> 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. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------ >>> 1 file changed, 22 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c >>> index e1e08f57af99..c3bcc5484ade 100644 >>> --- a/drivers/staging/typec/fusb302/fusb302.c >>> +++ b/drivers/staging/typec/fusb302/fusb302.c >>> @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client, >>> return -EPROBE_DEFER; >>> } >>> + /* >>> + * Devicetree platforms should get vbus from their dt-node. >>> + * On ACPI platforms, we need to get the vbus by a system wide unique >>> + * name, which is set in a device prop by the platform code. >>> + */ >>> + if (device_property_read_string(dev, "fcs,vbus-regulator-name", >>> + &name) == 0) { >> >> 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. Ok. >> Also, isn't there a better way to get regulator names for dt- and non-dt systems ? >> This would apply to every driver supporting both and using regulators, which seems >> awkward. > > While working on this I noticed that it is possible to add a regulator_match > table entry when registering a regulator, but that requires describing this > in regulator_init_data. Which would mean passing regulator_init_data from the > place where it is instantiated to where it gets registered, which would > mean passing a pointer through a device-property, given that this is purely kernel > internal that is possible, but not really how device-props are supposed to be used. > > Also since the regulator-core only adds the mapping when registering the > regulator, this means that if we try to get the regulator before it has been > registered; and there is another regulator with the rather generic "vbus" > name then that will be returned instead. > > Basically regulators are practically almost unused on x86 systems. I had to > add CONFIG_REGULATOR=y to my .config which is based on the Fedora 26 kernel > .config, so it has pretty much everything under the sun enabled. So it seems > that we are covering new ground here. > We have some in hwmon, but they get by with using devm_regulator_get_optional() for both dt and non-dt systems. Only problem with that is that it returns -ENODEV if regulators are not configured, which by itself is weird/odd (and there have been endless discussions about it). > An alternative would be to not use the regulator subsys for this at all, but > it does seem the logical thing to use and using get-by-name is no different > then what we've doing for setting the the "fusb302-typec-source" psy as supplier > for the charger psy class device registered by the bq24190_charger driver. > > 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. > Messy :-(. I don't have a better idea, unfortunately. >>> + /* >>> + * Use regulator_get_optional so that we can detect if we need >>> + * to defer the probe rather then getting the dummy-regulator. >>> + */ >> >> Wouldn't this apply to dt systems as well ? > > No because there will be a property named "vbus-supply" in the fusb302 > node containing a phandle to the regulator, if the regulator to which the phandle > points has not been registered yet regulator_get will automatically return > -EPROBE_DEFER because there is a "vbus-supply" property, only if there is > no such property at all will it return a dummy regulator. > More messy. Again, I don't have a better idea, but it is really weird that we need all this code. There should really be some generic code handling all those differences. >>> + chip->vbus = devm_regulator_get_optional(dev, name); >>> + if (IS_ERR(chip->vbus)) { >>> + ret = PTR_ERR(chip->vbus); >>> + return (ret == -ENODEV) ? -EPROBE_DEFER : ret; This will be stuck in returning -EPROBE_DEFER if the regulator subsystem is disabled. Is this acceptable ? >>> + } >>> + } else { >>> + chip->vbus = devm_regulator_get(dev, "vbus"); >>> + if (IS_ERR(chip->vbus)) >>> + return PTR_ERR(chip->vbus); >>> + } >>> + >> >> You might also want to explain why you moved this code. > > Right, I did that because it may fail with -EPROBE_DEFER and > I wanted to do that before the register_psy. But as I just > explained the old code could do that too, so I properly should > just put the register_psy later. > > Regards, > > Hans > > > >>> ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev, >>> "fusb302-typec-source"); >>> if (ret < 0) >>> @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client, >>> INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); >>> init_tcpc_dev(&chip->tcpc_dev); >>> - chip->vbus = devm_regulator_get(chip->dev, "vbus"); >>> - if (IS_ERR(chip->vbus)) { >>> - ret = PTR_ERR(chip->vbus); >>> - goto destroy_workqueue; >>> - } >>> - >>> if (client->irq) { >>> chip->gpio_int_n_irq = client->irq; >>> } else { >>> >> >
<resend with Liam and Mark added to the Cc as they may want to weigh in on this too> Hi, On 06-08-17 16:30, Guenter Roeck wrote: > On 08/06/2017 05:35 AM, Hans de Goede wrote: >> On devicetree platforms the fusb302 dt-node will have a vbus regulator >> property with a phandle to the regulator. >> >> 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. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------ >> 1 file changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c >> index e1e08f57af99..c3bcc5484ade 100644 >> --- a/drivers/staging/typec/fusb302/fusb302.c >> +++ b/drivers/staging/typec/fusb302/fusb302.c >> @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client, >> return -EPROBE_DEFER; >> } >> + /* >> + * Devicetree platforms should get vbus from their dt-node. >> + * On ACPI platforms, we need to get the vbus by a system wide unique >> + * name, which is set in a device prop by the platform code. >> + */ >> + if (device_property_read_string(dev, "fcs,vbus-regulator-name", >> + &name) == 0) { > > 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. > Also, isn't there a better way to get regulator names for dt- and non-dt systems ? > This would apply to every driver supporting both and using regulators, which seems > awkward. While working on this I noticed that it is possible to add a regulator_match table entry when registering a regulator, but that requires describing this in regulator_init_data. Which would mean passing regulator_init_data from the place where it is instantiated to where it gets registered, which would mean passing a pointer through a device-property, given that this is purely kernel internal that is possible, but not really how device-props are supposed to be used. Also since the regulator-core only adds the mapping when registering the regulator, this means that if we try to get the regulator before it has been registered; and there is another regulator with the rather generic "vbus" name then that will be returned instead. Basically regulators are practically almost unused on x86 systems. I had to add CONFIG_REGULATOR=y to my .config which is based on the Fedora 26 kernel .config, so it has pretty much everything under the sun enabled. So it seems that we are covering new ground here. An alternative would be to not use the regulator subsys for this at all, but it does seem the logical thing to use and using get-by-name is no different then what we've doing for setting the the "fusb302-typec-source" psy as supplier for the charger psy class device registered by the bq24190_charger driver. 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. >> + /* >> + * Use regulator_get_optional so that we can detect if we need >> + * to defer the probe rather then getting the dummy-regulator. >> + */ > > Wouldn't this apply to dt systems as well ? No because there will be a property named "vbus-supply" in the fusb302 node containing a phandle to the regulator, if the regulator to which the phandle points has not been registered yet regulator_get will automatically return -EPROBE_DEFER because there is a "vbus-supply" property, only if there is no such property at all will it return a dummy regulator. >> + chip->vbus = devm_regulator_get_optional(dev, name); >> + if (IS_ERR(chip->vbus)) { >> + ret = PTR_ERR(chip->vbus); >> + return (ret == -ENODEV) ? -EPROBE_DEFER : ret; >> + } >> + } else { >> + chip->vbus = devm_regulator_get(dev, "vbus"); >> + if (IS_ERR(chip->vbus)) >> + return PTR_ERR(chip->vbus); >> + } >> + > > You might also want to explain why you moved this code. Right, I did that because it may fail with -EPROBE_DEFER and I wanted to do that before the register_psy. But as I just explained the old code could do that too, so I properly should just put the register_psy later. Regards, Hans >> ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev, >> "fusb302-typec-source"); >> if (ret < 0) >> @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client, >> INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); >> init_tcpc_dev(&chip->tcpc_dev); >> - chip->vbus = devm_regulator_get(chip->dev, "vbus"); >> - if (IS_ERR(chip->vbus)) { >> - ret = PTR_ERR(chip->vbus); >> - goto destroy_workqueue; >> - } >> - >> if (client->irq) { >> chip->gpio_int_n_irq = client->irq; >> } else { >> >
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. > 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.
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
On Mon, Aug 07, 2017 at 04:41:18PM +0200, Hans de Goede wrote: > On 07-08-17 13:10, Mark Brown wrote: > 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: ... > 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 I2C has a perfectly good platform_data pointer in the board info for this stuff. > 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. If you don't have control over the instantiation ordering but you have a firmware which claims to provide a complete description of regulators then you'd need to add an interface that allows mappings to be registered separately to regulator registration. Whatever you're doing the answer isn't to try to specify the name of the supply through some firmware binding, that's just obviously not sensible both in terms of a firmware abstraction and in terms of how the abstractions in Linux work.
Hi, On 07-08-17 17:41, Mark Brown wrote: > On Mon, Aug 07, 2017 at 04:41:18PM +0200, Hans de Goede wrote: >> On 07-08-17 13:10, Mark Brown wrote: > >> 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: > > ... > >> 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 > > I2C has a perfectly good platform_data pointer in the board info for > this stuff. True, so you are suggesting that I define a bq24190_platform_data struct with a regulator_init_data pointer in there I guess? At least I would not want to just claim that pointer for just regulator_init_data and more-over assuming that what is in there will be regulator_init_data feels wrong. I don't think the power-supply maintainers will be enthusiastic about this (hi Sebastian). But that does make sense and is actually a good idea for tackling the problem of regulator_init_data. >> 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. > > If you don't have control over the instantiation ordering It is not just device-instantiation ordering, it is also driver loading order, the event around which ordering needs to happen is the registration of the regulator (as things are now). > but you have a firmware which claims to provide a complete description of regulators > then you'd need to add an interface that allows mappings to be > registered separately to regulator registration. So the pwm subsys has this pwm_add_table thing which can add lookup entries indepdentent of pwm_registration and which uses supply/device_name matching to find the entry for the caller of pwm_get which is the same as the current lookup code in the regulator-core, but since it is independent of the registration the lookup-table does not contain direct pointers to pwmchip-s instead it uses a string which gets matches against the pwm (parent) dev's dev_name(). Would extending the struct regulator_map with a const char *provider_name: struct regulator_map { struct list_head list; const char *dev_name; /* The dev_name() for the consumer */ const char *supply; struct regulator_dev *regulator; const char *provider; /* The dev_name() for the regulator parent-dev */ }; And having a regulator_add_lookup function which adds an entry to the regulator_map_list which sets provider_name instead of regulator be acceptable ? lookup of such entries would look for regulators where supply matches the regulator-name and provider matches the regulators parent-dev-name. Alternatively the entry could additionally contain a provider_supply_name so that we can make arbitrary consumer-dev-name + consumer-supply-name provider-dev-name + provider-supply-name matches. That would probably be more flexible then requiring the supply name to match. So would something like this (including returning -EPROBE_DEFER if there is a pwm_map_list entry and no matching regulator can be found) acceptable ? > Whatever you're doing the answer isn't to try to specify the name of the > supply through some firmware binding, that's just obviously not sensible > both in terms of a firmware abstraction and in terms of how the > abstractions in Linux work. Ok. Regards, Hans
On Mon, Aug 07, 2017 at 09:20:05PM +0200, Hans de Goede wrote: > On 07-08-17 17:41, Mark Brown wrote: > > I2C has a perfectly good platform_data pointer in the board info for > > this stuff. > True, so you are suggesting that I define a bq24190_platform_data > struct with a regulator_init_data pointer in there I guess? Yes. > I don't think the power-supply maintainers will be enthusiastic > about this (hi Sebastian). But that does make sense and is > actually a good idea for tackling the problem of regulator_init_data. Why not? This is just really standard usage of platform data. > Would extending the struct regulator_map with a const char *provider_name: > struct regulator_map { > struct list_head list; > const char *dev_name; /* The dev_name() for the consumer */ > const char *supply; > struct regulator_dev *regulator; > const char *provider; /* The dev_name() for the regulator parent-dev */ > }; Please don't invent new terminology like this. Just call it a regulator name. > Alternatively the entry could additionally contain a provider_supply_name > so that we can make arbitrary consumer-dev-name + consumer-supply-name > provider-dev-name + provider-supply-name matches. That would probably > be more flexible then requiring the supply name to match. I'm sorry but I can't follow what you mean here. What do you mean by "provider_supply_name"?
<resend with the CC really added back> Hi, On 08/08/2017 04:42 PM, Mark Brown wrote: > On Tue, Aug 08, 2017 at 02:56:46PM +0100, Hans de Goede wrote: >> Hi, > > Please don't take things off-list unless there is a really strong reason > to do so. Sending things to the list ensures that everyone gets a > chance to read and comment on things. Sorry, that was unintentional I probably accidentally hit reply instead of reply-to-all. I've re-added the lists to the Cc. >> On 08/08/2017 10:39 AM, Mark Brown wrote: >>> On Mon, Aug 07, 2017 at 09:20:05PM +0200, Hans de Goede wrote: > >>> Why not? This is just really standard usage of platform data. > >> Right, but in general in most cases we are trying to get rid of >> platform data (where possible). So introducing new platform_data >> is not going to be popular, but I agree that it likely is the >> best solution here. > > No, we aren't. The majority of architectures are still platform data > only and x86 as you're finding uses it extensively along with ACPI. Ok. >>>> Alternatively the entry could additionally contain a provider_supply_name >>>> so that we can make arbitrary consumer-dev-name + consumer-supply-name >>>> provider-dev-name + provider-supply-name matches. That would probably >>>> be more flexible then requiring the supply name to match. > >>> I'm sorry but I can't follow what you mean here. What do you mean by >>> "provider_supply_name"? > >> The current "const char *supply" in regulator_map is the supply name >> passed to regulator_get, so the rdev_get_name requested by the consumer >> (assuming no mapping is in place) > > The name on the parent is *NOT* something anything else should > reference, it's just some internal documentation intended exclusively > for human consmption and can be overridden by the platforms. It should > never be referenced by anything outside the device. > >> One regulator parent-device can register multiple regulator names, iow >> multiple supplies, basically what I want to do is have the map >> (when not using the regulator pointer) match the following 2 pairs: > >> dev_name + supply > >> regulator_parent_dev_name + rdev_get_name > > Have your platform register identifiers that are useful within your > platform, don't rely on the drivers. Ok, I need to think a bit about this. I think I've enough info to come up with a new patch-set not introducing the fcs,vbus-regulator-name device-property ugliness. But this is a side project and I'm rather busy with $dayjob atm, so it may take a while for me to come up with a new patch. Regards, Hans
diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c index e1e08f57af99..c3bcc5484ade 100644 --- a/drivers/staging/typec/fusb302/fusb302.c +++ b/drivers/staging/typec/fusb302/fusb302.c @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client, return -EPROBE_DEFER; } + /* + * Devicetree platforms should get vbus from their dt-node. + * On ACPI platforms, we need to get the vbus by a system wide unique + * name, which is set in a device prop by the platform code. + */ + if (device_property_read_string(dev, "fcs,vbus-regulator-name", + &name) == 0) { + /* + * Use regulator_get_optional so that we can detect if we need + * to defer the probe rather then getting the dummy-regulator. + */ + chip->vbus = devm_regulator_get_optional(dev, name); + if (IS_ERR(chip->vbus)) { + ret = PTR_ERR(chip->vbus); + return (ret == -ENODEV) ? -EPROBE_DEFER : ret; + } + } else { + chip->vbus = devm_regulator_get(dev, "vbus"); + if (IS_ERR(chip->vbus)) + return PTR_ERR(chip->vbus); + } + ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev, "fusb302-typec-source"); if (ret < 0) @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client, INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); init_tcpc_dev(&chip->tcpc_dev); - chip->vbus = devm_regulator_get(chip->dev, "vbus"); - if (IS_ERR(chip->vbus)) { - ret = PTR_ERR(chip->vbus); - goto destroy_workqueue; - } - if (client->irq) { chip->gpio_int_n_irq = client->irq; } else {
On devicetree platforms the fusb302 dt-node will have a vbus regulator property with a phandle to the regulator. 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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)