Re: [PATCH] media: iris: Be explicit in naming of VPU2 power off handlers

[Vikash Garodia]

On 7/2/2025 10:59 PM, Krzysztof Kozlowski wrote:
> On 02/07/2025 19:20, Vikash Garodia wrote:
>>>  
>>>  const struct vpu_ops iris_vpu2_ops = {
>>> -	.power_off_hw = iris_vpu_power_off_hw,
>>> -	.power_off_controller = iris_vpu_power_off_controller,
>>> +	.power_off_hw = iris_vpu2_power_off_hw,
>>> +	.power_off_controller = iris_vpu2_power_off_controller,
>> There was a reason to name it as VPU* independent, as it can be used for
>> multiple VPUs. There isn't any VPU specific code within iris_vpu_power_off_hw
>> that it needs to be associated to any VPU.>>
>>>  	.calc_freq = iris_vpu2_calc_freq,
>>>  };
>>> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> index 9b7c9a1495ee..a2c8a1650153 100644
>>> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> @@ -104,7 +104,7 @@ static void iris_vpu3_power_off_hardware(struct iris_core *core)
>>>  	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>>  
>>>  disable_power:
>>> -	iris_vpu_power_off_hw(core);
>>> +	iris_vpu2_power_off_hw(core);
>> Again, its like VPU3 does something specific and then reuses the common handling.
>>
>> I do not see a point in making this change.
> 
> The point is expressed in commit msg so address that. Also, this will
> not be even correct for SM8750.
When changes are raised for SM8750, the need of it can be reviewed then. Raise
the patch for power off for SM8750 to review the incorrectness better.

Regards,
Vikash
> 
>>
>> Regards,
>> Vikash
>>>  }
>>>  
>>>  static void iris_vpu33_power_off_hardware(struct iris_core *core)
>>> @@ -142,7 +142,7 @@ static void iris_vpu33_power_off_hardware(struct iris_core *core)
>>>  	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>>  
> 
> Please kindly trim the replies from unnecessary context. It makes it
> much easier to find new content.
> 
> Best regards,
> Krzysztof