Hi Hans, thanks for the input...

On Mon, Aug 28, 2017 at 5:13 AM, Hans de Goede [off-list ref] wrote:

Hi,

On 26-08-17 07:14, Liam Breck wrote:

quoted

From: Liam Breck <redacted>

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 :-/

quoted

Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <redacted>
Signed-off-by: Liam Breck <redacted>
---
  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)

quoted

+
+       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.

quoted

+
+#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