| Message ID | 1891546521.01624267081897.JavaMail.epsvc@epcpadp4 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed() | expand |
On 21/06/21 11:51 am, Keoseong Park wrote: > Change conditional compilation to IS_ENABLED macro, > and simplify if else statement to return statement. > No functional change. > > Signed-off-by: Keoseong Park <keosung.park@samsung.com> > --- > drivers/scsi/ufs/ufshcd.h | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index c98d540ac044..6d239a855753 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -893,16 +893,15 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) > > static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) > { > -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ > -#ifndef CONFIG_SCSI_UFS_DWC > - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && > - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) > + /* > + * DWC UFS Core has the Interrupt aggregation feature > + * but is not detectable. > + */ > + if (IS_ENABLED(CONFIG_SCSI_UFS_DWC)) Why is this needed? It seems like you could just set UFSHCD_CAP_INTR_AGGR and clear UFSHCD_QUIRK_BROKEN_INTR_AGGR instead? > return true; > - else > - return false; > -#else > -return true; > -#endif > + > + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && > + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); > } > > static inline bool ufshcd_can_aggressive_pc(struct ufs_hba *hba) >
>On 21/06/21 11:51 am, Keoseong Park wrote: >> Change conditional compilation to IS_ENABLED macro, >> and simplify if else statement to return statement. >> No functional change. >> >> Signed-off-by: Keoseong Park <keosung.park@samsung.com> >> --- >> drivers/scsi/ufs/ufshcd.h | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index c98d540ac044..6d239a855753 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -893,16 +893,15 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) >> >> static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) >> { >> -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ >> -#ifndef CONFIG_SCSI_UFS_DWC >> - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && >> - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) >> + /* >> + * DWC UFS Core has the Interrupt aggregation feature >> + * but is not detectable. >> + */ >> + if (IS_ENABLED(CONFIG_SCSI_UFS_DWC)) > >Why is this needed? It seems like you could just set UFSHCD_CAP_INTR_AGGR >and clear UFSHCD_QUIRK_BROKEN_INTR_AGGR instead? Hello Adrian, Sorry for late reply. The code that returns true when CONFIG_SCSI_UFS_DWC is set in the original code is only changed using the IS_ENABLED macro. (Linux kernel coding style, 21) Conditional Compilation) When CONFIG_SCSI_UFS_DWC is not defined, the code for checking quirk and caps has been moved to the newly added return statement below. Thanks, Keoseong > >> return true; >> - else >> - return false; >> -#else >> -return true; >> -#endif >> + >> + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && >> + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); >> } >> >> static inline bool ufshcd_can_aggressive_pc(struct ufs_hba *hba) >> >
On 24/06/21 9:41 am, Keoseong Park wrote: >> On 21/06/21 11:51 am, Keoseong Park wrote: >>> Change conditional compilation to IS_ENABLED macro, >>> and simplify if else statement to return statement. >>> No functional change. >>> >>> Signed-off-by: Keoseong Park <keosung.park@samsung.com> >>> --- >>> drivers/scsi/ufs/ufshcd.h | 17 ++++++++--------- >>> 1 file changed, 8 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >>> index c98d540ac044..6d239a855753 100644 >>> --- a/drivers/scsi/ufs/ufshcd.h >>> +++ b/drivers/scsi/ufs/ufshcd.h >>> @@ -893,16 +893,15 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) >>> >>> static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) >>> { >>> -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ >>> -#ifndef CONFIG_SCSI_UFS_DWC >>> - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && >>> - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) >>> + /* >>> + * DWC UFS Core has the Interrupt aggregation feature >>> + * but is not detectable. >>> + */ >>> + if (IS_ENABLED(CONFIG_SCSI_UFS_DWC)) >> >> Why is this needed? It seems like you could just set UFSHCD_CAP_INTR_AGGR >> and clear UFSHCD_QUIRK_BROKEN_INTR_AGGR instead? > > Hello Adrian, > Sorry for late reply. > > The code that returns true when CONFIG_SCSI_UFS_DWC is set in the original code > is only changed using the IS_ENABLED macro. > (Linux kernel coding style, 21) Conditional Compilation) > > When CONFIG_SCSI_UFS_DWC is not defined, the code for checking quirk > and caps has been moved to the newly added return statement below. Looking closer I cannot find CONFIG_SCSI_UFS_DWC at all. It seems like it never existed. Why should we not remove the code related to CONFIG_SCSI_UFS_DWC entirely? > > Thanks, > Keoseong > >> >>> return true; >>> - else >>> - return false; >>> -#else >>> -return true; >>> -#endif >>> + >>> + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && >>> + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); >>> } >>> >>> static inline bool ufshcd_can_aggressive_pc(struct ufs_hba *hba) >>> >>
>On 24/06/21 9:41 am, Keoseong Park wrote: >>> On 21/06/21 11:51 am, Keoseong Park wrote: >>>> Change conditional compilation to IS_ENABLED macro, >>>> and simplify if else statement to return statement. >>>> No functional change. >>>> >>>> Signed-off-by: Keoseong Park <keosung.park@samsung.com> >>>> --- >>>> drivers/scsi/ufs/ufshcd.h | 17 ++++++++--------- >>>> 1 file changed, 8 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >>>> index c98d540ac044..6d239a855753 100644 >>>> --- a/drivers/scsi/ufs/ufshcd.h >>>> +++ b/drivers/scsi/ufs/ufshcd.h >>>> @@ -893,16 +893,15 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) >>>> >>>> static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) >>>> { >>>> -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ >>>> -#ifndef CONFIG_SCSI_UFS_DWC >>>> - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && >>>> - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) >>>> + /* >>>> + * DWC UFS Core has the Interrupt aggregation feature >>>> + * but is not detectable. >>>> + */ >>>> + if (IS_ENABLED(CONFIG_SCSI_UFS_DWC)) >>> >>> Why is this needed? It seems like you could just set UFSHCD_CAP_INTR_AGGR >>> and clear UFSHCD_QUIRK_BROKEN_INTR_AGGR instead? >> >> Hello Adrian, >> Sorry for late reply. >> >> The code that returns true when CONFIG_SCSI_UFS_DWC is set in the original code >> is only changed using the IS_ENABLED macro. >> (Linux kernel coding style, 21) Conditional Compilation) >> >> When CONFIG_SCSI_UFS_DWC is not defined, the code for checking quirk >> and caps has been moved to the newly added return statement below. > >Looking closer I cannot find CONFIG_SCSI_UFS_DWC at all. It seems like it >never existed. > >Why should we not remove the code related to CONFIG_SCSI_UFS_DWC entirely? You're right. What do you think of deleting the code related to CONFIG_SCSI_UFS_DWC and changing it to the patch below? --- diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index c98d540ac044..c9faca237290 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -893,16 +893,8 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) { -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ -#ifndef CONFIG_SCSI_UFS_DWC - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) - return true; - else - return false; -#else -return true; -#endif + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); } > > >> >> Thanks, >> Keoseong >> >>> >>>> return true; >>>> - else >>>> - return false; >>>> -#else >>>> -return true; >>>> -#endif >>>> + >>>> + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && >>>> + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); >>>> } >>>> >>>> static inline bool ufshcd_can_aggressive_pc(struct ufs_hba *hba) >>>> >>> >
On 24/06/21 1:44 pm, Keoseong Park wrote: >> On 24/06/21 9:41 am, Keoseong Park wrote: >>>> On 21/06/21 11:51 am, Keoseong Park wrote: >>>>> Change conditional compilation to IS_ENABLED macro, >>>>> and simplify if else statement to return statement. >>>>> No functional change. >>>>> >>>>> Signed-off-by: Keoseong Park <keosung.park@samsung.com> >>>>> --- >>>>> drivers/scsi/ufs/ufshcd.h | 17 ++++++++--------- >>>>> 1 file changed, 8 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >>>>> index c98d540ac044..6d239a855753 100644 >>>>> --- a/drivers/scsi/ufs/ufshcd.h >>>>> +++ b/drivers/scsi/ufs/ufshcd.h >>>>> @@ -893,16 +893,15 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) >>>>> >>>>> static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) >>>>> { >>>>> -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ >>>>> -#ifndef CONFIG_SCSI_UFS_DWC >>>>> - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && >>>>> - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) >>>>> + /* >>>>> + * DWC UFS Core has the Interrupt aggregation feature >>>>> + * but is not detectable. >>>>> + */ >>>>> + if (IS_ENABLED(CONFIG_SCSI_UFS_DWC)) >>>> >>>> Why is this needed? It seems like you could just set UFSHCD_CAP_INTR_AGGR >>>> and clear UFSHCD_QUIRK_BROKEN_INTR_AGGR instead? >>> >>> Hello Adrian, >>> Sorry for late reply. >>> >>> The code that returns true when CONFIG_SCSI_UFS_DWC is set in the original code >>> is only changed using the IS_ENABLED macro. >>> (Linux kernel coding style, 21) Conditional Compilation) >>> >>> When CONFIG_SCSI_UFS_DWC is not defined, the code for checking quirk >>> and caps has been moved to the newly added return statement below. >> >> Looking closer I cannot find CONFIG_SCSI_UFS_DWC at all. It seems like it >> never existed. >> >> Why should we not remove the code related to CONFIG_SCSI_UFS_DWC entirely? > > You're right. What do you think of deleting the code related to CONFIG_SCSI_UFS_DWC > and changing it to the patch below? Yes, but cc Joao Pinto <jpinto@synopsys.com> who introduced the code > > --- > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index c98d540ac044..c9faca237290 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -893,16 +893,8 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) > > static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) > { > -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ > -#ifndef CONFIG_SCSI_UFS_DWC > - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && > - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) > - return true; > - else > - return false; > -#else > -return true; > -#endif > + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && > + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); > } > >> >> >>> >>> Thanks, >>> Keoseong >>> >>>> >>>>> return true; >>>>> - else >>>>> - return false; >>>>> -#else >>>>> -return true; >>>>> -#endif >>>>> + >>>>> + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && >>>>> + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); >>>>> } >>>>> >>>>> static inline bool ufshcd_can_aggressive_pc(struct ufs_hba *hba) >>>>> >>>> >>
>On 24/06/21 1:44 pm, Keoseong Park wrote: >>> On 24/06/21 9:41 am, Keoseong Park wrote: >>>>> On 21/06/21 11:51 am, Keoseong Park wrote: >>>>>> Change conditional compilation to IS_ENABLED macro, >>>>>> and simplify if else statement to return statement. >>>>>> No functional change. >>>>>> >>>>>> Signed-off-by: Keoseong Park <keosung.park@samsung.com> >>>>>> --- >>>>>> drivers/scsi/ufs/ufshcd.h | 17 ++++++++--------- >>>>>> 1 file changed, 8 insertions(+), 9 deletions(-) >>>>>> >>>>>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >>>>>> index c98d540ac044..6d239a855753 100644 >>>>>> --- a/drivers/scsi/ufs/ufshcd.h >>>>>> +++ b/drivers/scsi/ufs/ufshcd.h >>>>>> @@ -893,16 +893,15 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) >>>>>> >>>>>> static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) >>>>>> { >>>>>> -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ >>>>>> -#ifndef CONFIG_SCSI_UFS_DWC >>>>>> - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && >>>>>> - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) >>>>>> + /* >>>>>> + * DWC UFS Core has the Interrupt aggregation feature >>>>>> + * but is not detectable. >>>>>> + */ >>>>>> + if (IS_ENABLED(CONFIG_SCSI_UFS_DWC)) >>>>> >>>>> Why is this needed? It seems like you could just set UFSHCD_CAP_INTR_AGGR >>>>> and clear UFSHCD_QUIRK_BROKEN_INTR_AGGR instead? >>>> >>>> Hello Adrian, >>>> Sorry for late reply. >>>> >>>> The code that returns true when CONFIG_SCSI_UFS_DWC is set in the original code >>>> is only changed using the IS_ENABLED macro. >>>> (Linux kernel coding style, 21) Conditional Compilation) >>>> >>>> When CONFIG_SCSI_UFS_DWC is not defined, the code for checking quirk >>>> and caps has been moved to the newly added return statement below. >>> >>> Looking closer I cannot find CONFIG_SCSI_UFS_DWC at all. It seems like it >>> never existed. >>> >>> Why should we not remove the code related to CONFIG_SCSI_UFS_DWC entirely? >> >> You're right. What do you think of deleting the code related to CONFIG_SCSI_UFS_DWC >> and changing it to the patch below? > >Yes, but cc Joao Pinto <jpinto@synopsys.com> who introduced the code Thanks for your advice. I will upload next version patch by adding cc. Thanks, Keoseong > >> >> --- >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index c98d540ac044..c9faca237290 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -893,16 +893,8 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) >> >> static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) >> { >> -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ >> -#ifndef CONFIG_SCSI_UFS_DWC >> - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && >> - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) >> - return true; >> - else >> - return false; >> -#else >> -return true; >> -#endif >> + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && >> + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); >> } >> >>> >>> >>>> >>>> Thanks, >>>> Keoseong >>>> >>>>> >>>>>> return true; >>>>>> - else >>>>>> - return false; >>>>>> -#else >>>>>> -return true; >>>>>> -#endif >>>>>> + >>>>>> + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && >>>>>> + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); >>>>>> } >>>>>> >>>>>> static inline bool ufshcd_can_aggressive_pc(struct ufs_hba *hba) >>>>>> >>>>> >>> >
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index c98d540ac044..6d239a855753 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -893,16 +893,15 @@ static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba) static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba) { -/* DWC UFS Core has the Interrupt aggregation feature but is not detectable*/ -#ifndef CONFIG_SCSI_UFS_DWC - if ((hba->caps & UFSHCD_CAP_INTR_AGGR) && - !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR)) + /* + * DWC UFS Core has the Interrupt aggregation feature + * but is not detectable. + */ + if (IS_ENABLED(CONFIG_SCSI_UFS_DWC)) return true; - else - return false; -#else -return true; -#endif + + return (hba->caps & UFSHCD_CAP_INTR_AGGR) && + !(hba->quirks & UFSHCD_QUIRK_BROKEN_INTR_AGGR); } static inline bool ufshcd_can_aggressive_pc(struct ufs_hba *hba)
Change conditional compilation to IS_ENABLED macro, and simplify if else statement to return statement. No functional change. Signed-off-by: Keoseong Park <keosung.park@samsung.com> --- drivers/scsi/ufs/ufshcd.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)