| Message ID | 1499667800-69055-5-git-send-email-preid@electromag.com.au (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 10/07/2017 14:23, Phil Reid wrote: > There seems little point in resetting the capacity mode bit when > reading a capacity register that is affected by this bit. The > next call to the register read will set the bit if it's not in > the correct mode. > Thinking about this a bit more and I don't think this is a good idea. Reading the spec again I'm guessing the runtime till empty calcs will be different based on this mode bit. ie: in uW mode, the spec is making an assumption of constant power, ie Switch Mode regulator load. in mA mode, the spec is make an assumption of constant current, ie linear regulator load What it probably needs is a mode selection option and set that at probe or when a battery is detected. I'm not sure what impact changing the mode has to data calcs that uses averaging. Please drop this patch for now. > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- > drivers/power/supply/sbs-battery.c | 31 ++++++++++++------------------- > 1 file changed, 12 insertions(+), 19 deletions(-) > > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c > index 3473b45..441a576 100644 > --- a/drivers/power/supply/sbs-battery.c > +++ b/drivers/power/supply/sbs-battery.c > @@ -491,22 +491,19 @@ static void sbs_unit_adjustment(struct i2c_client *client, > } > } > > -static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client, > +static int sbs_set_battery_mode(struct i2c_client *client, > enum sbs_battery_mode mode) > { > - int ret, original_val; > + int ret; > > - original_val = sbs_read_word_data(client, BATTERY_MODE_OFFSET); > - if (original_val < 0) > - return original_val; > + ret = sbs_read_word_data(client, BATTERY_MODE_OFFSET); > + if (ret < 0) > + return ret; > > - if ((original_val & BATTERY_MODE_MASK) == mode) > - return mode; > + if ((ret & BATTERY_MODE_MASK) == mode) > + return 0; > > - if (mode == BATTERY_MODE_AMPS) > - ret = original_val & ~BATTERY_MODE_MASK; > - else > - ret = original_val | BATTERY_MODE_MASK; > + ret = (ret & ~BATTERY_MODE_MASK) | mode; > > ret = sbs_write_word_data(client, BATTERY_MODE_OFFSET, ret); > if (ret < 0) > @@ -514,7 +511,7 @@ static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client, > > usleep_range(1000, 2000); > > - return original_val & BATTERY_MODE_MASK; > + return 0; > } > > static int sbs_get_battery_capacity(struct i2c_client *client, > @@ -527,9 +524,9 @@ static int sbs_get_battery_capacity(struct i2c_client *client, > if (power_supply_is_amp_property(psp)) > mode = BATTERY_MODE_AMPS; > > - mode = sbs_set_battery_mode(client, mode); > - if (mode < 0) > - return mode; > + ret = sbs_set_battery_mode(client, mode); > + if (ret < 0) > + return ret; > > ret = sbs_read_word_data(client, sbs_data[reg_offset].addr); > if (ret < 0) > @@ -542,10 +539,6 @@ static int sbs_get_battery_capacity(struct i2c_client *client, > } else > val->intval = ret; > > - ret = sbs_set_battery_mode(client, mode); > - if (ret < 0) > - return ret; > - > return 0; > } > >
On 2017-07-11 06:57, Phil Reid wrote: > On 10/07/2017 14:23, Phil Reid wrote: >> There seems little point in resetting the capacity mode bit when >> reading a capacity register that is affected by this bit. The >> next call to the register read will set the bit if it's not in >> the correct mode. >> > > Thinking about this a bit more and I don't think this is a good idea. > Reading the spec again I'm guessing the runtime till empty calcs will > be different based on this mode bit. > > ie: > in uW mode, the spec is making an assumption of constant power, ie > Switch Mode regulator load. > in mA mode, the spec is make an assumption of constant current, ie > linear regulator load > > What it probably needs is a mode selection option and set that at > probe or when a battery is detected. > > I'm not sure what impact changing the mode has to data calcs that uses > averaging. > > Please drop this patch for now. > I agree that changing the mode implictly might lead to trouble. So getting back to the initial mode might be wise to do. I applied your patches (without Patch 4/4) and made the following observation: Coming fresh from the charger the battery is in Amps mode. But sometimes it is suddenly in Watts mode. I guess switching the mode back failed at some point. So maybe it would make sense to define a default mode to which it is always set back. This might also be configurable (devicetree?). But for a Battery I guess Amps should be the default Mode. (Actually it confused me intially that WATTS mode is handled as the default from code perspective.) > > >> Signed-off-by: Phil Reid <preid@electromag.com.au> >> --- >> drivers/power/supply/sbs-battery.c | 31 >> ++++++++++++------------------- >> 1 file changed, 12 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/power/supply/sbs-battery.c >> b/drivers/power/supply/sbs-battery.c >> index 3473b45..441a576 100644 >> --- a/drivers/power/supply/sbs-battery.c >> +++ b/drivers/power/supply/sbs-battery.c >> @@ -491,22 +491,19 @@ static void sbs_unit_adjustment(struct >> i2c_client *client, >> } >> } >> -static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client >> *client, >> +static int sbs_set_battery_mode(struct i2c_client *client, >> enum sbs_battery_mode mode) >> { >> - int ret, original_val; >> + int ret; >> - original_val = sbs_read_word_data(client, BATTERY_MODE_OFFSET); >> - if (original_val < 0) >> - return original_val; >> + ret = sbs_read_word_data(client, BATTERY_MODE_OFFSET); >> + if (ret < 0) >> + return ret; >> - if ((original_val & BATTERY_MODE_MASK) == mode) >> - return mode; >> + if ((ret & BATTERY_MODE_MASK) == mode) >> + return 0; >> - if (mode == BATTERY_MODE_AMPS) >> - ret = original_val & ~BATTERY_MODE_MASK; >> - else >> - ret = original_val | BATTERY_MODE_MASK; >> + ret = (ret & ~BATTERY_MODE_MASK) | mode; >> ret = sbs_write_word_data(client, BATTERY_MODE_OFFSET, ret); >> if (ret < 0) >> @@ -514,7 +511,7 @@ static enum sbs_battery_mode >> sbs_set_battery_mode(struct i2c_client *client, >> usleep_range(1000, 2000); >> - return original_val & BATTERY_MODE_MASK; >> + return 0; >> } >> static int sbs_get_battery_capacity(struct i2c_client *client, >> @@ -527,9 +524,9 @@ static int sbs_get_battery_capacity(struct >> i2c_client *client, >> if (power_supply_is_amp_property(psp)) >> mode = BATTERY_MODE_AMPS; >> - mode = sbs_set_battery_mode(client, mode); >> - if (mode < 0) >> - return mode; >> + ret = sbs_set_battery_mode(client, mode); >> + if (ret < 0) >> + return ret; >> ret = sbs_read_word_data(client, sbs_data[reg_offset].addr); >> if (ret < 0) >> @@ -542,10 +539,6 @@ static int sbs_get_battery_capacity(struct >> i2c_client *client, >> } else >> val->intval = ret; >> - ret = sbs_set_battery_mode(client, mode); >> - if (ret < 0) >> - return ret; >> - >> return 0; >> } >> Regards, Michael
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c index 3473b45..441a576 100644 --- a/drivers/power/supply/sbs-battery.c +++ b/drivers/power/supply/sbs-battery.c @@ -491,22 +491,19 @@ static void sbs_unit_adjustment(struct i2c_client *client, } } -static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client, +static int sbs_set_battery_mode(struct i2c_client *client, enum sbs_battery_mode mode) { - int ret, original_val; + int ret; - original_val = sbs_read_word_data(client, BATTERY_MODE_OFFSET); - if (original_val < 0) - return original_val; + ret = sbs_read_word_data(client, BATTERY_MODE_OFFSET); + if (ret < 0) + return ret; - if ((original_val & BATTERY_MODE_MASK) == mode) - return mode; + if ((ret & BATTERY_MODE_MASK) == mode) + return 0; - if (mode == BATTERY_MODE_AMPS) - ret = original_val & ~BATTERY_MODE_MASK; - else - ret = original_val | BATTERY_MODE_MASK; + ret = (ret & ~BATTERY_MODE_MASK) | mode; ret = sbs_write_word_data(client, BATTERY_MODE_OFFSET, ret); if (ret < 0) @@ -514,7 +511,7 @@ static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client, usleep_range(1000, 2000); - return original_val & BATTERY_MODE_MASK; + return 0; } static int sbs_get_battery_capacity(struct i2c_client *client, @@ -527,9 +524,9 @@ static int sbs_get_battery_capacity(struct i2c_client *client, if (power_supply_is_amp_property(psp)) mode = BATTERY_MODE_AMPS; - mode = sbs_set_battery_mode(client, mode); - if (mode < 0) - return mode; + ret = sbs_set_battery_mode(client, mode); + if (ret < 0) + return ret; ret = sbs_read_word_data(client, sbs_data[reg_offset].addr); if (ret < 0) @@ -542,10 +539,6 @@ static int sbs_get_battery_capacity(struct i2c_client *client, } else val->intval = ret; - ret = sbs_set_battery_mode(client, mode); - if (ret < 0) - return ret; - return 0; }
There seems little point in resetting the capacity mode bit when reading a capacity register that is affected by this bit. The next call to the register read will set the bit if it's not in the correct mode. Signed-off-by: Phil Reid <preid@electromag.com.au> --- drivers/power/supply/sbs-battery.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-)