Driver implements different callbacks for power off hardware
(.power_off_hw) and power off controller (.power_off_controller):

 - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
 - iris_vpu3_power_off_hardware,
 - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,

The first group (iris_vpu_power_off_hw() and
iris_vpu_power_off_controller()) is used on older VPU2 designs but also
called from newer ones: iris_vpu3_power_off_hardware() calls
iris_vpu_power_off_controller().

In the same time there is wrapper iris_vpu_power_off() which calls
respective callbacks (the VPU2, VPU3 etc).

Let's make it more obvious which function is a generic wrapper over
specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
the callback by adding "2" to callbacks used on VPU2.  No functional
changes.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/media/platform/qcom/iris/iris_vpu2.c       | 4 ++--
 drivers/media/platform/qcom/iris/iris_vpu3x.c      | 6 +++---
 drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 ++--
 drivers/media/platform/qcom/iris/iris_vpu_common.h | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/qcom/iris/iris_vpu2.c b/drivers/media/platform/qcom/iris/iris_vpu2.c
index 7cf1bfc352d3..2570e65816f6 100644
--- a/drivers/media/platform/qcom/iris/iris_vpu2.c
+++ b/drivers/media/platform/qcom/iris/iris_vpu2.c
@@ -33,7 +33,7 @@ static u64 iris_vpu2_calc_freq(struct iris_inst *inst, size_t data_size)
 }
 
 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,
 	.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);
 }
 
 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);
 
 disable_power:
-	iris_vpu_power_off_hw(core);
+	iris_vpu2_power_off_hw(core);
 }
 
 static int iris_vpu33_power_off_controller(struct iris_core *core)
@@ -264,7 +264,7 @@ static u64 iris_vpu3x_calculate_frequency(struct iris_inst *inst, size_t data_si
 
 const struct vpu_ops iris_vpu3_ops = {
 	.power_off_hw = iris_vpu3_power_off_hardware,
-	.power_off_controller = iris_vpu_power_off_controller,
+	.power_off_controller = iris_vpu2_power_off_controller,
 	.calc_freq = iris_vpu3x_calculate_frequency,
 };
 
diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
index 42a7c53ce48e..22f190e0c7c6 100644
--- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
+++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
@@ -211,7 +211,7 @@ int iris_vpu_prepare_pc(struct iris_core *core)
 	return -EAGAIN;
 }
 
-int iris_vpu_power_off_controller(struct iris_core *core)
+int iris_vpu2_power_off_controller(struct iris_core *core)
 {
 	u32 val = 0;
 	int ret;
@@ -253,7 +253,7 @@ int iris_vpu_power_off_controller(struct iris_core *core)
 	return 0;
 }
 
