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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `sgm4154x_charger_get_property()` silently overwrites `sgm->state`, destroying the edge detection history required by the IRQ handler.
- [Low] Build failure and missing hardware initialization due to non-existent `.init` field in `struct power_supply_desc`
- [Low] `sgm4154x_resume()` unconditionally queues the watchdog worker even when the watchdog is disabled.
--

--- Patch [2]: [PATCH V10 2/4] power: supply: sgm41542: Add SG Micro sgm41542 charger ---
commit 55bbb1d289f7a6f8202dd62e6767e32f2ffb138b
Author: Chris Morgan <macromorgan@hotmail.com>

power: supply: sgm41542: Add SG Micro sgm41542 charger

Adds support for the SG Micro SGM41542 charger/boost converter.
Driver was adapted from Rockchip BSP driver and confirmed with
vendor datasheet.

[1] https://github.com/rockchip-linux/kernel/blob/develop-6.6/drivers/power/supply/sgm41542_charger.c
[2] https://www.sg-micro.de/rect/assets/1e8de70b-657e-4156-be68-a64fdbe8e418/SGM41541_SGM41542.pdf

> diff --git a/drivers/power/supply/sgm41542_charger.c b/drivers/power/supply/sgm41542_charger.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/power/supply/sgm41542_charger.c

[ ... ]

> +static int sgm4154x_charger_get_property(struct power_supply *psy,
> +					 enum power_supply_property psp,
> +					 union power_supply_propval *val)
> +{
> +	struct sgm4154x_device *sgm = power_supply_get_drvdata(psy);
> +	struct sgm4154x_state state;
> +	int ret;
> +
> +	scoped_guard(mutex, &sgm->lock) {
> +		ret = sgm4154x_get_state(sgm, &state);
> +		if (ret) {
> +			dev_err(sgm->dev, "get state error!\n");
> +			return ret;
> +		}
> +		sgm->state = state;

[Severity: High]
Does this sysfs property read overwrite the state history needed by the IRQ
handler?

If userspace reads a property (like status) after a hardware state change
but before the IRQ handler thread runs and acquires sgm->lock, this will
overwrite the global sgm->state.

[ ... ]

> +static irqreturn_t sgm4154x_irq_handler_thread(int irq, void *private)
> +{
> +	struct sgm4154x_device *sgm = private;
> +	struct sgm4154x_state oldstate, state;
> +	int ret;
> +
> +	guard(mutex)(&sgm->lock);
> +
> +	oldstate = sgm->state;
> +
> +	ret = sgm4154x_get_state(sgm, &state);
> +	if (ret) {
> +		dev_err(sgm->dev, "get state error!\n");
> +		return IRQ_NONE;
> +	}
> +
> +	sgm->state = state;
> +
> +	if (state.vbus_gd && !oldstate.vbus_gd) {

Because oldstate was prematurely updated by the sysfs read above, this edge
detection will evaluate to false. Will this bypass the input limit restoration
logic entirely?

[ ... ]

> +static struct power_supply_desc sgm4154x_power_supply_desc = {
> +	.name = "sgm4154x-charger",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.init = sgm4154x_hw_init,

[Severity: Low]
Will this cause a build failure? 

The struct power_supply_desc in the mainline kernel does not have an .init
field. 

If this assignment is removed to fix the build, the hardware initialization
function sgm4154x_hw_init() will be completely orphaned, leaving the watchdog,
limits, and defaults unconfigured. Should this initialization be called
manually from the probe function instead?

[ ... ]

> +static int __maybe_unused sgm4154x_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sgm4154x_device *sgm = i2c_get_clientdata(client);
> +	int ret;
> +
> +	if (sgm->watchdog_enable) {
> +		ret = sgm4154x_set_watchdog_timer(sgm, SGM4154X_WDT_TIMER_40S);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	queue_delayed_work(sgm->sgm_monitor_wq,
> +			   &sgm->sgm_delay_work, 0);

[Severity: Low]
Does this unconditionally queue the watchdog worker even when the watchdog
is disabled?

If the watchdog was disabled prior to suspend, queue_delayed_work() will
still execute immediately upon resume. This causes an unnecessary I2C
transaction to reset a disabled watchdog timer before the worker terminates
itself. Should this queue call be moved inside the if block?

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728203832.166402-1-macroalpha82@gmail.com?part=2