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

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

On 2/27/26 8:07 AM, Can Guo wrote:
> Before power mode change to a target power mode, TX Equalzation Training

"Equalzation" -> "Equalization"

> (EQTR) needs be done for that target power mode. In addition, before TX
> EQTR we need to change the power mode to HS-G1. These cannot happen
> before the vops pwr_change_notify(PRE_CHANGE) because we don't know the
> negotiated target power mode yet. It is neither approprite if all these
> happen post the vops pwr_change_notify(PRE_CHANGE) as we are going to
> change the power mode to HS-G1 for TX EQTR.

approprite -> appropriate

Additionally, if "neither" occurs in a sentence, "nor" should occur in
the same sentence. I don't see "nor" in the above sentence?

> Introduce a new ufshcd vops negotiate_pwr_mode(), so that TX EQTR can be
> done after vops negotiate_pwr_mode() and before vops pwr_change_notify().

This patch does much more than only introducing a new vendor operation.
Please make sure the patch description is complete.

> -	return -ENOTSUPP;
> +	return -EOPNOTSUPP;

Why has ENOTSUPP been changed into EOPNOTSUPP?

> -static int ufshcd_change_power_mode(struct ufs_hba *hba,
> -			     struct ufs_pa_layer_attr *pwr_mode)
> +static int __ufshcd_change_power_mode(struct ufs_hba *hba,
> +				      struct ufs_pa_layer_attr *pwr_mode)
>   {
>   	int ret;

The double underscore prefix is typically used in the Linux kernel to
indicate that the caller holds a lock. That is not the case here. Please
choose another name for this function, e.g.
ufshcd_dme_change_power_mode().

Thanks,

Bart.