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

[Can Guo]

Hi Bart,

On 3/14/2026 6:09 AM, Bart Van Assche wrote:
> 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.
I considered the same when I made the change but I didn't do so because 
I wanted to keep
the original codes as same as possible in vendor specific implementations...

I will remove the check in next version and see if their maintainers are 
OK with that or not.
> > +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.
Will do.
> > +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.
Sure.
> 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?
Point taken.

Thanks,
Can Guo.
> Thanks,
>
> Bart.