Re: [PATCH 5/8] media: atomisp: support the Yoga Book OV2740 link

[Andy Shevchenko]

On Wed, Aug 26, 2026 at 03:22:53PM +0200, Maurizio Casciano wrote:
> The YB1-X91L firmware does not describe the complete camera link. Its
> front OV2740 uses two CSI-2 lanes at a 288 MHz link frequency and sends
> a 1932x1092 BGGR transport frame with 12 pixels of horizontal and
> vertical padding around the 1920x1080 image.
> 
> Allow the AtomISP bridge to provide per-sensor link frequencies and
> padding, add the matching OV2740 mode, and derive ISP2401 D-PHY timing
> from the sensor link-frequency control.
> 
> The register values are hardware configuration facts checked against
> the Lenovo YB1-X91L configuration and physical captures; no proprietary
> driver code or tuning binary is included.
> 
> Tested on a Lenovo Yoga Book YB1-X91L with continuous front-camera raw
> capture.

...

>  #define OV2740_LINK_FREQ_360MHZ		360000000ULL
> +#define OV2740_LINK_FREQ_288MHZ		288000000ULL
>  #define OV2740_LINK_FREQ_180MHZ		180000000ULL

At some point would be good to use HZ_PER_MHZ multiplier from units.h.

...

> struct ov2740_mode {

>  	/* Link frequency needed for this resolution */
>  	u32 link_freq_index;
>  
> +	/* Optional common settings applied before the mode-specific settings */
> +	const struct ov2740_reg_list init_reg_list;
> +
>  	/* Sensor register settings for this resolution */
>  	const struct ov2740_reg_list reg_list;
> +
> +	/* Bayer order produced by this mode */
> +	u32 code;

Check with `pahole`.

>  };

...

> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -520,11 +520,16 @@ static void ipu_bridge_create_fwnode_properties(
>  					sensor->prop_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);
> +	if (sensor->nr_link_freqs > 0)
> +		sensor->ep_properties[3] =
> +			PROPERTY_ENTRY_U64_ARRAY_LEN(names->link_frequencies,
> +						     sensor->link_freqs,
> +						     sensor->nr_link_freqs);
> +	else if (cfg->nr_link_freqs > 0)
> +		sensor->ep_properties[3] =
> +			PROPERTY_ENTRY_U64_ARRAY_LEN(names->link_frequencies,
> +						     cfg->link_freqs,
> +						     cfg->nr_link_freqs);

Why do we need this? The sensors are listed in the supported ones in the IPU
bridge code. Also, if required, some data can be altered by using driver_data
field in ACPI ID table.

...

> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.h

>  int atomisp_csi2_bridge_init(struct atomisp_device *isp);
>  int atomisp_csi2_bridge_parse_firmware(struct atomisp_device *isp);
> +bool atomisp_csi2_get_sensor_padding(struct device *dev, u32 *padding_w,
> +				     u32 *padding_h);

Better to split logically.

bool atomisp_csi2_get_sensor_padding(struct device *dev,
				     u32 *padding_w, u32 *padding_h);

>  void atomisp_csi2_configure(struct atomisp_sub_device *asd);

...

> +bool atomisp_csi2_get_sensor_padding(struct device *dev, u32 *padding_w,
> +				     u32 *padding_h)
> +{

Ditto.

> +	struct acpi_device *adev = ACPI_COMPANION(dev);

Split the assignment. The current way is prone to subtle mistakes in the
future.

> +	bool override = false;
> +	char *str_val;
> +	unsigned int val;

Try to use reversed xmas tree order.

> +	*padding_w = pad_w;
> +	*padding_h = pad_h;

	adev = ACPI_COMPANION(dev);

> +	if (!adev)
> +		return false;
> +
> +	str_val = gmin_cfg_get(adev, "CsiPaddingWidth");
> +	if (str_val) {
> +		if (!kstrtouint(str_val, 0, &val) && val <= 64 && !(val & 1)) {
> +			*padding_w = val;
> +			override = true;
> +		} else {
> +			acpi_handle_warn(adev->handle,
> +					 "%s: Invalid CSI padding width %s\n",
> +					 dev_name(dev), str_val);
> +		}
> +		kfree(str_val);
> +	}
> +
> +	str_val = gmin_cfg_get(adev, "CsiPaddingHeight");
> +	if (str_val) {
> +		if (!kstrtouint(str_val, 0, &val) && val <= 64 && !(val & 1)) {
> +			*padding_h = val;
> +			override = true;
> +		} else {
> +			acpi_handle_warn(adev->handle,
> +					 "%s: Invalid CSI padding height %s\n",
> +					 dev_name(dev), str_val);
> +		}
> +		kfree(str_val);
> +	}

This is dup of the code, please create a helper to deduplicate these.
Also, what's wrong with gmin_cfg_get_int()?

> +	return override;
> +}

...

>  {
>  	const struct acpi_device_id *id;
> +	char *link_freq_str;
> +	unsigned long long link_freq;

Reversed xmas tree order.

>  	int ret, clock_num;
>  	bool vcm = false;
>  	int lanes = 1;

...

> +	link_freq_str = gmin_cfg_get(adev, "CsiLinkFreq");
> +	if (link_freq_str) {
> +		ret = kstrtoull(link_freq_str, 0, &link_freq);
> +		kfree(link_freq_str);
> +		if (ret || !link_freq) {
> +			acpi_handle_err(adev->handle,
> +					"%s: Invalid CSI link frequency\n",
> +					dev_name(&adev->dev));
> +			return ret ?: -EINVAL;
> +		}

gmin_cfg_get_int()

> +		sensor->link_freqs[0] = link_freq;
> +		sensor->nr_link_freqs = 1;
> +	}

-- 
With Best Regards,
Andy Shevchenko