[PATCH v3 0/4] BQ24190 charger fixes

[PATCH v3 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
initial extcon patch, let me know. In future please wait for my ack to merge
charger patches, so I do not have to mend others' code.

Patch 3 (repost) fixes some issues with PM runtime error handling.

Patch 4 fixes a nit in register_reset().

Thanks for your help,
Liam

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 | 127 ++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 64 deletions(-)

-- 
2.9.3

[PATCH v3 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 getting logged 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>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 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 v3 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 | 75 ++++++++++++++++------------------
 1 file changed, 36 insertions(+), 39 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 64a48b9..ae27cdc 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -38,12 +38,13 @@
 #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_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)
 #define BQ24190_REG_POC_BOOST_LIM_SHIFT		0
+#define BQ24190_REG_POC_CHG_CONFIG_DISABLE	0
+#define BQ24190_REG_POC_CHG_CONFIG_CHARGE	1
+#define BQ24190_REG_POC_CHG_CONFIG_OTG		2
 
 #define BQ24190_REG_CCC		0x02 /* Charge Current Control */
 #define BQ24190_REG_CCC_ICHG_MASK		(BIT(7) | BIT(6) | BIT(5) | \
@@ -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,10 +1427,10 @@ 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) {
+	if (!device_property_read_string(dev, "extcon-name", &name)) {
 		bdi->extcon = extcon_get_extcon_dev(name);
 		if (!bdi->extcon)
 			return -EPROBE_DEFER;
@@ -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 v3 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>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 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 ae27cdc..52389c5 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 v3 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 52389c5..898faed 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 v3 2/4] power: bq24190_charger: Clean up extcon code

[Hans de Goede]

Hi,

On 01-04-17 22:25, 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 | 75 ++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 64a48b9..ae27cdc 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -38,12 +38,13 @@
>  #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_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)
>  #define BQ24190_REG_POC_BOOST_LIM_SHIFT		0
> +#define BQ24190_REG_POC_CHG_CONFIG_DISABLE	0
> +#define BQ24190_REG_POC_CHG_CONFIG_CHARGE	1
> +#define BQ24190_REG_POC_CHG_CONFIG_OTG		2
>
>  #define BQ24190_REG_CCC		0x02 /* Charge Current Control */
>  #define BQ24190_REG_CCC_ICHG_MASK		(BIT(7) | BIT(6) | BIT(5) | \

This is inconsistent with existing defines which put value defines
like this together with the MASK and SHIFT defines as I did,
see BQ24190_REG_VPRS_PN_* defines for example.

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

The replacing of ret with error is an IMHO useless style change
and if you insist on doing this belongs in a separate patch you are
batching way to much different changes together in a single commit here.

> +		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +		pm_runtime_put_noidle(bdi->dev);

This _really_ belongs in your "power: bq24190_charger: Uniform pm_runtime_get() failure handling"
patch.

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

Another useless style change and this sort of indentation is quite uncommon
in the kernel, please don't do this.

>  	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);
>  	}

As said the s/ret/error/ stuff really should be in a different patch.

