Re: [PATCH v1 01/16] ufs: core: Probe for ext_iid support

Bart Van Assche
2022-09-30T18:23:25Z

On 9/22/22 18:05, Asutosh Das wrote:
> + hba->mcq_capabilities = ufshcd_readl(hba, REG_MCQCAP);

What value is reported when reading the REG_MCQCAP register on an UFSHCI
3.0 controller? -1 or 0?

> + hba->ext_iid_sup = (hba->mcq_capabilities & MASK_EXT_IID_SUPPORT) >>
> + EXT_IID_CAP_SHIFT;

[ ... ]

> + if (dev_info->wspecversion < 0x400)
> + goto out;

Isn't this version check superfluous? Only UFSHCI 4.0 controllers should
support the extended IID feature.

> + ext_ufs_feature = get_unaligned_be32(desc_buf + DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);

The above change introduces a third instance of this code. Please
introduce a helper function that does something like the following and
replace the above line with a call to that helper function:

if (hba->desc_size[QUERY_DESC_IDN_DEVICE] <
DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
return 0;

return get_unaligned_be32(desc_buf +
DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);

> +out:
> + dev_info->b_ext_iid_en = !!ext_iid_en;

Please remove "!!". This conversion happens implicitly when assigning to
a boolean variable.

Thanks,

Bart.