[PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed()
       [not found] <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p4>
@ 2021-06-21  8:51 ` Keoseong Park
  2021-06-21  9:29   ` Adrian Hunter
       [not found]   ` <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p1>
  0 siblings, 2 replies; 7+ messages in thread
From: Keoseong Park @ 2021-06-21  8:51 UTC (permalink / raw)
  To: joe, ALIM AKHTAR, avri.altman, jejb, martin.petersen,
	stanley.chu, cang, beanhuo, asutoshd, adrian.hunter, Kiwoong Kim,
	satyat, bvanassche, linux-scsi, linux-kernel

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))
 		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)
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed()
  2021-06-21  8:51 ` [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed() Keoseong Park
@ 2021-06-21  9:29   ` Adrian Hunter
       [not found]   ` <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p1>
  1 sibling, 0 replies; 7+ messages in thread
From: Adrian Hunter @ 2021-06-21  9:29 UTC (permalink / raw)
  To: keosung.park, joe, ALIM AKHTAR, avri.altman, jejb,
	martin.petersen, stanley.chu, cang, beanhuo, asutoshd,
	Kiwoong Kim, satyat, bvanassche, linux-scsi, linux-kernel

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)
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

[parent not found: <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p1>]

* RE: Re: [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed()
       [not found]   ` <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p1>
@ 2021-06-24  6:41     ` Keoseong Park
  2021-06-24  7:25       ` Adrian Hunter
  2021-06-24 10:44     ` Keoseong Park
  2021-06-25  2:38     ` Keoseong Park
  2 siblings, 1 reply; 7+ messages in thread
From: Keoseong Park @ 2021-06-24  6:41 UTC (permalink / raw)
  To: Adrian Hunter, Keoseong Park, joe, ALIM AKHTAR, avri.altman,
	jejb, martin.petersen, stanley.chu, cang, beanhuo, asutoshd,
	Kiwoong Kim, satyat, bvanassche, linux-scsi, linux-kernel

>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)
>> 
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed()
  2021-06-24  6:41     ` Keoseong Park
@ 2021-06-24  7:25       ` Adrian Hunter
  0 siblings, 0 replies; 7+ messages in thread
From: Adrian Hunter @ 2021-06-24  7:25 UTC (permalink / raw)
  To: keosung.park, joe, ALIM AKHTAR, avri.altman, jejb,
	martin.petersen, stanley.chu, cang, beanhuo, asutoshd,
	Kiwoong Kim, satyat, bvanassche, linux-scsi, linux-kernel,
	Joao Pinto, Pedro Sousa, Pedro Sousa

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)
>>>
>>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: Re: [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed()
       [not found]   ` <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p1>
  2021-06-24  6:41     ` Keoseong Park
@ 2021-06-24 10:44     ` Keoseong Park
  2021-06-24 11:46       ` Adrian Hunter
  2021-06-25  2:38     ` Keoseong Park
  2 siblings, 1 reply; 7+ messages in thread
From: Keoseong Park @ 2021-06-24 10:44 UTC (permalink / raw)
  To: Adrian Hunter, Keoseong Park, joe, ALIM AKHTAR, avri.altman,
	jejb, martin.petersen, stanley.chu, cang, beanhuo, asutoshd,
	Kiwoong Kim, satyat, bvanassche, linux-scsi, linux-kernel,
	Joao Pinto, Pedro Sousa, Pedro Sousa

>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)
>>>>
>>>
>

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed()
  2021-06-24 10:44     ` Keoseong Park
@ 2021-06-24 11:46       ` Adrian Hunter
  0 siblings, 0 replies; 7+ messages in thread
From: Adrian Hunter @ 2021-06-24 11:46 UTC (permalink / raw)
  To: keosung.park, joe, ALIM AKHTAR, avri.altman, jejb,
	martin.petersen, stanley.chu, cang, beanhuo, asutoshd,
	Kiwoong Kim, satyat, bvanassche, linux-scsi, linux-kernel,
	Joao Pinto, Pedro Sousa, Pedro Sousa

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)
>>>>>
>>>>
>>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: Re: [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed()
       [not found]   ` <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p1>
  2021-06-24  6:41     ` Keoseong Park
  2021-06-24 10:44     ` Keoseong Park
@ 2021-06-25  2:38     ` Keoseong Park
  2 siblings, 0 replies; 7+ messages in thread
From: Keoseong Park @ 2021-06-25  2:38 UTC (permalink / raw)
  To: Adrian Hunter, Keoseong Park, joe, ALIM AKHTAR, avri.altman,
	jejb, martin.petersen, stanley.chu, cang, beanhuo, asutoshd,
	Kiwoong Kim, satyat, bvanassche, linux-scsi, linux-kernel,
	Joao Pinto, Pedro Sousa, Pedro Sousa

>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)
>>>>>>
>>>>>
>>>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-06-25  2:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p4>
2021-06-21  8:51 ` [PATCH] scsi: ufs: Refactor ufshcd_is_intr_aggr_allowed() Keoseong Park
2021-06-21  9:29   ` Adrian Hunter
     [not found]   ` <CGME20210621085158epcms2p46170ba48174547df00b9720dbc843110@epcms2p1>
2021-06-24  6:41     ` Keoseong Park
2021-06-24  7:25       ` Adrian Hunter
2021-06-24 10:44     ` Keoseong Park
2021-06-24 11:46       ` Adrian Hunter
2021-06-25  2:38     ` Keoseong Park

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.