> -	/*
> -	 * 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);

The new code here is IMHO close to unreadable with the actual condition hidden
3 lines down into the function-call arguments and all that to save 3 lines of
code: NACK.

>
>  	pm_runtime_mark_last_busy(bdi->dev);
>  	pm_runtime_put_autosuspend(bdi->dev);
> @@ -1432,10 +1427,10 @@ 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) {
> +	if (!device_property_read_string(dev, "extcon-name", &name)) {

We want to check for success here, not for 'not true" success == 0.

>  		bdi->extcon = extcon_get_extcon_dev(name);
>  		if (!bdi->extcon)
>  			return -EPROBE_DEFER;
> @@ -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 v3 4/4] power: bq24190_charger: Delay before polling reset flag

[Hans de Goede]

Hi,

On 01-04-17 22:25, 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.

Why ? What does this buy us ?

Also I've yet to hear back from you on my patch to remove doing
resets of the device altogether have you find any bad side-effects
of that patch in your testing ?

If not I would like us to proceed with simply removing the reset
code as my patch does.

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 52389c5..898faed 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 v3 4/4] power: bq24190_charger: Delay before polling reset flag

[Liam Breck]

On Sun, Apr 2, 2017 at 5:29 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 01-04-17 22:25, 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.
>
>
> Why ? What does this buy us ?
>
> Also I've yet to hear back from you on my patch to remove doing
> resets of the device altogether have you find any bad side-effects
> of that patch in your testing ?

On a DT-configured device, we don't want any charger settings done
prior to probe(), as we are going to apply DT values. So
register_reset() on probe seems right. If that can trigger a charger
bug, we should find a way to make the charger recover.

I will test for the possible bug you described when I have a chance;
I'm occupied with other stuff near-future. For the time being, let's
use the no_register_reset property from your first patchset.


> If not I would like us to proceed with simply removing the reset
> code as my patch does.
>
> 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 52389c5..898faed 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 v3 4/4] power: bq24190_charger: Delay before polling reset flag

[Hans de Goede]

Hi,

On 02-04-17 22:16, Liam Breck wrote:
> On Sun, Apr 2, 2017 at 5:29 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 01-04-17 22:25, 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.
>>
>>
>> Why ? What does this buy us ?
>>
>> Also I've yet to hear back from you on my patch to remove doing
>> resets of the device altogether have you find any bad side-effects
>> of that patch in your testing ?
>
> On a DT-configured device, we don't want any charger settings done
> prior to probe(), as we are going to apply DT values. So
> register_reset() on probe seems right.

If no charger settings are done prior to probe, why bother with
a reset ? "seems right" does not seem like a good argument when
reset has shown to cause problems in real world scenarios and
as I explained in my commit msg there really are no strong
arguments in favor of doing the reset and several against it.

If you really really want to keep doing a reset for no good
reasons then we could introduce a flag which DT can set to
force a reset on probe, but IMHO the reset should just be
removed all together.

> If that can trigger a charger
> bug, we should find a way to make the charger recover.
>
> I will test for the possible bug you described when I have a chance;
> I'm occupied with other stuff near-future. For the time being, let's
> use the no_register_reset property from your first patchset.

As I said above I really see nothing gained from doing the
reset and "seems right" is not really a good technical argument
for keeping it.

Regards,

Hans

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

[Liam Breck]

G'day Hans,

On Sun, Apr 2, 2017 at 5:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 01-04-17 22:25, 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 | 75
>> ++++++++++++++++------------------
>>  1 file changed, 36 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c
>> b/drivers/power/supply/bq24190_charger.c
>> index 64a48b9..ae27cdc 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -38,12 +38,13 @@
>>  #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_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)
>>  #define BQ24190_REG_POC_BOOST_LIM_SHIFT                0
>> +#define BQ24190_REG_POC_CHG_CONFIG_DISABLE     0
>> +#define BQ24190_REG_POC_CHG_CONFIG_CHARGE      1
>> +#define BQ24190_REG_POC_CHG_CONFIG_OTG         2
>>
>>  #define BQ24190_REG_CCC                0x02 /* Charge Current Control */
>>  #define BQ24190_REG_CCC_ICHG_MASK              (BIT(7) | BIT(6) | BIT(5)
>> | \
>
>
> This is inconsistent with existing defines which put value defines
> like this together with the MASK and SHIFT defines as I did,
> see BQ24190_REG_VPRS_PN_* defines for example.

Ah, Mark wrote this differently than either of us. I'll apply his
style in next rev, thanks.


