Message-ID: <aFwFhYoaWoSXcFdR@fedora>
Date: Wed, 25 Jun 2025 10:19:49 -0400
From: Samuel Kayode <samuel.kayode@...oirfairelinux.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Frank Li <Frank.li@....com>, imx@...ts.linux.dev,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-input@...r.kernel.org, linux-pm@...r.kernel.org,
	Abel Vesa <abelvesa@...nel.org>, Abel Vesa <abelvesa@...ux.com>,
	Robin Gong <b38343@...escale.com>, Robin Gong <yibin.gong@....com>,
	Enric Balletbo i Serra <eballetbo@...il.com>
Subject: Re: [PATCH v7 5/6] power: supply: pf1550: add battery charger support

Hi Sebastian,

Thanks a lot for the review.

On Sun, Jun 22, 2025 at 02:43:36AM +0200, Sebastian Reichel wrote:
> > +static int pf1550_get_battery_health(struct regmap *regmap, int *val)
> > +{
> > +	unsigned int data;
> > +	int ret;
> > +
> > +	ret = regmap_read(regmap, PF1550_CHARG_REG_BATT_SNS, &data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	data &= PF1550_BAT_SNS_MASK;
> > +
> > +	switch (data) {
> > +	case PF1550_BAT_NO_DETECT:
> > +		*val = POWER_SUPPLY_HEALTH_DEAD;
> 
> POWER_SUPPLY_HEALTH_NO_BATTERY ?
Yes, it should be no battery. Will update in the next version.
> 
> > +		break;
> > +	case PF1550_BAT_NO_VBUS:
> > +	case PF1550_BAT_LOW_THAN_PRECHARG:
> > +	case PF1550_BAT_CHARG_FAIL:
> > +	case PF1550_BAT_HIGH_THAN_PRECHARG:
> > +		*val = POWER_SUPPLY_HEALTH_GOOD;
> > +		break;
> > +	case PF1550_BAT_OVER_VOL:
> > +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> > +		break;
> > +	default:
> > +		*val = POWER_SUPPLY_HEALTH_UNKNOWN;
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int pf1550_get_present(struct regmap *regmap, int *val)
> > +{
> > +	unsigned int data;
> > +	int ret;
> > +
> > +	ret = regmap_read(regmap, PF1550_CHARG_REG_BATT_SNS, &data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	data &= PF1550_BAT_SNS_MASK;
> > +	*val = (data == PF1550_BAT_NO_DETECT) ? 0 : 1;
> > +
> > +	return 0;
> > +}
> 
> You can drop this function + property. It's meant for a battery type
> device.
The pf1550 switches the load between battery and external power depending on
whichever is available.
> > +static void pf1550_chg_bat_work(struct work_struct *work)
> > +{
> > +	struct pf1550_charger *chg = container_of(to_delayed_work(work),
> > +						  struct pf1550_charger,
> > +						  bat_sense_work);
> > +	unsigned int data;
> > +
> > +	if (!chg->charger)
> > +		return;
> 
> This can't be called without a chg->charger being set.
> 
Will drop.
> > +	if (regmap_read(chg->pf1550->regmap, PF1550_CHARG_REG_BATT_SNS, &data)) {
> > +		dev_err(chg->dev, "Read BATT_SNS error.\n");
> > +		return;
> > +	}
> > +
> > +	switch (data & PF1550_BAT_SNS_MASK) {
> > +	case PF1550_BAT_NO_VBUS:
> > +		dev_dbg(chg->dev, "No valid VBUS input.\n");
> > +		break;
> > +	case PF1550_BAT_LOW_THAN_PRECHARG:
> > +		dev_dbg(chg->dev, "VBAT < VPRECHG.LB.\n");
> > +		break;
> > +	case PF1550_BAT_CHARG_FAIL:
> > +		dev_dbg(chg->dev, "Battery charging failed.\n");
> > +		break;
> > +	case PF1550_BAT_HIGH_THAN_PRECHARG:
> > +		dev_dbg(chg->dev, "VBAT > VPRECHG.LB.\n");
> > +		break;
> > +	case PF1550_BAT_OVER_VOL:
> > +		dev_dbg(chg->dev, "VBAT > VBATOV.\n");
> > +		break;
> > +	case PF1550_BAT_NO_DETECT:
> > +		dev_dbg(chg->dev, "Battery not detected.\n");
> > +		break;
> > +	default:
> > +		dev_err(chg->dev, "Unknown value read:%x\n",
> > +			data & PF1550_CHG_SNS_MASK);
> > +	}
> 
> So the whole handler is just for debug purposes?
>
Yes, it is solely for debugging.
> > +}
> > +
> > +static void pf1550_chg_chg_work(struct work_struct *work)
> > +{
> > +	struct pf1550_charger *chg = container_of(to_delayed_work(work),
> > +						  struct pf1550_charger,
> > +						  chg_sense_work);
> > +	unsigned int data;
> > +
> > +	if (!chg->charger)
> > +		return;
> 
> same as pf1550_chg_bat_work.
> 
Will drop.
> > +	if (regmap_read(chg->pf1550->regmap, PF1550_CHARG_REG_CHG_SNS, &data)) {
> > +		dev_err(chg->dev, "Read CHG_SNS error.\n");
> > +		return;
> > +	}
> > +
> > +	switch (data & PF1550_CHG_SNS_MASK) {
> > +	case PF1550_CHG_PRECHARGE:
> > +		dev_dbg(chg->dev, "In pre-charger mode.\n");
> > +		break;
> > +	case PF1550_CHG_CONSTANT_CURRENT:
> > +		dev_dbg(chg->dev, "In fast-charge constant current mode.\n");
> > +		break;
> > +	case PF1550_CHG_CONSTANT_VOL:
> > +		dev_dbg(chg->dev, "In fast-charge constant voltage mode.\n");
> > +		break;
> > +	case PF1550_CHG_EOC:
> > +		dev_dbg(chg->dev, "In EOC mode.\n");
> > +		break;
> > +	case PF1550_CHG_DONE:
> > +		dev_dbg(chg->dev, "In DONE mode.\n");
> > +		break;
> > +	case PF1550_CHG_TIMER_FAULT:
> > +		dev_info(chg->dev, "In timer fault mode.\n");
> > +		break;
> > +	case PF1550_CHG_SUSPEND:
> > +		dev_info(chg->dev, "In thermistor suspend mode.\n");
> > +		break;
> > +	case PF1550_CHG_OFF_INV:
> > +		dev_info(chg->dev, "Input invalid, charger off.\n");
> > +		break;
> > +	case PF1550_CHG_BAT_OVER:
> > +		dev_info(chg->dev, "Battery over-voltage.\n");
> 
> dev_warn possibly?
>
Will update.
> > +		break;
> > +	case PF1550_CHG_OFF_TEMP:
> > +		dev_info(chg->dev, "Temp high, charger off.\n");
> > +		break;
> > +	case PF1550_CHG_LINEAR_ONLY:
> > +		dev_dbg(chg->dev, "In Linear mode, not charging.\n");
> > +		break;
> > +	default:
> > +		dev_err(chg->dev, "Unknown value read:%x\n",
> > +			data & PF1550_CHG_SNS_MASK);
> > +	}
> 
> also all just for debug and othewise the IRQ is not used?
>
Yes, this is also just for debug.
> > +}
> > +
> > +static void pf1550_chg_vbus_work(struct work_struct *work)
> > +{
> > +	struct pf1550_charger *chg = container_of(to_delayed_work(work),
> > +						  struct pf1550_charger,
> > +						  vbus_sense_work);
> > +	bool psy_changed = false;
> > +	unsigned int data;
> > +
> > +	if (!chg->charger)
> > +		return;
> 
> same as pf1550_chg_bat_work.
>
Will drop.
> > +	if (regmap_read(chg->pf1550->regmap, PF1550_CHARG_REG_VBUS_SNS, &data)) {
> > +		dev_err(chg->dev, "Read VBUS_SNS error.\n");
> > +		return;
> > +	}
> > +
> > +	if (data & PF1550_VBUS_UVLO) {
> > +		chg->psy_desc.type = POWER_SUPPLY_TYPE_BATTERY;
> > +		psy_changed = true;
> > +		dev_dbg(chg->dev, "VBUS detached.\n");
> > +	}
> > +	if (data & PF1550_VBUS_IN2SYS)
> > +		dev_dbg(chg->dev, "VBUS_IN2SYS_SNS.\n");
> > +	if (data & PF1550_VBUS_OVLO)
> > +		dev_dbg(chg->dev, "VBUS_OVLO_SNS.\n");
> > +	if (data & PF1550_VBUS_VALID) {
> > +		chg->psy_desc.type = POWER_SUPPLY_TYPE_MAINS;
> > +		psy_changed = true;
> > +		dev_dbg(chg->dev, "VBUS attached.\n");
> > +	}
> 
> Changing the power_supply type dynamically is a big no go. FWIW I'm
> not sure how you came up with this weird type change in the first
> place. As far as I can see this is a normal charger and should just
> set POWER_SUPPLY_TYPE_MAINS in the probe routine and not touch it
> otherwise. 
>
The pf1550 charger receives a VBUS power input which can be provided either by
an AC adapter or a USB bus. A depleted battery is charged using the VBUS power
input (VBUSIN). When no power is supplied to VBUSIN, the pf1550 switches the
load to the connected non-depleted battery.

I could have two power_supply_desc, one for battery and one for the external
power?
> > +
> > +	if (psy_changed)
> > +		power_supply_changed(chg->charger);
> > +}
> > +
> > +static irqreturn_t pf1550_charger_irq_handler(int irq, void *data)
> > +{
> > +	struct pf1550_charger *chg = data;
> > +	struct device *dev = chg->dev;
> > +	int i, irq_type = -1;
> > +
> > +	for (i = 0; i < PF1550_CHARGER_IRQ_NR; i++)
> > +		if (irq == chg->virqs[i])
> > +			irq_type = i;
> > +
> > +	switch (irq_type) {
> > +	case PF1550_CHARG_IRQ_BAT2SOCI:
> > +		dev_info(dev, "BAT to SYS Overcurrent interrupt.\n");
> > +		break;
> > +	case PF1550_CHARG_IRQ_BATI:
> > +		schedule_delayed_work(&chg->bat_sense_work,
> > +				      msecs_to_jiffies(10));
> > +		break;
> > +	case PF1550_CHARG_IRQ_CHGI:
> > +		schedule_delayed_work(&chg->chg_sense_work,
> > +				      msecs_to_jiffies(10));
> > +		break;
> > +	case PF1550_CHARG_IRQ_VBUSI:
> > +		schedule_delayed_work(&chg->vbus_sense_work,
> > +				      msecs_to_jiffies(10));
> > +		break;
> > +	case PF1550_CHARG_IRQ_THMI:
> > +		dev_info(dev, "Thermal interrupt.\n");
> > +		break;
> > +	default:
> > +		dev_err(dev, "unknown interrupt occurred.\n");
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static enum power_supply_property pf1550_charger_props[] = {
> > +	POWER_SUPPLY_PROP_STATUS,
> > +	POWER_SUPPLY_PROP_CHARGE_TYPE,
> > +	POWER_SUPPLY_PROP_HEALTH,
> > +	POWER_SUPPLY_PROP_PRESENT,
> > +	POWER_SUPPLY_PROP_ONLINE,
> > +	POWER_SUPPLY_PROP_MODEL_NAME,
> > +	POWER_SUPPLY_PROP_MANUFACTURER,
> > +};
> > +
> > +static int pf1550_charger_get_property(struct power_supply *psy,
> > +				       enum power_supply_property psp,
> > +				       union power_supply_propval *val)
> > +{
> > +	struct pf1550_charger *chg = power_supply_get_drvdata(psy);
> > +	struct regmap *regmap = chg->pf1550->regmap;
> > +	int ret = 0;
> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_STATUS:
> > +		ret = pf1550_get_charger_state(regmap, &val->intval);
> > +		break;
> > +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> > +		ret = pf1550_get_charge_type(regmap, &val->intval);
> > +		break;
> > +	case POWER_SUPPLY_PROP_HEALTH:
> > +		ret = pf1550_get_battery_health(regmap, &val->intval);
> > +		break;
> > +	case POWER_SUPPLY_PROP_PRESENT:
> > +		ret = pf1550_get_present(regmap, &val->intval);
> > +		break;
> > +	case POWER_SUPPLY_PROP_ONLINE:
> > +		ret = pf1550_get_online(regmap, &val->intval);
> > +		break;
> > +	case POWER_SUPPLY_PROP_MODEL_NAME:
> > +		val->strval = "PF1550";
> > +		break;
> > +	case POWER_SUPPLY_PROP_MANUFACTURER:
> > +		val->strval = "NXP";
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int pf1550_set_constant_volt(struct pf1550_charger *chg,
> > +				    unsigned int uvolt)
> > +{
> > +	unsigned int data;
> > +
> > +	if (uvolt >= 3500000 && uvolt <= 4440000)
> > +		data = 8 + (uvolt - 3500000) / 20000;
> > +	else
> > +		return dev_err_probe(chg->dev, -EINVAL,
> > +				     "Wrong value for constant voltage\n");
> > +
> > +	dev_dbg(chg->dev, "Charging constant voltage: %u (0x%x)\n", uvolt,
> > +		data);
> > +
> > +	return regmap_update_bits(chg->pf1550->regmap,
> > +				  PF1550_CHARG_REG_BATT_REG,
> > +				  PF1550_CHARG_REG_BATT_REG_CHGCV_MASK, data);
> > +}
> > +
> > +static int pf1550_set_min_system_volt(struct pf1550_charger *chg,
> > +				      unsigned int uvolt)
> > +{
> > +	unsigned int data;
> > +
> > +	switch (uvolt) {
> > +	case 3500000:
> > +		data = 0x0;
> > +		break;
> > +	case 3700000:
> > +		data = 0x1;
> > +		break;
> > +	case 4300000:
> > +		data = 0x2;
> > +		break;
> > +	default:
> > +		return dev_err_probe(chg->dev, -EINVAL,
> > +				     "Wrong value for minimum system voltage\n");
> > +	}
> > +
> > +	data <<= PF1550_CHARG_REG_BATT_REG_VMINSYS_SHIFT;
> > +
> > +	dev_dbg(chg->dev, "Minimum system regulation voltage: %u (0x%x)\n",
> > +		uvolt, data);
> > +
> > +	return regmap_update_bits(chg->pf1550->regmap,
> > +				  PF1550_CHARG_REG_BATT_REG,
> > +				  PF1550_CHARG_REG_BATT_REG_VMINSYS_MASK, data);
> > +}
> > +
> > +static int pf1550_set_thermal_regulation_temp(struct pf1550_charger *chg,
> > +					      unsigned int cells)
> > +{
> > +	unsigned int data;
> > +
> > +	switch (cells) {
> > +	case 60:
> > +		data = 0x0;
> > +		break;
> > +	case 75:
> > +		data = 0x1;
> > +		break;
> > +	case 90:
> > +		data = 0x2;
> > +		break;
> > +	case 105:
> > +		data = 0x3;
> > +		break;
> > +	default:
> > +		return dev_err_probe(chg->dev, -EINVAL,
> > +				     "Wrong value for thermal temperature\n");
> > +	}
> > +
> > +	data <<= PF1550_CHARG_REG_THM_REG_CNFG_REGTEMP_SHIFT;
> > +
> > +	dev_dbg(chg->dev, "Thermal regulation loop temperature: %u (0x%x)\n",
> > +		cells, data);
> > +
> > +	return regmap_update_bits(chg->pf1550->regmap,
> > +				  PF1550_CHARG_REG_THM_REG_CNFG,
> > +				  PF1550_CHARG_REG_THM_REG_CNFG_REGTEMP_MASK,
> > +				  data);
> > +}
> > +
> > +/*
> > + * Sets charger registers to proper and safe default values.
> > + */
> > +static int pf1550_reg_init(struct pf1550_charger *chg)
> > +{
> > +	struct device *dev = chg->dev;
> > +	unsigned int data;
> > +	int ret;
> > +
> > +	/* Unmask charger interrupt, mask DPMI and reserved bit */
> > +	ret =  regmap_write(chg->pf1550->regmap, PF1550_CHARG_REG_CHG_INT_MASK,
> > +			    PF1550_CHG_INT_MASK);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Error unmask charger interrupt\n");
> > +
> > +	ret = regmap_read(chg->pf1550->regmap, PF1550_CHARG_REG_VBUS_SNS,
> > +			  &data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Read charg vbus_sns error\n");
> > +
> > +	if (data & PF1550_VBUS_VALID)
> > +		chg->psy_desc.type = POWER_SUPPLY_TYPE_MAINS;
> > +
> > +	ret = pf1550_set_constant_volt(chg, chg->constant_volt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = pf1550_set_min_system_volt(chg, chg->min_system_volt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = pf1550_set_thermal_regulation_temp(chg,
> > +						 chg->thermal_regulation_temp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Turn on charger */
> > +	ret = regmap_write(chg->pf1550->regmap, PF1550_CHARG_REG_CHG_OPER,
> > +			   PF1550_CHG_TURNON);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Error turn on charger\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static void pf1550_dt_parse_dev_info(struct pf1550_charger *chg)
> > +{
> > +	struct power_supply_battery_info *info;
> > +	struct device *dev = chg->dev;
> > +
> > +	if (device_property_read_u32(dev->parent, "nxp,min-system-microvolt",
> > +				     &chg->min_system_volt))
> > +		chg->min_system_volt = PF1550_DEFAULT_MIN_SYSTEM_VOLT;
> > +
> > +	if (device_property_read_u32(dev->parent,
> > +				     "nxp,thermal-regulation-celsius",
> > +				     &chg->thermal_regulation_temp))
> > +		chg->thermal_regulation_temp = PF1550_DEFAULT_THERMAL_TEMP;
> > +
> > +	if (power_supply_get_battery_info(chg->charger, &info))
> > +		chg->constant_volt = PF1550_DEFAULT_CONSTANT_VOLT;
> > +	else
> > +		chg->constant_volt = info->constant_charge_voltage_max_uv;
> > +}
> > +
> > +static int pf1550_charger_probe(struct platform_device *pdev)
> > +{
> > +	const struct pf1550_dev *pf1550 = dev_get_drvdata(pdev->dev.parent);
> > +	struct power_supply_config psy_cfg = {};
> > +	struct pf1550_charger *chg;
> > +	int i, irq, ret;
> > +
> > +	chg = devm_kzalloc(&pdev->dev, sizeof(*chg), GFP_KERNEL);
> > +	if (!chg)
> > +		return -ENOMEM;
> > +
> > +	chg->dev = &pdev->dev;
> > +	chg->pf1550 = pf1550;
> > +
> > +	if (!chg->pf1550->regmap)
> > +		return dev_err_probe(&pdev->dev, -ENODEV,
> > +				     "failed to get regmap\n");
> > +
> > +	platform_set_drvdata(pdev, chg);
> > +
> > +	INIT_DELAYED_WORK(&chg->vbus_sense_work, pf1550_chg_vbus_work);
> > +	INIT_DELAYED_WORK(&chg->chg_sense_work, pf1550_chg_chg_work);
> > +	INIT_DELAYED_WORK(&chg->bat_sense_work, pf1550_chg_bat_work);
> 
> use devm_delayed_work_autocancel(). It's not just a cleanup, but
> also fixed a race condition during module removal, as you are right
> now cancelling the job while a new one might be scheduled directly
> afterwards before the power-supply device is deregistered.
>
Will do.
> > +	for (i = 0; i < PF1550_CHARGER_IRQ_NR; i++) {
> > +		irq = platform_get_irq(pdev, i);
> > +		if (irq < 0)
> > +			return irq;
> > +
> > +		chg->virqs[i] = irq;
> > +
> > +		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > +						pf1550_charger_irq_handler,
> > +						IRQF_NO_SUSPEND,
> > +						"pf1550-charger", chg);
> > +		if (ret)
> > +			return dev_err_probe(&pdev->dev, ret,
> > +					     "failed irq request\n");
> > +	}
> > +
> > +	psy_cfg.drv_data = chg;
> > +
> > +	chg->psy_desc.name = PF1550_CHARGER_NAME;
> > +	chg->psy_desc.type = POWER_SUPPLY_TYPE_BATTERY;
> > +	chg->psy_desc.get_property = pf1550_charger_get_property;
> > +	chg->psy_desc.properties = pf1550_charger_props;
> > +	chg->psy_desc.num_properties = ARRAY_SIZE(pf1550_charger_props);
> > +
> > +	chg->charger = devm_power_supply_register(&pdev->dev, &chg->psy_desc,
> > +						  &psy_cfg);
> > +	if (IS_ERR(chg->charger))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(chg->charger),
> > +				     "failed: power supply register\n");
> > +
> > +	pf1550_dt_parse_dev_info(chg);
> > +
> > +	return pf1550_reg_init(chg);
> > +}
> > +

Thanks,
Sam

Powered by blists - more mailing lists