linux-kernel - Re: [PATCH v5 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: <aEmS8N8gdz8-6GBD@fedora>
Date: Wed, 11 Jun 2025 10:30:08 -0400
From: Samuel Kayode <samuel.kayode@...oirfairelinux.com>
To: Frank Li <Frank.li@....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>, 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 v5 5/6] power: supply: pf1550: add battery charger support

On Tue, Jun 10, 2025 at 05:11:31PM -0400, Frank Li wrote:
> > +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);
> > +	unsigned int data;
> > +	bool psy_changed = false;
> > +
> > +	if (!chg->charger)
> > +		return;
> > +
> > +	if (regmap_read(chg->pf1550->regmap, PF1550_CHARG_REG_VBUS_SNS, &data)) {
> > +		dev_err(chg->dev, "Read VBUS_SNS error.\n");
> > +		return;
> > +	}
> > +
> > +	mutex_lock(&chg->mutex);
> > +
> > +	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");
> > +	}
> > +
> > +	mutex_unlock(&chg->mutex);
> 
> not sure why need lock here, you just update chg->psy_desc.type?
>
It prevents concurrent access in this delayed work and in pf1550_reg_init.
However, it's unlikely that the probe is still active during the first execution
of the delayed work. So, i also think this mutex can be removed.
> > +
> > +	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;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	int i, irq_type = -1;
> > +
> > +	for (i = 0; i < PF1550_CHARGER_IRQ_NR; i++)
> > +		if (irq == platform_get_irq(pdev, 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 int pf1550_charger_probe(struct platform_device *pdev)
> > +	mutex_init(&chg->mutex);
...
> > +
> > +	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);
...
> > +	ret = pf1550_reg_init(chg);

Thanks,
Sam