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

[Peter Wang <peter.wang@mediatek.com>]

On 8/4/22 3:28 AM, Bart Van Assche wrote:
> 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,

Hi Bart,


Will fix next version.


Thanks

Peter

>
> Bart.