Re: [PATCH v2 3/4] hwmon: chipcap2: enable IRQ processing when regulator is already enabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Bypassing `regulator_enable()` but calling `regulator_disable()` leads to unbalanced operations and kernel WARNs.
- [High] Incorrect endianness conversion for I2C SMBus word write corrupts data on Big Endian architectures.
- [Medium] `regulator_is_enabled()` can return negative error codes, which are improperly evaluated as true.
- [Medium] The early return path skips clearing pending completions, potentially causing stale reads.
- [Medium] Alarm configuration (command mode) fails unconditionally with timeouts if the regulator is always-on.
- [Low] `try_wait_for_completion()` is improperly used to reset the completion structure.
--

--- Patch [3]: [PATCH v2 3/4] hwmon: chipcap2: enable IRQ processing when regulator is already enabled ---
commit d7e6228cb08396bc9407928fe3b3e28aa4aaf084
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

hwmon: chipcap2: enable IRQ processing when regulator is already enabled

cc2_enable() returns early when the regulator is already enabled.
This can happen if a previous regulator_disable() failed or if the
regulator was configured as always-on.

> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -119,8 +119,10 @@ static int cc2_enable(struct cc2_data *data)
>  	int ret;
>  
>  	/* exclusive regulator, check in case a disable failed */
> -	if (regulator_is_enabled(data->regulator))
> +	if (regulator_is_enabled(data->regulator)) {

[Severity: Medium]
This is a pre-existing issue, but regulator_is_enabled() can return negative
error codes. Does this condition in cc2_enable() evaluate to true for negative
error codes like -EINVAL, incorrectly bypassing the enable sequence?

Similarly, in cc2_disable(), this check incorrectly tries to disable the
regulator if its state check failed.

[Severity: High]
This is a pre-existing issue, but returning early here bypasses taking a
software reference via regulator_enable() if the regulator is physically
enabled.

If the regulator is physically always-on (e.g., fixed-regulator),
regulator_is_enabled() returns 1, and cc2_enable() skips regulator_enable().
Then cc2_disable() calls regulator_disable() without a matching enable,
triggering a kernel WARN because the consumer's enable_count is 0.

Should this code ensure regulator_enable() is always called to maintain
balance?

[Severity: Medium]
This is a pre-existing issue, but does the command mode retry logic in
cc2_command_mode_start() fundamentally assume regulator_disable() guarantees a
physical power cycle?

cc2_command_mode_start() calls cc2_disable() followed by an msleep() to power
cycle the device. If the regulator is always-on, power is never physically
removed, the 10ms startup window is permanently missed, and the START_CM
command is ignored, causing alarm configuration to fail with -ETIMEDOUT.

> +		data->process_irqs = true;
>  		return 0;

[Severity: Medium]
This is a pre-existing issue, but does this early return skip clearing the
pending completion?

If a late interrupt from a prior timeout leaves data->complete.done > 0, and
the regulator is already enabled, the early return in cc2_enable() bypasses
the reset.

Subsequently, the wait in cc2_read_measurement():

ret = wait_for_completion_timeout(&data->complete, timeout);

will succeed immediately, potentially fetching stale data prematurely before
the sensor finishes its measurement, returning -EBUSY.

> +	}
>  
>  	/* clear any pending completion */
>  	try_wait_for_completion(&data->complete);

[Severity: Low]
This is a pre-existing issue, but is try_wait_for_completion() improperly used
to reset the completion structure?

try_wait_for_completion() only decrements the done counter by 1. If multiple
interrupts occurred, done might be > 1, so the next wait could succeed
immediately.

Should this use reinit_completion() instead?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the combination of cpu_to_be16s() and
i2c_smbus_write_word_data() in cc2_write_reg() write swapped byte sequences on
Big Endian platforms?

On Big Endian systems, cpu_to_be16s() is a no-op. i2c_smbus_write_word_data()
mathematically sends the least significant 8 bits first. Thus, on Big Endian,
the original LSB is sent first, whereas the sensor expects MSB first,
which corrupts the alarm threshold configuration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823-chipcap2_locks-v2-0-6a26c8e9e2fc@gmail.com?part=3