Re: [PATCH v2] ufs: core: Add HID support

[Bart Van Assche <bvanassche@acm.org>]

On 5/12/25 6:15 AM, Huan Tang wrote:
> +What:		/sys/bus/platform/drivers/ufshcd/*/ufs_hid/hid_analysis_trigger
> +What:		/sys/bus/platform/devices/*.ufs/ufs_hid/hid_analysis_trigger
> +Date:		April 2025
> +Contact:	Huan Tang <tanghuan@vivo.com>
> +Description:
> +		The host can enable or disable HID analysis operation.
> +
> +		=======  =========================================
> +		disable   disable HID analysis operation
> +		enable    enable HID analysis operation
> +		=======  =========================================
> +
> +		The file is write only.

All HID sysfs attributes occur in the "ufs_hid" directory and have a
"hid_" prefix. That's two times "hid". Please remove the "hid_" prefix
from the sysfs attribute names since it is redundant.

> +static struct attribute *ufs_sysfs_ufs_hid[] = {
> +	&dev_attr_hid_analysis_trigger.attr,
> +	&dev_attr_hid_defrag_trigger.attr,
> +	&dev_attr_hid_fragmented_size.attr,
> +	&dev_attr_hid_defrag_size.attr,
> +	&dev_attr_hid_progress_ratio.attr,
> +	&dev_attr_hid_state.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ufs_sysfs_ufs_hid_group = {
> +	.name = "ufs_hid",
> +	.attrs = ufs_sysfs_ufs_hid,
> +};

Isn't the prefix "ufs_" in "ufs_hid" redundant since this sysfs group
occurs under a UFS host controller directory?

Regarding the name of this sysfs group, "ufs" occurs twice in that
name (ufs_sysfs_ufs_hid_group). Please make sure that "ufs" only occurs
once in that sysfs group name.

>   #define UFS_LUN_DESC_PARAM(_pname, _puname, _duname, _size)		\
>   static ssize_t _pname##_show(struct device *dev,			\
>   	struct device_attribute *attr, char *buf)			\
> @@ -1898,6 +2079,7 @@ const struct attribute_group ufs_sysfs_lun_attributes_group = {
>   
>   void ufs_sysfs_add_nodes(struct device *dev)
>   {
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
>   	int ret;
>   
>   	ret = sysfs_create_groups(&dev->kobj, ufs_sysfs_groups);
> @@ -1905,9 +2087,22 @@ void ufs_sysfs_add_nodes(struct device *dev)
>   		dev_err(dev,
>   			"%s: sysfs groups creation failed (err = %d)\n",
>   			__func__, ret);
> +
> +	if (hba->dev_info.hid_sup) {
> +		ret = sysfs_create_group(&dev->kobj, &ufs_sysfs_ufs_hid_group);
> +		if (ret)
> +			dev_err(dev,
> +				"%s: sysfs ufs_hid group creation failed (err = %d)\n",
> +				__func__, ret);
> +	}
>   }

Please merge ufs_sysfs_ufs_hid_group into ufs_sysfs_groups, remove the
new sysfs_create_group() call and add a visibility callback in
ufs_sysfs_ufs_hid_group that only makes the attributes in this group
visible if hba->dev_info.hid_sup == true.

Otherwise this patch looks good to me.

Thanks,

Bart.