* [PATCH v3 1/5] power: bq24190_charger: Add ti,bq24192i to devicetree table
2017-08-26 5:14 [PATCH v3 0/5] BQ24190 devicetree config Liam Breck
@ 2017-08-26 5:14 ` Liam Breck
2017-08-28 15:59 ` Tony Lindgren
[not found] ` <20170826051413.24569-1-liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Liam Breck @ 2017-08-26 5:14 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, Liam Breck
From: Liam Breck <kernel@networkimprov.net>
bq24192i was previously only in ID table, so add it to DT table.
Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
drivers/power/supply/bq24190_charger.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 14199561..40b4bba7 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1732,6 +1732,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
#ifdef CONFIG_OF
static const struct of_device_id bq24190_of_match[] = {
{ .compatible = "ti,bq24190", },
+ { .compatible = "ti,bq24192i", },
{ },
};
MODULE_DEVICE_TABLE(of, bq24190_of_match);
--
2.13.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/5] power: bq24190_charger: Add ti,bq24192i to devicetree table
2017-08-26 5:14 ` [PATCH v3 1/5] power: bq24190_charger: Add ti,bq24192i to devicetree table Liam Breck
@ 2017-08-28 15:59 ` Tony Lindgren
0 siblings, 0 replies; 13+ messages in thread
From: Tony Lindgren @ 2017-08-28 15:59 UTC (permalink / raw)
To: Liam Breck; +Cc: Sebastian Reichel, linux-pm, Hans de Goede, Liam Breck
* Liam Breck <liam@networkimprov.net> [170825 22:15]:
> From: Liam Breck <kernel@networkimprov.net>
>
> bq24192i was previously only in ID table, so add it to DT table.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Tony Lindgren <tony@atomide.com>
> ---
> drivers/power/supply/bq24190_charger.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 14199561..40b4bba7 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1732,6 +1732,7 @@ MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
> #ifdef CONFIG_OF
> static const struct of_device_id bq24190_of_match[] = {
> { .compatible = "ti,bq24190", },
> + { .compatible = "ti,bq24192i", },
> { },
> };
> MODULE_DEVICE_TABLE(of, bq24190_of_match);
> --
> 2.13.2
>
^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <20170826051413.24569-1-liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>]
* [PATCH v3 2/5] devicetree: power: Add docs for TI BQ24190 battery charger
[not found] ` <20170826051413.24569-1-liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
@ 2017-08-26 5:14 ` Liam Breck
2017-08-28 16:00 ` Tony Lindgren
0 siblings, 1 reply; 13+ messages in thread
From: Liam Breck @ 2017-08-26 5:14 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm-u79uwXL29TY76Z2rM5mHXA
Cc: Tony Lindgren, Hans de Goede, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA, Liam Breck
From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
Document monitored-battery and ti,system-minimum-microvolt properties.
Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
---
.../devicetree/bindings/power/supply/bq24190.txt | 51 ++++++++++++++++++++++
1 file changed, 51 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/bq24190.txt
diff --git a/Documentation/devicetree/bindings/power/supply/bq24190.txt b/Documentation/devicetree/bindings/power/supply/bq24190.txt
new file mode 100644
index 00000000..9e517d30
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/bq24190.txt
@@ -0,0 +1,51 @@
+TI BQ24190 Li-Ion Battery Charger
+
+Required properties:
+- compatible: contains one of the following:
+ * "ti,bq24190"
+ * "ti,bq24192i"
+- reg: integer, I2C address of the charger.
+- interrupts[-extended]: configuration for charger INT pin.
+
+Optional properties:
+- monitored-battery: phandle of battery characteristics devicetree node
+ The charger uses the following battery properties:
+ + precharge-current-microamp: maximum charge current during precharge
+ phase (typically 20% of battery capacity).
+ + charge-term-current-microamp: a charge cycle terminates when the
+ battery voltage is above recharge threshold, and the current is below
+ this setting (typically 10% of battery capacity).
+ See also Documentation/devicetree/bindings/power/supply/battery.txt
+- ti,system-minimum-microvolt: when power is connected and the battery is below
+ minimum system voltage, the system will be regulated above this setting.
+
+Notes:
+- Some circuit boards wire the chip's "OTG" pin high (enabling 500mA default
+ charge current on USB SDP ports, among other features). To simulate this on
+ boards that wire the pin to a GPIO, set a gpio-hog.
+
+Example:
+
+ bat: battery {
+ compatible = "simple-battery";
+ precharge-current-microamp = <256000>;
+ charge-term-current-microamp = <128000>;
+ // etc.
+ };
+
+ bq24190: charger@6a {
+ compatible = "ti,bq24190";
+ reg = <0x6a>;
+ interrupts-extended = <&gpiochip 10 IRQ_TYPE_EDGE_FALLING>;
+ monitored-battery = <&bat>;
+ ti,system-minimum-microvolt = <3200000>;
+ };
+
+ &twl_gpio {
+ otg {
+ gpio-hog;
+ gpios = <6 0>;
+ output-high;
+ line-name = "otg-gpio";
+ };
+ };
--
2.13.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config
2017-08-26 5:14 [PATCH v3 0/5] BQ24190 devicetree config Liam Breck
2017-08-26 5:14 ` [PATCH v3 1/5] power: bq24190_charger: Add ti,bq24192i to devicetree table Liam Breck
[not found] ` <20170826051413.24569-1-liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
@ 2017-08-26 5:14 ` Liam Breck
2017-08-28 12:13 ` Hans de Goede
2017-08-26 5:14 ` [PATCH v3 4/5] power: bq24190_charger: Add property system-minimum-microvolt Liam Breck
2017-08-26 5:14 ` [PATCH v3 5/5] power: bq24190_charger: Add power_supply_battery_info support Liam Breck
4 siblings, 1 reply; 13+ messages in thread
From: Liam Breck @ 2017-08-26 5:14 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, Liam Breck
From: Liam Breck <kernel@networkimprov.net>
Add get_config(). Rename set_mode_host() to set_config().
Call get_config() and hw_init() after power_supply_register().
No functional changes.
Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
drivers/power/supply/bq24190_charger.c | 63 +++++++++++++++++++++++-----------
1 file changed, 43 insertions(+), 20 deletions(-)
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 40b4bba7..c6be00d7 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -504,15 +504,7 @@ static int bq24190_sysfs_create_group(struct bq24190_dev_info *bdi)
static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info *bdi) {}
#endif
-/*
- * According to the "Host Mode and default Mode" section of the
- * manual, a write to any register causes the bq24190 to switch
- * from default mode to host mode. It will switch back to default
- * mode after a WDT timeout unless the WDT is turned off as well.
- * So, by simply turning off the WDT, we accomplish both with the
- * same write.
- */
-static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
+static int bq24190_set_config(struct bq24190_dev_info *bdi)
{
int ret;
u8 v;
@@ -523,9 +515,22 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
bdi->watchdog = ((v & BQ24190_REG_CTTC_WATCHDOG_MASK) >>
BQ24190_REG_CTTC_WATCHDOG_SHIFT);
+
+ /*
+ * According to the "Host Mode and default Mode" section of the
+ * manual, a write to any register causes the bq24190 to switch
+ * from default mode to host mode. It will switch back to default
+ * mode after a WDT timeout unless the WDT is turned off as well.
+ * So, by simply turning off the WDT, we accomplish both with the
+ * same write.
+ */
v &= ~BQ24190_REG_CTTC_WATCHDOG_MASK;
- return bq24190_write(bdi, BQ24190_REG_CTTC, v);
+ ret = bq24190_write(bdi, BQ24190_REG_CTTC, v);
+ if (ret < 0)
+ return ret;
+
+ return 0;
}
static int bq24190_register_reset(struct bq24190_dev_info *bdi)
@@ -1456,13 +1461,25 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
if (ret < 0)
return ret;
- ret = bq24190_set_mode_host(bdi);
+ ret = bq24190_set_config(bdi);
if (ret < 0)
return ret;
return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
}
+static int bq24190_get_config(struct bq24190_dev_info *bdi)
+{
+#ifdef CONFIG_OF
+ int v;
+
+ if (!bdi->dev->of_node)
+ return -EINVAL;
+
+#endif
+ return 0;
+}
+
static int bq24190_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -1493,7 +1510,7 @@ static int bq24190_probe(struct i2c_client *client,
i2c_set_clientdata(client, bdi);
- if (!client->irq) {
+ if (client->irq <= 0) {
dev_err(dev, "Can't get irq info\n");
return -EINVAL;
}
@@ -1526,12 +1543,6 @@ static int bq24190_probe(struct i2c_client *client,
goto out_pmrt;
}
- ret = bq24190_hw_init(bdi);
- if (ret < 0) {
- dev_err(dev, "Hardware init failed\n");
- goto out_pmrt;
- }
-
charger_cfg.drv_data = bdi;
charger_cfg.supplied_to = bq24190_charger_supplied_to;
charger_cfg.num_supplicants = ARRAY_SIZE(bq24190_charger_supplied_to),
@@ -1556,8 +1567,20 @@ static int bq24190_probe(struct i2c_client *client,
}
}
+ ret = bq24190_get_config(bdi);
+ if (ret < 0) {
+ dev_err(dev, "Can't get devicetree config\n");
+ goto out_charger;
+ }
+
+ ret = bq24190_hw_init(bdi);
+ if (ret < 0) {
+ dev_err(dev, "Hardware init failed\n");
+ goto out_charger;
+ }
+
ret = bq24190_sysfs_create_group(bdi);
- if (ret) {
+ if (ret < 0) {
dev_err(dev, "Can't create sysfs entries\n");
goto out_charger;
}
@@ -1700,7 +1723,7 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
}
bq24190_register_reset(bdi);
- bq24190_set_mode_host(bdi);
+ bq24190_set_config(bdi);
bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
if (error >= 0) {
--
2.13.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config
2017-08-26 5:14 ` [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config Liam Breck
@ 2017-08-28 12:13 ` Hans de Goede
2017-08-28 17:59 ` Liam Breck
0 siblings, 1 reply; 13+ messages in thread
From: Hans de Goede @ 2017-08-28 12:13 UTC (permalink / raw)
To: Liam Breck, Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Liam Breck
Hi,
On 26-08-17 07:14, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> Add get_config(). Rename set_mode_host() to set_config().
> Call get_config() and hw_init() after power_supply_register().
> No functional changes.
Doing hw_init after power_supply_register() means that userspace
can get/set power_supply properties before hw_init has run, this
generally is not a good idea.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
> drivers/power/supply/bq24190_charger.c | 63 +++++++++++++++++++++++-----------
> 1 file changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 40b4bba7..c6be00d7 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -504,15 +504,7 @@ static int bq24190_sysfs_create_group(struct bq24190_dev_info *bdi)
> static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info *bdi) {}
> #endif
>
> -/*
> - * According to the "Host Mode and default Mode" section of the
> - * manual, a write to any register causes the bq24190 to switch
> - * from default mode to host mode. It will switch back to default
> - * mode after a WDT timeout unless the WDT is turned off as well.
> - * So, by simply turning off the WDT, we accomplish both with the
> - * same write.
> - */
> -static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
> +static int bq24190_set_config(struct bq24190_dev_info *bdi)
> {
> int ret;
> u8 v;
> @@ -523,9 +515,22 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>
> bdi->watchdog = ((v & BQ24190_REG_CTTC_WATCHDOG_MASK) >>
> BQ24190_REG_CTTC_WATCHDOG_SHIFT);
> +
> + /*
> + * According to the "Host Mode and default Mode" section of the
> + * manual, a write to any register causes the bq24190 to switch
> + * from default mode to host mode. It will switch back to default
> + * mode after a WDT timeout unless the WDT is turned off as well.
> + * So, by simply turning off the WDT, we accomplish both with the
> + * same write.
> + */
> v &= ~BQ24190_REG_CTTC_WATCHDOG_MASK;
>
> - return bq24190_write(bdi, BQ24190_REG_CTTC, v);
> + ret = bq24190_write(bdi, BQ24190_REG_CTTC, v);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> }
>
> static int bq24190_register_reset(struct bq24190_dev_info *bdi)
> @@ -1456,13 +1461,25 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
> if (ret < 0)
> return ret;
>
> - ret = bq24190_set_mode_host(bdi);
> + ret = bq24190_set_config(bdi);
> if (ret < 0)
> return ret;
>
> return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> }
>
> +static int bq24190_get_config(struct bq24190_dev_info *bdi)
> +{
> +#ifdef CONFIG_OF
> + int v;
This variable is unused (in this patch)
> +
> + if (!bdi->dev->of_node)
> + return -EINVAL;
I don't think that this is a good idea, at least on ARM
(AFAIK) a kernel can be configured to support both DT and
ACPI, just having CONFIG_OF enabled as such is not a good
reason to fail to probe when there is no of_node.
IMHO it would be better to instead put the reading of the
"ti,system-minimum-microvolt" property introduced in the next
patch inside a: if (bdi->dev->of_node) { ... } block.
> +
> +#endif
> + return 0;
> +}
> +
> static int bq24190_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -1493,7 +1510,7 @@ static int bq24190_probe(struct i2c_client *client,
>
> i2c_set_clientdata(client, bdi);
>
> - if (!client->irq) {
> + if (client->irq <= 0) {
> dev_err(dev, "Can't get irq info\n");
> return -EINVAL;
> }
> @@ -1526,12 +1543,6 @@ static int bq24190_probe(struct i2c_client *client,
> goto out_pmrt;
> }
>
> - ret = bq24190_hw_init(bdi);
> - if (ret < 0) {
> - dev_err(dev, "Hardware init failed\n");
> - goto out_pmrt;
> - }
> -
> charger_cfg.drv_data = bdi;
> charger_cfg.supplied_to = bq24190_charger_supplied_to;
> charger_cfg.num_supplicants = ARRAY_SIZE(bq24190_charger_supplied_to),
> @@ -1556,8 +1567,20 @@ static int bq24190_probe(struct i2c_client *client,
> }
> }
>
> + ret = bq24190_get_config(bdi);
> + if (ret < 0) {
> + dev_err(dev, "Can't get devicetree config\n");
> + goto out_charger;
> + }
> +
> + ret = bq24190_hw_init(bdi);
> + if (ret < 0) {
> + dev_err(dev, "Hardware init failed\n");
> + goto out_charger;
> + }
> +
> ret = bq24190_sysfs_create_group(bdi);
> - if (ret) {
> + if (ret < 0) {
> dev_err(dev, "Can't create sysfs entries\n");
> goto out_charger;
> }
> @@ -1700,7 +1723,7 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
> }
>
> bq24190_register_reset(bdi);
> - bq24190_set_mode_host(bdi);
> + bq24190_set_config(bdi);
> bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>
> if (error >= 0) {
>
Regards,
Hans
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config
2017-08-28 12:13 ` Hans de Goede
@ 2017-08-28 17:59 ` Liam Breck
2017-08-28 19:09 ` Hans de Goede
0 siblings, 1 reply; 13+ messages in thread
From: Liam Breck @ 2017-08-28 17:59 UTC (permalink / raw)
To: Hans de Goede; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck
Hi Hans, thanks for the input...
On Mon, Aug 28, 2017 at 5:13 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 26-08-17 07:14, Liam Breck wrote:
>>
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Add get_config(). Rename set_mode_host() to set_config().
>> Call get_config() and hw_init() after power_supply_register().
>> No functional changes.
>
>
> Doing hw_init after power_supply_register() means that userspace
> can get/set power_supply properties before hw_init has run, this
> generally is not a good idea.
hw_init() needs data from DT, and power_supply_get_battery_info()
(called by get_config() in later patch) gets the DT data. It takes a
power_supply argument, hence the new ordering.
Is there a way to defer availability of a registered power_supply to
userspace during probe()? I recall looking into this when I wrote that
and concluding that it was safe, but I don't recall why :-/
>
>>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>> drivers/power/supply/bq24190_charger.c | 63
>> +++++++++++++++++++++++-----------
>> 1 file changed, 43 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c
>> b/drivers/power/supply/bq24190_charger.c
>> index 40b4bba7..c6be00d7 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -504,15 +504,7 @@ static int bq24190_sysfs_create_group(struct
>> bq24190_dev_info *bdi)
>> static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info
>> *bdi) {}
>> #endif
>> -/*
>> - * According to the "Host Mode and default Mode" section of the
>> - * manual, a write to any register causes the bq24190 to switch
>> - * from default mode to host mode. It will switch back to default
>> - * mode after a WDT timeout unless the WDT is turned off as well.
>> - * So, by simply turning off the WDT, we accomplish both with the
>> - * same write.
>> - */
>> -static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>> +static int bq24190_set_config(struct bq24190_dev_info *bdi)
>> {
>> int ret;
>> u8 v;
>> @@ -523,9 +515,22 @@ static int bq24190_set_mode_host(struct
>> bq24190_dev_info *bdi)
>> bdi->watchdog = ((v & BQ24190_REG_CTTC_WATCHDOG_MASK) >>
>> BQ24190_REG_CTTC_WATCHDOG_SHIFT);
>> +
>> + /*
>> + * According to the "Host Mode and default Mode" section of the
>> + * manual, a write to any register causes the bq24190 to switch
>> + * from default mode to host mode. It will switch back to default
>> + * mode after a WDT timeout unless the WDT is turned off as well.
>> + * So, by simply turning off the WDT, we accomplish both with the
>> + * same write.
>> + */
>> v &= ~BQ24190_REG_CTTC_WATCHDOG_MASK;
>> - return bq24190_write(bdi, BQ24190_REG_CTTC, v);
>> + ret = bq24190_write(bdi, BQ24190_REG_CTTC, v);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> }
>> static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>> @@ -1456,13 +1461,25 @@ static int bq24190_hw_init(struct bq24190_dev_info
>> *bdi)
>> if (ret < 0)
>> return ret;
>> - ret = bq24190_set_mode_host(bdi);
>> + ret = bq24190_set_config(bdi);
>> if (ret < 0)
>> return ret;
>> return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>> }
>> +static int bq24190_get_config(struct bq24190_dev_info *bdi)
>> +{
>> +#ifdef CONFIG_OF
>> + int v;
>
>
> This variable is unused (in this patch)
>
>> +
>> + if (!bdi->dev->of_node)
>> + return -EINVAL;
>
>
> I don't think that this is a good idea, at least on ARM
> (AFAIK) a kernel can be configured to support both DT and
> ACPI, just having CONFIG_OF enabled as such is not a good
> reason to fail to probe when there is no of_node.
>
> IMHO it would be better to instead put the reading of the
> "ti,system-minimum-microvolt" property introduced in the next
> patch inside a: if (bdi->dev->of_node) { ... } block.
Well that one is read with device_property_read_ so mebbe I should
drop ifdef config_of and only check dev->of_node before
_get_battery_info() which requires DT.
>> +
>> +#endif
>> + return 0;
>> +}
>> +
>> static int bq24190_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -1493,7 +1510,7 @@ static int bq24190_probe(struct i2c_client *client,
>> i2c_set_clientdata(client, bdi);
>> - if (!client->irq) {
>> + if (client->irq <= 0) {
>> dev_err(dev, "Can't get irq info\n");
>> return -EINVAL;
>> }
>> @@ -1526,12 +1543,6 @@ static int bq24190_probe(struct i2c_client *client,
>> goto out_pmrt;
>> }
>> - ret = bq24190_hw_init(bdi);
>> - if (ret < 0) {
>> - dev_err(dev, "Hardware init failed\n");
>> - goto out_pmrt;
>> - }
>> -
>> charger_cfg.drv_data = bdi;
>> charger_cfg.supplied_to = bq24190_charger_supplied_to;
>> charger_cfg.num_supplicants =
>> ARRAY_SIZE(bq24190_charger_supplied_to),
>> @@ -1556,8 +1567,20 @@ static int bq24190_probe(struct i2c_client *client,
>> }
>> }
>> + ret = bq24190_get_config(bdi);
>> + if (ret < 0) {
>> + dev_err(dev, "Can't get devicetree config\n");
>> + goto out_charger;
>> + }
>> +
>> + ret = bq24190_hw_init(bdi);
>> + if (ret < 0) {
>> + dev_err(dev, "Hardware init failed\n");
>> + goto out_charger;
>> + }
>> +
>> ret = bq24190_sysfs_create_group(bdi);
>> - if (ret) {
>> + if (ret < 0) {
>> dev_err(dev, "Can't create sysfs entries\n");
>> goto out_charger;
>> }
>> @@ -1700,7 +1723,7 @@ static __maybe_unused int bq24190_pm_resume(struct
>> device *dev)
>> }
>> bq24190_register_reset(bdi);
>> - bq24190_set_mode_host(bdi);
>> + bq24190_set_config(bdi);
>> bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>> if (error >= 0) {
>>
>
> Regards,
>
> Hans
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config
2017-08-28 17:59 ` Liam Breck
@ 2017-08-28 19:09 ` Hans de Goede
0 siblings, 0 replies; 13+ messages in thread
From: Hans de Goede @ 2017-08-28 19:09 UTC (permalink / raw)
To: Liam Breck; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck
Hi,
On 28-08-17 19:59, Liam Breck wrote:
> Hi Hans, thanks for the input...
>
> On Mon, Aug 28, 2017 at 5:13 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 26-08-17 07:14, Liam Breck wrote:
>>>
>>> From: Liam Breck <kernel@networkimprov.net>
>>>
>>> Add get_config(). Rename set_mode_host() to set_config().
>>> Call get_config() and hw_init() after power_supply_register().
>>> No functional changes.
>>
>>
>> Doing hw_init after power_supply_register() means that userspace
>> can get/set power_supply properties before hw_init has run, this
>> generally is not a good idea.
>
> hw_init() needs data from DT, and power_supply_get_battery_info()
> (called by get_config() in later patch) gets the DT data. It takes a
> power_supply argument, hence the new ordering.
>
> Is there a way to defer availability of a registered power_supply to
> userspace during probe()?
No I don't think so.
> I recall looking into this when I wrote that
> and concluding that it was safe, but I don't recall why :-/
I guess you came to that conclusion because non of the properties
we export really depend on any of the settings which are setup by
hw_init, so doing that after registering, although unusual should
be fine I guess. So lets just keep this as is.
>
>>
>>>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>> ---
>>> drivers/power/supply/bq24190_charger.c | 63
>>> +++++++++++++++++++++++-----------
>>> 1 file changed, 43 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index 40b4bba7..c6be00d7 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -504,15 +504,7 @@ static int bq24190_sysfs_create_group(struct
>>> bq24190_dev_info *bdi)
>>> static inline void bq24190_sysfs_remove_group(struct bq24190_dev_info
>>> *bdi) {}
>>> #endif
>>> -/*
>>> - * According to the "Host Mode and default Mode" section of the
>>> - * manual, a write to any register causes the bq24190 to switch
>>> - * from default mode to host mode. It will switch back to default
>>> - * mode after a WDT timeout unless the WDT is turned off as well.
>>> - * So, by simply turning off the WDT, we accomplish both with the
>>> - * same write.
>>> - */
>>> -static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>>> +static int bq24190_set_config(struct bq24190_dev_info *bdi)
>>> {
>>> int ret;
>>> u8 v;
>>> @@ -523,9 +515,22 @@ static int bq24190_set_mode_host(struct
>>> bq24190_dev_info *bdi)
>>> bdi->watchdog = ((v & BQ24190_REG_CTTC_WATCHDOG_MASK) >>
>>> BQ24190_REG_CTTC_WATCHDOG_SHIFT);
>>> +
>>> + /*
>>> + * According to the "Host Mode and default Mode" section of the
>>> + * manual, a write to any register causes the bq24190 to switch
>>> + * from default mode to host mode. It will switch back to default
>>> + * mode after a WDT timeout unless the WDT is turned off as well.
>>> + * So, by simply turning off the WDT, we accomplish both with the
>>> + * same write.
>>> + */
>>> v &= ~BQ24190_REG_CTTC_WATCHDOG_MASK;
>>> - return bq24190_write(bdi, BQ24190_REG_CTTC, v);
>>> + ret = bq24190_write(bdi, BQ24190_REG_CTTC, v);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return 0;
>>> }
>>> static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>>> @@ -1456,13 +1461,25 @@ static int bq24190_hw_init(struct bq24190_dev_info
>>> *bdi)
>>> if (ret < 0)
>>> return ret;
>>> - ret = bq24190_set_mode_host(bdi);
>>> + ret = bq24190_set_config(bdi);
>>> if (ret < 0)
>>> return ret;
>>> return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>>> }
>>> +static int bq24190_get_config(struct bq24190_dev_info *bdi)
>>> +{
>>> +#ifdef CONFIG_OF
>>> + int v;
>>
>>
>> This variable is unused (in this patch)
>>
>>> +
>>> + if (!bdi->dev->of_node)
>>> + return -EINVAL;
>>
>>
>> I don't think that this is a good idea, at least on ARM
>> (AFAIK) a kernel can be configured to support both DT and
>> ACPI, just having CONFIG_OF enabled as such is not a good
>> reason to fail to probe when there is no of_node.
>>
>> IMHO it would be better to instead put the reading of the
>> "ti,system-minimum-microvolt" property introduced in the next
>> patch inside a: if (bdi->dev->of_node) { ... } block.
>
> Well that one is read with device_property_read_ so mebbe I should
> drop ifdef config_of and only check dev->of_node before
> _get_battery_info() which requires DT.
Sounds good to me.
Regards,
Hans
>
>>> +
>>> +#endif
>>> + return 0;
>>> +}
>>> +
>>> static int bq24190_probe(struct i2c_client *client,
>>> const struct i2c_device_id *id)
>>> {
>>> @@ -1493,7 +1510,7 @@ static int bq24190_probe(struct i2c_client *client,
>>> i2c_set_clientdata(client, bdi);
>>> - if (!client->irq) {
>>> + if (client->irq <= 0) {
>>> dev_err(dev, "Can't get irq info\n");
>>> return -EINVAL;
>>> }
>>> @@ -1526,12 +1543,6 @@ static int bq24190_probe(struct i2c_client *client,
>>> goto out_pmrt;
>>> }
>>> - ret = bq24190_hw_init(bdi);
>>> - if (ret < 0) {
>>> - dev_err(dev, "Hardware init failed\n");
>>> - goto out_pmrt;
>>> - }
>>> -
>>> charger_cfg.drv_data = bdi;
>>> charger_cfg.supplied_to = bq24190_charger_supplied_to;
>>> charger_cfg.num_supplicants =
>>> ARRAY_SIZE(bq24190_charger_supplied_to),
>>> @@ -1556,8 +1567,20 @@ static int bq24190_probe(struct i2c_client *client,
>>> }
>>> }
>>> + ret = bq24190_get_config(bdi);
>>> + if (ret < 0) {
>>> + dev_err(dev, "Can't get devicetree config\n");
>>> + goto out_charger;
>>> + }
>>> +
>>> + ret = bq24190_hw_init(bdi);
>>> + if (ret < 0) {
>>> + dev_err(dev, "Hardware init failed\n");
>>> + goto out_charger;
>>> + }
>>> +
>>> ret = bq24190_sysfs_create_group(bdi);
>>> - if (ret) {
>>> + if (ret < 0) {
>>> dev_err(dev, "Can't create sysfs entries\n");
>>> goto out_charger;
>>> }
>>> @@ -1700,7 +1723,7 @@ static __maybe_unused int bq24190_pm_resume(struct
>>> device *dev)
>>> }
>>> bq24190_register_reset(bdi);
>>> - bq24190_set_mode_host(bdi);
>>> + bq24190_set_config(bdi);
>>> bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>>> if (error >= 0) {
>>>
>>
>> Regards,
>>
>> Hans
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 4/5] power: bq24190_charger: Add property system-minimum-microvolt
2017-08-26 5:14 [PATCH v3 0/5] BQ24190 devicetree config Liam Breck
` (2 preceding siblings ...)
2017-08-26 5:14 ` [PATCH v3 3/5] power: bq24190_charger: Enable devicetree config Liam Breck
@ 2017-08-26 5:14 ` Liam Breck
2017-08-28 16:01 ` Tony Lindgren
2017-08-26 5:14 ` [PATCH v3 5/5] power: bq24190_charger: Add power_supply_battery_info support Liam Breck
4 siblings, 1 reply; 13+ messages in thread
From: Liam Breck @ 2017-08-26 5:14 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, Liam Breck
From: Liam Breck <kernel@networkimprov.net>
Set minimum system voltage limit obtained from device property.
Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
drivers/power/supply/bq24190_charger.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index c6be00d7..450f3ab3 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -43,6 +43,8 @@
#define BQ24190_REG_POC_CHG_CONFIG_OTG 0x2
#define BQ24190_REG_POC_SYS_MIN_MASK (BIT(3) | BIT(2) | BIT(1))
#define BQ24190_REG_POC_SYS_MIN_SHIFT 1
+#define BQ24190_REG_POC_SYS_MIN_MIN 3000
+#define BQ24190_REG_POC_SYS_MIN_MAX 3700
#define BQ24190_REG_POC_BOOST_LIM_MASK BIT(0)
#define BQ24190_REG_POC_BOOST_LIM_SHIFT 0
@@ -159,6 +161,7 @@ struct bq24190_dev_info {
char model_name[I2C_NAME_SIZE];
bool initialized;
bool irq_event;
+ u16 sys_min;
struct mutex f_reg_lock;
u8 f_reg;
u8 ss_reg;
@@ -530,6 +533,16 @@ static int bq24190_set_config(struct bq24190_dev_info *bdi)
if (ret < 0)
return ret;
+ if (bdi->sys_min) {
+ v = bdi->sys_min / 100 - 30; // manual section 9.5.1.2, table 9
+ ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
+ BQ24190_REG_POC_SYS_MIN_MASK,
+ BQ24190_REG_POC_SYS_MIN_SHIFT,
+ v);
+ if (ret < 0)
+ return ret;
+ }
+
return 0;
}
@@ -1471,11 +1484,21 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
static int bq24190_get_config(struct bq24190_dev_info *bdi)
{
#ifdef CONFIG_OF
+ const char * const s = "ti,system-minimum-microvolt";
int v;
if (!bdi->dev->of_node)
return -EINVAL;
+ if (device_property_read_u32(bdi->dev, s, &v) == 0) {
+ v /= 1000;
+ if (v >= BQ24190_REG_POC_SYS_MIN_MIN
+ && v <= BQ24190_REG_POC_SYS_MIN_MAX)
+ bdi->sys_min = v;
+ else
+ dev_warn(bdi->dev, "invalid value for %s: %u\n", s, v);
+ }
+
#endif
return 0;
}
--
2.13.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 5/5] power: bq24190_charger: Add power_supply_battery_info support
2017-08-26 5:14 [PATCH v3 0/5] BQ24190 devicetree config Liam Breck
` (3 preceding siblings ...)
2017-08-26 5:14 ` [PATCH v3 4/5] power: bq24190_charger: Add property system-minimum-microvolt Liam Breck
@ 2017-08-26 5:14 ` Liam Breck
2017-08-28 16:02 ` Tony Lindgren
4 siblings, 1 reply; 13+ messages in thread
From: Liam Breck @ 2017-08-26 5:14 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, Liam Breck
From: Liam Breck <kernel@networkimprov.net>
Set pre-charge and charge-term current, obtained from power_supply_battery_info.
Add sysfs attributes precharge_current & charge_term_current.
Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
drivers/power/supply/bq24190_charger.c | 86 ++++++++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 450f3ab3..7681a67a 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -59,9 +59,13 @@
#define BQ24190_REG_PCTCC_IPRECHG_MASK (BIT(7) | BIT(6) | BIT(5) | \
BIT(4))
#define BQ24190_REG_PCTCC_IPRECHG_SHIFT 4
+#define BQ24190_REG_PCTCC_IPRECHG_MIN 128
+#define BQ24190_REG_PCTCC_IPRECHG_MAX 2048
#define BQ24190_REG_PCTCC_ITERM_MASK (BIT(3) | BIT(2) | BIT(1) | \
BIT(0))
#define BQ24190_REG_PCTCC_ITERM_SHIFT 0
+#define BQ24190_REG_PCTCC_ITERM_MIN 128
+#define BQ24190_REG_PCTCC_ITERM_MAX 2048
#define BQ24190_REG_CVC 0x04 /* Charge Voltage Control */
#define BQ24190_REG_CVC_VREG_MASK (BIT(7) | BIT(6) | BIT(5) | \
@@ -162,6 +166,8 @@ struct bq24190_dev_info {
bool initialized;
bool irq_event;
u16 sys_min;
+ u16 iprechg;
+ u16 iterm;
struct mutex f_reg_lock;
u8 f_reg;
u8 ss_reg;
@@ -543,6 +549,26 @@ static int bq24190_set_config(struct bq24190_dev_info *bdi)
return ret;
}
+ if (bdi->iprechg) {
+ v = bdi->iprechg / 128 - 1; // manual section 9.5.1.4, table 11
+ ret = bq24190_write_mask(bdi, BQ24190_REG_PCTCC,
+ BQ24190_REG_PCTCC_IPRECHG_MASK,
+ BQ24190_REG_PCTCC_IPRECHG_SHIFT,
+ v);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (bdi->iterm) {
+ v = bdi->iterm / 128 - 1; // manual section 9.5.1.4, table 11
+ ret = bq24190_write_mask(bdi, BQ24190_REG_PCTCC,
+ BQ24190_REG_PCTCC_ITERM_MASK,
+ BQ24190_REG_PCTCC_ITERM_SHIFT,
+ v);
+ if (ret < 0)
+ return ret;
+ }
+
return 0;
}
@@ -787,6 +813,38 @@ static int bq24190_charger_set_temp_alert_max(struct bq24190_dev_info *bdi,
return bq24190_battery_set_temp_alert_max(bdi, val);
}
+static int bq24190_charger_get_precharge(struct bq24190_dev_info *bdi,
+ union power_supply_propval *val)
+{
+ u8 v;
+ int ret;
+
+ ret = bq24190_read_mask(bdi, BQ24190_REG_PCTCC,
+ BQ24190_REG_PCTCC_IPRECHG_MASK,
+ BQ24190_REG_PCTCC_IPRECHG_SHIFT, &v);
+ if (ret < 0)
+ return ret;
+
+ val->intval = ++v * 128 * 1000;
+ return 0;
+}
+
+static int bq24190_charger_get_charge_term(struct bq24190_dev_info *bdi,
+ union power_supply_propval *val)
+{
+ u8 v;
+ int ret;
+
+ ret = bq24190_read_mask(bdi, BQ24190_REG_PCTCC,
+ BQ24190_REG_PCTCC_ITERM_MASK,
+ BQ24190_REG_PCTCC_ITERM_SHIFT, &v);
+ if (ret < 0)
+ return ret;
+
+ val->intval = ++v * 128 * 1000;
+ return 0;
+}
+
static int bq24190_charger_get_current(struct bq24190_dev_info *bdi,
union power_supply_propval *val)
{
@@ -907,6 +965,12 @@ static int bq24190_charger_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
ret = bq24190_charger_get_temp_alert_max(bdi, val);
break;
+ case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
+ ret = bq24190_charger_get_precharge(bdi, val);
+ break;
+ case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+ ret = bq24190_charger_get_charge_term(bdi, val);
+ break;
case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
ret = bq24190_charger_get_current(bdi, val);
break;
@@ -1006,6 +1070,8 @@ static enum power_supply_property bq24190_charger_properties[] = {
POWER_SUPPLY_PROP_ONLINE,
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
+ POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
+ POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
@@ -1485,6 +1551,7 @@ static int bq24190_get_config(struct bq24190_dev_info *bdi)
{
#ifdef CONFIG_OF
const char * const s = "ti,system-minimum-microvolt";
+ struct power_supply_battery_info info = {};
int v;
if (!bdi->dev->of_node)
@@ -1499,6 +1566,24 @@ static int bq24190_get_config(struct bq24190_dev_info *bdi)
dev_warn(bdi->dev, "invalid value for %s: %u\n", s, v);
}
+ if (!power_supply_get_battery_info(bdi->charger, &info)) {
+ v = info.precharge_current_ua / 1000;
+ if (v >= BQ24190_REG_PCTCC_IPRECHG_MIN
+ && v <= BQ24190_REG_PCTCC_IPRECHG_MAX)
+ bdi->iprechg = v;
+ else
+ dev_warn(bdi->dev, "invalid value for battery:precharge-current-microamp: %d\n",
+ v);
+
+ v = info.charge_term_current_ua / 1000;
+ if (v >= BQ24190_REG_PCTCC_ITERM_MIN
+ && v <= BQ24190_REG_PCTCC_ITERM_MAX)
+ bdi->iterm = v;
+ else
+ dev_warn(bdi->dev, "invalid value for battery:charge-term-current-microamp: %d\n",
+ v);
+ }
+
#endif
return 0;
}
@@ -1567,6 +1652,7 @@ static int bq24190_probe(struct i2c_client *client,
}
charger_cfg.drv_data = bdi;
+ charger_cfg.of_node = dev->of_node;
charger_cfg.supplied_to = bq24190_charger_supplied_to;
charger_cfg.num_supplicants = ARRAY_SIZE(bq24190_charger_supplied_to),
bdi->charger = power_supply_register(dev, &bq24190_charger_desc,
--
2.13.2
^ permalink raw reply related [flat|nested] 13+ messages in thread