| Message ID | 20170806123555.5124-14-hdegoede@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
* 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
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?
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
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)
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
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");
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(+)