Re: [Intel-gfx] [PATCH 09/13] drm/i915/pps: rename intel_dp_check_edp to intel_pps_check_power_unlocked

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 29 Dec 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> On 2020-12-22 at 20:19:49 +0530, Jani Nikula wrote:
>> Follow the usual naming pattern for functions.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp.c  | 2 +-
>>  drivers/gpu/drm/i915/display/intel_pps.c | 2 +-
>>  drivers/gpu/drm/i915/display/intel_pps.h | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 334ba1775cd3..65406d4ccdbe 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -1046,7 +1046,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>>  	 */
>>  	cpu_latency_qos_update_request(&i915->pm_qos, 0);
>>  
>> -	intel_dp_check_edp(intel_dp);
>> +	intel_pps_check_power_unlocked(intel_dp);
>>  
>>  	/* Try to wait for any previous AUX channel activity */
>>  	for (try = 0; try < 3; try++) {
>> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
>> index 3e62d1450682..dfd6722bc40e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_pps.c
>> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
>> @@ -431,7 +431,7 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
>>  	return intel_de_read(dev_priv, _pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
>>  }
>>  
>> -void intel_dp_check_edp(struct intel_dp *intel_dp)
>> +void intel_pps_check_power_unlocked(struct intel_dp *intel_dp)
> IMHO comment to take pps_lock would be useful here.

Part of the point of this change is to name it _unlocked to highlight it
does not take the lock, i.e. you should be aware of locking. You see
this pattern all over the kernel. It's self-documenting code.

Moreover, after the edp check, the calls here have:

	lockdep_assert_held(&dev_priv->pps_mutex);

which both documents the requirement as well as ensures the proper usage
in lockdep builds. I don't think a comment adds any value.

BR,
Jani.


> Thanks,
> Anshuman.
>>  {
>>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
>> index 4780b59a59df..8dda282abd42 100644
>> --- a/drivers/gpu/drm/i915/display/intel_pps.h
>> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
>> @@ -22,7 +22,6 @@ intel_wakeref_t intel_pps_unlock(struct intel_dp *intel_dp, intel_wakeref_t wake
>>  #define with_intel_pps_lock(dp, wf)						\
>>  	for ((wf) = intel_pps_lock(dp); (wf); (wf) = intel_pps_unlock((dp), (wf)))
>>  
>> -void intel_dp_check_edp(struct intel_dp *intel_dp);
>>  void intel_pps_backlight_on(struct intel_dp *intel_dp);
>>  void intel_pps_backlight_off(struct intel_dp *intel_dp);
>>  void intel_pps_backlight_power(struct intel_connector *connector, bool enable);
>> @@ -31,6 +30,7 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp);
>>  void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync);
>>  void intel_pps_on_unlocked(struct intel_dp *intel_dp);
>>  void intel_pps_off_unlocked(struct intel_dp *intel_dp);
>> +void intel_pps_check_power_unlocked(struct intel_dp *intel_dp);
>>  
>>  void intel_pps_vdd_on(struct intel_dp *intel_dp);
>>  void intel_pps_on(struct intel_dp *intel_dp);
>> -- 
>> 2.20.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx