[PATCH v4 0/4] BQ24190 charger fixes

[PATCH v4 0/4] BQ24190 charger fixes

[Liam Breck]

Hi Sebastian,

Here in one patchset are two patches I posted recently, and two more:

Patch 1 is a replacement (as agreed) for
  "power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug"
Patch 2 fixes up Hans' extcon patch. If you would like me to squash it with his
  original patch, let me know.
Patch 3 (repost) fixes some issues with PM runtime error handling.
Patch 4 fixes a nit in register_reset().

Changes in v4:
* fix definition of bq24190_reg_poc_chg_config_*
* revert to if (device_property_read... == 0)

Liam Breck (4):
  power: bq24190_charger: Limit over/under voltage fault logging
  power: bq24190_charger: Clean up extcon code
  power: bq24190_charger: Uniform pm_runtime_get() failure handling
  power: bq24190_charger: Delay before polling reset flag

 drivers/power/supply/bq24190_charger.c | 125 ++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 63 deletions(-)

-- 
2.9.3

[PATCH v4 1/4] power: bq24190_charger: Limit over/under voltage fault logging

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

If the charger is unplugged before the battery is full we may
see an over/under voltage fault. Ignore this rather then emitting
a message or uevent.

This prevents messages like these on charger unplug/replug:
bq24190-charger 15-006b: Fault: boost 0, charge 1, battery 0, ntc 0
bq24190-charger 15-006b: Fault: boost 0, charge 0, battery 0, ntc 0

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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index a699ad3..64a48b9 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1222,8 +1222,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 		}
 	} while (f_reg && ++i < 2);
 
+	/* ignore over/under voltage fault after disconnect */
+	if (f_reg == (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT) &&
+	    !(ss_reg & BQ24190_REG_SS_PG_STAT_MASK))
+		f_reg = 0;
+
 	if (f_reg != bdi->f_reg) {
-		dev_info(bdi->dev,
+		dev_warn(bdi->dev,
 			"Fault: boost %d, charge %d, battery %d, ntc %d\n",
 			!!(f_reg & BQ24190_REG_F_BOOST_FAULT_MASK),
 			!!(f_reg & BQ24190_REG_F_CHRG_FAULT_MASK),
-- 
2.9.3

[PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Polishing and fixes for initial extcon patch.

Fixes: 4db249b6f3b4 ("power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost")
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 | 73 ++++++++++++++++------------------
 1 file changed, 35 insertions(+), 38 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 64a48b9..10e5324 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -38,8 +38,9 @@
 #define BQ24190_REG_POC_WDT_RESET_SHIFT		6
 #define BQ24190_REG_POC_CHG_CONFIG_MASK		(BIT(5) | BIT(4))
 #define BQ24190_REG_POC_CHG_CONFIG_SHIFT	4
-#define BQ24190_REG_POC_CHG_CONFIG_CHARGE	1
-#define BQ24190_REG_POC_CHG_CONFIG_OTG		2
+#define BQ24190_REG_POC_CHG_CONFIG_DISABLE		0x0
+#define BQ24190_REG_POC_CHG_CONFIG_CHARGE		0x1
+#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_BOOST_LIM_MASK		BIT(0)
@@ -173,8 +174,9 @@ struct bq24190_dev_info {
  */
 
 /* REG00[2:0] (IINLIM) in uAh */
-static const int bq24190_iinlim_values[] = {
-	100000, 150000, 500000, 900000, 1200000, 1500000, 2000000, 3000000 };
+static const int bq24190_isc_iinlim_values[] = {
+	 100000,  150000,  500000,  900000, 1200000, 1500000, 2000000, 3000000
+};
 
 /* REG02[7:2] (ICHG) in uAh */
 static const int bq24190_ccc_ichg_values[] = {
@@ -1298,16 +1300,17 @@ static void bq24190_extcon_work(struct work_struct *work)
 {
 	struct bq24190_dev_info *bdi =
 		container_of(work, struct bq24190_dev_info, extcon_work.work);
-	int ret, iinlim = 0;
+	int error, iinlim = 0;
 
-	ret = pm_runtime_get_sync(bdi->dev);
-	if (ret < 0) {
-		dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
 		return;
 	}
 
-	if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
-		iinlim = 500000;
+	if      (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
+		iinlim =  500000;
 	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
 		 extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
 		iinlim = 1500000;
@@ -1315,33 +1318,25 @@ static void bq24190_extcon_work(struct work_struct *work)
 		iinlim = 2000000;
 
 	if (iinlim) {
-		ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
-				BQ24190_REG_ISC_IINLIM_MASK,
-				BQ24190_REG_ISC_IINLIM_SHIFT,
-				bq24190_iinlim_values,
-				ARRAY_SIZE(bq24190_iinlim_values),
-				iinlim);
-		if (ret)
-			dev_err(bdi->dev, "Can't set IINLIM: %d\n", ret);
+		error = 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),
+					      iinlim);
+		if (error < 0)
+			dev_err(bdi->dev, "Can't set IINLIM: %d\n", error);
 	}
 
-	/*
-	 * If no charger has been detected and host mode is requested, activate
-	 * the 5V boost converter, otherwise deactivate it.
-	 */
-	if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) {
-		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);
-	} else {
-		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);
-	}
-	if (ret)
-		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
+	/* Set OTG 5V boost: on if no charger detected and in USB host mode, else off */
+	error = bq24190_write_mask(bdi, BQ24190_REG_POC,
+				   BQ24190_REG_POC_CHG_CONFIG_MASK,
+				   BQ24190_REG_POC_CHG_CONFIG_SHIFT,
+			!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1
+				 ? BQ24190_REG_POC_CHG_CONFIG_OTG
+				 : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
+	if (error < 0)
+		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
 
 	pm_runtime_mark_last_busy(bdi->dev);
 	pm_runtime_put_autosuspend(bdi->dev);
@@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	/*
-	 * The extcon-name property is purely an in kernel interface for
-	 * x86/ACPI use, DT platforms should get extcon via phandle.
+	 * ACPI platforms should use device_property "extcon-name".
+	 * Devicetree platforms should get extcon via phandle (not yet supported).
 	 */
 	if (device_property_read_string(dev, "extcon-name", &name) == 0) {
 		bdi->extcon = extcon_get_extcon_dev(name);
@@ -1498,8 +1493,10 @@ static int bq24190_probe(struct i2c_client *client,
 		bdi->extcon_nb.notifier_call = bq24190_extcon_event;
 		ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
 						    &bdi->extcon_nb);
-		if (ret)
+		if (ret) {
+			dev_err(dev, "Can't register extcon\n");
 			goto out5;
+		}
 
 		/* Sync initial cable state */
 		queue_delayed_work(system_wq, &bdi->extcon_work, 0);
-- 
2.9.3

[PATCH v4 3/4] power: bq24190_charger: Uniform pm_runtime_get() failure handling

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

On pm_runtime_get() failure, always emit an error message.
Prevent unbalanced pm_runtime_get by calling:
  pm_runtime_put_noidle() in irq handler
  pm_runtime_put_sync() on any probe() failure
Rename probe() out labels instead of renumbering them.

Fixes: 13d6fa8447fa ("power: bq24190_charger: Use PM runtime autosuspend")
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 | 37 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 10e5324..fa29cb3 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1280,12 +1280,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
-	int ret;
+	int error;
 
 	bdi->irq_event = true;
-	ret = pm_runtime_get_sync(bdi->dev);
-	if (ret < 0) {
-		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
 		return IRQ_NONE;
 	}
 	bq24190_check_status(bdi);
@@ -1442,13 +1443,15 @@ static int bq24190_probe(struct i2c_client *client,
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, 600);
 	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		goto out1;
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get failed: %i\n", ret);
+		goto out_pmrt;
+	}
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out2;
+		goto out_pmrt;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1459,7 +1462,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out2;
+		goto out_pmrt;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1468,13 +1471,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out3;
+		goto out_charger;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out4;
+		goto out_battery;
 	}
 
 	bdi->initialized = true;
@@ -1485,7 +1488,7 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out5;
+		goto out_sysfs;
 	}
 
 	if (bdi->extcon) {
@@ -1495,7 +1498,7 @@ static int bq24190_probe(struct i2c_client *client,
 						    &bdi->extcon_nb);
 		if (ret) {
 			dev_err(dev, "Can't register extcon\n");
-			goto out5;
+			goto out_sysfs;
 		}
 
 		/* Sync initial cable state */
@@ -1509,19 +1512,17 @@ static int bq24190_probe(struct i2c_client *client,
 
 	return 0;
 
-out5:
+out_sysfs:
 	bq24190_sysfs_remove_group(bdi);
 
-out4:
+out_battery:
 	power_supply_unregister(bdi->battery);
 
-out3:
+out_charger:
 	power_supply_unregister(bdi->charger);
 
-out2:
+out_pmrt:
 	pm_runtime_put_sync(dev);
-
-out1:
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	return ret;
-- 
2.9.3

[PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

On chip reset, polling loop was reading reset register immediately.
Instead, call udelay() before reading chip register.

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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index fa29cb3..71a796e 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -543,18 +543,14 @@ static int bq24190_register_reset(struct bq24190_dev_info *bdi)
 
 	/* Reset bit will be cleared by hardware so poll until it is */
 	do {
+		udelay(10);
 		ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
 				BQ24190_REG_POC_RESET_MASK,
 				BQ24190_REG_POC_RESET_SHIFT,
 				&v);
 		if (ret < 0)
 			return ret;
-
-		if (!v)
-			break;
-
-		udelay(10);
-	} while (--limit);
+	} while (v && --limit);
 
 	if (!limit)
 		return -EIO;
-- 
2.9.3

Re: [PATCH v4 1/4] power: bq24190_charger: Limit over/under voltage fault logging

[Hans de Goede]

Hi,

On 06-04-17 05:10, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> If the charger is unplugged before the battery is full we may
> see an over/under voltage fault. Ignore this rather then emitting
> a message or uevent.
>
> This prevents messages like these on charger unplug/replug:
> bq24190-charger 15-006b: Fault: boost 0, charge 1, battery 0, ntc 0
> bq24190-charger 15-006b: Fault: boost 0, charge 0, battery 0, ntc 0
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  drivers/power/supply/bq24190_charger.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index a699ad3..64a48b9 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1222,8 +1222,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  		}
>  	} while (f_reg && ++i < 2);
>
> +	/* ignore over/under voltage fault after disconnect */
> +	if (f_reg == (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT) &&
> +	    !(ss_reg & BQ24190_REG_SS_PG_STAT_MASK))
> +		f_reg = 0;
> +
>  	if (f_reg != bdi->f_reg) {
> -		dev_info(bdi->dev,
> +		dev_warn(bdi->dev,
>  			"Fault: boost %d, charge %d, battery %d, ntc %d\n",
>  			!!(f_reg & BQ24190_REG_F_BOOST_FAULT_MASK),
>  			!!(f_reg & BQ24190_REG_F_CHRG_FAULT_MASK),
>

Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Hans de Goede]

Hi,

On 06-04-17 05:10, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> Polishing and fixes for initial extcon patch.
>
> Fixes: 4db249b6f3b4 ("power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost")
> 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 | 73 ++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 64a48b9..10e5324 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -38,8 +38,9 @@
>  #define BQ24190_REG_POC_WDT_RESET_SHIFT		6
>  #define BQ24190_REG_POC_CHG_CONFIG_MASK		(BIT(5) | BIT(4))
>  #define BQ24190_REG_POC_CHG_CONFIG_SHIFT	4
> -#define BQ24190_REG_POC_CHG_CONFIG_CHARGE	1
> -#define BQ24190_REG_POC_CHG_CONFIG_OTG		2
> +#define BQ24190_REG_POC_CHG_CONFIG_DISABLE		0x0
> +#define BQ24190_REG_POC_CHG_CONFIG_CHARGE		0x1
> +#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_BOOST_LIM_MASK		BIT(0)
> @@ -173,8 +174,9 @@ struct bq24190_dev_info {
>   */
>
>  /* REG00[2:0] (IINLIM) in uAh */
> -static const int bq24190_iinlim_values[] = {
> -	100000, 150000, 500000, 900000, 1200000, 1500000, 2000000, 3000000 };
> +static const int bq24190_isc_iinlim_values[] = {
> +	 100000,  150000,  500000,  900000, 1200000, 1500000, 2000000, 3000000
> +};
>
>  /* REG02[7:2] (ICHG) in uAh */
>  static const int bq24190_ccc_ichg_values[] = {
> @@ -1298,16 +1300,17 @@ static void bq24190_extcon_work(struct work_struct *work)
>  {
>  	struct bq24190_dev_info *bdi =
>  		container_of(work, struct bq24190_dev_info, extcon_work.work);
> -	int ret, iinlim = 0;
> +	int error, iinlim = 0;
>
> -	ret = pm_runtime_get_sync(bdi->dev);
> -	if (ret < 0) {
> -		dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret);
> +	error = pm_runtime_get_sync(bdi->dev);
> +	if (error < 0) {
> +		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +		pm_runtime_put_noidle(bdi->dev);
>  		return;
>  	}
>
> -	if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
> -		iinlim = 500000;
> +	if      (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
> +		iinlim =  500000;
>  	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
>  		 extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
>  		iinlim = 1500000;
> @@ -1315,33 +1318,25 @@ static void bq24190_extcon_work(struct work_struct *work)
>  		iinlim = 2000000;
>
>  	if (iinlim) {
> -		ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
> -				BQ24190_REG_ISC_IINLIM_MASK,
> -				BQ24190_REG_ISC_IINLIM_SHIFT,
> -				bq24190_iinlim_values,
> -				ARRAY_SIZE(bq24190_iinlim_values),
> -				iinlim);
> -		if (ret)
> -			dev_err(bdi->dev, "Can't set IINLIM: %d\n", ret);
> +		error = 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),
> +					      iinlim);
> +		if (error < 0)
> +			dev_err(bdi->dev, "Can't set IINLIM: %d\n", error);
>  	}

The above changes are fine with me,

> -	/*
> -	 * If no charger has been detected and host mode is requested, activate
> -	 * the 5V boost converter, otherwise deactivate it.
> -	 */
> -	if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) {
> -		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);
> -	} else {
> -		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);
> -	}
> -	if (ret)
> -		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
> +	/* Set OTG 5V boost: on if no charger detected and in USB host mode, else off */
> +	error = bq24190_write_mask(bdi, BQ24190_REG_POC,
> +				   BQ24190_REG_POC_CHG_CONFIG_MASK,
> +				   BQ24190_REG_POC_CHG_CONFIG_SHIFT,
> +			!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1
> +				 ? BQ24190_REG_POC_CHG_CONFIG_OTG
> +				 : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
> +	if (error < 0)
> +		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>
>  	pm_runtime_mark_last_busy(bdi->dev);
>  	pm_runtime_put_autosuspend(bdi->dev);

I still find the refactored code you're proposing here a lot less readable
then the original code, and the same goes for the comment. If you insist
in changing this, then I suggest changing it into this:


	/*
	 * If no charger has been detected and host mode is requested, activate
	 * the 5V boost converter, otherwise deactivate it.
	 */
	if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
		chg_config = BQ24190_REG_POC_CHG_CONFIG_OTG;
	else
		chg_config = BQ24190_REG_POC_CHG_CONFIG_CHARGE;

	error = bq24190_write_mask(bdi, BQ24190_REG_POC,
				   BQ24190_REG_POC_CHG_CONFIG_MASK,
				   BQ24190_REG_POC_CHG_CONFIG_SHIFT,
				   chg_config);
	if (error)
		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);

