* [PATCH v3 0/4] BQ24190 charger fixes
@ 2017-04-01 20:25 Liam Breck
2017-04-01 20:25 ` [PATCH v3 1/4] power: bq24190_charger: Limit over/under voltage fault logging Liam Breck
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Liam Breck @ 2017-04-01 20:25 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v3 1/4] power: bq24190_charger: Limit over/under voltage fault logging 2017-04-01 20:25 [PATCH v3 0/4] BQ24190 charger fixes Liam Breck @ 2017-04-01 20:25 ` Liam Breck 2017-04-01 20:25 ` [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code Liam Breck ` (2 subsequent siblings) 3 siblings, 0 replies; 14+ messages in thread From: Liam Breck @ 2017-04-01 20:25 UTC (permalink / raw) To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code 2017-04-01 20:25 [PATCH v3 0/4] BQ24190 charger fixes Liam Breck 2017-04-01 20:25 ` [PATCH v3 1/4] power: bq24190_charger: Limit over/under voltage fault logging Liam Breck @ 2017-04-01 20:25 ` Liam Breck 2017-04-02 12:27 ` Hans de Goede 2017-04-01 20:25 ` [PATCH v3 3/4] power: bq24190_charger: Uniform pm_runtime_get() failure handling Liam Breck 2017-04-01 20:25 ` [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag Liam Breck 3 siblings, 1 reply; 14+ messages in thread From: Liam Breck @ 2017-04-01 20:25 UTC (permalink / raw) To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code 2017-04-01 20:25 ` [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code Liam Breck @ 2017-04-02 12:27 ` Hans de Goede 2017-04-02 22:03 ` Liam Breck 0 siblings, 1 reply; 14+ messages in thread From: Hans de Goede @ 2017-04-02 12:27 UTC (permalink / raw) To: Liam Breck, Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Liam Breck 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code 2017-04-02 12:27 ` Hans de Goede @ 2017-04-02 22:03 ` Liam Breck 2017-04-03 6:53 ` Hans de Goede 0 siblings, 1 reply; 14+ messages in thread From: Liam Breck @ 2017-04-02 22:03 UTC (permalink / raw) To: Hans de Goede; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, 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 :-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code 2017-04-02 22:03 ` Liam Breck @ 2017-04-03 6:53 ` Hans de Goede 2017-04-03 8:17 ` Hans de Goede 2017-04-03 21:40 ` Liam Breck 0 siblings, 2 replies; 14+ messages in thread From: Hans de Goede @ 2017-04-03 6:53 UTC (permalink / raw) To: Liam Breck; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code 2017-04-03 6:53 ` Hans de Goede @ 2017-04-03 8:17 ` Hans de Goede 2017-04-03 21:40 ` Liam Breck 1 sibling, 0 replies; 14+ messages in thread From: Hans de Goede @ 2017-04-03 8:17 UTC (permalink / raw) To: Liam Breck; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code 2017-04-03 6:53 ` Hans de Goede 2017-04-03 8:17 ` Hans de Goede @ 2017-04-03 21:40 ` Liam Breck 1 sibling, 0 replies; 14+ messages in thread From: Liam Breck @ 2017-04-03 21:40 UTC (permalink / raw) To: Hans de Goede; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 3/4] power: bq24190_charger: Uniform pm_runtime_get() failure handling 2017-04-01 20:25 [PATCH v3 0/4] BQ24190 charger fixes Liam Breck 2017-04-01 20:25 ` [PATCH v3 1/4] power: bq24190_charger: Limit over/under voltage fault logging Liam Breck 2017-04-01 20:25 ` [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code Liam Breck @ 2017-04-01 20:25 ` Liam Breck 2017-04-01 20:25 ` [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag Liam Breck 3 siblings, 0 replies; 14+ messages in thread From: Liam Breck @ 2017-04-01 20:25 UTC (permalink / raw) To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag 2017-04-01 20:25 [PATCH v3 0/4] BQ24190 charger fixes Liam Breck ` (2 preceding siblings ...) 2017-04-01 20:25 ` [PATCH v3 3/4] power: bq24190_charger: Uniform pm_runtime_get() failure handling Liam Breck @ 2017-04-01 20:25 ` Liam Breck 2017-04-02 12:29 ` Hans de Goede 3 siblings, 1 reply; 14+ messages in thread From: Liam Breck @ 2017-04-01 20:25 UTC (permalink / raw) To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag 2017-04-01 20:25 ` [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag Liam Breck @ 2017-04-02 12:29 ` Hans de Goede 2017-04-02 20:16 ` Liam Breck 0 siblings, 1 reply; 14+ messages in thread From: Hans de Goede @ 2017-04-02 12:29 UTC (permalink / raw) To: Liam Breck, Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Liam Breck 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; > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag 2017-04-02 12:29 ` Hans de Goede @ 2017-04-02 20:16 ` Liam Breck 2017-04-02 20:35 ` Hans de Goede 0 siblings, 1 reply; 14+ messages in thread From: Liam Breck @ 2017-04-02 20:16 UTC (permalink / raw) To: Hans de Goede; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, 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; >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag 2017-04-02 20:16 ` Liam Breck @ 2017-04-02 20:35 ` Hans de Goede 2017-04-04 4:36 ` Liam Breck 0 siblings, 1 reply; 14+ messages in thread From: Hans de Goede @ 2017-04-02 20:35 UTC (permalink / raw) To: Liam Breck; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag 2017-04-02 20:35 ` Hans de Goede @ 2017-04-04 4:36 ` Liam Breck 0 siblings, 0 replies; 14+ messages in thread From: Liam Breck @ 2017-04-04 4:36 UTC (permalink / raw) To: Hans de Goede; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Matt Ranostay 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-04-04 4:36 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-01 20:25 [PATCH v3 0/4] BQ24190 charger fixes Liam Breck 2017-04-01 20:25 ` [PATCH v3 1/4] power: bq24190_charger: Limit over/under voltage fault logging Liam Breck 2017-04-01 20:25 ` [PATCH v3 2/4] power: bq24190_charger: Clean up extcon code Liam Breck 2017-04-02 12:27 ` Hans de Goede 2017-04-02 22:03 ` Liam Breck 2017-04-03 6:53 ` Hans de Goede 2017-04-03 8:17 ` Hans de Goede 2017-04-03 21:40 ` Liam Breck 2017-04-01 20:25 ` [PATCH v3 3/4] power: bq24190_charger: Uniform pm_runtime_get() failure handling Liam Breck 2017-04-01 20:25 ` [PATCH v3 4/4] power: bq24190_charger: Delay before polling reset flag Liam Breck 2017-04-02 12:29 ` Hans de Goede 2017-04-02 20:16 ` Liam Breck 2017-04-02 20:35 ` Hans de Goede 2017-04-04 4:36 ` Liam Breck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.