| Message ID | 20170401202519.16280-3-liam@networkimprov.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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 :-)
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
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
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.
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);