diff mbox series

[v1,1/6] scsi: ufs-mediatek: Assign arguments with correct type

Message ID 20201029115750.24391-2-stanley.chu@mediatek.com
State New
Headers show
Series [v1,1/6] scsi: ufs-mediatek: Assign arguments with correct type | expand

Commit Message

Stanley Chu Oct. 29, 2020, 11:57 a.m. UTC 
In ufs_mtk_unipro_set_lpm(), use specific unsigned values
as the argument to invoke ufshcd_dme_set().

In the same time, change the name of ufs_mtk_unipro_set_pm()
to ufs_mtk_unipro_set_lpm() to align the naming convention
in MediaTek UFS driver.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>

---
 drivers/scsi/ufs/ufs-mediatek.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.18.0

Comments

Avri Altman Nov. 3, 2020, 7:12 a.m. UTC | #1 
> 
> In ufs_mtk_unipro_set_lpm(), use specific unsigned values
> as the argument to invoke ufshcd_dme_set().
> 
> In the same time, change the name of ufs_mtk_unipro_set_pm()
> to ufs_mtk_unipro_set_lpm() to align the naming convention
> in MediaTek UFS driver.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Looks like this patch is gilding the lily?

> ---
>  drivers/scsi/ufs/ufs-mediatek.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> index 8df73bc2f8cb..0196a89055b5 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -639,14 +639,14 @@ static int ufs_mtk_pwr_change_notify(struct
> ufs_hba *hba,
>         return ret;
>  }
> 
> -static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)
> +static int ufs_mtk_unipro_set_lpm(struct ufs_hba *hba, bool lpm)
>  {
>         int ret;
>         struct ufs_mtk_host *host = ufshcd_get_variant(hba);
> 
>         ret = ufshcd_dme_set(hba,
>                              UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL, 0),
> -                            lpm);
> +                            lpm ? 1 : 0);
bool is implicitly converted to int anyway?

>         if (!ret || !lpm) {
>                 /*
>                  * Forcibly set as non-LPM mode if UIC commands is failed
> @@ -664,7 +664,7 @@ static int ufs_mtk_pre_link(struct ufs_hba *hba)
>         int ret;
>         u32 tmp;
> 
> -       ret = ufs_mtk_unipro_set_pm(hba, false);
> +       ret = ufs_mtk_unipro_set_lpm(hba, false);
>         if (ret)
>                 return ret;
> 
> @@ -774,7 +774,7 @@ static int ufs_mtk_link_set_hpm(struct ufs_hba
> *hba)
>         if (err)
>                 return err;
> 
> -       err = ufs_mtk_unipro_set_pm(hba, false);
> +       err = ufs_mtk_unipro_set_lpm(hba, false);
>         if (err)
>                 return err;
> 
> @@ -795,10 +795,10 @@ static int ufs_mtk_link_set_lpm(struct ufs_hba
> *hba)
>  {
>         int err;
> 
> -       err = ufs_mtk_unipro_set_pm(hba, true);
> +       err = ufs_mtk_unipro_set_lpm(hba, true);
>         if (err) {
>                 /* Resume UniPro state for following error recovery */
> -               ufs_mtk_unipro_set_pm(hba, false);
> +               ufs_mtk_unipro_set_lpm(hba, false);
>                 return err;
>         }
> 
> --
> 2.18.0
Stanley Chu Nov. 3, 2020, 7:39 a.m. UTC | #2 
Hi Avri,

On Tue, 2020-11-03 at 07:12 +0000, Avri Altman wrote:
> > 

> > In ufs_mtk_unipro_set_lpm(), use specific unsigned values

> > as the argument to invoke ufshcd_dme_set().

> > 

> > In the same time, change the name of ufs_mtk_unipro_set_pm()

> > to ufs_mtk_unipro_set_lpm() to align the naming convention

> > in MediaTek UFS driver.

> > 

> > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>

> Looks like this patch is gilding the lily?


Thanks for the review.

Please see explanation below.

> 

> > ---

> >  drivers/scsi/ufs/ufs-mediatek.c | 12 ++++++------

> >  1 file changed, 6 insertions(+), 6 deletions(-)

> > 

> > diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c

> > index 8df73bc2f8cb..0196a89055b5 100644

> > --- a/drivers/scsi/ufs/ufs-mediatek.c

> > +++ b/drivers/scsi/ufs/ufs-mediatek.c

> > @@ -639,14 +639,14 @@ static int ufs_mtk_pwr_change_notify(struct

> > ufs_hba *hba,

> >         return ret;

> >  }

> > 

> > -static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)

> > +static int ufs_mtk_unipro_set_lpm(struct ufs_hba *hba, bool lpm)

> >  {

> >         int ret;

> >         struct ufs_mtk_host *host = ufshcd_get_variant(hba);

> > 

> >         ret = ufshcd_dme_set(hba,

> >                              UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL, 0),

> > -                            lpm);

> > +                            lpm ? 1 : 0);

> bool is implicitly converted to int anyway?


This change is the echo of your suggestion in
https://patchwork.kernel.org/project/linux-scsi/patch/20200908064507.30774-4-stanley.chu@mediatek.com/

Actually I think your suggestion is constructive that the accepted
values of VS_UNIPROPOWERDOWNCONTROL are better clearly defined so I use
the casting here in this patch.

> 

> >         if (!ret || !lpm) {

> >                 /*

> >                  * Forcibly set as non-LPM mode if UIC commands is failed

> > @@ -664,7 +664,7 @@ static int ufs_mtk_pre_link(struct ufs_hba *hba)

> >         int ret;

> >         u32 tmp;

> > 

> > -       ret = ufs_mtk_unipro_set_pm(hba, false);

> > +       ret = ufs_mtk_unipro_set_lpm(hba, false);

> >         if (ret)

> >                 return ret;

> > 

> > @@ -774,7 +774,7 @@ static int ufs_mtk_link_set_hpm(struct ufs_hba

> > *hba)

> >         if (err)

> >                 return err;

> > 

> > -       err = ufs_mtk_unipro_set_pm(hba, false);

> > +       err = ufs_mtk_unipro_set_lpm(hba, false);

> >         if (err)

> >                 return err;

> > 

> > @@ -795,10 +795,10 @@ static int ufs_mtk_link_set_lpm(struct ufs_hba

> > *hba)

> >  {

> >         int err;

> > 

> > -       err = ufs_mtk_unipro_set_pm(hba, true);

> > +       err = ufs_mtk_unipro_set_lpm(hba, true);

> >         if (err) {

> >                 /* Resume UniPro state for following error recovery */

> > -               ufs_mtk_unipro_set_pm(hba, false);

> > +               ufs_mtk_unipro_set_lpm(hba, false);

> >                 return err;

> >         }

> > 

> > --

> > 2.18.0


Thanks,
Stanley Chu
Avri Altman Nov. 3, 2020, 8:55 a.m. UTC | #3 
> 

> Hi Avri,

> 

> On Tue, 2020-11-03 at 07:12 +0000, Avri Altman wrote:

> > >

> > > In ufs_mtk_unipro_set_lpm(), use specific unsigned values

> > > as the argument to invoke ufshcd_dme_set().

> > >

> > > In the same time, change the name of ufs_mtk_unipro_set_pm()

> > > to ufs_mtk_unipro_set_lpm() to align the naming convention

> > > in MediaTek UFS driver.

> > >

> > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>

> > Looks like this patch is gilding the lily?

> 

> Thanks for the review.

> 

> Please see explanation below.

> 

> >

> > > ---

> > >  drivers/scsi/ufs/ufs-mediatek.c | 12 ++++++------

> > >  1 file changed, 6 insertions(+), 6 deletions(-)

> > >

> > > diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-

> mediatek.c

> > > index 8df73bc2f8cb..0196a89055b5 100644

> > > --- a/drivers/scsi/ufs/ufs-mediatek.c

> > > +++ b/drivers/scsi/ufs/ufs-mediatek.c

> > > @@ -639,14 +639,14 @@ static int ufs_mtk_pwr_change_notify(struct

> > > ufs_hba *hba,

> > >         return ret;

> > >  }

> > >

> > > -static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)

> > > +static int ufs_mtk_unipro_set_lpm(struct ufs_hba *hba, bool lpm)

> > >  {

> > >         int ret;

> > >         struct ufs_mtk_host *host = ufshcd_get_variant(hba);

> > >

> > >         ret = ufshcd_dme_set(hba,

> > >                              UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL,

> 0),

> > > -                            lpm);

> > > +                            lpm ? 1 : 0);

> > bool is implicitly converted to int anyway?

> 

> This change is the echo of your suggestion in

> https://patchwork.kernel.org/project/linux-

> scsi/patch/20200908064507.30774-4-stanley.chu@mediatek.com/

> 

> Actually I think your suggestion is constructive that the accepted

> values of VS_UNIPROPOWERDOWNCONTROL are better clearly defined so I

> use

> the casting here in this patch.

My previous comment that you refer to was against using bool as well,
But to leave it u32.
Anyway, I am not religious about it.

Looks fine,
Avri
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index 8df73bc2f8cb..0196a89055b5 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -639,14 +639,14 @@  static int ufs_mtk_pwr_change_notify(struct ufs_hba *hba,
 	return ret;
 }
 
-static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)
+static int ufs_mtk_unipro_set_lpm(struct ufs_hba *hba, bool lpm)
 {
 	int ret;
 	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
 
 	ret = ufshcd_dme_set(hba,
 			     UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL, 0),
-			     lpm);
+			     lpm ? 1 : 0);
 	if (!ret || !lpm) {
 		/*
 		 * Forcibly set as non-LPM mode if UIC commands is failed
@@ -664,7 +664,7 @@  static int ufs_mtk_pre_link(struct ufs_hba *hba)
 	int ret;
 	u32 tmp;
 
-	ret = ufs_mtk_unipro_set_pm(hba, false);
+	ret = ufs_mtk_unipro_set_lpm(hba, false);
 	if (ret)
 		return ret;
 
@@ -774,7 +774,7 @@  static int ufs_mtk_link_set_hpm(struct ufs_hba *hba)
 	if (err)
 		return err;
 
-	err = ufs_mtk_unipro_set_pm(hba, false);
+	err = ufs_mtk_unipro_set_lpm(hba, false);
 	if (err)
 		return err;
 
@@ -795,10 +795,10 @@  static int ufs_mtk_link_set_lpm(struct ufs_hba *hba)
 {
 	int err;
 
-	err = ufs_mtk_unipro_set_pm(hba, true);
+	err = ufs_mtk_unipro_set_lpm(hba, true);
 	if (err) {
 		/* Resume UniPro state for following error recovery */
-		ufs_mtk_unipro_set_pm(hba, false);
+		ufs_mtk_unipro_set_lpm(hba, false);
 		return err;
 	}