Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1704829 > unrolled thread

[PATCH 00/18] Hookup typec power-negotation to the PMIC and charger

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/18] Hookup typec power-negotation to the PMIC and charger Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:40 +0200
    [PATCH 12/18] power: supply: Add power_supply_set_input_current_limit_from_supplier helper Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:40 +0200
    [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:40 +0200
      Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost  converter as regulator Tony Lindgren <tony@atomide.com> - 2017-08-08 06:20 +0200
      Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost  converter as regulator Liam Breck <liam@networkimprov.net> - 2017-08-08 10:50 +0200
        Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost  converter as regulator Hans de Goede <hdegoede@redhat.com> - 2017-08-08 11:10 +0200
          Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost  converter as regulator Liam Breck <liam@networkimprov.net> - 2017-08-08 21:00 +0200
            Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost  converter as regulator Hans de Goede <hdegoede@redhat.com> - 2017-08-08 23:10 +0200
    [PATCH 10/18] staging: typec: fusb302: Add support for fcs,vbus-regulator-name device-property Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:40 +0200
      Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Guenter Roeck <linux@roeck-us.net> - 2017-08-06 16:40 +0200
        Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Hans de Goede <hdegoede@redhat.com> - 2017-08-06 17:00 +0200
          Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Guenter Roeck <linux@roeck-us.net> - 2017-08-06 17:30 +0200
        Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Hans de Goede <hdegoede@redhat.com> - 2017-08-06 17:50 +0200
          Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Mark Brown <broonie@kernel.org> - 2017-08-07 13:20 +0200
            Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Hans de Goede <hdegoede@redhat.com> - 2017-08-07 16:50 +0200
              Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Mark Brown <broonie@kernel.org> - 2017-08-07 17:50 +0200
                Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Hans de Goede <hdegoede@redhat.com> - 2017-08-07 21:30 +0200
                  Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Mark Brown <broonie@kernel.org> - 2017-08-08 11:50 +0200
                    Re: [PATCH 10/18] staging: typec: fusb302: Add support for  fcs,vbus-regulator-name device-property Hans de Goede <hdegoede@redhat.com> - 2017-08-08 23:00 +0200
    [PATCH 14/18] power: supply: bq24190_charger: Add input_current_limit property Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:40 +0200
    [PATCH 09/18] staging: typec: fusb302: Use tcpm_set_current_limit_psy Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:40 +0200
      Re: [PATCH 09/18] staging: typec: fusb302: Use  tcpm_set_current_limit_psy Guenter Roeck <linux@roeck-us.net> - 2017-08-06 16:30 +0200
    [PATCH 05/18] staging: typec: fusb302: Set max supply voltage to 5V Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:40 +0200
    [PATCH 03/18] staging: typec: tcpm: Split tcpm code into tcpm-core.c and tcpm-helpers.c Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:40 +0200
    [PATCH 02/18] staging: typec: tcpm: Add extcon helper functions for USB2 current limit detect Hans de Goede <hdegoede@redhat.com> - 2017-08-06 14:50 +0200
      Re: [PATCH 02/18] staging: typec: tcpm: Add extcon helper functions  for USB2 current limit detect Guenter Roeck <linux@roeck-us.net> - 2017-08-06 16:10 +0200

Page 1 of 2  [1] 2  Next page →

#1704829 — [PATCH 00/18] Hookup typec power-negotation to the PMIC and charger

Hi All,

This series implements a number of typec changes discussed a while back:

- It exports the negotiated voltage and max-current in the form of a
  power-supply class device which represents the USB Type-C power-brick
  (adapter/charger)
- It adds a power_supply_set_input_current_limit_from_supplier helper
  function which charger drivers can use to get the max-current from
  their supplier
- It adds regulator support to the charger IC on the device I've. The
  exported regulator controls the 5v boost convertor which generates the
  5V USB vbus which gets output when the Type-C port is in host / power-src
  mode
- It adds a bunch of misc. related fixes and glue code to tie everything
  together

One thing which was undecided in the previous discussion was how to make
port-controller drivers hookup to external ICs (e.g. a non Type-C aware PMIC)
to decect the input-current-limit for USB2 power-sources (through e.g. BC1.2
detection). Since a number of existing drivers, including the one for the
PMIC used on the 2 mini laptops I'm working on, already use the extcon
framework to communicate the detected USB2 charger-type, I've decided to
simply hook into this existing code. As this patch set shows this can be
done with zero changes to the existing PMIC/extcon drivers.

With this series the GPD win and GPD pocket mini laptops both fully
support any type of Type-C charging. When hooked up with:
-A -> C cable and plugged into a regular port they charge at 5V 0.5A
-A -> C cable and plugged into a dedictaed charger they charge at 5V 2A
-C -> C cable and plugged into a fixed 5V 3A charger, at 5V 3A
-C -> C cable and plugged into a PD capable charger, which delivers max 12V, 2A
 they charge at 12V, 2A

And when a Type-C to USB-A receptacle (so host mode) cable gets plugged in
the port correctly supplies 5V to any plugged in USB-A peripherals.

Assuming this series gets a favorable review then the question becomes how
to merge this. This series has staging/typec, drivers/power/supply,
drivers/platform/x86 and drivers/i2c patches.

All these patches can be merged indepently, but the drivers/platform/x86
and drivers/i2c glue patches really should not be applied until the other
patches are in place.

Assuming no one nacks the concept of using a power-supply class device which
represents the USB Type-C power-brick, then all the typec and power patches
can be merged indepdently and as soon as they are deemed ready.

The only exception is the "power: supply: bq24190_charger: Remove extcon
handling" patch, which should not be merged until the drivers/i2c patch
removing the extcon handling from the bq24190 i2c-client instantiated there
has been merged.

Please let me know what you think and feel free to merge any patches you
like as is, then I can do a v2 addressing comments in the remaining patches.

Regards,

Hans

[toc] | [next] | [standalone]

#1704830 — [PATCH 12/18] power: supply: Add power_supply_set_input_current_limit_from_supplier helper

On some devices the USB Type-C port power (USB PD 2.0) negotiation is
done by a separate port-controller IC, while the current limit is
controlled through another (charger) IC.

It has been decided to model this by modelling the external Type-C
power brick (adapter/charger) as a power-supply class device which
supplies the charger-IC, with its voltage-now and current-max representing
the negotiated voltage and max current draw.

This commit adds a power_supply_set_input_current_limit_from_supplier
helper function which charger power-supply drivers can call to get
the max-current from their supplier and have this applied
through their set_property call-back to their input-current-limit.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/power_supply_core.c | 41 ++++++++++++++++++++++++++++++++
 include/linux/power_supply.h             |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 0741fcef3b44..3f92574222de 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -375,6 +375,47 @@ int power_supply_is_system_supplied(void)
 }
 EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
 
