Re: [PATCH 2/6] power: supply: sgm41542: Add SG Micro sgm41542 charger

[Krzysztof Kozlowski <krzk@kernel.org>]

On Mon, Apr 27, 2026 at 12:09:10PM -0500, Chris Morgan wrote:
> +	strscpy(sgm->model_name, id->name, I2C_NAME_SIZE);
> +
> +	sgm->regmap = devm_regmap_init_i2c(client, &sgm4154x_regmap_config);
> +	if (IS_ERR(sgm->regmap))
> +		return dev_err_probe(dev, PTR_ERR(sgm->regmap),
> +				     "Failed to allocate register map\n");
> +
> +	i2c_set_clientdata(client, sgm);
> +
> +	ret = sgm4154x_hw_chipid_detect(sgm);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Unable to read HW ID\n");
> +
> +	device_init_wakeup(dev, 1);
> +
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +						sgm4154x_irq_handler_thread,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"sgm41542-irq", sgm);

Interrupt should be requested and enabled at the end or almost at the
end. If it is triggered now, which supply are you marking as "changed"?

Code looks at least correct in relation between interrupt and workqueue,
but it is not really obvious and thus not following established coding
practice. Practice is usually: first you allocate driver-like or
memory-like resources, then you request hardware related resources.
That's why every probe starts with devm_kzalloc(). Similarly with
workqueue, initializing power supply etc.

> +		if (ret)
> +			return ret;
> +		enable_irq_wake(client->irq);
> +	}
> +
> +	ret = sgm4154x_power_supply_init(sgm, dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register power supply\n");
> +
> +	ret = sgm4154x_hw_init(sgm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot initialize the chip.\n");
> +
> +	sgm->sgm_monitor_wq = alloc_ordered_workqueue("%s",
> +			WQ_MEM_RECLAIM | WQ_FREEZABLE, "sgm-monitor-wq");
> +	if (!sgm->sgm_monitor_wq)
> +		return -EINVAL;
> +
> +	ret = devm_add_action_or_reset(dev, sgm4154x_destroy_workqueue,
> +				       sgm->sgm_monitor_wq);

Use rather devm_alloc_ordered_workqueue().

Best regards,
Krzysztof