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

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Tue, 2025-05-20 at 17:40 +0800, Huan Tang wrote:
> 
> +static const char * const hid_trigger_mode[] = {"disable",
> "enable"};
> +
> +static ssize_t analysis_trigger_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf,
> size_t count)
> +{
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +       int mode;
> +       int ret;
> +
> +       mode = sysfs_match_string(hid_trigger_mode, buf);
> +       if (mode < 0)
> +               return -EINVAL;
> 

Hi Haun,

Consider use below coding style for readability.

if (sysfs_streq(buf, "enable"))
    mode = ...;
else if (sysfs_streq(buf, "disable"))
    mode = ...;
else
    return -EINVAL;



> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3e2097e65964..8ccd923a5761 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8390,6 +8390,10 @@ static int ufs_get_device_desc(struct ufs_hba
> *hba)
> 
>         dev_info->rtt_cap = desc_buf[DEVICE_DESC_PARAM_RTT_CAP];
> 
> +       dev_info->hid_sup = get_unaligned_be32(desc_buf +
> +                              
> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
> +                               UFS_DEV_HID_SUPPORT;
> +
> 

Could add the double negation (!!) ensures the value is exactly 0 or 1.
dev_info->hid_sup = !!(get_unaligned_be32(desc_buf +
            DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP) &
            UFS_DEV_HID_SUPPORT);

The rest of the parts looks good to me.

Reviewed-by: Peter Wang <peter.wang@mediatek.com>


Thanks
Peter