Re: [PATCH v4 5/6] media: ipu-bridge: Match sensor configs per IPU and add config flags

[Sakari Ailus]

Hi Fernando,

On Mon, Aug 31, 2026 at 08:18:57PM +0200, Fernando Rimoli wrote:
> Some sensors need different treatment depending on which IPU they are
> connected to, so the sensor's ACPI HID alone is not always enough to
> describe what the bridge has to set up.
> 
> Add an optional IPU PCI product ID and a set of flags to struct
> ipu_sensor_config, along with an IPU_SENSOR_CONFIG_MATCH_FL() macro to
> define such an entry. A config naming a PCI product ID only applies to
> that IPU and takes precedence over a generic config for the same sensor,
> so that a sensor covered by both is connected once, through the more
> specific entry. Existing entries are unchanged and keep matching any IPU.
> 
> No flags are defined yet and no entry uses the new macro, so there is no
> functional change.

There's quite a bit of irrelevant information here.

> 
> Signed-off-by: Fernando Rimoli <fernandorimoli11@xxxxxxxxx>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 31 ++++++++++++++++++++++++++++
>  include/media/ipu-bridge.h           | 29 +++++++++++++++++++++-----
>  2 files changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index cd3c36d44..38ad3e54e 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -8,6 +8,7 @@
>  #include <linux/dmi.h>
>  #include <linux/i2c.h>
>  #include <linux/mei_cl_bus.h>
> +#include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/property.h>
> @@ -853,6 +854,32 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>  	return ret;
>  }
>  
> +/*
> + * Whether a sensor config applies to the IPU the bridge sits on. A config
> + * naming a PCI product ID only applies to that IPU, and takes precedence over
> + * a generic config for the same sensor, which is skipped so that the sensor is
> + * not connected twice.
> + */
> +static bool ipu_bridge_config_matches(const struct ipu_sensor_config *cfg,
> +				      struct ipu_bridge *bridge)
> +{
> +	unsigned int i;
> +
> +	if (cfg->pci_id)
> +		return cfg->pci_id == bridge->pci_id;
> +
> +	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {

Is there really a need to go through the entire array for each entry? Can't
you simply arrange the entries with a pci_id before the generic one?

> +		const struct ipu_sensor_config *sp =
> +			&ipu_supported_sensors[i];
> +
> +		if (sp->pci_id && sp->pci_id == bridge->pci_id &&
> +		    !strcmp(sp->hid, cfg->hid))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
>  {
>  	unsigned int i;
> @@ -862,6 +889,9 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
>  		const struct ipu_sensor_config *cfg =
>  			&ipu_supported_sensors[i];
>  
> +		if (!ipu_bridge_config_matches(cfg, bridge))
> +			continue;
> +
>  		ret = ipu_bridge_connect_sensor(cfg, bridge);
>  		if (ret)
>  			goto err_unregister_sensors;
> @@ -948,6 +978,7 @@ int ipu_bridge_init(struct device *dev,
>  		sizeof(bridge->ipu_node_name));
>  	bridge->ipu_hid_node.name = bridge->ipu_node_name;
>  	bridge->dev = dev;
> +	bridge->pci_id = dev_is_pci(dev) ? to_pci_dev(dev)->device : 0;
>  	bridge->parse_sensor_fwnode = parse_sensor_fwnode;
>  
>  	ret = software_node_register(&bridge->ipu_hid_node);
> diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
> index 61e10cef1..d12e51336 100644
> --- a/include/media/ipu-bridge.h
> +++ b/include/media/ipu-bridge.h
> @@ -17,13 +17,27 @@
>  #define IPU_SENSOR_ROTATION_NORMAL		0
>  #define IPU_SENSOR_ROTATION_INVERTED		1
>  
> -#define IPU_SENSOR_CONFIG(_HID, _NR, ...)	\
> -	(const struct ipu_sensor_config) {	\
> -		.hid = _HID,			\
> -		.nr_link_freqs = _NR,		\
> -		.link_freqs = { __VA_ARGS__ }	\
> +/* Flags for struct ipu_sensor_config */
> +#define IPU_BR_FL_NONE				0
> +
> +/*
> + * Sensor config specific to a single IPU, identified by its PCI product ID,
> + * with flags describing what the sensor needs on that IPU. Where both a
> + * specific and a generic (IPU_SENSOR_CONFIG) entry exist for the same HID,
> + * the specific one takes precedence.
> + */
> +#define IPU_SENSOR_CONFIG_MATCH_FL(_HID, _ID, _FLAGS, _NR, ...)	\
> +	(const struct ipu_sensor_config) {			\
> +		.hid = _HID,					\
> +		.pci_id = _ID,					\
> +		.flags = IPU_BR_FL_##_FLAGS,			\

Please don't assume a flag; setting multiple flags also doesn't work this
way.

> +		.nr_link_freqs = _NR,				\
> +		.link_freqs = { __VA_ARGS__ }			\
>  	}
>  
> +#define IPU_SENSOR_CONFIG(_HID, _NR, ...)			\
> +	IPU_SENSOR_CONFIG_MATCH_FL(_HID, 0, NONE, _NR, __VA_ARGS__)
> +
>  #define NODE_SENSOR(_HID, _PROPS)		\
>  	(const struct software_node) {		\
>  		.name = _HID,			\
> @@ -132,6 +146,9 @@ struct ipu_node_names {
>  
>  struct ipu_sensor_config {
>  	const char *hid;
> +	/* IPU PCI product ID this config is specific to, 0 for any */
> +	const u16 pci_id;

In later patches we already get two extra entries per sensor that only
differ on pci_id. How about making this a pointer to an array? Zero
termination should be fine here.

> +	const u32 flags;
>  	const u8 nr_link_freqs;
>  	const u64 link_freqs[MAX_NUM_LINK_FREQS];
>  };
> @@ -177,6 +194,8 @@ typedef int (*ipu_parse_sensor_fwnode_t)(struct acpi_device *adev,
>  
>  struct ipu_bridge {
>  	struct device *dev;
> +	/* PCI product ID of the IPU, 0 if it is not a PCI device */

All IPUs are PCI devices. ipu_bridge_init() should fail if a device isn't.
I think I might just omit the check.

> +	u16 pci_id;
>  	ipu_parse_sensor_fwnode_t parse_sensor_fwnode;
>  	char ipu_node_name[ACPI_ID_LEN];
>  	struct software_node ipu_hid_node;

-- 
Regards,

Sakari Ailus