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

[Samuel Kayode]

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