Re: [PATCH v3 01/12] scsi: ufs: core: Introduce a new ufshcd vops negotiate_pwr_mode()

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

On 3/8/26 8:13 AM, Can Guo wrote:
> +static int exynos_ufs_negotiate_pwr_mode(struct ufs_hba *hba,
> +					 const struct ufs_pa_layer_attr *dev_max_params,
> +					 struct ufs_pa_layer_attr *dev_req_params)
> +{
> +	struct ufs_host_params host_params;
> +	int ret;
> +
> +	if (!dev_req_params) {
> +		pr_err("%s: incoming dev_req_params is NULL\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ufshcd_init_host_params(&host_params);
> +
> +	/* This driver only support symmetric gear setting e.g. hs_tx_gear == hs_rx_gear */
> +	host_params.hs_tx_gear = exynos_ufs_get_hs_gear(hba);
> +	host_params.hs_rx_gear = exynos_ufs_get_hs_gear(hba);
> +
> +	ret = ufshcd_negotiate_pwr_params(&host_params, dev_max_params, dev_req_params);
> +	if (ret)
> +		pr_err("%s: failed to determine capabilities\n", __func__);
> +
> +	return ret;
> +}

The dev_req_params test is not necessary since the UFS core never passes 
a NULL pointer as third argument, isn't it? I propose to remove the
dev_req_params test.

> +static int ufs_hisi_negotiate_pwr_mode(struct ufs_hba *hba,
> +				       const struct ufs_pa_layer_attr *dev_max_params,
> +				       struct ufs_pa_layer_attr *dev_req_params)
> +{
> +	struct ufs_host_params host_params;
> +	int ret;
> +
> +	if (!dev_req_params) {
> +		dev_err(hba->dev, "%s: incoming dev_req_params is NULL\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ufs_hisi_set_dev_cap(&host_params);
> +	ret = ufshcd_negotiate_pwr_params(&host_params, dev_max_params, dev_req_params);
> +	if (ret)
> +		dev_err(hba->dev, "%s: failed to determine capabilities\n", __func__);
> +
> +	return ret;
> +}

Same comment here - please remove the dev_req_params test.

> +static int ufs_qcom_negotiate_pwr_mode(struct ufs_hba *hba,
> +				       const struct ufs_pa_layer_attr *dev_max_params,
> +				       struct ufs_pa_layer_attr *dev_req_params)
>   {
>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>   	struct ufs_host_params *host_params = &host->host_params;
> +	int ret;
> +
> +	if (!dev_req_params) {
> +		pr_err("%s: incoming dev_req_params is NULL\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ret = ufshcd_negotiate_pwr_params(host_params, dev_max_params, dev_req_params);
> +	if (ret)
> +		dev_err(hba->dev, "%s: failed to determine capabilities\n", __func__);
> +
> +	return ret;
> +}

Also here, please remove the dev_req_params test.

Additionally, I see that identical "if (ret) dev_err(...)" code occurs
in the three callbacks shown above. Shouldn't that code be moved into
the only caller of these functions in the UFS core?

Thanks,

Bart.