On 8/4/22 3:28 AM, Bart Van Assche wrote:
On 8/2/22 20:03, peter.wang@xxxxxxxxxxxx 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.