> @@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client *client,
>  	}
>
>  	/*
> -	 * The extcon-name property is purely an in kernel interface for
> -	 * x86/ACPI use, DT platforms should get extcon via phandle.
> +	 * ACPI platforms should use device_property "extcon-name".
> +	 * Devicetree platforms should get extcon via phandle (not yet supported).
>  	 */
>  	if (device_property_read_string(dev, "extcon-name", &name) == 0) {
>  		bdi->extcon = extcon_get_extcon_dev(name);

The new comment suggest that extcon-name is something which should actually
be used inside ACPI DSDT tables, which it is not, please leave the comment
as is.

> @@ -1498,8 +1493,10 @@ static int bq24190_probe(struct i2c_client *client,
>  		bdi->extcon_nb.notifier_call = bq24190_extcon_event;
>  		ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
>  						    &bdi->extcon_nb);
> -		if (ret)
> +		if (ret) {
> +			dev_err(dev, "Can't register extcon\n");
>  			goto out5;
> +		}
>
>  		/* Sync initial cable state */
>  		queue_delayed_work(system_wq, &bdi->extcon_work, 0);
>


Regards,

Hans

Re: [PATCH v4 3/4] power: bq24190_charger: Uniform pm_runtime_get() failure handling

[Hans de Goede]

Hi,

On 06-04-17 05:10, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> On pm_runtime_get() failure, always emit an error message.
> Prevent unbalanced pm_runtime_get by calling:
>   pm_runtime_put_noidle() in irq handler
>   pm_runtime_put_sync() on any probe() failure
> Rename probe() out labels instead of renumbering them.
>
> Fixes: 13d6fa8447fa ("power: bq24190_charger: Use PM runtime autosuspend")
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/power/supply/bq24190_charger.c | 37 +++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 10e5324..fa29cb3 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1280,12 +1280,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>  	struct bq24190_dev_info *bdi = data;
> -	int ret;
> +	int error;
>
>  	bdi->irq_event = true;
> -	ret = pm_runtime_get_sync(bdi->dev);
> -	if (ret < 0) {
> -		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
> +	error = pm_runtime_get_sync(bdi->dev);
> +	if (error < 0) {
> +		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +		pm_runtime_put_noidle(bdi->dev);
>  		return IRQ_NONE;
>  	}
>  	bq24190_check_status(bdi);
> @@ -1442,13 +1443,15 @@ static int bq24190_probe(struct i2c_client *client,
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_set_autosuspend_delay(dev, 600);
>  	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> -		goto out1;
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get failed: %i\n", ret);
> +		goto out_pmrt;
> +	}
>
>  	ret = bq24190_hw_init(bdi);
>  	if (ret < 0) {
>  		dev_err(dev, "Hardware init failed\n");
> -		goto out2;
> +		goto out_pmrt;
>  	}
>
>  	charger_cfg.drv_data = bdi;
> @@ -1459,7 +1462,7 @@ static int bq24190_probe(struct i2c_client *client,
>  	if (IS_ERR(bdi->charger)) {
>  		dev_err(dev, "Can't register charger\n");
>  		ret = PTR_ERR(bdi->charger);
> -		goto out2;
> +		goto out_pmrt;
>  	}
>
>  	battery_cfg.drv_data = bdi;
> @@ -1468,13 +1471,13 @@ static int bq24190_probe(struct i2c_client *client,
>  	if (IS_ERR(bdi->battery)) {
>  		dev_err(dev, "Can't register battery\n");
>  		ret = PTR_ERR(bdi->battery);
> -		goto out3;
> +		goto out_charger;
>  	}
>
>  	ret = bq24190_sysfs_create_group(bdi);
>  	if (ret) {
>  		dev_err(dev, "Can't create sysfs entries\n");
> -		goto out4;
> +		goto out_battery;
>  	}
>
>  	bdi->initialized = true;
> @@ -1485,7 +1488,7 @@ static int bq24190_probe(struct i2c_client *client,
>  			"bq24190-charger", bdi);
>  	if (ret < 0) {
>  		dev_err(dev, "Can't set up irq handler\n");
> -		goto out5;
> +		goto out_sysfs;
>  	}
>
>  	if (bdi->extcon) {
> @@ -1495,7 +1498,7 @@ static int bq24190_probe(struct i2c_client *client,
>  						    &bdi->extcon_nb);
>  		if (ret) {
>  			dev_err(dev, "Can't register extcon\n");
> -			goto out5;
> +			goto out_sysfs;
>  		}
>
>  		/* Sync initial cable state */
> @@ -1509,19 +1512,17 @@ static int bq24190_probe(struct i2c_client *client,
>
>  	return 0;
>
> -out5:
> +out_sysfs:
>  	bq24190_sysfs_remove_group(bdi);
>
> -out4:
> +out_battery:
>  	power_supply_unregister(bdi->battery);
>
> -out3:
> +out_charger:
>  	power_supply_unregister(bdi->charger);
>
> -out2:
> +out_pmrt:
>  	pm_runtime_put_sync(dev);
> -
> -out1:
>  	pm_runtime_dont_use_autosuspend(dev);
>  	pm_runtime_disable(dev);
>  	return ret;
>

Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Hans de Goede]

Hi,

On 06-04-17 05:10, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> On chip reset, polling loop was reading reset register immediately.
> Instead, call udelay() before reading chip register.

Can you extend the commit message to explain why udelay is being
called ?  IOW what is the problem with reading the register
immediately which this patch addresses ?

Regards,

Hans



>
> 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 | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index fa29cb3..71a796e 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -543,18 +543,14 @@ static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>
>  	/* Reset bit will be cleared by hardware so poll until it is */
>  	do {
> +		udelay(10);
>  		ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>  				BQ24190_REG_POC_RESET_MASK,
>  				BQ24190_REG_POC_RESET_SHIFT,
>  				&v);
>  		if (ret < 0)
>  			return ret;
> -
> -		if (!v)
> -			break;
> -
> -		udelay(10);
> -	} while (--limit);
> +	} while (v && --limit);
>
>  	if (!limit)
>  		return -EIO;
>

Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Liam Breck]

Hi Hans,

On Thu, Apr 6, 2017 at 12:20 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 06-04-17 05:10, Liam Breck wrote:
>>
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> On chip reset, polling loop was reading reset register immediately.
>> Instead, call udelay() before reading chip register.
>
>
> Can you extend the commit message to explain why udelay is being
> called ?  IOW what is the problem with reading the register
> immediately which this patch addresses ?

I thought I'd read that reset isn't instantaneous, but in testing it
seems to be. So I will revert to the original post-reset delay, and
change udelay() to usleep_range(10, 20). Any thoughts on those
numbers?



>
>
>
>
>>
>> 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 | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c
>> b/drivers/power/supply/bq24190_charger.c
>> index fa29cb3..71a796e 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -543,18 +543,14 @@ static int bq24190_register_reset(struct
>> bq24190_dev_info *bdi)
>>
>>         /* Reset bit will be cleared by hardware so poll until it is */
>>         do {
>> +               udelay(10);
>>                 ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>>                                 BQ24190_REG_POC_RESET_MASK,
>>                                 BQ24190_REG_POC_RESET_SHIFT,
>>                                 &v);
>>                 if (ret < 0)
>>                         return ret;
>> -
>> -               if (!v)
>> -                       break;
>> -
>> -               udelay(10);
>> -       } while (--limit);
>> +       } while (v && --limit);
>>
>>         if (!limit)
>>                 return -EIO;
>>
>

Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Liam Breck]

Hallo Hans,

