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

[Can Guo]

Hi Bart,

On 2/28/2026 3:31 AM, Bart Van Assche wrote:
> On 2/27/26 8:07 AM, Can Guo wrote:
> > Before power mode change to a target power mode, TX Equalzation Training
>
> "Equalzation" -> "Equalization"
Done.
> > (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
Done.
> Additionally, if "neither" occurs in a sentence, "nor" should occur in
> the same sentence. I don't see "nor" in the above sentence?
Will improve the commit message in next version.
> > 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.
Done.
> > -    return -ENOTSUPP;
> > +    return -EOPNOTSUPP;
>
> Why has ENOTSUPP been changed into EOPNOTSUPP?
I got a warning from checkpatch.pl when I add the new vops, so I changed 
the same for
ufshcd_vops_pwr_change_notify() too.

WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
#59: FILE: drivers/ufs/core/ufshcd-priv.h:178:
+       return -ENOTSUPP;
> > -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().
Done.
> Thanks,
>
> Bart.