linux-kernel - Re: [PATCH v8 5/6] power: supply: pf1550: add battery charger support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aHE7d3gDITZMWEcH@fedora>
Date: Fri, 11 Jul 2025 12:27:35 -0400
From: Samuel Kayode <samuel.kayode@...oirfairelinux.com>
To: Sean Nyekjaer <sean@...nix.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>,
	Sebastian Reichel <sre@...nel.org>, 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 v8 5/6] power: supply: pf1550: add battery charger support

On Fri, Jul 11, 2025 at 09:02:18AM +0000, Sean Nyekjaer wrote:
> > +#define PF1550_DEFAULT_THERMAL_TEMP	75
> 
> Default is 95
>
Will make changes.
> > +
> > +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;
> 
> From the datasheet 80, 95, 110 and 125c is supported
>
Yeah, the temp values should be as you mentioned. I'll be replacing the current
temperature range.
> > +	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");
> 
> data is unused here :/
>
Yeah, that should be dropped.
> > +
> > +	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");
> 
> There are 3 modes for the charger operation:
> 0: charger = off, linear = off
> 1: charger = off, linear = on
> 2: charger = on, linear = on
> 
> The driver is hardcoded to use no. 2.
> 
> We are using the mode 1, and setting it to 2 causes my system to boot loop.
> 
> I don't know how we should select mode, maybe it could be an option from
> the devicetree or use power_supply_get_battery_info() to look for a
> battery and the only select between 1 or 2, but 0 would also be a valid
> option.
> 
I like the latter solution of using the power_supply_get_battery_info. I think
selecting between only 1 or 2 should be fine.

Thanks,
Sam