On Thu, Apr 6, 2017 at 12:17 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 06-04-17 05:10, Liam Breck wrote:
>>
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Polishing and fixes for initial extcon patch.
>>
>> Fixes: 4db249b6f3b4 ("power: supply: bq24190_charger: Use extcon to
>> determine ilimit, 5v boost")
>> 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 | 73
>> ++++++++++++++++------------------
>>  1 file changed, 35 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c
>> b/drivers/power/supply/bq24190_charger.c
>> index 64a48b9..10e5324 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -38,8 +38,9 @@
>>  #define BQ24190_REG_POC_WDT_RESET_SHIFT                6
>>  #define BQ24190_REG_POC_CHG_CONFIG_MASK                (BIT(5) | BIT(4))
>>  #define BQ24190_REG_POC_CHG_CONFIG_SHIFT       4
>> -#define BQ24190_REG_POC_CHG_CONFIG_CHARGE      1
>> -#define BQ24190_REG_POC_CHG_CONFIG_OTG         2
>> +#define BQ24190_REG_POC_CHG_CONFIG_DISABLE             0x0
>> +#define BQ24190_REG_POC_CHG_CONFIG_CHARGE              0x1
>> +#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_BOOST_LIM_MASK         BIT(0)
>> @@ -173,8 +174,9 @@ struct bq24190_dev_info {
>>   */
>>
>>  /* REG00[2:0] (IINLIM) in uAh */
>> -static const int bq24190_iinlim_values[] = {
>> -       100000, 150000, 500000, 900000, 1200000, 1500000, 2000000, 3000000
>> };
>> +static const int bq24190_isc_iinlim_values[] = {
>> +        100000,  150000,  500000,  900000, 1200000, 1500000, 2000000,
>> 3000000
>> +};
>>
>>  /* REG02[7:2] (ICHG) in uAh */
>>  static const int bq24190_ccc_ichg_values[] = {
>> @@ -1298,16 +1300,17 @@ static void bq24190_extcon_work(struct work_struct
>> *work)
>>  {
>>         struct bq24190_dev_info *bdi =
>>                 container_of(work, struct bq24190_dev_info,
>> extcon_work.work);
>> -       int ret, iinlim = 0;
>> +       int error, iinlim = 0;
>>
>> -       ret = pm_runtime_get_sync(bdi->dev);
>> -       if (ret < 0) {
>> -               dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n",
>> ret);
>> +       error = pm_runtime_get_sync(bdi->dev);
>> +       if (error < 0) {
>> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
>> +               pm_runtime_put_noidle(bdi->dev);
>>                 return;
>>         }
>>
>> -       if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
>> -               iinlim = 500000;
>> +       if      (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
>> +               iinlim =  500000;
>>         else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
>>                  extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
>>                 iinlim = 1500000;
>> @@ -1315,33 +1318,25 @@ static void bq24190_extcon_work(struct work_struct
>> *work)
>>                 iinlim = 2000000;
>>
>>         if (iinlim) {
>> -               ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
>> -                               BQ24190_REG_ISC_IINLIM_MASK,
>> -                               BQ24190_REG_ISC_IINLIM_SHIFT,
>> -                               bq24190_iinlim_values,
>> -                               ARRAY_SIZE(bq24190_iinlim_values),
>> -                               iinlim);
>> -               if (ret)
>> -                       dev_err(bdi->dev, "Can't set IINLIM: %d\n", ret);
>> +               error = 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),
>> +                                             iinlim);
>> +               if (error < 0)
>> +                       dev_err(bdi->dev, "Can't set IINLIM: %d\n",
>> error);
>>         }
>
>
> The above changes are fine with me,

High-five :-)

>> -       /*
>> -        * If no charger has been detected and host mode is requested,
>> activate
>> -        * the 5V boost converter, otherwise deactivate it.
>> -        */
>> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) ==
>> 1) {
>> -               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);
>> -       } else {
>> -               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);
>> -       }
>> -       if (ret)
>> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
>> +       /* Set OTG 5V boost: on if no charger detected and in USB host
>> mode, else off */
>> +       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>> +                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
>> +                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>> +                       !iinlim && extcon_get_state(bdi->extcon,
>> EXTCON_USB_HOST) == 1
>> +                                ? BQ24190_REG_POC_CHG_CONFIG_OTG
>> +                                : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
>> +       if (error < 0)
>> +               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>
>>         pm_runtime_mark_last_busy(bdi->dev);
>>         pm_runtime_put_autosuspend(bdi->dev);
>
>
> I still find the refactored code you're proposing here a lot less readable
> then the original code, and the same goes for the comment. If you insist
> in changing this, then I suggest changing it into this:

OK. I'll collapse the comment to one line tho.

>         /*
>          * If no charger has been detected and host mode is requested,
> activate
>          * the 5V boost converter, otherwise deactivate it.
>          */
>         if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_OTG;
>         else
>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
>
>         error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>                                    BQ24190_REG_POC_CHG_CONFIG_MASK,
>                                    BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>                                    chg_config);
>         if (error)
>                 dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>
>> @@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client *client,
>>         }
>>
>>         /*
>> -        * The extcon-name property is purely an in kernel interface for
>> -        * x86/ACPI use, DT platforms should get extcon via phandle.
>> +        * ACPI platforms should use device_property "extcon-name".
>> +        * Devicetree platforms should get extcon via phandle (not yet
>> supported).
>>          */
>>         if (device_property_read_string(dev, "extcon-name", &name) == 0) {
>>                 bdi->extcon = extcon_get_extcon_dev(name);
>
>
> The new comment suggest that extcon-name is something which should actually
> be used inside ACPI DSDT tables, which it is not, please leave the comment
> as is.

OK, right. But "in kernel interface" sounds official, and this is
really a custom msg passing method. A platform-data struct is
documented in a header, and a module parameter has its own docs
scheme; and use of those methods is well documented. I can't tell how
I would invoke this one. How about this...

A Devicetree should provide extcon-name via phandle (not yet supported).
ACPI extcon drivers/clients[?] should insert[?] device_property
extcon-name into our device with relevant_api_call(args).


>> @@ -1498,8 +1493,10 @@ static int bq24190_probe(struct i2c_client *client,
>>                 bdi->extcon_nb.notifier_call = bq24190_extcon_event;
>>                 ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
>>                                                     &bdi->extcon_nb);
>> -               if (ret)
>> +               if (ret) {
>> +                       dev_err(dev, "Can't register extcon\n");
>>                         goto out5;
>> +               }
>>
>>                 /* Sync initial cable state */
>>                 queue_delayed_work(system_wq, &bdi->extcon_work, 0);
>>
>
>
> Regards,
>
> Hans

Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Hans de Goede]

Hi,

On 07-04-17 23:46, Liam Breck wrote:
> Hi Hans,
>
> On Thu, Apr 6, 2017 at 12:20 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 06-04-17 05:10, Liam Breck wrote:
>>>
>>> From: Liam Breck <kernel@networkimprov.net>
>>>
>>> On chip reset, polling loop was reading reset register immediately.
>>> Instead, call udelay() before reading chip register.
>>
>>
>> Can you extend the commit message to explain why udelay is being
>> called ?  IOW what is the problem with reading the register
>> immediately which this patch addresses ?
>
> I thought I'd read that reset isn't instantaneous, but in testing it
> seems to be. So I will revert to the original post-reset delay, and
> change udelay() to usleep_range(10, 20). Any thoughts on those
> numbers?

Using usleep makes no sense for such short delays, so I would stick
with udelay.

More importantly you still have not explained why we need to delay
*at all* esp. such a short delay, 10us is a single bit at 100kHz i2c
so the set bq24190 register-pointer part of the i2c transfer which
happens before reading back the reset-bit alone takes 20 times as
long as the delay.

I still don't understand why a delay is needed at all, what are
you trying to achieve with this delay ? Are you seeing a specific
problem you are trying to fix ?

Regards,

Hans





>
>
>
>>
>>
>>
>>
>>>
>>> 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 | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index fa29cb3..71a796e 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -543,18 +543,14 @@ static int bq24190_register_reset(struct
>>> bq24190_dev_info *bdi)
>>>
>>>         /* Reset bit will be cleared by hardware so poll until it is */
>>>         do {
>>> +               udelay(10);
>>>                 ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>>>                                 BQ24190_REG_POC_RESET_MASK,
>>>                                 BQ24190_REG_POC_RESET_SHIFT,
>>>                                 &v);
>>>                 if (ret < 0)
>>>                         return ret;
>>> -
>>> -               if (!v)
>>> -                       break;
>>> -
>>> -               udelay(10);
>>> -       } while (--limit);
>>> +       } while (v && --limit);
>>>
>>>         if (!limit)
>>>                 return -EIO;
>>>
>>

Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Hans de Goede]

Hi,

On 08-04-17 00:27, Liam Breck wrote:
> Hallo Hans,
>
> On Thu, Apr 6, 2017 at 12:17 AM, Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>>> -       /*
>>> -        * If no charger has been detected and host mode is requested,
>>> activate
>>> -        * the 5V boost converter, otherwise deactivate it.
>>> -        */
>>> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) ==
>>> 1) {
>>> -               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);
>>> -       } else {
>>> -               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);
>>> -       }
>>> -       if (ret)
>>> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
>>> +       /* Set OTG 5V boost: on if no charger detected and in USB host
>>> mode, else off */
>>> +       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>> +                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
>>> +                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>> +                       !iinlim && extcon_get_state(bdi->extcon,
>>> EXTCON_USB_HOST) == 1
>>> +                                ? BQ24190_REG_POC_CHG_CONFIG_OTG
>>> +                                : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
>>> +       if (error < 0)
>>> +               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>>
>>>         pm_runtime_mark_last_busy(bdi->dev);
>>>         pm_runtime_put_autosuspend(bdi->dev);
>>
>>
>> I still find the refactored code you're proposing here a lot less readable
>> then the original code, and the same goes for the comment. If you insist
>> in changing this, then I suggest changing it into this:
>
> OK. I'll collapse the comment to one line tho.

Please don't having this as 2 lines really is not a problem in any way and
I find the longer comment a lot easier to parse.

