[v3,0/3] Add support for CellWise cw2015 fuel gauge

This patchset adds support for the CellWise cw2015 fuel gauge.

The CellWise cw2015 fuel gauge is a shuntless, single-cell Li-Ion fuel
gauge. It is used in the pine64 Pinebook Pro laptop.

This is v3 of the patchset. This version incorporates a review by Andy and
includes a commit documenting the cellwise vendor prefix which I forgot to
send in v2.

I've kept the cellwise,battery-profile property in the device tree. Its
content describes characteristics of the battery built into a device. The
exact format is unknown and not publicly documented. It is likely
comprised of some key parameters of the battery (chemistry, voltages,
design capacity) and parameters for tuning the internal state of charge
approximation function.
Since v2 CellWise has confirmed to me that the only way to obtain the
profile blob is to mail them batteries for testing. Thus we will need to
keep that property.

In general I'm not 100 % sure about my json-schema binding for the gauge.
It is my first time ever writing a json-schema binding and I'm not sure
whether properties like power-supplies or monitored-battery need to be
added to a separate, common schema for power supplies or not.


Best Regards,

Tobias Schramm

Changelog:
 v2:
  * Change subject to "Add support for CellWise cw2015 fuel gauge"
  * Rewrite bindings as json-schema
  * Use default power-supplies handling
  * Use regmap for register access
  * Use standard simple-battery node
  * Replace printk/pr_* by dev_{dbg,info,warn,err}
  * Use cancel_delayed_work_sync in remove
  * General code cleanup
 v3:
  * Incorporate review by Andy
  * Add cellwise vendor prefix
  * Rename cellwise,bat-config-info property to cellwise,battery-profile
  * Remove most state of charge post-processing
  * Use fwnode interface
  * General code cleanup
  * Lots of code style fixes

Tobias Schramm (3):
  dt-bindings: Document cellwise vendor-prefix
  dt-bindings: power: supply: add cw2015_battery bindings
  power: supply: add CellWise cw2015 fuel gauge driver

 .../bindings/power/supply/cw2015_battery.yaml |  83 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/power/supply/Kconfig                  |   8 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/cw2015_battery.c         | 785 ++++++++++++++++++
 6 files changed, 885 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml
 create mode 100644 drivers/power/supply/cw2015_battery.c

On Wed, Mar 11, 2020 at 10:30:43AM +0100, Tobias Schramm wrote:
> This patch adds a driver for the CellWise cw2015 fuel gauge.
> 
> The CellWise cw2015 is a shuntless, single-cell Li-Ion fuel gauge used
> in the pine64 Pinebook Pro laptop and some Raspberry Pi UPS HATs.

Thank you for an update!
My comments below.

...

> +	/* wait for gauge to become ready */
> +	for (i = 0; i < CW2015_READ_TRIES; i++) {
> +		ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &reg_val);
> +		if (ret)
> +			return ret;
> +		/* SoC must not be more than 100% */
> +		else if (reg_val <= 100)
> +			break;
> +
> +		msleep(100);
> +	}

Have you considered to use regmap_read_poll_timeout()?

> +
> +	if (i >= CW2015_READ_TRIES) {
> +		reg_val = CW2015_MODE_SLEEP;
> +		regmap_write(cw_bat->regmap, CW2015_REG_MODE, reg_val);
> +		dev_err(cw_bat->dev,
> +			"Gauge did not become ready after profile upload");
> +		return -ETIMEDOUT;
> +	}

...

