Re: [PATCH v2 1/2] power: supply: cpcap-battery: Add battery type auto detection for mapphone devices

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 7984 bytes --]

Hi,

On Fri, Apr 23, 2021 at 02:55:43PM +0200, Carl Philipp Klemm wrote:
> This allows cpcap-battery to detect whitch battery is inserted, HW4X or BW8X for
> xt875 and EB41 for xt894 by examining the battery nvmem. If no known battery is
> detected sane defaults are used.
> 
> Signed-off-by: Carl Philipp Klemm <philipp@uvos.xyz>
> ---
>  drivers/power/supply/cpcap-battery.c | 131 ++++++++++++++++++++-------
>  1 file changed, 98 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
> index 6d5bcdb9f45d..386d269e699f 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/nvmem-consumer.h>

You need to squash PATCH 2/2 into this patch. The kernel tree should
be bisectable and this patch adds the requirement for NVMEM.

>  #include <linux/moduleparam.h>
>  
>  #include <linux/iio/consumer.h>
> @@ -73,6 +74,9 @@
>  
>  #define CPCAP_BATTERY_CC_SAMPLE_PERIOD_MS	250
>  
> +#define CPCAP_BATTERY_EB41_HW4X_ID 0x9E
> +#define CPCAP_BATTERY_BW8X_ID 0x98
> +
>  enum {
>  	CPCAP_BATTERY_IIO_BATTDET,
>  	CPCAP_BATTERY_IIO_VOLTAGE,
> @@ -97,6 +101,7 @@ struct cpcap_interrupt_desc {
>  
>  struct cpcap_battery_config {
>  	int cd_factor;
> +	char id;

id is unused?

>  	struct power_supply_info info;
>  	struct power_supply_battery_info bat;
>  };
> @@ -138,6 +143,8 @@ struct cpcap_battery_ddata {
>  	int charge_full;
>  	int status;
>  	u16 vendor;
> +	bool check_nvmem;
> +	bool no_nvmem_warned;
>  	unsigned int is_full:1;
>  };
>  
> @@ -354,6 +361,91 @@ cpcap_battery_read_accumulated(struct cpcap_battery_ddata *ddata,
>  				       ccd->offset);
>  }
>  
> +
> +/*
> + * Based on the values from Motorola mapphone Linux kernel for the
> + * stock Droid 4 battery eb41. In the Motorola mapphone Linux
> + * kernel tree the value for pm_cd_factor is passed to the kernel
> + * via device tree. If it turns out to be something device specific
> + * we can consider that too later. These values are also fine for
> + * Bionic's hw4x.
> + *
> + * And looking at the battery full and shutdown values for the stock
> + * kernel on droid 4, full is 4351000 and software initiates shutdown
> + * at 3078000. The device will die around 2743000.
> + */
> +static const struct cpcap_battery_config cpcap_battery_eb41_data = {
> +	.cd_factor = 0x3cc,
> +	.info.technology = POWER_SUPPLY_TECHNOLOGY_LION,
> +	.info.voltage_max_design = 4351000,
> +	.info.voltage_min_design = 3100000,
> +	.info.charge_full_design = 1740000,
> +	.bat.constant_charge_voltage_max_uv = 4200000,
> +};
> +
> +/* Values for the extended Droid Bionic battery bw8x. */
> +static const struct cpcap_battery_config cpcap_battery_bw8x_data = {
> +	.cd_factor = 0x3cc,
> +	.info.technology = POWER_SUPPLY_TECHNOLOGY_LION,
> +	.info.voltage_max_design = 4200000,
> +	.info.voltage_min_design = 3200000,
> +	.info.charge_full_design = 2760000,
> +	.bat.constant_charge_voltage_max_uv = 4200000,
> +};
> +
> +/*
> + * Safe values for any lipo battery likely to fit into a mapphone
> + * battery bay.
> + */
> +static const struct cpcap_battery_config cpcap_battery_unkown_data = {
> +	.cd_factor = 0x3cc,
> +	.info.technology = POWER_SUPPLY_TECHNOLOGY_LION,
> +	.info.voltage_max_design = 4200000,
> +	.info.voltage_min_design = 3200000,
> +	.info.charge_full_design = 3000000,
> +	.bat.constant_charge_voltage_max_uv = 4200000,
> +};
> +
> +static int cpcap_battery_match_nvmem(struct device *dev, const void *data)
> +{
> +	if (strcmp(dev_name(dev), "89-500029ba0f73") == 0)
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +static void cpcap_battery_detect_battery_type(struct cpcap_battery_ddata *ddata)
> +{
> +	struct nvmem_device *nvmem;
> +	u8 battery_id = 0;
> +
> +	ddata->check_nvmem = false;
> +
> +	nvmem = nvmem_device_find(NULL, &cpcap_battery_match_nvmem);
> +	if (IS_ERR_OR_NULL(nvmem)) {
> +		ddata->check_nvmem = true;
> +		if (!ddata->no_nvmem_warned) {
> +			dev_info(ddata->dev, "Can not find battery nvmem device. Assuming generic lipo battery\n");
> +			ddata->no_nvmem_warned = true;
> +		}

Please use dev_info_once() and drop no_nvmem_warned.

> +	} else if (nvmem_device_read(nvmem, 2, 1, &battery_id) < 0) {
> +		battery_id = 0;
> +		ddata->check_nvmem = true;
> +		dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
> +	}
> +
> +	switch (battery_id) {
> +	case CPCAP_BATTERY_EB41_HW4X_ID:
> +		memcpy(&ddata->config, &cpcap_battery_eb41_data, sizeof(ddata->config));
> +		break;
> +	case CPCAP_BATTERY_BW8X_ID:
> +		memcpy(&ddata->config, &cpcap_battery_bw8x_data, sizeof(ddata->config));
> +		break;
> +	default:
> +		memcpy(&ddata->config, &cpcap_battery_unkown_data, sizeof(ddata->config));
> +	}

No need to use memcpy, you can just assign the struct and get free
type checking, e.g.:

ddata->config = cpcap_battery_eb41_data;

> +}
> +
>  /**
>   * cpcap_battery_cc_get_avg_current - read cpcap coulumb counter
>   * @ddata: cpcap battery driver device data
> @@ -571,6 +663,9 @@ static int cpcap_battery_get_property(struct power_supply *psy,
>  	latest = cpcap_battery_latest(ddata);
>  	previous = cpcap_battery_previous(ddata);
>  
> +	if (ddata->check_nvmem)
> +		cpcap_battery_detect_battery_type(ddata);
> +
>  	switch (psp) {
>  	case POWER_SUPPLY_PROP_PRESENT:
>  		if (latest->temperature > CPCAP_NO_BATTERY || ignore_temperature_probe)
> @@ -969,30 +1064,10 @@ static int cpcap_battery_calibrate(struct cpcap_battery_ddata *ddata)
>  	return error;
>  }
>  
> -/*
> - * Based on the values from Motorola mapphone Linux kernel. In the
> - * the Motorola mapphone Linux kernel tree the value for pm_cd_factor
> - * is passed to the kernel via device tree. If it turns out to be
> - * something device specific we can consider that too later.
> - *
> - * And looking at the battery full and shutdown values for the stock
> - * kernel on droid 4, full is 4351000 and software initiates shutdown
> - * at 3078000. The device will die around 2743000.
> - */
> -static const struct cpcap_battery_config cpcap_battery_default_data = {
> -	.cd_factor = 0x3cc,
> -	.info.technology = POWER_SUPPLY_TECHNOLOGY_LION,
> -	.info.voltage_max_design = 4351000,
> -	.info.voltage_min_design = 3100000,
> -	.info.charge_full_design = 1740000,
> -	.bat.constant_charge_voltage_max_uv = 4200000,
> -};
> -
>  #ifdef CONFIG_OF
>  static const struct of_device_id cpcap_battery_id_table[] = {
>  	{
>  		.compatible = "motorola,cpcap-battery",
> -		.data = &cpcap_battery_default_data,
>  	},
>  	{},
>  };
> @@ -1010,31 +1085,21 @@ static const struct power_supply_desc cpcap_charger_battery_desc = {
>  	.external_power_changed = cpcap_battery_external_power_changed,
>  };
>  
> +

spurious newline.

>  static int cpcap_battery_probe(struct platform_device *pdev)
>  {
>  	struct cpcap_battery_ddata *ddata;
> -	const struct of_device_id *match;
>  	struct power_supply_config psy_cfg = {};
>  	int error;
>  
> -	match = of_match_device(of_match_ptr(cpcap_battery_id_table),
> -				&pdev->dev);
> -	if (!match)
> -		return -EINVAL;
> -
> -	if (!match->data) {
> -		dev_err(&pdev->dev, "no configuration data found\n");
> -
> -		return -ENODEV;
> -	}
> -
>  	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
>  	if (!ddata)
>  		return -ENOMEM;
>  
> +	cpcap_battery_detect_battery_type(ddata);
> +
>  	INIT_LIST_HEAD(&ddata->irq_list);
>  	ddata->dev = &pdev->dev;
> -	memcpy(&ddata->config, match->data, sizeof(ddata->config));
>  
>  	ddata->reg = dev_get_regmap(ddata->dev->parent, NULL);
>  	if (!ddata->reg)

Otherwise LGTM.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]