Re: [PATCH v5 4/6] media: qcom: camss: Add sc8280xp resource details

[Konrad Dybcio]

On 10.11.2023 02:04, Bryan O'Donoghue wrote:
> This commit describes the hardware layout for the sc8280xp for the
> following hardware blocks:
> 
> - 4 x VFE, 4 RDI per VFE
> - 4 x VFE Lite, 4 RDI per VFE
> - 4 x CSID
> - 4 x CSID Lite
> - 4 x CSI PHY
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
[...]

> +static const struct camss_subdev_resources vfe_res_sc8280xp[] = {
> +	/* IFE0 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "gcc_axi_sf", "cpas_ahb", "camnoc_axi", "vfe0", "vfe0_axi" },
> +		.clock_rate = { { 0 },
> +				{ 0 },
> +				{ 19200000, 80000000},
> +				{ 19200000, 150000000, 266666667, 320000000, 400000000, 480000000 },
> +				{ 400000000, 558000000, 637000000, 760000000 },
> +				{ 0 }, },
> +		.reg = { "vfe0" },
> +		.interrupt = { "vfe0" },
> +		.pd_name = "ife0",
So, the comments before each array member, the reg/intr and pd names
are all over the place between IFE and VFE.. Is there a reason to this?


On top of that, another ideas to add onto your cleanup stack:

- Are VFEs within a CAMSS block actually different? Can we just do "vfe
  data" and "vfe number" + "vfe_lite data" & "vfe_lite number"?

- Should we move these platform structs into separate files?

- Reminder about the clk_bulk_enable for clk_rate=0 clocks suggestion 

- OPP

- Use _num instead of sentinels and magic scary while (not null)

Konrad