On 7/1/22 00:46, Jinyoung CHOI wrote:
The Function names were changed clearly, and the location of the
comments was modified and added properly.
In addition, the conditional test of the toggle functions was
different, so it was modified.
Unnecessary logs were removed and modified appropriately.
There are too many changes in this patch. Please split this patch, e.g.
one patch that introduces ufshcd_is_wb_buf_flush_allowed(), one patch
that renames ufshcd_wb_toggle_flush_during_h8() and its arguments and
another patch that renames ufshcd_wb_config().
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 0a088b47d557..6253606b93b4 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -230,7 +230,7 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
* If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB
* on/off will be done while clock scaling up/down.
*/
- dev_warn(dev, "To control WB through wb_on is not allowed!\n");
+ dev_warn(dev, "It is not allowed to control WB!\n");
I suggest to change "control" into "configure".
static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
@@ -1289,9 +1288,10 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
}
}
- /* Enable Write Booster if we have scaled up else disable it */
downgrade_write(&hba->clk_scaling_lock);
is_writelock = false;
+
+ /* Enable Write Booster if we have scaled up else disable it */
ufshcd_wb_toggle(hba, scale_up);
The above change could be yet another patch.
out_unprepare:
@@ -5715,6 +5715,9 @@ static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG :
UPIU_QUERY_OPCODE_CLEAR_FLAG;
+ if (!ufshcd_is_wb_allowed(hba))
+ return -EPERM;
+
Moving the ufshcd_is_wb_allowed() call from ufshcd_wb_toggle() into
__ufshcd_wb_toggle() should be yet another patch.
- if (!(enable ^ hba->dev_info.wb_enabled))
+ if (hba->dev_info.wb_enabled == enable)
return 0;
This change is independent of the rest of this patch and hence could be
yet another patch.
- dev_info(hba->dev, "%s Write Booster %s\n",
- __func__, enable ? "enabled" : "disabled");
Why has this code been left out?
- ret = __ufshcd_wb_toggle(hba, set,
- QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8);
- if (ret) {
- dev_err(hba->dev, "%s: WB-Buf Flush during H8 %s failed: %d\n",
- __func__, set ? "enable" : "disable", ret);
- return;
- }
- dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n",
- __func__, set ? "enabled" : "disabled");
+ ret = __ufshcd_wb_toggle(hba, enable,
+ QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8);
+ if (ret)
+ dev_err(hba->dev, "%s: failed to %s WB buf flush during H8 %d\n",
+ __func__, enable ? "enable" : "disable", ret);
The above error message is worse than the original error message. Please
either keep the original message or change it into something better than
the original, e.g. "Failed to %s WB buffer flushing during H8".
Thanks,
Bart.
