| Message ID | 20201029115750.24391-2-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/6] scsi: ufs-mediatek: Assign arguments with correct type | expand |
> > 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
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
> > 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 --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; }
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(-)