Re: [PATCH v3] ufs: core: Add WB buffer resize support

[Bart Van Assche]

On 10/28/24 6:52 AM, Huan Tang wrote:
> +What:		/sys/bus/platform/drivers/ufshcd/*/wb_toggle_buf_resize
> +What:		/sys/bus/platform/devices/*.ufs/wb_toggle_buf_resize
> +Date:		Qct 2024
> +Contact:	Huan Tang <tanghuan@xxxxxxxx>
> +Description:
> +		The host can decrease or increase the WriteBooster Buffer size by setting
> +		this file.
> +
> +		======  ======================================
> +		00h  Idle (There is no resize operation)
> +		01h  Decrease WriteBooster Buffer Size
> +		02h  Increase WriteBooster Buffer Size
> +		Others  Reserved
> +		======  ======================================
> +
> +		The file is write only.

The name "wb_toggle_buf_resize" is not clear and will make users guess
what the purpose of this sysfs attribute is. Please choose a name that
is more clear, e.g. "wb_resize_action".

Additionally, please change the word "file" into "attribute" to maintain
consistency with the rest of the sysfs documentation.

Please also make sure that the documentation is consistent with the
code. The above documentation mentions the value 0 while the code
doesn't allow writing the value zero.

> +
> +What:		/sys/bus/platform/drivers/ufshcd/*/attributes/wb_buf_resize_status
> +What:		/sys/bus/platform/devices/*.ufs/attributes/wb_buf_resize_status
> +Date:		Qct 2024
> +Contact:	Huan Tang <tanghuan@xxxxxxxx>
> +Description:
> +		The host can check the Resize operation status of the WriteBooster Buffer
> +		by reading this file.
> +
> +		======  ========================================
> +		00h  Idle (resize operation is not issued)
> +		01h  Resize operation in progress
> +		02h  Resize operation completed successfully
> +		03h  Resize operation general failure
> +		Others  Reserved
> +		======  ========================================

For the three new attributes: please use words in sysfs instead of 
numbers. Using numbers is not user-friendly.

> +static ssize_t wb_toggle_buf_resize_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	unsigned int wb_buf_resize_op;

Please introduce an enumeration type instead of using 'unsigned int'. 
Please also choose a more descriptive variable name than 'op'.

> +int ufshcd_wb_toggle_buf_resize(struct ufs_hba *hba, u32 op)
> +{
> +	int ret;
> +	u8 index;
> +
> +	index = ufshcd_wb_get_query_index(hba);
> +	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +				   QUERY_ATTR_IDN_WB_BUF_RESIZE_EN, index, 0, &op);
> +	if (ret)
> +		dev_err(hba->dev, "%s: Enable WB buf resize operation failed %d\n",
> +			__func__, ret);
> +
> +	return ret;
> +}

This function doesn't toggle anything - it sets the value of the
bWriteBoosterBufferResizeEn attribute. Please reflect this in the
function name, e.g. by renaming this function into
ufshcd_wb_set_resize_en(). Additionally, the name of the 'op' argument
doesn't reflect the purpose of that argument. How about renaming it into
'enum wb_resize_en' where the wb_resize_en type support the three values
idle = 0, decrease = 1 and increase = 2?

Thanks,

Bart.