-void iris_vpu_power_off_hw(struct iris_core *core)
+void iris_vpu2_power_off_hw(struct iris_core *core)
 {
 	dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
 	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
index 93b7fa27be3b..8f63f243dd0d 100644
--- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
+++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
@@ -24,8 +24,8 @@ void iris_vpu_clear_interrupt(struct iris_core *core);
 int iris_vpu_watchdog(struct iris_core *core, u32 intr_status);
 int iris_vpu_prepare_pc(struct iris_core *core);
 int iris_vpu_power_on(struct iris_core *core);
-int iris_vpu_power_off_controller(struct iris_core *core);
-void iris_vpu_power_off_hw(struct iris_core *core);
+int iris_vpu2_power_off_controller(struct iris_core *core);
+void iris_vpu2_power_off_hw(struct iris_core *core);
 void iris_vpu_power_off(struct iris_core *core);
 
 #endif
-- 
2.43.0

On 7/2/2025 7:12 PM, Krzysztof Kozlowski wrote:
> Driver implements different callbacks for power off hardware
> (.power_off_hw) and power off controller (.power_off_controller):
> 
>  - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
>  - iris_vpu3_power_off_hardware,
>  - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,
> 
> The first group (iris_vpu_power_off_hw() and
> iris_vpu_power_off_controller()) is used on older VPU2 designs but also
> called from newer ones: iris_vpu3_power_off_hardware() calls
> iris_vpu_power_off_controller().
> 
> In the same time there is wrapper iris_vpu_power_off() which calls
> respective callbacks (the VPU2, VPU3 etc).
> 
> Let's make it more obvious which function is a generic wrapper over
> specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
> the callback by adding "2" to callbacks used on VPU2.  No functional
> changes.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/media/platform/qcom/iris/iris_vpu2.c       | 4 ++--
>  drivers/media/platform/qcom/iris/iris_vpu3x.c      | 6 +++---
>  drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 ++--
>  drivers/media/platform/qcom/iris/iris_vpu_common.h | 4 ++--
>  4 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu2.c b/drivers/media/platform/qcom/iris/iris_vpu2.c
> index 7cf1bfc352d3..2570e65816f6 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu2.c
> @@ -33,7 +33,7 @@ static u64 iris_vpu2_calc_freq(struct iris_inst *inst, size_t data_size)
>  }
>  
>  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.

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);
>  
>  disable_power:
> -	iris_vpu_power_off_hw(core);
> +	iris_vpu2_power_off_hw(core);
>  }
>  
>  static int iris_vpu33_power_off_controller(struct iris_core *core)
> @@ -264,7 +264,7 @@ static u64 iris_vpu3x_calculate_frequency(struct iris_inst *inst, size_t data_si
>  
>  const struct vpu_ops iris_vpu3_ops = {
>  	.power_off_hw = iris_vpu3_power_off_hardware,
> -	.power_off_controller = iris_vpu_power_off_controller,
> +	.power_off_controller = iris_vpu2_power_off_controller,
>  	.calc_freq = iris_vpu3x_calculate_frequency,
>  };
>  
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 42a7c53ce48e..22f190e0c7c6 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -211,7 +211,7 @@ int iris_vpu_prepare_pc(struct iris_core *core)
>  	return -EAGAIN;
>  }
>  
> -int iris_vpu_power_off_controller(struct iris_core *core)
> +int iris_vpu2_power_off_controller(struct iris_core *core)
>  {
>  	u32 val = 0;
>  	int ret;
> @@ -253,7 +253,7 @@ int iris_vpu_power_off_controller(struct iris_core *core)
>  	return 0;
>  }
>  
> -void iris_vpu_power_off_hw(struct iris_core *core)
> +void iris_vpu2_power_off_hw(struct iris_core *core)
>  {
>  	dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
>  	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> index 93b7fa27be3b..8f63f243dd0d 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> @@ -24,8 +24,8 @@ void iris_vpu_clear_interrupt(struct iris_core *core);
>  int iris_vpu_watchdog(struct iris_core *core, u32 intr_status);
>  int iris_vpu_prepare_pc(struct iris_core *core);
>  int iris_vpu_power_on(struct iris_core *core);
> -int iris_vpu_power_off_controller(struct iris_core *core);
> -void iris_vpu_power_off_hw(struct iris_core *core);
> +int iris_vpu2_power_off_controller(struct iris_core *core);
> +void iris_vpu2_power_off_hw(struct iris_core *core);
>  void iris_vpu_power_off(struct iris_core *core);
>  
>  #endif

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.

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

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

On 02/07/2025 14:42, Krzysztof Kozlowski wrote:
> Driver implements different callbacks for power off hardware
> (.power_off_hw) and power off controller (.power_off_controller):
> 
>   - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
>   - iris_vpu3_power_off_hardware,
>   - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,
> 
> The first group (iris_vpu_power_off_hw() and
> iris_vpu_power_off_controller()) is used on older VPU2 designs but also
> called from newer ones: iris_vpu3_power_off_hardware() calls
> iris_vpu_power_off_controller().
> 
> In the same time there is wrapper iris_vpu_power_off() which calls
> respective callbacks (the VPU2, VPU3 etc).
> 
> Let's make it more obvious which function is a generic wrapper over
> specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
> the callback by adding "2" to callbacks used on VPU2.  No functional
> changes.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   drivers/media/platform/qcom/iris/iris_vpu2.c       | 4 ++--
>   drivers/media/platform/qcom/iris/iris_vpu3x.c      | 6 +++---
>   drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 ++--
>   drivers/media/platform/qcom/iris/iris_vpu_common.h | 4 ++--
>   4 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu2.c b/drivers/media/platform/qcom/iris/iris_vpu2.c
> index 7cf1bfc352d3..2570e65816f6 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu2.c
> @@ -33,7 +33,7 @@ static u64 iris_vpu2_calc_freq(struct iris_inst *inst, size_t data_size)
>   }
>   
>   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,
>   	.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);
>   }
>   
>   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);
>   
>   disable_power:
> -	iris_vpu_power_off_hw(core);
> +	iris_vpu2_power_off_hw(core);
>   }
>   
>   static int iris_vpu33_power_off_controller(struct iris_core *core)
> @@ -264,7 +264,7 @@ static u64 iris_vpu3x_calculate_frequency(struct iris_inst *inst, size_t data_si
>   
>   const struct vpu_ops iris_vpu3_ops = {
>   	.power_off_hw = iris_vpu3_power_off_hardware,
> -	.power_off_controller = iris_vpu_power_off_controller,
> +	.power_off_controller = iris_vpu2_power_off_controller,
>   	.calc_freq = iris_vpu3x_calculate_frequency,
>   };
>   
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 42a7c53ce48e..22f190e0c7c6 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -211,7 +211,7 @@ int iris_vpu_prepare_pc(struct iris_core *core)
>   	return -EAGAIN;
>   }
>   
> -int iris_vpu_power_off_controller(struct iris_core *core)
> +int iris_vpu2_power_off_controller(struct iris_core *core)
>   {
>   	u32 val = 0;
>   	int ret;
> @@ -253,7 +253,7 @@ int iris_vpu_power_off_controller(struct iris_core *core)
>   	return 0;
>   }
>   
> -void iris_vpu_power_off_hw(struct iris_core *core)
> +void iris_vpu2_power_off_hw(struct iris_core *core)
>   {
>   	dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
>   	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> index 93b7fa27be3b..8f63f243dd0d 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
> @@ -24,8 +24,8 @@ void iris_vpu_clear_interrupt(struct iris_core *core);
>   int iris_vpu_watchdog(struct iris_core *core, u32 intr_status);
>   int iris_vpu_prepare_pc(struct iris_core *core);
>   int iris_vpu_power_on(struct iris_core *core);
> -int iris_vpu_power_off_controller(struct iris_core *core);
> -void iris_vpu_power_off_hw(struct iris_core *core);
> +int iris_vpu2_power_off_controller(struct iris_core *core);
> +void iris_vpu2_power_off_hw(struct iris_core *core);
>   void iris_vpu_power_off(struct iris_core *core);
>   
>   #endif

I prefer these names with an explicit v2 more logical/consistent with 
the hw names.

Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

On 02/07/2025 17:07, Bryan O'Donoghue wrote:
>>   {
>>   	dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
>>   	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
>> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.h b/drivers/media/platform/qcom/iris/iris_vpu_common.h
>> index 93b7fa27be3b..8f63f243dd0d 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.h
>> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.h
>> @@ -24,8 +24,8 @@ void iris_vpu_clear_interrupt(struct iris_core *core);
>>   int iris_vpu_watchdog(struct iris_core *core, u32 intr_status);
>>   int iris_vpu_prepare_pc(struct iris_core *core);
>>   int iris_vpu_power_on(struct iris_core *core);
>> -int iris_vpu_power_off_controller(struct iris_core *core);
>> -void iris_vpu_power_off_hw(struct iris_core *core);
>> +int iris_vpu2_power_off_controller(struct iris_core *core);
>> +void iris_vpu2_power_off_hw(struct iris_core *core);
>>   void iris_vpu_power_off(struct iris_core *core);
>>   
>>   #endif
> 
> I prefer these names with an explicit v2 more logical/consistent with 
> the hw names.
> 
> Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>


Thanks.

I got some patchwork complains, so it seems I missed something which
clang did not catch. There will be a v2.


Best regards,
Krzysztof

On 7/2/25 3:42 PM, Krzysztof Kozlowski wrote:
> Driver implements different callbacks for power off hardware
> (.power_off_hw) and power off controller (.power_off_controller):
> 
>  - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
>  - iris_vpu3_power_off_hardware,
>  - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,
> 
> The first group (iris_vpu_power_off_hw() and
> iris_vpu_power_off_controller()) is used on older VPU2 designs but also
> called from newer ones: iris_vpu3_power_off_hardware() calls
> iris_vpu_power_off_controller().
> 
> In the same time there is wrapper iris_vpu_power_off() which calls
> respective callbacks (the VPU2, VPU3 etc).
> 
> Let's make it more obvious which function is a generic wrapper over
> specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
> the callback by adding "2" to callbacks used on VPU2.  No functional
> changes.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---

[...]

> 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);
>  }
>  
>  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);
>  
>  disable_power:
> -	iris_vpu_power_off_hw(core);
> +	iris_vpu2_power_off_hw(core);
>  }

