Re: [PATCH] regulator: sgm3804: handle gpiod_get_value_cansleep() error returns

[neil.armstrong@linaro.org]

On 7/24/26 04:46, kr494167@gmail.com wrote:
> From: Surendra Singh Chouhan <kr494167@gmail.com>
> 
> sgm3804_sync_regcache_state() evaluated !gpiod_get_value_cansleep(...) to
> check if the enable GPIOs were down.
> 
> gpiod_get_value_cansleep() returns 1 if active, 0 if inactive, and a
> negative error code (e.g. -EIO or -EINVAL) on failure. Evaluating
> !gpiod_get_value_cansleep(...) treats a negative error code as boolean
> false, swallowing GPIO read failures and misinterpreting a failed read as
> an active GPIO. This improperly attempts to sync regcache registers
> when the IC may be powered down.
> 
> Fix this by capturing the return values of gpiod_get_value_cansleep() for
> both positive and negative rails and propagating any error immediately.
> In addition, replace mutex_init() with devm_mutex_init() in probe().
> 
> Fixes: 0c47e1a8cf5d ("regulator: add SGM3804 Dual Output driver")
> Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
> ---
>   drivers/regulator/sgm3804-regulator.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/sgm3804-regulator.c b/drivers/regulator/sgm3804-regulator.c
> index c3406cfb73d0..a430c89b4d5d 100644
> --- a/drivers/regulator/sgm3804-regulator.c
> +++ b/drivers/regulator/sgm3804-regulator.c
> @@ -97,11 +97,20 @@ static const struct regmap_config sgm3804_regmap_config = {
>   
>   static int sgm3804_sync_regcache_state(struct sgm3804_data *ctx)
>   {
> +	int pos, neg;
> +
>   	guard(mutex)(&ctx->lock);
>   
> +	pos = gpiod_get_value_cansleep(ctx->gpios[SGM3804_POS_RAIL]);
> +	if (pos < 0)
> +		return pos;
> +
> +	neg = gpiod_get_value_cansleep(ctx->gpios[SGM3804_NEG_RAIL]);
> +	if (neg < 0)
> +		return neg;
> +
>   	/* If both GPIOs are down, IC is powered down and I2C writes will fail */
> -	if (!gpiod_get_value_cansleep(ctx->gpios[SGM3804_POS_RAIL]) &&
> -	    !gpiod_get_value_cansleep(ctx->gpios[SGM3804_NEG_RAIL])) {
> +	if (!pos && !neg) {
>   		regcache_cache_only(ctx->regmap, true);
>   		regcache_mark_dirty(ctx->regmap);
>   	} else {
> @@ -235,7 +244,9 @@ static int sgm3804_probe(struct i2c_client *i2c)
>   	if (!ctx)
>   		return -ENOMEM;
>   
> -	mutex_init(&ctx->lock);
> +	ret = devm_mutex_init(dev, &ctx->lock);
> +	if (ret)
> +		return ret;
>   
>   	ctx->regmap = devm_regmap_init_i2c(i2c, &sgm3804_regmap_config);
>   	if (IS_ERR(ctx->regmap))

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks,
Neil