>
>>         /*
>>          * If no charger has been detected and host mode is requested,
>> activate
>>          * the 5V boost converter, otherwise deactivate it.
>>          */
>>         if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_OTG;
>>         else
>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
>>
>>         error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>                                    BQ24190_REG_POC_CHG_CONFIG_MASK,
>>                                    BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>                                    chg_config);
>>         if (error)
>>                 dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>
>>> @@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client *client,
>>>         }
>>>
>>>         /*
>>> -        * The extcon-name property is purely an in kernel interface for
>>> -        * x86/ACPI use, DT platforms should get extcon via phandle.
>>> +        * ACPI platforms should use device_property "extcon-name".
>>> +        * Devicetree platforms should get extcon via phandle (not yet
>>> supported).
>>>          */
>>>         if (device_property_read_string(dev, "extcon-name", &name) == 0) {
>>>                 bdi->extcon = extcon_get_extcon_dev(name);
>>
>>
>> The new comment suggest that extcon-name is something which should actually
>> be used inside ACPI DSDT tables, which it is not, please leave the comment
>> as is.
>
> OK, right. But "in kernel interface" sounds official, and this is
> really a custom msg passing method. A platform-data struct is
> documented in a header, and a module parameter has its own docs
> scheme; and use of those methods is well documented. I can't tell how
> I would invoke this one. How about this...
>
> A Devicetree should provide extcon-name via phandle (not yet supported).

No a devicetree should not provide a name it would provide a phandle
pointing to the extcon, no name (and name based lookup) should be
involved.

> ACPI extcon drivers/clients[?] should insert[?] device_property
> extcon-name into our device with relevant_api_call(args).

For an example of how this is used see:

https://github.com/jwrdegoede/linux-sunxi/commit/7fa4460e2a18a3f6ffb39ae4940fee1dd2a880a4

Around line 214 of i2c-cht-wc.c and later.


Regards,

Hans

Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Liam Breck]

On Sat, Apr 8, 2017 at 3:02 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 08-04-17 00:27, Liam Breck wrote:
>>
>> Hallo Hans,
>>
>> On Thu, Apr 6, 2017 at 12:17 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>
>
> <snip>
>
>
>>>> -       /*
>>>> -        * If no charger has been detected and host mode is requested,
>>>> activate
>>>> -        * the 5V boost converter, otherwise deactivate it.
>>>> -        */
>>>> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) ==
>>>> 1) {
>>>> -               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);
>>>> -       } else {
>>>> -               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);
>>>> -       }
>>>> -       if (ret)
>>>> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
>>>> +       /* Set OTG 5V boost: on if no charger detected and in USB host
>>>> mode, else off */
>>>> +       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>> +                       !iinlim && extcon_get_state(bdi->extcon,
>>>> EXTCON_USB_HOST) == 1
>>>> +                                ? BQ24190_REG_POC_CHG_CONFIG_OTG
>>>> +                                : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
>>>> +       if (error < 0)
>>>> +               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>>>
>>>>         pm_runtime_mark_last_busy(bdi->dev);
>>>>         pm_runtime_put_autosuspend(bdi->dev);
>>>
>>>
>>>
>>> I still find the refactored code you're proposing here a lot less
>>> readable
>>> then the original code, and the same goes for the comment. If you insist
>>> in changing this, then I suggest changing it into this:
>>
>>
>> OK. I'll collapse the comment to one line tho.
>
>
> Please don't having this as 2 lines really is not a problem in any way and
> I find the longer comment a lot easier to parse.
>
>
>>
>>>         /*
>>>          * If no charger has been detected and host mode is requested,
>>> activate
>>>          * the 5V boost converter, otherwise deactivate it.
>>>          */
>>>         if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) ==
>>> 1)
>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_OTG;
>>>         else
>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
>>>
>>>         error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>                                    BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>                                    BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>                                    chg_config);
>>>         if (error)
>>>                 dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>>
>>>> @@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client
>>>> *client,
>>>>         }
>>>>
>>>>         /*
>>>> -        * The extcon-name property is purely an in kernel interface for
>>>> -        * x86/ACPI use, DT platforms should get extcon via phandle.
>>>> +        * ACPI platforms should use device_property "extcon-name".
>>>> +        * Devicetree platforms should get extcon via phandle (not yet
>>>> supported).
>>>>          */
>>>>         if (device_property_read_string(dev, "extcon-name", &name) == 0)
>>>> {
>>>>                 bdi->extcon = extcon_get_extcon_dev(name);
>>>
>>>
>>>
>>> The new comment suggest that extcon-name is something which should
>>> actually
>>> be used inside ACPI DSDT tables, which it is not, please leave the
>>> comment
>>> as is.
>>
>>
>> OK, right. But "in kernel interface" sounds official, and this is
>> really a custom msg passing method. A platform-data struct is
>> documented in a header, and a module parameter has its own docs
>> scheme; and use of those methods is well documented. I can't tell how
>> I would invoke this one. How about this...
>>
>> A Devicetree should provide extcon-name via phandle (not yet supported).
>
>
> No a devicetree should not provide a name it would provide a phandle
> pointing to the extcon, no name (and name based lookup) should be
> involved.

OK.

>> ACPI extcon drivers/clients[?] should insert[?] device_property
>> extcon-name into our device with relevant_api_call(args).
>
>
> For an example of how this is used see:
>
> https://github.com/jwrdegoede/linux-sunxi/commit/7fa4460e2a18a3f6ffb39ae4940fee1dd2a880a4
>
> Around line 214 of i2c-cht-wc.c and later.

So a comment like...

On ACPI platforms, extcon clients should invoke this driver with:
struct i2c_adapter a = { ... };
i2c_add_adapter(&a);
struct property_entry pe[] = { PROPERTY_ENTRY_STRING("extcon-name",
<name>), ... };
struct i2c_board_info bi = { .type = "bq24190", .addr = 0x6b,
.properties = pe, .irq = <irq> };
i2c_new_device(&a, &bi);

Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Liam Breck]

On Sat, Apr 8, 2017 at 2:58 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 07-04-17 23:46, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Thu, Apr 6, 2017 at 12:20 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 06-04-17 05:10, Liam Breck wrote:
>>>>
>>>>
>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>
>>>> On chip reset, polling loop was reading reset register immediately.
>>>> Instead, call udelay() before reading chip register.
>>>
>>>
>>>
>>> Can you extend the commit message to explain why udelay is being
>>> called ?  IOW what is the problem with reading the register
>>> immediately which this patch addresses ?
>>
>>
>> I thought I'd read that reset isn't instantaneous, but in testing it
>> seems to be. So I will revert to the original post-reset delay, and
>> change udelay() to usleep_range(10, 20). Any thoughts on those
>> numbers?
>
>
> Using usleep makes no sense for such short delays, so I would stick
> with udelay.
>
> More importantly you still have not explained why we need to delay
> *at all* esp. such a short delay, 10us is a single bit at 100kHz i2c
> so the set bq24190 register-pointer part of the i2c transfer which
> happens before reading back the reset-bit alone takes 20 times as
> long as the delay.
>
> I still don't understand why a delay is needed at all, what are
> you trying to achieve with this delay ? Are you seeing a specific
> problem you are trying to fix ?

A loop with udelay(10) was in the code already, because at manual
9.5.1.2 the note for bit 7 says "Back to 0 after register reset".

But I think you're right, we don't need to loop checking this; I'll
just return -EIO (?) if bit 7 isn't 0 -- which presumably means
charger is kaput.



>
>
>
>
>>
>>
>>
>>>
>>>
>>>
>>>
>>>>
>>>> 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 | 8 ++------
>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>> b/drivers/power/supply/bq24190_charger.c
>>>> index fa29cb3..71a796e 100644
>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>> @@ -543,18 +543,14 @@ static int bq24190_register_reset(struct
>>>> bq24190_dev_info *bdi)
>>>>
>>>>         /* Reset bit will be cleared by hardware so poll until it is */
>>>>         do {
>>>> +               udelay(10);
>>>>                 ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>>>>                                 BQ24190_REG_POC_RESET_MASK,
>>>>                                 BQ24190_REG_POC_RESET_SHIFT,
>>>>                                 &v);
>>>>                 if (ret < 0)
>>>>                         return ret;
>>>> -
>>>> -               if (!v)
>>>> -                       break;
>>>> -
>>>> -               udelay(10);
>>>> -       } while (--limit);
>>>> +       } while (v && --limit);
>>>>
>>>>         if (!limit)
>>>>                 return -EIO;
>>>>
>>>
>

Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Hans de Goede]

Hi,

