Just two notes, apart from that looks like a nice cleanup to me.
On 5/8/26 04:47, Andre Hirata wrote:
> @@ -46,10 +47,9 @@ int amdgpu_dpm_get_sclk(struct amdgpu_device *adev, bool
> low)
> if (!pp_funcs->get_sclk)
> return 0;
>
> - mutex_lock(&adev->pm.mutex);
> + guard(mutex)(&adev->pm.mutex);
> ret = pp_funcs->get_sclk((adev)->powerplay.pp_handle,
> low);
> - mutex_unlock(&adev->pm.mutex);
>
> return ret;
In a lot of cases you can now turn the patter "ret = f(...); return ret;" into
just return f(..); and potentially drop the ret variable.
> }
>
> @@ -291,9 +265,8 @@ bool amdgpu_dpm_is_mode1_reset_supported(struct
> amdgpu_device *adev)
> bool support_mode1_reset = false;
>
> if (is_support_sw_smu(adev)) {
> - mutex_lock(&adev->pm.mutex);
> + guard(mutex)(&adev->pm.mutex);
> support_mode1_reset = smu_mode1_reset_is_support(smu);
> - mutex_unlock(&adev->pm.mutex);
> }
>
> return support_mode1_reset;
For cases like this here the coding pattern to check the pre-requisites and
abort early is usually better.
So this example here would become:
if (!is_support_sw_smu(adev))
return false;
guard(mutex)(&adev->pm.mutex);
return smu_mode1_reset_is_support(smu);
Which as far as I can see is less code and much easier to read/understand.
But both suggestions could be a separate patch if you want to keep this one as
it is.
Regards,
Christian.