> +		if (memcmp(bat_info, cw_bat->bat_profile,
> +				CW2015_SIZE_BATINFO)) {

I think it's pretty much okay to have this on one line, disregard 80 limit
(it's only 1 extra).

...

> +static int cw_get_soc(struct cw_battery *cw_bat)
> +{
> +	unsigned int soc;
> +	int ret;
> +
> +	ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &soc);
> +	if (ret)
> +		return ret;
> +
> +	if (soc > 100) {

> +		int max_error_cycles = CW2015_BAT_SOC_ERROR_MS /
> +					cw_bat->poll_interval_ms;

The following looks better

		int max_error_cycles =
			CW2015_BAT_SOC_ERROR_MS / cw_bat->poll_interval_ms;

Applies to all similar places in the code.

> +		dev_err(cw_bat->dev, "Invalid SoC %d%%", soc);
> +		cw_bat->read_errors++;
> +		if (cw_bat->read_errors > max_error_cycles) {
> +			dev_warn(cw_bat->dev,
> +				"Too many invalid SoC reports, resetting gauge");
> +			cw_power_on_reset(cw_bat);
> +			cw_bat->read_errors = 0;
> +		}
> +		return cw_bat->soc;
> +	}
> +	cw_bat->read_errors = 0;
> +
> +	/* Reset gauge if stuck while charging */

> +	if (cw_bat->status == POWER_SUPPLY_STATUS_CHARGING &&
> +			soc == cw_bat->soc) {

A bit strange indentation, and honestly I would leave it on one line, but it's up to you.

> +		int max_stuck_cycles = CW2015_BAT_CHARGING_STUCK_MS /
> +					cw_bat->poll_interval_ms;
> +
> +		cw_bat->charge_stuck_cnt++;
> +		if (cw_bat->charge_stuck_cnt > max_stuck_cycles) {
> +			dev_warn(cw_bat->dev,
> +				"SoC stuck @%u%%, resetting gauge", soc);
> +			cw_power_on_reset(cw_bat);
> +			cw_bat->charge_stuck_cnt = 0;
> +		}
> +	} else {
> +		cw_bat->charge_stuck_cnt = 0;
> +	}
> +
> +	/* Ignore voltage dips during charge */

> +	if (cw_bat->charger_attached &&
> +			HYSTERESIS(soc, cw_bat->soc, 0, 3)) {

This is pretty much one line (77), check your editor settings and update all
similar places in the code.

> +		soc = cw_bat->soc;
> +	}
> +
> +	/* Ignore voltage spikes during discharge */
> +	if (!cw_bat->charger_attached &&
> +			HYSTERESIS(soc, cw_bat->soc, 3, 0)) {
> +		soc = cw_bat->soc;
> +	}
> +
> +	return soc;
> +}

...

> +	cw_bat =
> +		container_of(delay_work, struct cw_battery, battery_delay_work);

It will be better to read if it would be one line.

...

> +static bool cw_battery_valid_time_to_empty(struct cw_battery *cw_bat)
> +{
> +	return cw_bat->time_to_empty > 0 &&
> +		cw_bat->time_to_empty < CW2015_MASK_SOC &&
> +		cw_bat->status == POWER_SUPPLY_STATUS_DISCHARGING;

Fix indentation to be all 'c':s in one column.

> +}

...

> +static int cw2015_parse_properties(struct cw_battery *cw_bat)
> +{
> +	struct device *dev = cw_bat->dev;
> +	int length;
> +	u32 value;
> +	int ret;
> +

> +	length = device_property_read_u8_array(dev, "cellwise,battery-profile",
> +						NULL, 0);

device_property_count_u8();

> +	if (length) {
> +		if (length != CW2015_SIZE_BATINFO) {
> +			dev_err(cw_bat->dev, "battery-profile must be %d bytes",
> +				CW2015_SIZE_BATINFO);
> +			return -EINVAL;
> +		}
> +
> +		cw_bat->bat_profile =
> +			devm_kzalloc(dev, CW2015_SIZE_BATINFO, GFP_KERNEL);

Replace with length (so, you will have one point of validation), and put on
one line.

> +		if (!cw_bat->bat_profile) {
> +			dev_err(cw_bat->dev,
> +				"Failed to allocate memory for battery config info");
> +			return -ENOMEM;
> +		}
> +
> +		ret = device_property_read_u8_array(dev,
> +						"cellwise,battery-profile",
> +						cw_bat->bat_profile,

> +						CW2015_SIZE_BATINFO);

length.

> +		if (ret)
> +			return ret;
> +	} else {
> +		dev_warn(cw_bat->dev,
> +			"No battery-profile found, rolling with current flash contents");
> +	}
> +
> +	cw_bat->poll_interval_ms = CW2015_DEFAULT_POLL_INTERVAL_MS;

> +	ret = device_property_read_u32_array(dev,
> +						"cellwise,monitor-interval-ms",

It's fine to have it on one line.

> +						&value, 1);
> +	if (ret >= 0) {
> +		dev_dbg(cw_bat->dev, "Overriding default monitor-interval with %u ms",
> +			value);
> +		cw_bat->poll_interval_ms = value;
> +	}
> +
> +	return 0;
> +}

...

> +	regmap_reg_range(CW2015_REG_BATINFO,
> +				CW2015_REG_BATINFO + CW2015_SIZE_BATINFO - 1),

Indentation issue. Check all similar places.

...

> +	cw_bat->rk_bat = devm_power_supply_register(&client->dev,
> +		&cw2015_bat_desc, &psy_cfg);
> +	if (IS_ERR(cw_bat->rk_bat)) {
> +		dev_err(cw_bat->dev, "Failed to register power supply");

> +		return -1;

Do not shadow an error code.

> +	}

On Wed, Mar 11, 2020 at 12:18:30PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 11, 2020 at 10:30:43AM +0100, Tobias Schramm wrote:

...

> > +	ret = device_property_read_u32_array(dev,
> > +						"cellwise,monitor-interval-ms",
> 
> It's fine to have it on one line.
> 
> > +						&value, 1);


Actually this is simple
	ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", &value);
(You can use one or two lines on your choice)

Hi Andy,

thanks for reviewing again.

>> +	/* wait for gauge to become ready */
>> +	for (i = 0; i < CW2015_READ_TRIES; i++) {
>> +		ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &reg_val);
>> +		if (ret)
>> +			return ret;
>> +		/* SoC must not be more than 100% */
>> +		else if (reg_val <= 100)
>> +			break;
>> +
>> +		msleep(100);
>> +	}
> 
> Have you considered to use regmap_read_poll_timeout()?

Neat! That is a much cleaner solution. Will use that in v4.

> 
>> +
>> +	if (i >= CW2015_READ_TRIES) {
>> +		reg_val = CW2015_MODE_SLEEP;
>> +		regmap_write(cw_bat->regmap, CW2015_REG_MODE, reg_val);
>> +		dev_err(cw_bat->dev,
>> +			"Gauge did not become ready after profile upload");
>> +		return -ETIMEDOUT;
>> +	}
> 
> ...
> 
>> +		if (memcmp(bat_info, cw_bat->bat_profile,
>> +				CW2015_SIZE_BATINFO)) {
> 
> I think it's pretty much okay to have this on one line, disregard 80 limit
> (it's only 1 extra).

Ok, will probably do that in a few places.


Best Regards,

Tobias