On 08-04-17 21:09, Liam Breck wrote:
> On Sat, Apr 8, 2017 at 3:02 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 08-04-17 00:27, Liam Breck wrote:
>>>
>>> Hallo Hans,
>>>
>>> On Thu, Apr 6, 2017 at 12:17 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>
>>
>> <snip>
>>
>>
>>>>> -       /*
>>>>> -        * If no charger has been detected and host mode is requested,
>>>>> activate
>>>>> -        * the 5V boost converter, otherwise deactivate it.
>>>>> -        */
>>>>> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) ==
>>>>> 1) {
>>>>> -               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);
>>>>> -       } else {
>>>>> -               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);
>>>>> -       }
>>>>> -       if (ret)
>>>>> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
>>>>> +       /* Set OTG 5V boost: on if no charger detected and in USB host
>>>>> mode, else off */
>>>>> +       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>> +                       !iinlim && extcon_get_state(bdi->extcon,
>>>>> EXTCON_USB_HOST) == 1
>>>>> +                                ? BQ24190_REG_POC_CHG_CONFIG_OTG
>>>>> +                                : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
>>>>> +       if (error < 0)
>>>>> +               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>>>>
>>>>>         pm_runtime_mark_last_busy(bdi->dev);
>>>>>         pm_runtime_put_autosuspend(bdi->dev);
>>>>
>>>>
>>>>
>>>> I still find the refactored code you're proposing here a lot less
>>>> readable
>>>> then the original code, and the same goes for the comment. If you insist
>>>> in changing this, then I suggest changing it into this:
>>>
>>>
>>> OK. I'll collapse the comment to one line tho.
>>
>>
>> Please don't having this as 2 lines really is not a problem in any way and
>> I find the longer comment a lot easier to parse.
>>
>>
>>>
>>>>         /*
>>>>          * If no charger has been detected and host mode is requested,
>>>> activate
>>>>          * the 5V boost converter, otherwise deactivate it.
>>>>          */
>>>>         if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) ==
>>>> 1)
>>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_OTG;
>>>>         else
>>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
>>>>
>>>>         error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>                                    BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>                                    BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>                                    chg_config);
>>>>         if (error)
>>>>                 dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>>>
>>>>> @@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client
>>>>> *client,
>>>>>         }
>>>>>
>>>>>         /*
>>>>> -        * The extcon-name property is purely an in kernel interface for
>>>>> -        * x86/ACPI use, DT platforms should get extcon via phandle.
>>>>> +        * ACPI platforms should use device_property "extcon-name".
>>>>> +        * Devicetree platforms should get extcon via phandle (not yet
>>>>> supported).
>>>>>          */
>>>>>         if (device_property_read_string(dev, "extcon-name", &name) == 0)
>>>>> {
>>>>>                 bdi->extcon = extcon_get_extcon_dev(name);
>>>>
>>>>
>>>>
>>>> The new comment suggest that extcon-name is something which should
>>>> actually
>>>> be used inside ACPI DSDT tables, which it is not, please leave the
>>>> comment
>>>> as is.
>>>
>>>
>>> OK, right. But "in kernel interface" sounds official, and this is
>>> really a custom msg passing method. A platform-data struct is
>>> documented in a header, and a module parameter has its own docs
>>> scheme; and use of those methods is well documented. I can't tell how
>>> I would invoke this one. How about this...
>>>
>>> A Devicetree should provide extcon-name via phandle (not yet supported).
>>
>>
>> No a devicetree should not provide a name it would provide a phandle
>> pointing to the extcon, no name (and name based lookup) should be
>> involved.
>
> OK.
>
>>> ACPI extcon drivers/clients[?] should insert[?] device_property
>>> extcon-name into our device with relevant_api_call(args).
>>
>>
>> For an example of how this is used see:
>>
>> https://github.com/jwrdegoede/linux-sunxi/commit/7fa4460e2a18a3f6ffb39ae4940fee1dd2a880a4
>>
>> Around line 214 of i2c-cht-wc.c and later.
>
> So a comment like...
>
> On ACPI platforms, extcon clients should invoke this driver with:
> struct i2c_adapter a = { ... };
> i2c_add_adapter(&a);
> struct property_entry pe[] = { PROPERTY_ENTRY_STRING("extcon-name",
> <name>), ... };
> struct i2c_board_info bi = { .type = "bq24190", .addr = 0x6b,
> .properties = pe, .irq = <irq> };
> i2c_new_device(&a, &bi);

Sure that works for me.

Regards,

Hans

Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Hans de Goede]

Hi,

On 08-04-17 21:25, Liam Breck wrote:
> On Sat, Apr 8, 2017 at 2:58 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 07-04-17 23:46, Liam Breck wrote:
>>>
>>> Hi Hans,
>>>
>>> On Thu, Apr 6, 2017 at 12:20 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 06-04-17 05:10, Liam Breck wrote:
>>>>>
>>>>>
>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>
>>>>> On chip reset, polling loop was reading reset register immediately.
>>>>> Instead, call udelay() before reading chip register.
>>>>
>>>>
>>>>
>>>> Can you extend the commit message to explain why udelay is being
>>>> called ?  IOW what is the problem with reading the register
>>>> immediately which this patch addresses ?
>>>
>>>
>>> I thought I'd read that reset isn't instantaneous, but in testing it
>>> seems to be. So I will revert to the original post-reset delay, and
>>> change udelay() to usleep_range(10, 20). Any thoughts on those
>>> numbers?
>>
>>
>> Using usleep makes no sense for such short delays, so I would stick
>> with udelay.
>>
>> More importantly you still have not explained why we need to delay
>> *at all* esp. such a short delay, 10us is a single bit at 100kHz i2c
>> so the set bq24190 register-pointer part of the i2c transfer which
>> happens before reading back the reset-bit alone takes 20 times as
>> long as the delay.
>>
>> I still don't understand why a delay is needed at all, what are
>> you trying to achieve with this delay ? Are you seeing a specific
>> problem you are trying to fix ?
>
> A loop with udelay(10) was in the code already, because at manual
> 9.5.1.2 the note for bit 7 says "Back to 0 after register reset".
>
> But I think you're right, we don't need to loop checking this; I'll
> just return -EIO (?) if bit 7 isn't 0 -- which presumably means
> charger is kaput.

Having a loop to check for this is not unusual, I think it would
be fine to just keep the code as is unless you've a specific
reason to change it.

Regards,

Hans




>
>
>
>>
>>
>>
>>
>>>
>>>
>>>
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> 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 | 8 ++------
>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>>> b/drivers/power/supply/bq24190_charger.c
>>>>> index fa29cb3..71a796e 100644
>>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>>> @@ -543,18 +543,14 @@ static int bq24190_register_reset(struct
>>>>> bq24190_dev_info *bdi)
>>>>>
>>>>>         /* Reset bit will be cleared by hardware so poll until it is */
>>>>>         do {
>>>>> +               udelay(10);
>>>>>                 ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>>>>>                                 BQ24190_REG_POC_RESET_MASK,
>>>>>                                 BQ24190_REG_POC_RESET_SHIFT,
>>>>>                                 &v);
>>>>>                 if (ret < 0)
>>>>>                         return ret;
>>>>> -
>>>>> -               if (!v)
>>>>> -                       break;
>>>>> -
>>>>> -               udelay(10);
>>>>> -       } while (--limit);
>>>>> +       } while (v && --limit);
>>>>>
>>>>>         if (!limit)
>>>>>                 return -EIO;
>>>>>
>>>>
>>

Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Liam Breck]

On Sat, Apr 8, 2017 at 1:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 08-04-17 21:25, Liam Breck wrote:
>>
>> On Sat, Apr 8, 2017 at 2:58 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 07-04-17 23:46, Liam Breck wrote:
>>>>
>>>>
>>>> Hi Hans,
>>>>
>>>> On Thu, Apr 6, 2017 at 12:20 AM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 06-04-17 05:10, Liam Breck wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>>
>>>>>> On chip reset, polling loop was reading reset register immediately.
>>>>>> Instead, call udelay() before reading chip register.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Can you extend the commit message to explain why udelay is being
>>>>> called ?  IOW what is the problem with reading the register
>>>>> immediately which this patch addresses ?
>>>>
>>>>
>>>>
>>>> I thought I'd read that reset isn't instantaneous, but in testing it
>>>> seems to be. So I will revert to the original post-reset delay, and
>>>> change udelay() to usleep_range(10, 20). Any thoughts on those
>>>> numbers?
>>>
>>>
>>>
>>> Using usleep makes no sense for such short delays, so I would stick
>>> with udelay.
>>>
>>> More importantly you still have not explained why we need to delay
>>> *at all* esp. such a short delay, 10us is a single bit at 100kHz i2c
>>> so the set bq24190 register-pointer part of the i2c transfer which
>>> happens before reading back the reset-bit alone takes 20 times as
>>> long as the delay.
>>>
>>> I still don't understand why a delay is needed at all, what are
>>> you trying to achieve with this delay ? Are you seeing a specific
>>> problem you are trying to fix ?
>>
>>
>> A loop with udelay(10) was in the code already, because at manual
>> 9.5.1.2 the note for bit 7 says "Back to 0 after register reset".
>>
>> But I think you're right, we don't need to loop checking this; I'll
>> just return -EIO (?) if bit 7 isn't 0 -- which presumably means
>> charger is kaput.
>
>
> Having a loop to check for this is not unusual, I think it would
> be fine to just keep the code as is unless you've a specific
> reason to change it.

