| Message ID | 20220729045045epcms2p8caf00317889ed4da8531b7466ec6e368@epcms2p8 |
|---|---|
| Headers | show |
| Series | scsi: ufs: wb: Add sysfs attribute and cleanup | expand |
On Fri, Jul 29, 2022 at 12:54 PM Jinyoung CHOI <j-young.choi@samsung.com> wrote: > > Changed to improve readability. > As implemented in ufshcd_wb_togle_flush(), the conditional test is > modified in the same way. > > Reviewed-by: Avri Altman <avri.altman@wdc.com> > Reviewed-by: Bean Huo <beanhuo@micron.com> > Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com> Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
>On 7/28/22 21:56, Jinyoung CHOI wrote: >> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs >> index 6b248abb1bd7..7e9e1db55d60 100644 >> --- a/Documentation/ABI/testing/sysfs-driver-ufs >> +++ b/Documentation/ABI/testing/sysfs-driver-ufs >> @@ -1417,6 +1417,16 @@ Description: This node is used to set or display whether UFS WriteBooster is >> platform that doesn't support UFSHCD_CAP_CLK_SCALING, we can >> disable/enable WriteBooster through this sysfs node. >> >> +What: /sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en >> +What: /sys/bus/platform/devices/*.ufs/wb_buf_flush_en >> +Date: July 2022 >> +Contact: Jinyoung Choi <j-young.choi@samsung.com> >> +Description: This node is used to set or display whether WriteBooster > ^^^^ > >Please change "node" into "attribute" (here and below). Sysfs files are >called attributes. > The wb_on description is also written as node. This will also be changed to other commit. >> + buffer flusing is enabled. The data written in the WriteBooster > ^^^^^^^ > flushing? >> + Buffer can be flushed by an explicit host command or >> + implicitly while in hibernate (HIBERN8) state. > >The above sentence is misleading because it suggests that setting this >attribute causes the WB buffer to be flushed in its entirety. That is >not correct - what this attribute controls is whether or not the UFS >device is allowed to start with flushing the WB buffer. > It seems to have been written so briefly. Because each device manufacturer may have different flush policies, So, I did not describe the amount. This is the same as the ufs spec. I will add more explanation. And, I don't fully understand your comment (That is not correct ~) If you explain it more, I will consider it. >> + if (!ufshcd_is_wb_allowed(hba) || >> + (hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) { >> + dev_warn(dev, "It is not allowed to configure WB buf flush!\n"); > >flush -> flushing > OK, I will fix it. >> + ufshcd_rpm_get_sync(hba); >> + res = ufshcd_wb_toggle_buf_flush(hba, wb_buf_flush_en); >> + ufshcd_rpm_put_sync(hba); >> +out: >> + up(&hba->host_sem); >> + return res < 0 ? res : count; >> +} > >Please leave a blank line above goto labels as requested by the kernel >coding style guide. > >Thanks, > >Bart. OK, I will fix it. wb_on_store() will also be modified with this. (other commit) Kind Regards, Jinyoung
>On 7/28/22 21:57, Jinyoung CHOI wrote: >> ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_EN); >> if (ret) { >> - dev_err(hba->dev, "%s Write Booster %s failed %d\n", >> + dev_err(hba->dev, "%s: Write Booster %s failed %d\n", >> __func__, enable ? "enable" : "disable", ret); >> return ret; >> } > >Please also fix the grammar (enable -> enabling; disable -> disabling). > >Otherwise this patch looks good to me. > >Thanks, > >Bart. OK, I will fix it. Thanks, Jinyoung.