| Message ID | 20201020225312.b4ea29c9dc3ae00f23e39280@uvos.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] power: supply: cpcap-battery: improve handling of 3rd party and XT875 batteries. | expand |
Hi, Please do one change per patch: On Tue, Oct 20, 2020 at 10:53:12PM +0200, Dev Null wrote: > Adds a module option to ignore a missing temerature sensor. first patch > Invalidates empty->counter_uah and charge_full when charge_now indicates that they are grossly wrong second patch > Makes charge_full_design writeable, so that different/custom batteries can be used. third patch > This is especially usfull on XTT875 where both HW4X and BW8X exsist. missing Signed-off-by. > diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c > index 3be76cd7584a..ffa70c31c1cb 100644 > --- a/drivers/power/supply/cpcap-battery.c > +++ b/drivers/power/supply/cpcap-battery.c > @@ -28,6 +28,7 @@ > #include <linux/power_supply.h> > #include <linux/reboot.h> > #include <linux/regmap.h> > +#include <linux/moduleparam.h> > > #include <linux/iio/consumer.h> > #include <linux/iio/types.h> > @@ -162,6 +163,9 @@ static const struct cpcap_battery_capacity cpcap_battery_cap[] = { > > #define CPCAP_NO_BATTERY -400 > > +static bool ignore_temperature_probe; > +module_param(ignore_temperature_probe, bool, 0660); Can this be deferred from DT (i.e. always disable temperature probe on XT875)? Having to specify a module parameter to get a working system is not very user friendly. > static struct cpcap_battery_state_data * > cpcap_battery_get_state(struct cpcap_battery_ddata *ddata, > enum cpcap_battery_state state) > @@ -205,7 +209,8 @@ static int cpcap_charger_battery_temperature(struct cpcap_battery_ddata *ddata, > channel = ddata->channels[CPCAP_BATTERY_IIO_BATTDET]; > error = iio_read_channel_processed(channel, value); > if (error < 0) { > - dev_warn(ddata->dev, "%s failed: %i\n", __func__, error); > + if (!ignore_temperature_probe) > + dev_warn(ddata->dev, "%s failed: %i\n", __func__, error); > *value = CPCAP_NO_BATTERY; > > return error; > @@ -558,7 +563,7 @@ static int cpcap_battery_get_property(struct power_supply *psy, > > switch (psp) { > case POWER_SUPPLY_PROP_PRESENT: > - if (latest->temperature > CPCAP_NO_BATTERY) > + if (latest->temperature > CPCAP_NO_BATTERY || ignore_temperature_probe) > val->intval = 1; > else > val->intval = 0; > @@ -641,10 +646,22 @@ static int cpcap_battery_get_property(struct power_supply *psy, > if (!empty->voltage) > return -ENODATA; > val->intval = empty->counter_uah - latest->counter_uah; > - if (val->intval < 0) > + if (val->intval < 0) { > + if (ddata->charge_full && abs(val->intval) > ddata->charge_full/5) { > + empty->voltage = 0; > + ddata->charge_full = 0; > + return -ENODATA; > + } > val->intval = 0; > - else if (ddata->charge_full && ddata->charge_full < val->intval) > + } > + else if (ddata->charge_full && ddata->charge_full < val->intval) { > + if (val->intval > (6*ddata->charge_full)/5) { > + empty->voltage = 0; > + ddata->charge_full = 0; > + return -ENODATA; > + } > val->intval = ddata->charge_full; > + } The context of this patch is not available in cpcap-battery driver from mainline. So this has dependencies on other patches, which need to be submitted first. I currently do not have any pending cpcap patches, but IIRC there was a big patchset which had to be resubmitted. > break; > case POWER_SUPPLY_PROP_CHARGE_FULL: > if (!ddata->charge_full) > @@ -658,6 +675,8 @@ static int cpcap_battery_get_property(struct power_supply *psy, > val->intval = POWER_SUPPLY_SCOPE_SYSTEM; > break; > case POWER_SUPPLY_PROP_TEMP: > + if (ignore_temperature_probe) > + return -ENODATA; > val->intval = latest->temperature; > break; > default: > @@ -715,11 +734,18 @@ static int cpcap_battery_set_property(struct power_supply *psy, > case POWER_SUPPLY_PROP_CHARGE_FULL: > if (val->intval < 0) > return -EINVAL; > - if (val->intval > ddata->config.info.charge_full_design) > + if (val->intval > (6*ddata->config.info.charge_full_design)/5) This deserves a comment. Why do we allow to set charge full to be above full design capacity? > return -EINVAL; > > ddata->charge_full = val->intval; > > + return 0; > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > + if (val->intval < 0) > + return -EINVAL; > + > + ddata->config.info.charge_full_design = val->intval; > + > return 0; > default: > return -EINVAL; > @@ -734,6 +760,7 @@ static int cpcap_battery_property_is_writeable(struct power_supply *psy, > switch (psp) { > case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > case POWER_SUPPLY_PROP_CHARGE_FULL: > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > return 1; > default: > return 0; > > -- > Dev Null <devnull@uvos.xyz> -- Sebastian
Hi, * Sebastian Reichel <sre@kernel.org> [201020 21:41]: > The context of this patch is not available in cpcap-battery driver > from mainline. So this has dependencies on other patches, which > need to be submitted first. I currently do not have any pending > cpcap patches, but IIRC there was a big patchset which had to be > resubmitted. Yes this $subject patch as a split up series should be based on the mainline kernel, and not on the WIP battery capacity patches. I doubt there will be any severe rebase issues for the capacity patches. FYI, I'm slowly working on the battery capacity series but still need to decouple the battrery full status from charge current as that won't work for cases where we have CPU load :) We just need to set the status based on charger full interrupt. Regards, Tony
diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c index 3be76cd7584a..ffa70c31c1cb 100644 --- a/drivers/power/supply/cpcap-battery.c +++ b/drivers/power/supply/cpcap-battery.c @@ -28,6 +28,7 @@ #include <linux/power_supply.h> #include <linux/reboot.h> #include <linux/regmap.h> +#include <linux/moduleparam.h> #include <linux/iio/consumer.h> #include <linux/iio/types.h> @@ -162,6 +163,9 @@ static const struct cpcap_battery_capacity cpcap_battery_cap[] = { #define CPCAP_NO_BATTERY -400 +static bool ignore_temperature_probe; +module_param(ignore_temperature_probe, bool, 0660); + static struct cpcap_battery_state_data * cpcap_battery_get_state(struct cpcap_battery_ddata *ddata, enum cpcap_battery_state state) @@ -205,7 +209,8 @@ static int cpcap_charger_battery_temperature(struct cpcap_battery_ddata *ddata, channel = ddata->channels[CPCAP_BATTERY_IIO_BATTDET]; error = iio_read_channel_processed(channel, value); if (error < 0) { - dev_warn(ddata->dev, "%s failed: %i\n", __func__, error); + if (!ignore_temperature_probe) + dev_warn(ddata->dev, "%s failed: %i\n", __func__, error); *value = CPCAP_NO_BATTERY; return error; @@ -558,7 +563,7 @@ static int cpcap_battery_get_property(struct power_supply *psy, switch (psp) { case POWER_SUPPLY_PROP_PRESENT: - if (latest->temperature > CPCAP_NO_BATTERY) + if (latest->temperature > CPCAP_NO_BATTERY || ignore_temperature_probe) val->intval = 1; else val->intval = 0; @@ -641,10 +646,22 @@ static int cpcap_battery_get_property(struct power_supply *psy, if (!empty->voltage) return -ENODATA; val->intval = empty->counter_uah - latest->counter_uah; - if (val->intval < 0) + if (val->intval < 0) { + if (ddata->charge_full && abs(val->intval) > ddata->charge_full/5) { + empty->voltage = 0; + ddata->charge_full = 0; + return -ENODATA; + } val->intval = 0; - else if (ddata->charge_full && ddata->charge_full < val->intval) + } + else if (ddata->charge_full && ddata->charge_full < val->intval) { + if (val->intval > (6*ddata->charge_full)/5) { + empty->voltage = 0; + ddata->charge_full = 0; + return -ENODATA; + } val->intval = ddata->charge_full; + } break; case POWER_SUPPLY_PROP_CHARGE_FULL: if (!ddata->charge_full) @@ -658,6 +675,8 @@ static int cpcap_battery_get_property(struct power_supply *psy, val->intval = POWER_SUPPLY_SCOPE_SYSTEM; break; case POWER_SUPPLY_PROP_TEMP: + if (ignore_temperature_probe) + return -ENODATA; val->intval = latest->temperature; break; default: @@ -715,11 +734,18 @@ static int cpcap_battery_set_property(struct power_supply *psy, case POWER_SUPPLY_PROP_CHARGE_FULL: if (val->intval < 0) return -EINVAL; - if (val->intval > ddata->config.info.charge_full_design) + if (val->intval > (6*ddata->config.info.charge_full_design)/5) return -EINVAL; ddata->charge_full = val->intval; + return 0; + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: + if (val->intval < 0) + return -EINVAL; + + ddata->config.info.charge_full_design = val->intval; + return 0; default: return -EINVAL; @@ -734,6 +760,7 @@ static int cpcap_battery_property_is_writeable(struct power_supply *psy, switch (psp) { case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: case POWER_SUPPLY_PROP_CHARGE_FULL: + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: return 1; default: return 0;