Well you did point out that the delay is too short to be useful. Our
bus runs at 400kHz, and we retry 100x, so we can wait 400 clicks
(1ms). We should probably wait longer or not at all.

>
>
>
>
>
>>
>>
>>
>>>
>>>
>>>
>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> 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 | 8 ++------
>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>>>> b/drivers/power/supply/bq24190_charger.c
>>>>>> index fa29cb3..71a796e 100644
>>>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>>>> @@ -543,18 +543,14 @@ static int bq24190_register_reset(struct
>>>>>> bq24190_dev_info *bdi)
>>>>>>
>>>>>>         /* Reset bit will be cleared by hardware so poll until it is
>>>>>> */
>>>>>>         do {
>>>>>> +               udelay(10);
>>>>>>                 ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>>>>>>                                 BQ24190_REG_POC_RESET_MASK,
>>>>>>                                 BQ24190_REG_POC_RESET_SHIFT,
>>>>>>                                 &v);
>>>>>>                 if (ret < 0)
>>>>>>                         return ret;
>>>>>> -
>>>>>> -               if (!v)
>>>>>> -                       break;
>>>>>> -
>>>>>> -               udelay(10);
>>>>>> -       } while (--limit);
>>>>>> +       } while (v && --limit);
>>>>>>
>>>>>>         if (!limit)
>>>>>>                 return -EIO;
>>>>>>
>>>>>
>>>
>

Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Hans de Goede]

Hi,

On 08-04-17 22:46, Liam Breck wrote:
> On Sat, Apr 8, 2017 at 1:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 08-04-17 21:25, Liam Breck wrote:
>>>
>>> On Sat, Apr 8, 2017 at 2:58 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 07-04-17 23:46, Liam Breck wrote:
>>>>>
>>>>>
>>>>> Hi Hans,
>>>>>
>>>>> On Thu, Apr 6, 2017 at 12:20 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 06-04-17 05:10, Liam Breck wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>>>
>>>>>>> On chip reset, polling loop was reading reset register immediately.
>>>>>>> Instead, call udelay() before reading chip register.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Can you extend the commit message to explain why udelay is being
>>>>>> called ?  IOW what is the problem with reading the register
>>>>>> immediately which this patch addresses ?
>>>>>
>>>>>
>>>>>
>>>>> I thought I'd read that reset isn't instantaneous, but in testing it
>>>>> seems to be. So I will revert to the original post-reset delay, and
>>>>> change udelay() to usleep_range(10, 20). Any thoughts on those
>>>>> numbers?
>>>>
>>>>
>>>>
>>>> Using usleep makes no sense for such short delays, so I would stick
>>>> with udelay.
>>>>
>>>> More importantly you still have not explained why we need to delay
>>>> *at all* esp. such a short delay, 10us is a single bit at 100kHz i2c
>>>> so the set bq24190 register-pointer part of the i2c transfer which
>>>> happens before reading back the reset-bit alone takes 20 times as
>>>> long as the delay.
>>>>
>>>> I still don't understand why a delay is needed at all, what are
>>>> you trying to achieve with this delay ? Are you seeing a specific
>>>> problem you are trying to fix ?
>>>
>>>
>>> A loop with udelay(10) was in the code already, because at manual
>>> 9.5.1.2 the note for bit 7 says "Back to 0 after register reset".
>>>
>>> But I think you're right, we don't need to loop checking this; I'll
>>> just return -EIO (?) if bit 7 isn't 0 -- which presumably means
>>> charger is kaput.
>>
>>
>> Having a loop to check for this is not unusual, I think it would
>> be fine to just keep the code as is unless you've a specific
>> reason to change it.
>
> Well you did point out that the delay is too short to be useful. Our
> bus runs at 400kHz, and we retry 100x, so we can wait 400 clicks
> (1ms). We should probably wait longer or not at all.

Ok, either way is fine by me.

Regards,

Hans



>
>>
>>
>>
>>
>>
>>>
>>>
>>>
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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 | 8 ++------
>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>>>>> b/drivers/power/supply/bq24190_charger.c
>>>>>>> index fa29cb3..71a796e 100644
>>>>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>>>>> @@ -543,18 +543,14 @@ static int bq24190_register_reset(struct
>>>>>>> bq24190_dev_info *bdi)
>>>>>>>
>>>>>>>         /* Reset bit will be cleared by hardware so poll until it is
>>>>>>> */
>>>>>>>         do {
>>>>>>> +               udelay(10);
>>>>>>>                 ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>>>>>>>                                 BQ24190_REG_POC_RESET_MASK,
>>>>>>>                                 BQ24190_REG_POC_RESET_SHIFT,
>>>>>>>                                 &v);
>>>>>>>                 if (ret < 0)
>>>>>>>                         return ret;
>>>>>>> -
>>>>>>> -               if (!v)
>>>>>>> -                       break;
>>>>>>> -
>>>>>>> -               udelay(10);
>>>>>>> -       } while (--limit);
>>>>>>> +       } while (v && --limit);
>>>>>>>
>>>>>>>         if (!limit)
>>>>>>>                 return -EIO;
>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Liam Breck]

