Re: [PATCH v4 4/6] media: ipu-bridge: Assign endpoint property indices dynamically

[Sakari Ailus]

On Mon, Aug 31, 2026 at 08:18:56PM +0200, Fernando Rimoli wrote:
> The endpoint property array is populated with hardcoded indices, so a
> property that is only set conditionally has to be placed at a fixed slot.
> As the array is NULL-terminated, such a property is silently dropped when
> an earlier optional slot is left empty: "link-frequencies" is skipped for
> configs with nr_link_freqs == 0, which would truncate the array before
> anything following it.
> 
> Name the endpoint property slots in an enum, size the array accordingly
> and assign the indices through a bounds-checked running index, as done
> for the MIPI DisCo for Imaging properties in mipi-disco-img.c. No
> functional change intended: the same properties are set in the same
> order.

There's a lot of unneeded information here. For simple patches like this
there's no need to go deep in details that can be easily seen from the
code. How about:

Index the ep_properties array dynamically instead of plain numerical values
as is done in mipi-disco-img.c.

> 
> Signed-off-by: Fernando Rimoli <fernandorimoli11@xxxxxxxxx>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 27 ++++++++++++++-------------
>  include/media/ipu-bridge.h           | 19 ++++++++++++++++++-
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 131c70844..cd3c36d44 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -449,6 +449,7 @@ static void ipu_bridge_create_fwnode_properties(
>  	struct ipu_bridge *bridge,
>  	const struct ipu_sensor_config *cfg)
>  {
> +	unsigned int i = IPU_SENSOR_EP_BUS_TYPE;

You can just initialise this to 0.

>  	struct ipu_property_names *names = &sensor->prop_names;
>  	struct software_node *nodes = sensor->swnodes;
>  
> @@ -508,21 +509,21 @@ static void ipu_bridge_create_fwnode_properties(
>  			PROPERTY_ENTRY_REF_ARRAY("lens-focus", sensor->vcm_ref);
>  	}
>  
> -	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(
> -					sensor->prop_names.bus_type,
> -					V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
> -	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(
> -					sensor->prop_names.data_lanes,
> -					bridge->data_lanes, sensor->lanes);
> -	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(
> -					sensor->prop_names.remote_endpoint,
> -					sensor->local_ref);
> +	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =

You should use the maximum value of the property index here. Same below.
Right now it's fairly simple, but in more complicated cases it helps
keeping track of the properties.

> +		PROPERTY_ENTRY_U32(names->bus_type,
> +				   V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
> +	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
> +		PROPERTY_ENTRY_U32_ARRAY_LEN(names->data_lanes,
> +					     bridge->data_lanes, sensor->lanes);
> +	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
> +		PROPERTY_ENTRY_REF_ARRAY(names->remote_endpoint,
> +					 sensor->local_ref);
>  
>  	if (cfg->nr_link_freqs > 0)
> -		sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
> -			sensor->prop_names.link_frequencies,
> -			cfg->link_freqs,
> -			cfg->nr_link_freqs);
> +		sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
> +			PROPERTY_ENTRY_U64_ARRAY_LEN(names->link_frequencies,
> +						     cfg->link_freqs,
> +						     cfg->nr_link_freqs);
>  
>  	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>  					sensor->prop_names.data_lanes,
> diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
> index 16fac7654..61e10cef1 100644
> --- a/include/media/ipu-bridge.h
> +++ b/include/media/ipu-bridge.h
> @@ -64,6 +64,23 @@ enum ipu_sensor_swnodes {
>  	SWNODE_COUNT
>  };
>  
> +enum ipu_sensor_ep_props {
> +	IPU_SENSOR_EP_BUS_TYPE,
> +	IPU_SENSOR_EP_DATA_LANES,
> +	IPU_SENSOR_EP_REMOTE_EP,
> +	IPU_SENSOR_EP_LINK_FREQUENCIES,
> +	IPU_SENSOR_EP_NUM_OF,
> +	IPU_SENSOR_EP_NUM_ENTRIES
> +};
> +
> +/*
> + * Get the index of the next endpoint property in the property array, with a
> + * given maximum value.
> + */
> +#define IPU_NEXT_EP_PROPERTY(index, max)		\

The macro isn't limited to endpoint properties, how about calling it
e.g. IPU_BRIDGE_NEXT_PROPERTY?

> +	(WARN_ON((index) > IPU_SENSOR_EP_##max) ?	\

How about:

s/SENSOR_EP/BRIDGE/

> +	 IPU_SENSOR_EP_##max : (index)++)
> +
>  /* Data representation as it is in ACPI SSDB buffer */
>  struct ipu_sensor_ssdb {
>  	u8 version;
> @@ -141,7 +158,7 @@ struct ipu_sensor {
>  	const char *vcm_type;
>  
>  	struct ipu_property_names prop_names;
> -	struct property_entry ep_properties[5];
> +	struct property_entry ep_properties[IPU_SENSOR_EP_NUM_ENTRIES];
>  	struct property_entry dev_properties[5];
>  	struct property_entry ipu_properties[3];
>  	struct property_entry ivsc_properties[1];

-- 
Regards,

Sakari Ailus