>> @@ -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) {
>
>
> The replacing of ret with error is an IMHO useless style change
> and if you insist on doing this belongs in a separate patch you are
> batching way to much different changes together in a single commit here.

ret is for use with return. There are examples of the above already in the code.

>> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
>> +               pm_runtime_put_noidle(bdi->dev);
>
>
> This _really_ belongs in your "power: bq24190_charger: Uniform
> pm_runtime_get() failure handling"
> patch.

There are examples of the above already in the code. The patch you
refer to fixes another patch.

>>                 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;
>
>
> Another useless style change and this sort of indentation is quite uncommon
> in the kernel, please don't do this.

The rationale for this alignment is immediately apparent.

>>         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);
>>         }
>
>
> As said the s/ret/error/ stuff really should be in a different patch.
>
>
>> -       /*
>> -        * 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);
>
>
> The new code here is IMHO close to unreadable with the actual condition
> hidden
> 3 lines down into the function-call arguments and all that to save 3 lines
> of
> code: NACK.

The condition selects a value, not a function or register. This is the
common f(x, y, e ? z1 : z2) with longer terms. We could store the
condition in a bool before the call, but it fits fine on one line.

>>
>>         pm_runtime_mark_last_busy(bdi->dev);
>>         pm_runtime_put_autosuspend(bdi->dev);
>> @@ -1432,10 +1427,10 @@ 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) {
>> +       if (!device_property_read_string(dev, "extcon-name", &name)) {
>
>
> We want to check for success here, not for 'not true" success == 0.

Sure; I'll go with that.

>>                 bdi->extcon = extcon_get_extcon_dev(name);
>>                 if (!bdi->extcon)
>>                         return -EPROBE_DEFER;
>> @@ -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

Could you adjust your tone to be more collaborative and less cutting?
You only have this code for the Atom device because I designed and
built hardware with this part and hired Mark to write the driver from
scratch. I'd like a little goodwill in exchange! And also gratitude
for taking time to fix two of your patches.

I expect discussion of charger patches with an experienced kernel
contributor to look like these threads:

https://patchwork.kernel.org/project/linux-pm/list/?submitter=109&state=*&q=bq24190_charger%3A+Use+PM+runtime+autosuspend&archive=both

Thanks :-)

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

[Hans de Goede]

Hi,

On 03-04-17 00:03, Liam Breck wrote:

<snip>

> Could you adjust your tone to be more collaborative and less cutting?

I'm sorry if I sounded a bit cutting.

I've tried nothing but to be collaborative and work with you
until now, but this patch introduces several changes I already
mentioned were not a good idea in your review of my original
patch and IMHO do nothing to improve the code.

> You only have this code for the Atom device because I designed and
> built hardware with this part and hired Mark to write the driver from
> scratch. I'd like a little goodwill in exchange!

And here we get to the root of the problem, that you keep claiming
ownership of the driver because you paid Mark to write it,
this is not how the Linux kernel works. Yes you paid Mark to write
a driver and in return you get an entire kernel. You no less own
the bq24190_charger driver then other people own other parts of
the kernel.

Your remarks against Sebastian that he MUST wait for your ack
before merging anything are quite frankly completely out of line
and I have been on the verge of saying something about this
several times already but I decided to stay quiet (until now).

So yes I apologize for my tone getting a bit cutting as I never
want that, but TBH I've found the way you're claiming to be
THE authority on the bq24190_charger driver and have been behaving
if everyone needs to do exactly as you say when it comes to that
driver quite off-putting / abrasive.

As for me "only having this code for the Atom devices", it would
have likely taken me way less time to knock out a new driver
(like I've done for the fuel-gauge) then all the coordination with
you is costing me...

> And also gratitude
> for taking time to fix two of your patches.

I reviewed and acked the first fix, which is my way of saying
thank you. As for this set of fixes, I never asked for those
and I think they are a bad idea.

> I expect discussion of charger patches with an experienced kernel
> contributor to look like these threads:
>
> https://patchwork.kernel.org/project/linux-pm/list/?submitter=109&state=*&q=bq24190_charger%3A+Use+PM+runtime+autosuspend&archive=both

So do I and usually conversations I take place in look like that
...

Regards,

Hans

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

[Hans de Goede]

Hi,

On 03-04-17 08:53, Hans de Goede wrote:

<snip>

>> You only have this code for the Atom device because I designed and
>> built hardware with this part and hired Mark to write the driver from
>> scratch. I'd like a little goodwill in exchange!
>
> And here we get to the root of the problem, that you keep claiming
> ownership of the driver because you paid Mark to write it,
> this is not how the Linux kernel works. Yes you paid Mark to write
> a driver and in return you get an entire kernel. You no less own
> the bq24190_charger driver then other people own other parts of
> the kernel.
>
> Your remarks against Sebastian that he MUST wait for your ack
> before merging anything are quite frankly completely out of line
> and I have been on the verge of saying something about this
> several times already but I decided to stay quiet (until now).
>
> So yes I apologize for my tone getting a bit cutting as I never
> want that, but TBH I've found the way you're claiming to be
> THE authority on the bq24190_charger driver and have been behaving
> if everyone needs to do exactly as you say when it comes to that
> driver quite off-putting / abrasive.

So thinking some more about this, let me try to explain better why
I feel some friction when discussing things with you.

Lets take for example the discussion about whether to remove the
resetting of the charger from the driver or to keep it, as
seen in 2 different threads through my admittedly colored glasses:

1) I reply to your "power: bq24190_charger: Clean up extcon code"
in what is admittedly a bit cutting tone.

2) We've some discussion about the reset stuff in the
"power: bq24190_charger: Delay before polling reset flag"
part of the thread. I notice that I've yet to hear "a good
technical argument for keeping it".

3) You rightfully point out my cutting tone in 1. is not
appreciated, which is good, please keep pointing out if
you find my tone to be not to your liking.

4) But in the same mail you also re-iterate the point where
you claim some sort of special authority over the driver
because you paid Mark for writing it.

4. Is where the friction in our discussions come from on
my side. The Linux kernel is a meritocracy if you disagree
with something I'm proposing please convince me with
technical arguments not some claimed authority over the
driver.

Or if I'm trying to fix a specific issue suggest a good
alternative. As for example Sebastian did with my
function-pointer to add extra properties and he suggested
to just make the fuel-gauge driver register a battery
interface and remove the battery interface from the
bq24190 driver. Something which I instantly recognized as
a nice and clean solution.

Regards,

Hans

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

[Liam Breck]

G'day Hans,

On Sun, Apr 2, 2017 at 11:53 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 03-04-17 00:03, Liam Breck wrote:
>
> <snip>
>
>> Could you adjust your tone to be more collaborative and less cutting?
>
>
> I'm sorry if I sounded a bit cutting.
>
> I've tried nothing but to be collaborative and work with you
> until now, but this patch introduces several changes I already
> mentioned were not a good idea in your review of my original
> patch and IMHO do nothing to improve the code.
>
>> You only have this code for the Atom device because I designed and
>> built hardware with this part and hired Mark to write the driver from
>> scratch. I'd like a little goodwill in exchange!
>
>
> And here we get to the root of the problem, that you keep claiming
> ownership of the driver because you paid Mark to write it,
> this is not how the Linux kernel works. Yes you paid Mark to write
> a driver and in return you get an entire kernel. You no less own
> the bq24190_charger driver then other people own other parts of
> the kernel.

The root of the problem is that I sent far too much critique on v1 of
your patchset, and too quickly. That was misconstrued as a wish to
fence off the codebase. My intent was really to help you produce a
better patchset asap. For myself, I prefer a lot of feedback up front,
as I know what it's like to get squirts of feedback over weeks. I wish
you had not read malevolence into my response!

I clarified my intent in a msg on March 19:
"I realize you put a ton of work into this patchset. Pls know that I
want you to end up with a solution that both works for you, and makes
the charger driver more generally useful. All of my critique is
offered in good faith, and not meant as rejection. I genuinely regret
it if you feel otherwise."

To which you responded with an attack that was startling. I hate
drama, so I let that go.

In the threads I pointed to re Tony's patches, I asked for numerous
technical and stylistic changes, which resulted in some 10 revisions.
He pushed back once, when what I suggested was technically unsound. A
series I've submitted to another driver has reached 17 revisions, and
virtually all of the feedback has been stylistic, vs technical. I have
debated some of these changes, but successfully refuted only a couple.
It's not the way I would style the code, but it's not my precinct. I
don't know why you would expect a different experience than these
examples.

I asked for changes to the extcon patch; you declined to provide any,
telling me to fix some of them. It's not my job to fix your code, but
I did. I then reversed 2 changes which you complained about, and
justified 4 others. Two of those bother you stylistically, but you
suggested no alternatives. Two stylistic differences is barely cause
for complaint, much less a fight.


> Your remarks against Sebastian that he MUST wait for your ack
> before merging anything are quite frankly completely out of line
> and I have been on the verge of saying something about this
> several times already but I decided to stay quiet (until now).

This statement is incorrect. Since I was forced to fix your code in
two cases, I asked that he wait for my ack in future. And I didn't see
the logic of committing your patches on the same day you posted them.
Everyone will be more productive if you solicit my comments, and work
to accommodate them.

> So yes I apologize for my tone getting a bit cutting as I never
> want that, but TBH I've found the way you're claiming to be
> THE authority on the bq24190_charger driver and have been behaving
> if everyone needs to do exactly as you say when it comes to that
> driver quite off-putting / abrasive.

This statement is also incorrect. I called myself the "de facto
maintainer" because Mark is no longer involved, Tony is busy as OMAP
maintainer, and I have spent a lot of time with the code as our
hardware relies on it, so I care about it. (Said hardware will become
commercially available.)

For your v1 and v2 series, I filed several legitimate technical
issues. Of those, only register_reset remains a matter of debate.

> As for me "only having this code for the Atom devices", it would
> have likely taken me way less time to knock out a new driver
> (like I've done for the fuel-gauge) then all the coordination with
> you is costing me...

Hyperbole. You seem to enjoy writing English as much as C :-) Do you
blog? I imagine it'd be a good read.

>> And also gratitude
>> for taking time to fix two of your patches.
>
>
> I reviewed and acked the first fix, which is my way of saying
> thank you. As for this set of fixes, I never asked for those
> and I think they are a bad idea.
>
>> I expect discussion of charger patches with an experienced kernel
>> contributor to look like these threads:
>>
>>
>> https://patchwork.kernel.org/project/linux-pm/list/?submitter=109&state=*&q=bq24190_charger%3A+Use+PM+runtime+autosuspend&archive=both
>
>
> So do I and usually conversations I take place in look like that

Glad to hear it, and I'd love to see it. Above, and previously, I
perceive defamation, which cannot foster collaboration.

Thanks.

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

[Liam Breck]

On Sun, Apr 2, 2017 at 1:35 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 02-04-17 22:16, Liam Breck wrote:
>>
>> On Sun, Apr 2, 2017 at 5:29 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 01-04-17 22:25, 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.
>>>
>>>
>>>
>>> Why ? What does this buy us ?
>>>
>>> Also I've yet to hear back from you on my patch to remove doing
>>> resets of the device altogether have you find any bad side-effects
>>> of that patch in your testing ?
>>
>>
>> On a DT-configured device, we don't want any charger settings done
>> prior to probe(), as we are going to apply DT values. So
>> register_reset() on probe seems right.
>
>
> If no charger settings are done prior to probe, why bother with
> a reset ? "seems right" does not seem like a good argument when
> reset has shown to cause problems in real world scenarios and

Did you check TI's docs for errata on the possible charger reset bug
you found? If there isn't any, it's probably a board-level issue.

> as I explained in my commit msg there really are no strong
> arguments in favor of doing the reset and several against it.

Tony's comments about en_hiz provide the argument in favor. Let's use
another device_property, as you did before. If we later find a chip
bug on reset, we'll come up with a workaround, and that's a separate
issue.

Thanks.