Re: [PATCH v6 06/14] media: iris: Add context bank hooks for platform specific initialization

[Dmitry Baryshkov]

On Tue, May 26, 2026 at 04:51:52PM +0530, Vishnu Reddy wrote:
> 
> On 5/17/2026 11:05 PM, Dmitry Baryshkov wrote:
> > On Fri, May 15, 2026 at 04:51:21PM +0530, Vishnu Reddy wrote:
> >> The Glymur platform requires a dedicated firmware context bank device
> >> which is mapped to the firmware stream ID to load the firmware.
> > Why is it required on Glymur? Is it _only_ on Glymur?
> 
> It is required for firmware booting on platforms where Linux runs as the
> hypervisor (KVM/EL2), where the driver needs to manually manage the firmware
> IOMMU mapping via a dedicated context bank device. This is currently specific
> to Glymur.

Is it explained in the commit message? Usually, reviewer's 'why'
questions mean only one thing: there is no good enough problem
description in the commit message.

> 
> >> Add init and deinit hooks in the platform data for context bank setup.
> >> These hooks allow platform specific code to initialize and tear down
> >> context banks.
> >>
> >> Reviewed-by: Vikash Garodia <vikash.garodia@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
> >> ---
> >>  .../platform/qcom/iris/iris_platform_common.h      |  2 ++
> >>  drivers/media/platform/qcom/iris/iris_probe.c      | 23 +++++++++++++++++++++-
> >>  2 files changed, 24 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> >> index 6a108173be35..84fc68128c70 100644
> >> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> >> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> >> @@ -263,6 +263,8 @@ struct iris_platform_data {
> >>  	 */
> >>  	const struct iris_firmware_desc *firmware_desc;
> >>  
> >> +	int (*init_cb_devs)(struct iris_core *core);
> >> +	void (*deinit_cb_devs)(struct iris_core *core);
> > Why are they being added directly to iris_platform_data? Why not the
> > vpu_ops?
> 
> Some vpu_ops are shared across more than one platform because of same
> power sequence.

That's still fine, it's better to have vpu_ops with same callbacks
(except for this one) rather than having a one-off callback not matching
anything else in struct.

> 
> >>  	const struct vpu_ops *vpu_ops;
> >>  	const struct icc_info *icc_tbl;
> >>  	unsigned int icc_tbl_size;

-- 
With best wishes
Dmitry