Re: [PATCH v4] ufs: allow host driver disable wb toggle druing clock scaling

[Bart Van Assche <bvanassche@acm.org>]

On 8/2/22 20:03, peter.wang@mediatek.com wrote:
> 

disable -> to disable?

toggle -> toggling?

druing -> during?

> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index 0a088b47d557..7f41f2a69b04 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
> @@ -225,7 +225,8 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
>   	unsigned int wb_enable;
>   	ssize_t res;
>   
> -	if (!ufshcd_is_wb_allowed(hba) || ufshcd_is_clkscaling_supported(hba)) {
> +	if (!ufshcd_is_wb_allowed(hba) || (ufshcd_is_clkscaling_supported(hba)
> +		&& ufshcd_enable_wb_if_scaling_up(hba))) {

The "&&" is misplaced - it should occur at the end of the previous line. 
Isn't this something that checkpatch complains about?

>   	/* Enable Write Booster if we have scaled up else disable it */
> -	downgrade_write(&hba->clk_scaling_lock);
> -	is_writelock = false;
> -	ufshcd_wb_toggle(hba, scale_up);
> +	if (ufshcd_enable_wb_if_scaling_up(hba)) {
> +		downgrade_write(&hba->clk_scaling_lock);
> +		is_writelock = false;
> +		ufshcd_wb_toggle(hba, scale_up);
> +	}

Since this code is being modified, please move the "/* Enable" comment 
to where it should occur (just above the ufshcd_wb_toggle() call).

> @@ -1004,6 +1010,10 @@ static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba)
>   {
>   	return hba->caps & UFSHCD_CAP_WB_EN;
>   }
> +static inline bool ufshcd_enable_wb_if_scaling_up(struct ufs_hba *hba)
> +{
> +	return hba->caps & UFSHCD_CAP_WB_WITH_CLK_SCALING;
> +}

It seems like a blank line is missing above the new function definition?

Otherwise this patch looks good to me.

Thanks,

Bart.