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

[Samuel Kayode <samuel.kayode@savoirfairelinux.com>]

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