+static int __power_supply_get_supplier_max_current(struct device *dev,
+						   void *data)
+{
+	union power_supply_propval ret = {0,};
+	struct power_supply *epsy = dev_get_drvdata(dev);
+	struct power_supply *psy = data;
+
+	if (__power_supply_is_supplied_by(epsy, psy))
+		if (!epsy->desc->get_property(epsy,
+					      POWER_SUPPLY_PROP_CURRENT_MAX,
+					      &ret))
+			return ret.intval;
+
+	return 0;
+}
+
+int power_supply_set_input_current_limit_from_supplier(struct power_supply *psy)
+{
+	union power_supply_propval val = {0,};
+	int curr;
+
+	if (!psy->desc->set_property)
+		return -EINVAL;
+
+	/*
+	 * This function is not intended for use with a supply with multiple
+	 * suppliers, we simply pick the first supply to report a non 0
+	 * max-current.
+	 */
+	curr = class_for_each_device(power_supply_class, NULL, psy,
+				      __power_supply_get_supplier_max_current);
+	if (curr <= 0)
+		return (curr == 0) ? -ENODEV : curr;
+
+	val.intval = curr;
+
+	return psy->desc->set_property(psy,
+				POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val);
+}
+EXPORT_SYMBOL_GPL(power_supply_set_input_current_limit_from_supplier);
+
 int power_supply_set_battery_charged(struct power_supply *psy)
 {
 	if (atomic_read(&psy->use_cnt) >= 0 &&
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index de89066b72b1..79e90b3d3288 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -332,6 +332,8 @@ extern int power_supply_get_battery_info(struct power_supply *psy,
 					 struct power_supply_battery_info *info);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
+extern int power_supply_set_input_current_limit_from_supplier(
+					 struct power_supply *psy);
 extern int power_supply_set_battery_charged(struct power_supply *psy);
 
 #ifdef CONFIG_POWER_SUPPLY
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1704831 — [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

Register the 5V boost converter as a regulator named
"regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
the bq24190 family is also used on ACPI devices where there are no
device-tree phandles, so regulator_get will fallback to the name and thus
it must be unique on the system.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 121 +++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index d5a707e14526..f25ea9c4acca 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -16,6 +16,8 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/power_supply.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include <linux/workqueue.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
@@ -504,6 +506,121 @@ static int bq24190_sysfs_create_group(struct bq24190_dev_info *bdi)
 static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info *bdi) {}
 #endif
 
+#ifdef CONFIG_REGULATOR
+static int bq24190_vbus_enable(struct regulator_dev *dev)
+{
+	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		pm_runtime_put_noidle(bdi->dev);
+		return ret;
+	}
+
+	ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
+				 BQ24190_REG_POC_CHG_CONFIG_MASK,
+				 BQ24190_REG_POC_CHG_CONFIG_SHIFT,
+				 BQ24190_REG_POC_CHG_CONFIG_OTG);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return ret;
+}
+
+static int bq24190_vbus_disable(struct regulator_dev *dev)
+{
+	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		pm_runtime_put_noidle(bdi->dev);
+		return ret;
+	}
+
+	ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
+				 BQ24190_REG_POC_CHG_CONFIG_MASK,
+				 BQ24190_REG_POC_CHG_CONFIG_SHIFT,
+				 BQ24190_REG_POC_CHG_CONFIG_CHARGE);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return ret;
+}
+
+static int bq24190_vbus_is_enabled(struct regulator_dev *dev)
+{
+	struct bq24190_dev_info *bdi = rdev_get_drvdata(dev);
+	int ret;
+	u8 val;
+
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		pm_runtime_put_noidle(bdi->dev);
+		return ret;
+	}
+
+	ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
+				BQ24190_REG_POC_CHG_CONFIG_MASK,
+				BQ24190_REG_POC_CHG_CONFIG_SHIFT, &val);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return ret ? ret : val == BQ24190_REG_POC_CHG_CONFIG_OTG;
+}
+
+static const struct regulator_ops bq24190_vbus_ops = {
+	.enable = bq24190_vbus_enable,
+	.disable = bq24190_vbus_disable,
+	.is_enabled = bq24190_vbus_is_enabled,
+};
+
+static const struct regulator_desc bq24190_vbus_desc = {
+	.name = "regulator-bq24190-usb-vbus",
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+	.ops = &bq24190_vbus_ops,
+	.fixed_uV = 5000000,
+	.n_voltages = 1,
+};
+
+static const struct regulator_init_data bq24190_vbus_init_data = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
+
+static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
+{
+	struct regulator_config cfg = { };
+	struct regulator_dev *reg;
+	int ret = 0;
+
+	cfg.dev = bdi->dev;
+	cfg.init_data = &bq24190_vbus_init_data;
+	cfg.driver_data = bdi;
+	reg = devm_regulator_register(bdi->dev, &bq24190_vbus_desc, &cfg);
+	if (IS_ERR(reg)) {
+		ret = PTR_ERR(reg);
+		dev_err(bdi->dev, "Can't register regulator: %d\n", ret);
+	}
+
+	return ret;
+}
+#else
+static int bq24190_register_vbus_regulator(struct bq24190_dev_info *bdi)
+{
+	return 0;
+}
+#endif
+
 /*
  * According to the "Host Mode and default Mode" section of the
  * manual, a write to any register causes the bq24190 to switch
@@ -1530,6 +1647,10 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out_pmrt;
 	}
 
+	ret = bq24190_register_vbus_regulator(bdi);
+	if (ret < 0)
+		goto out_pmrt;
+
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

#1706001 — Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

* Hans de Goede <hdegoede@redhat.com> [170806 05:37]:
> Register the 5V boost converter as a regulator named
> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
> the bq24190 family is also used on ACPI devices where there are no
> device-tree phandles, so regulator_get will fallback to the name and thus
> it must be unique on the system.

Nice, this makes VBUS easy to use for USB PHY drivers :)

Tony

[toc] | [prev] | [next] | [standalone]

#1706164 — Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

Hi Hans,

On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Register the 5V boost converter as a regulator named
> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
> the bq24190 family is also used on ACPI devices where there are no
> device-tree phandles, so regulator_get will fallback to the name and thus
> it must be unique on the system.

What we're enabling here is 5V boost for otg host mode, not vbus
generally, so maybe the name should indicate that...

regulator-bq24190-usb-5volt
regulator-bq24190-usb-host
regulator-bq24190-usb-otg-5v

Tony, thoughts?

[toc] | [prev] | [next] | [standalone]

#1706229 — Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

Hi,

On 08-08-17 10:39, Liam Breck wrote:
> Hi Hans,
> 
> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Register the 5V boost converter as a regulator named
>> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
>> the bq24190 family is also used on ACPI devices where there are no
>> device-tree phandles, so regulator_get will fallback to the name and thus
>> it must be unique on the system.
> 
> What we're enabling here is 5V boost for otg host mode, not vbus
> generally, so maybe the name should indicate that...
> 
> regulator-bq24190-usb-5volt
> regulator-bq24190-usb-host
> regulator-bq24190-usb-otg-5v

I picked vbus because that gets used a lot already in similar cases,
but I agree that we should probably come up with a better name.

I like "regulator-bq24190-usb-otg-5v", shall I use that for v2?

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1706825 — Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

On Tue, Aug 8, 2017 at 2:00 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 08-08-17 10:39, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Register the 5V boost converter as a regulator named
>>> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
>>> the bq24190 family is also used on ACPI devices where there are no
>>> device-tree phandles, so regulator_get will fallback to the name and thus
>>> it must be unique on the system.
>>
>>
>> What we're enabling here is 5V boost for otg host mode, not vbus
>> generally, so maybe the name should indicate that...
>>
>> regulator-bq24190-usb-5volt
>> regulator-bq24190-usb-host
>> regulator-bq24190-usb-otg-5v
>
>
> I picked vbus because that gets used a lot already in similar cases,
> but I agree that we should probably come up with a better name.
>
> I like "regulator-bq24190-usb-otg-5v", shall I use that for v2?

There is this upstream, with "otg-vbus":
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=61274eff0ddee8f10deaa5f79085e981db52930a

Related search:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/?qt=grep&q=otg+regulator

I don't think it needs a "regulator-" prefix, and maybe the driver
name is a suffix. We could also recommend the "regulator-name" value
for OTG here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/Documentation/devicetree/bindings/regulator/regulator.txt

a. Make the above patch wrong :-)
usb-otg-5v (generic)
usb-otg-5v-bq2419x (specific)

b. Follow a weak precedent
otg-vbus (generic)
otg-vbus-bq2419x (specific)

[toc] | [prev] | [next] | [standalone]

#1706887 — Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

Hi,

On 08/08/2017 08:57 PM, Liam Breck wrote:
> On Tue, Aug 8, 2017 at 2:00 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 08-08-17 10:39, Liam Breck wrote:
>>>
>>> Hi Hans,
>>>
>>> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Register the 5V boost converter as a regulator named
>>>> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
>>>> the bq24190 family is also used on ACPI devices where there are no
>>>> device-tree phandles, so regulator_get will fallback to the name and thus
>>>> it must be unique on the system.
>>>
>>>
>>> What we're enabling here is 5V boost for otg host mode, not vbus
>>> generally, so maybe the name should indicate that...
>>>
>>> regulator-bq24190-usb-5volt
>>> regulator-bq24190-usb-host
>>> regulator-bq24190-usb-otg-5v
>>
>>
>> I picked vbus because that gets used a lot already in similar cases,
>> but I agree that we should probably come up with a better name.
>>
>> I like "regulator-bq24190-usb-otg-5v", shall I use that for v2?
> 
> There is this upstream, with "otg-vbus":
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=61274eff0ddee8f10deaa5f79085e981db52930a
> 
> Related search:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/?qt=grep&q=otg+regulator
> 
> I don't think it needs a "regulator-" prefix, and maybe the driver
> name is a suffix. We could also recommend the "regulator-name" value
> for OTG here:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/Documentation/devicetree/bindings/regulator/regulator.txt
> 
> a. Make the above patch wrong :-)
> usb-otg-5v (generic)
> usb-otg-5v-bq2419x (specific)
> 
> b. Follow a weak precedent
> otg-vbus (generic)
> otg-vbus-bq2419x (specific)

Looking at dts files under arch/arm/boot/dts the most used name seems to be
usb_otg_vbus (with underscores) so I will use that for v2 of this patch.

As for making the name more specific, as Mark Brown has correctly pointed
out the right thing to do on x86 is to add a mapping for the consumer of the
regulator using regulator_init_data, which can be passed through platform_data
(and I do control the i2c_client instantiation on x86, so adding that is easy),
which means that we can keep the name generic, which is a much better solution
then relying on the name. This means that we can keep the name generic as
one normally does for a regulator-name.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

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

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) {
+		/*
+		 * 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 {
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1705600 — 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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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"?

[toc] | [prev] | [next] | [standalone]

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

<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

[toc] | [prev] | [next] | [standalone]

#1704833 — [PATCH 14/18] power: supply: bq24190_charger: Add input_current_limit property

Export the input current limit of the charger as a
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property on the charger
power_supply class device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 35 ++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index f25ea9c4acca..d78e2c6dc127 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -982,6 +982,33 @@ static int bq24190_charger_set_voltage(struct bq24190_dev_info *bdi,
 			ARRAY_SIZE(bq24190_cvc_vreg_values), val->intval);
 }
 
+static int bq24190_charger_get_iinlimit(struct bq24190_dev_info *bdi,
+		union power_supply_propval *val)
+{
+	int iinlimit, ret;
+
+	ret = bq24190_get_field_val(bdi, BQ24190_REG_ISC,
+			BQ24190_REG_ISC_IINLIM_MASK,
+			BQ24190_REG_ISC_IINLIM_SHIFT,
+			bq24190_isc_iinlim_values,
+			ARRAY_SIZE(bq24190_isc_iinlim_values), &iinlimit);
+	if (ret < 0)
+		return ret;
+
+	val->intval = iinlimit;
+	return 0;
+}
+
+static int bq24190_charger_set_iinlimit(struct bq24190_dev_info *bdi,
+		const union power_supply_propval *val)
+{
+	return bq24190_set_field_val(bdi, BQ24190_REG_ISC,
+			BQ24190_REG_ISC_IINLIM_MASK,
+			BQ24190_REG_ISC_IINLIM_SHIFT,
+			bq24190_isc_iinlim_values,
+			ARRAY_SIZE(bq24190_isc_iinlim_values), val->intval);
+}
+
 static int bq24190_charger_get_property(struct power_supply *psy,
 		enum power_supply_property psp, union power_supply_propval *val)
 {
@@ -1022,6 +1049,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
 		ret = bq24190_charger_get_voltage_max(bdi, val);
 		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = bq24190_charger_get_iinlimit(bdi, val);
+		break;
 	case POWER_SUPPLY_PROP_SCOPE:
 		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
 		ret = 0;
@@ -1073,6 +1103,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 		ret = bq24190_charger_set_voltage(bdi, val);
 		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = bq24190_charger_set_iinlimit(bdi, val);
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -1094,6 +1127,7 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		ret = 1;
 		break;
 	default:
@@ -1113,6 +1147,7 @@ static enum power_supply_property bq24190_charger_properties[] = {
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
 	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
-- 
2.13.3

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web