Re: [PATCH v4 5/9] usb: misc: qcom_eud: improve enable_store API

[Peter Chen]

On 26-05-01 10:06:31, Elson Serrao wrote:
> Currently enable_store() allows operations irrespective of the EUD state,
> which can result in redundant operations. Avoid this by adding duplicate
> state checks to skip requests when EUD is already in the desired state.
> Additionally, improve error handling with explicit logging to provide
> better feedback.
> 
> Signed-off-by: Elson Serrao <elson.serrao@xxxxxxxxxxxxxxxx>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>

Reviewed-by: Peter Chen <peter.chen@xxxxxxxxxx>

Peter
> ---
>  drivers/usb/misc/qcom_eud.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 7fbb74bf1599..f656ffc8818a 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -162,18 +162,27 @@ static ssize_t enable_store(struct device *dev,
>  	if (kstrtobool(buf, &enable))
>  		return -EINVAL;
>  
> +	/* Skip operation if already in desired state */
> +	if (chip->enabled == enable)
> +		return count;
> +
>  	if (enable) {
>  		ret = enable_eud(chip);
> -		if (!ret)
> -			chip->enabled = enable;
> -		else
> -			disable_eud(chip);
> -
> +		if (ret) {
> +			dev_err(chip->dev, "failed to enable eud\n");
> +			return ret;
> +		}
>  	} else {
>  		ret = disable_eud(chip);
> +		if (ret) {
> +			dev_err(chip->dev, "failed to disable eud\n");
> +			return ret;
> +		}
>  	}
>  
> -	return ret < 0 ? ret : count;
> +	chip->enabled = enable;
> +
> +	return count;
>  }
>  
>  static DEVICE_ATTR_RW(enable);
> -- 
> 2.34.1
> 
> 

-- 

Thanks,
Peter Chen