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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The hardware watchdog timer remains permanently disabled after the first suspend/resume cycle.
- [High] Concurrent sysfs reads mutate internal state without locking, silently swallowing IRQ edges.
- [High] The power supply is registered and exposed to userspace before driver data and hardware defaults are initialized.
- [Medium] Ignored return values from regmap_update_bits during hardware initialization.
- [Low] Pointer type mismatch when calling regmap_read.
--

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

This commit adds support for the SG Micro SGM41542 charger/boost converter.
The driver adapts a vendor BSP implementation to register a power supply,
initialize I2C regmap, and manage charge parameters.

Link: https://github.com/rockchip-linux/kernel/blob/develop-6.6/drivers/power/supply/sgm41542_charger.c
Link: 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
> index 0000000000000..dbce7ed2f8796
> --- /dev/null
> +++ b/drivers/power/supply/sgm41542_charger.c
> @@ -0,0 +1,1118 @@
[ ... ]
> +static int sgm4154x_get_state(struct sgm4154x_device *sgm,
> +			      struct sgm4154x_state *state)
> +{
> +	int reg, ret;
> +
> +	ret = regmap_read(sgm->regmap, SGM4154X_CHRG_STAT, &reg);

[Severity: Low]
Does passing an int pointer to regmap_read() cause a compiler warning here
since the API expects an unsigned int pointer?

[ ... ]
> +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;
> +
> +	ret = sgm4154x_get_state(sgm, &state);
> +	if (ret) {
> +		dev_err(sgm->dev, "get state error!\n");
> +		return ret;
> +	}
> +	sgm->state = state;

[Severity: High]
Could this un-locked assignment overwrite the cached state and break
interrupt edge detection?

If a sysfs read races with a VBUS insertion interrupt, this might update
sgm->state.vbus_gd to true before the IRQ thread runs. Then in
sgm4154x_irq_handler_thread():

    if (state.vbus_gd && !oldstate.vbus_gd) {

Since oldstate caches the overwritten sgm->state, it would evaluate to
false and silently drop the plug-in event, failing to apply the input
current limit. Should this assignment be protected by sgm->lock, or removed
if the getter only needs local state?

[ ... ]
> +static int sgm4154x_hw_init(struct sgm4154x_device *sgm)
> +{
[ ... ]
> +	regmap_update_bits(sgm->regmap,
> +			   SGM4154X_CHRG_CTRL_D,
> +			   SGM4154X_JEITA_EN,
> +			   0);
> +
> +	regmap_update_bits(sgm->regmap,
> +			   SGM4154X_CHRG_CTRL_A,
> +			   SGM4154X_IINDPM_INT_MASK,
> +			   SGM4154X_IINDPM_INT_MASK);
> +
> +	regmap_update_bits(sgm->regmap,
> +			   SGM4154X_CHRG_CTRL_A,
> +			   SGM4154X_VINDPM_INT_MASK,
> +			   SGM4154X_VINDPM_INT_MASK);

[Severity: Medium]
Is it intentional to ignore the return values from these hardware
configuration writes, unlike the surrounding calls in this function?

If the I2C bus encounters a communication error, this could silently fail
to configure the JEITA and DPM interrupt masks.

[ ... ]
> +static void sgm_charger_work(struct work_struct *work)
> +{
> +	struct sgm4154x_device *sgm =
> +		container_of(work,
> +			     struct sgm4154x_device,
> +			     sgm_delay_work.work);
> +
> +	sgm4154x_watchdog_timer_reset(sgm);
> +	if (sgm->watchdog_enable)
> +		queue_delayed_work(sgm->sgm_monitor_wq,
> +				   &sgm->sgm_delay_work,
> +				   msecs_to_jiffies(1000 * 5));
> +}

[Severity: High]
Can the hardware watchdog timer remain permanently disabled after a
suspend and resume cycle?

During suspend, sgm4154x_suspend() calls sgm4154x_set_watchdog_timer()
with SGM4154X_WDT_TIMER_DISABLE. Upon resume, sgm4154x_resume() restarts
this workqueue.

The worker function above pets the watchdog by setting WDT_RST, but does
it ever re-program the CHRG_CTRL_5 configuration register to re-enable the
timer duration? It seems the safety mechanism might remain turned off.

[ ... ]
> +static int sgm4154x_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = devm_delayed_work_autocancel(dev, &sgm->sgm_delay_work,
> +					   sgm_charger_work);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Unable to register delayed work\n");
> +
> +	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");

[Severity: High]
Does registering the power supply before hw_init expose uninitialized
limits to userspace?

If userspace triggers a sysfs write to change the charge voltage
immediately after registration but before sgm4154x_hw_init() populates
the default limits, sgm4154x_set_chrg_volt() might clamp the request
against a zero-value sgm->init_data.max_vreg, potentially misconfiguring
the hardware to its absolute minimum voltage. Should the hardware limits
be initialized before registering the power supply?

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