Re: [PATCH v3 7/7] media: iris: add platform data for kaanapali

[Krzysztof Kozlowski]

On 13/03/2026 16:46, Dmitry Baryshkov wrote:
> On Fri, Mar 13, 2026 at 06:49:41PM +0530, Vikash Garodia wrote:
>> Add support for the kaanapali platform by re-using the SM8550
>> definitions and using the vpu4 ops.
>> Move the configurations that differs in a per-SoC platform header, that
>> will contain SoC specific data.
>>
>> Co-developed-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Vikash Garodia <vikash.garodia@xxxxxxxxxxxxxxxx>
>> ---
>>  .../platform/qcom/iris/iris_platform_common.h      |  1 +
>>  .../media/platform/qcom/iris/iris_platform_gen2.c  | 90 ++++++++++++++++++++++
>>  .../platform/qcom/iris/iris_platform_kaanapali.h   | 83 ++++++++++++++++++++
>>  drivers/media/platform/qcom/iris/iris_probe.c      |  4 +
>>  4 files changed, 178 insertions(+)
>>
> 
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_kaanapali.h b/drivers/media/platform/qcom/iris/iris_platform_kaanapali.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..bdca1e5bf673353862c1554fb0420f73b3f519cb
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_kaanapali.h
>> @@ -0,0 +1,83 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef __IRIS_PLATFORM_KAANAPALI_H__
>> +#define __IRIS_PLATFORM_KAANAPALI_H__
>> +
>> +#include <dt-bindings/media/qcom,kaanapali-iris.h>
> 
> So, you are including the bindings here, from the header, which gets
> included from the C source file including headers for all the platforms.
> What if Kaanapali+1 (or +3) defines different sets of regions?

Different problem - header file MUST NOT have data definitions.

That's basic of C, we don't write such code. First because it leads to
multiplied, redundant data. Second, that's not C coding style.

This pattern in Qualcomm Iris is terrible and I could accept variations
of existing data like below:

>
>> +
>> +#define VIDEO_REGION_VM0_SECURE_NP_ID		1
>> +#define VIDEO_REGION_VM0_NONSECURE_NP_ID	5
>> +
>> +static const char *const kaanapali_clk_reset_table[] = {
>> +	"bus0",
>> +	"bus1",
>> +	"core",
>> +	"vcodec0_core",
>> +};
>> +
>> +static const char *const kaanapali_pmdomain_table[] = {
>> +	"venus",
>> +	"vcodec0",
>> +	"vpp0",
>> +	"vpp1",
>> +	"apv",
>> +};
>> +
>> +static const struct platform_clk_data kaanapali_clk_table[] = {
>> +	{ IRIS_AXI_CLK, "iface" },
>> +	{ IRIS_CTRL_CLK, "core" },
>> +	{ IRIS_HW_CLK, "vcodec0_core" },
>> +	{ IRIS_AXI1_CLK, "iface1" },
>> +	{ IRIS_CTRL_FREERUN_CLK, "core_freerun" },
>> +	{ IRIS_HW_FREERUN_CLK, "vcodec0_core_freerun" },
>> +	{ IRIS_BSE_HW_CLK, "vcodec_bse" },
>> +	{ IRIS_VPP0_HW_CLK, "vcodec_vpp0" },
>> +	{ IRIS_VPP1_HW_CLK, "vcodec_vpp1" },
>> +	{ IRIS_APV_HW_CLK, "vcodec_apv" },
>> +};
>> +
>> +static const char *const kaanapali_opp_clk_table[] = {
>> +	"vcodec0_core",
>> +	"vcodec_apv",
>> +	"vcodec_bse",
>> +	"core",
>> +	NULL,
>> +};
>> +
>> +static struct tz_cp_config tz_cp_config_kaanapali[] = {

But this is new thus NAK.

Don't grow this broken pattern. There is no single reason data
definition should be placed in a header. No single one.

Best regards,
Krzysztof