On Sat, Apr 8, 2017 at 1:05 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 08-04-17 21:09, Liam Breck wrote:
>>
>> On Sat, Apr 8, 2017 at 3:02 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 08-04-17 00:27, Liam Breck wrote:
>>>>
>>>>
>>>> Hallo Hans,
>>>>
>>>> On Thu, Apr 6, 2017 at 12:17 AM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>
>>>
>>>
>>> <snip>
>>>
>>>
>>>>>> -       /*
>>>>>> -        * If no charger has been detected and host mode is requested,
>>>>>> activate
>>>>>> -        * the 5V boost converter, otherwise deactivate it.
>>>>>> -        */
>>>>>> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST)
>>>>>> ==
>>>>>> 1) {
>>>>>> -               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);
>>>>>> -       } else {
>>>>>> -               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);
>>>>>> -       }
>>>>>> -       if (ret)
>>>>>> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
>>>>>> +       /* Set OTG 5V boost: on if no charger detected and in USB host
>>>>>> mode, else off */
>>>>>> +       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>>> +                       !iinlim && extcon_get_state(bdi->extcon,
>>>>>> EXTCON_USB_HOST) == 1
>>>>>> +                                ? BQ24190_REG_POC_CHG_CONFIG_OTG
>>>>>> +                                : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
>>>>>> +       if (error < 0)
>>>>>> +               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n",
>>>>>> error);
>>>>>>
>>>>>>         pm_runtime_mark_last_busy(bdi->dev);
>>>>>>         pm_runtime_put_autosuspend(bdi->dev);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I still find the refactored code you're proposing here a lot less
>>>>> readable
>>>>> then the original code, and the same goes for the comment. If you
>>>>> insist
>>>>> in changing this, then I suggest changing it into this:
>>>>
>>>>
>>>>
>>>> OK. I'll collapse the comment to one line tho.
>>>
>>>
>>>
>>> Please don't having this as 2 lines really is not a problem in any way
>>> and
>>> I find the longer comment a lot easier to parse.
>>>
>>>
>>>>
>>>>>         /*
>>>>>          * If no charger has been detected and host mode is requested,
>>>>> activate
>>>>>          * the 5V boost converter, otherwise deactivate it.
>>>>>          */
>>>>>         if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST)
>>>>> ==
>>>>> 1)
>>>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_OTG;
>>>>>         else
>>>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
>>>>>
>>>>>         error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>>                                    BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>>                                    BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>>                                    chg_config);
>>>>>         if (error)
>>>>>                 dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>>>>
>>>>>> @@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client
>>>>>> *client,
>>>>>>         }
>>>>>>
>>>>>>         /*
>>>>>> -        * The extcon-name property is purely an in kernel interface
>>>>>> for
>>>>>> -        * x86/ACPI use, DT platforms should get extcon via phandle.
>>>>>> +        * ACPI platforms should use device_property "extcon-name".
>>>>>> +        * Devicetree platforms should get extcon via phandle (not yet
>>>>>> supported).
>>>>>>          */
>>>>>>         if (device_property_read_string(dev, "extcon-name", &name) ==
>>>>>> 0)
>>>>>> {
>>>>>>                 bdi->extcon = extcon_get_extcon_dev(name);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> The new comment suggest that extcon-name is something which should
>>>>> actually
>>>>> be used inside ACPI DSDT tables, which it is not, please leave the
>>>>> comment
>>>>> as is.
>>>>
>>>>
>>>>
>>>> OK, right. But "in kernel interface" sounds official, and this is
>>>> really a custom msg passing method. A platform-data struct is
>>>> documented in a header, and a module parameter has its own docs
>>>> scheme; and use of those methods is well documented. I can't tell how
>>>> I would invoke this one. How about this...
>>>>
>>>> A Devicetree should provide extcon-name via phandle (not yet supported).
>>>
>>>
>>>
>>> No a devicetree should not provide a name it would provide a phandle
>>> pointing to the extcon, no name (and name based lookup) should be
>>> involved.
>>
>>
>> OK.
>>
>>>> ACPI extcon drivers/clients[?] should insert[?] device_property
>>>> extcon-name into our device with relevant_api_call(args).
>>>
>>>
>>>
>>> For an example of how this is used see:
>>>
>>>
>>> https://github.com/jwrdegoede/linux-sunxi/commit/7fa4460e2a18a3f6ffb39ae4940fee1dd2a880a4
>>>
>>> Around line 214 of i2c-cht-wc.c and later.
>>
>>
>> So a comment like...
>>
>> On ACPI platforms, extcon clients should invoke this driver with:
>> struct i2c_adapter a = { ... };
>> i2c_add_adapter(&a);
>> struct property_entry pe[] = { PROPERTY_ENTRY_STRING("extcon-name",
>> <name>), ... };
>> struct i2c_board_info bi = { .type = "bq24190", .addr = 0x6b,
>> .properties = pe, .irq = <irq> };
>> i2c_new_device(&a, &bi);
>
>
> Sure that works for me.

Should we export a function from the charger driver to do the above,
rather than requiring every client driver to replicate it?

Re: [PATCH v4 2/4] power: bq24190_charger: Clean up extcon code

[Hans de Goede]

Hi,

On 10-04-17 11:03, Liam Breck wrote:
> On Sat, Apr 8, 2017 at 1:05 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 08-04-17 21:09, Liam Breck wrote:
>>>
>>> On Sat, Apr 8, 2017 at 3:02 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 08-04-17 00:27, Liam Breck wrote:
>>>>>
>>>>>
>>>>> Hallo Hans,
>>>>>
>>>>> On Thu, Apr 6, 2017 at 12:17 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>>
>>>>
>>>>
>>>> <snip>
>>>>
>>>>
>>>>>>> -       /*
>>>>>>> -        * If no charger has been detected and host mode is requested,
>>>>>>> activate
>>>>>>> -        * the 5V boost converter, otherwise deactivate it.
>>>>>>> -        */
>>>>>>> -       if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST)
>>>>>>> ==
>>>>>>> 1) {
>>>>>>> -               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);
>>>>>>> -       } else {
>>>>>>> -               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);
>>>>>>> -       }
>>>>>>> -       if (ret)
>>>>>>> -               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
>>>>>>> +       /* Set OTG 5V boost: on if no charger detected and in USB host
>>>>>>> mode, else off */
>>>>>>> +       error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>>>> +                                  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>>>> +                       !iinlim && extcon_get_state(bdi->extcon,
>>>>>>> EXTCON_USB_HOST) == 1
>>>>>>> +                                ? BQ24190_REG_POC_CHG_CONFIG_OTG
>>>>>>> +                                : BQ24190_REG_POC_CHG_CONFIG_CHARGE);
>>>>>>> +       if (error < 0)
>>>>>>> +               dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n",
>>>>>>> error);
>>>>>>>
>>>>>>>         pm_runtime_mark_last_busy(bdi->dev);
>>>>>>>         pm_runtime_put_autosuspend(bdi->dev);
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I still find the refactored code you're proposing here a lot less
>>>>>> readable
>>>>>> then the original code, and the same goes for the comment. If you
>>>>>> insist
>>>>>> in changing this, then I suggest changing it into this:
>>>>>
>>>>>
>>>>>
>>>>> OK. I'll collapse the comment to one line tho.
>>>>
>>>>
>>>>
>>>> Please don't having this as 2 lines really is not a problem in any way
>>>> and
>>>> I find the longer comment a lot easier to parse.
>>>>
>>>>
>>>>>
>>>>>>         /*
>>>>>>          * If no charger has been detected and host mode is requested,
>>>>>> activate
>>>>>>          * the 5V boost converter, otherwise deactivate it.
>>>>>>          */
>>>>>>         if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST)
>>>>>> ==
>>>>>> 1)
>>>>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_OTG;
>>>>>>         else
>>>>>>                 chg_config = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
>>>>>>
>>>>>>         error = bq24190_write_mask(bdi, BQ24190_REG_POC,
>>>>>>                                    BQ24190_REG_POC_CHG_CONFIG_MASK,
>>>>>>                                    BQ24190_REG_POC_CHG_CONFIG_SHIFT,
>>>>>>                                    chg_config);
>>>>>>         if (error)
>>>>>>                 dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
>>>>>>
>>>>>>> @@ -1432,8 +1427,8 @@ static int bq24190_probe(struct i2c_client
>>>>>>> *client,
>>>>>>>         }
>>>>>>>
>>>>>>>         /*
>>>>>>> -        * The extcon-name property is purely an in kernel interface
>>>>>>> for
>>>>>>> -        * x86/ACPI use, DT platforms should get extcon via phandle.
>>>>>>> +        * ACPI platforms should use device_property "extcon-name".
>>>>>>> +        * Devicetree platforms should get extcon via phandle (not yet
>>>>>>> supported).
>>>>>>>          */
>>>>>>>         if (device_property_read_string(dev, "extcon-name", &name) ==
>>>>>>> 0)
>>>>>>> {
>>>>>>>                 bdi->extcon = extcon_get_extcon_dev(name);
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> The new comment suggest that extcon-name is something which should
>>>>>> actually
>>>>>> be used inside ACPI DSDT tables, which it is not, please leave the
>>>>>> comment
>>>>>> as is.
>>>>>
>>>>>
>>>>>
>>>>> OK, right. But "in kernel interface" sounds official, and this is
>>>>> really a custom msg passing method. A platform-data struct is
>>>>> documented in a header, and a module parameter has its own docs
>>>>> scheme; and use of those methods is well documented. I can't tell how
>>>>> I would invoke this one. How about this...
>>>>>
>>>>> A Devicetree should provide extcon-name via phandle (not yet supported).
>>>>
>>>>
>>>>
>>>> No a devicetree should not provide a name it would provide a phandle
>>>> pointing to the extcon, no name (and name based lookup) should be
>>>> involved.
>>>
>>>
>>> OK.
>>>
>>>>> ACPI extcon drivers/clients[?] should insert[?] device_property
>>>>> extcon-name into our device with relevant_api_call(args).
>>>>
>>>>
>>>>
>>>> For an example of how this is used see:
>>>>
>>>>
>>>> https://github.com/jwrdegoede/linux-sunxi/commit/7fa4460e2a18a3f6ffb39ae4940fee1dd2a880a4
>>>>
>>>> Around line 214 of i2c-cht-wc.c and later.
>>>
>>>
>>> So a comment like...
>>>
>>> On ACPI platforms, extcon clients should invoke this driver with:
>>> struct i2c_adapter a = { ... };
>>> i2c_add_adapter(&a);
>>> struct property_entry pe[] = { PROPERTY_ENTRY_STRING("extcon-name",
>>> <name>), ... };
>>> struct i2c_board_info bi = { .type = "bq24190", .addr = 0x6b,
>>> .properties = pe, .irq = <irq> };
>>> i2c_new_device(&a, &bi);
>>
>>
>> Sure that works for me.
>
> Should we export a function from the charger driver to do the above,
> rather than requiring every client driver to replicate it?

If we end up needing to do this in more places, then we could consider
making iinlim a property and adding something generic for this to
the power_supply core. But that is something to do when we've at least
2 users, I've a feeling this will look different per use-case
(what about e.g. the 5v boost converter?) so we really need a better
idea of what a generic solution should look like before writing one.

Regards,

Hans