I don't really like how v3 calls v2 ops internally.. and there's
nothing really vpu2-specific about what the function does.
Maybe something along the lines of "iris_disable_resources"?

Konrad

On 02/07/2025 16:02, Konrad Dybcio wrote:
> On 7/2/25 3:42 PM, Krzysztof Kozlowski wrote:
>> Driver implements different callbacks for power off hardware
>> (.power_off_hw) and power off controller (.power_off_controller):
>>
>>  - iris_vpu_power_off_hw + iris_vpu_power_off_controller,
>>  - iris_vpu3_power_off_hardware,
>>  - iris_vpu33_power_off_hardware + iris_vpu33_power_off_controller,
>>
>> The first group (iris_vpu_power_off_hw() and
>> iris_vpu_power_off_controller()) is used on older VPU2 designs but also
>> called from newer ones: iris_vpu3_power_off_hardware() calls
>> iris_vpu_power_off_controller().
>>
>> In the same time there is wrapper iris_vpu_power_off() which calls
>> respective callbacks (the VPU2, VPU3 etc).
>>
>> Let's make it more obvious which function is a generic wrapper over
>> specific VPU/platform callbacks (iris_vpu_power_off()) and which one is
>> the callback by adding "2" to callbacks used on VPU2.  No functional
>> changes.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
> 
> [...]
> 
>> 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);
>>  }
>>  
>>  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);
>>  
>>  disable_power:
>> -	iris_vpu_power_off_hw(core);
>> +	iris_vpu2_power_off_hw(core);
>>  }
> 
> I don't really like how v3 calls v2 ops internally.. and there's
> nothing really vpu2-specific about what the function does.
> Maybe something along the lines of "iris_disable_resources"?

Context: sm8750 comes with more resources, so it will come with vpu35
doing this differently.

That's why any generic name (non platform specific) is misleading IMO.
This is really disabling/power off for the VPU2, 3 and 3.3. Not newer,
at least after initial look at SM8750.

Best regards,
Krzysztof