Re: [PATCH v2 01/15] media: intel/ipu6: add Intel IPU6 PCI device driver

From: Andreas Helbech Kleist <andreaskleist@gmail.com>
To: bingbu.cao@intel.com, linux-media@vger.kernel.org,
	 sakari.ailus@linux.intel.com, laurent.pinchart@ideasonboard.com
Cc: andriy.shevchenko@linux.intel.com, hdegoede@redhat.com,
	 ilpo.jarvinen@linux.intel.com, claus.stovgaard@gmail.com,
	tfiga@chromium.org,  senozhatsky@chromium.org,
	tomi.valkeinen@ideasonboard.com,  bingbu.cao@linux.intel.com,
	tian.shu.qiu@intel.com, hongju.wang@intel.com
Subject: Re: [PATCH v2 01/15] media: intel/ipu6: add Intel IPU6 PCI device driver
Date: Thu, 23 Nov 2023 10:17:17 +0100	[thread overview]
Message-ID: <5d6dd0643db42ba818383f3d27c99859a5d270ff.camel@gmail.com> (raw)
In-Reply-To: <20231024112924.3934228-2-bingbu.cao@intel.com>

Hi,

On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> Intel Image Processing Unit 6th Gen includes input and processing
> systems
> but the hardware presents itself as a single PCI device in system.
> 
> IPU6 PCI device driver basically does PCI configurations and load
> the firmware binary, initialises IPU virtual bus, and sets up
> platform
> specific variants to support multiple IPU6 devices in single device
> driver.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Suggested-by: Andreas Helbech Kleist <andreaskleist@gmail.com>
> ---
>  .../media/pci/intel/ipu6/ipu6-platform-regs.h | 179 ++++
>  drivers/media/pci/intel/ipu6/ipu6.c           | 952
> ++++++++++++++++++
>  drivers/media/pci/intel/ipu6/ipu6.h           | 352 +++++++
>  3 files changed, 1483 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-platform-regs.h
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6.c
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6.h
...
> +static void ipu6_internal_pdata_init(struct ipu6_device *isp)
> +{
> +       u8 hw_ver = isp->hw_ver;
> +
> +       isys_ipdata.num_parallel_streams = IPU6_ISYS_NUM_STREAMS;
> +       isys_ipdata.sram_gran_shift = IPU6_SRAM_GRANULARITY_SHIFT;
> +       isys_ipdata.sram_gran_size = IPU6_SRAM_GRANULARITY_SIZE;
> +       isys_ipdata.max_sram_size = IPU6_MAX_SRAM_SIZE;
> +       isys_ipdata.sensor_type_start =
> IPU6_FW_ISYS_SENSOR_TYPE_START;
> +       isys_ipdata.sensor_type_end = IPU6_FW_ISYS_SENSOR_TYPE_END;
> +       isys_ipdata.max_streams = IPU6_ISYS_NUM_STREAMS;

num_parallel_streams and max_streams are set to the same value here and
to another (but also equal) value for ipu6se.

num_parallel_streams is then passed through ipu6_fw_isys_init to
ipu6_isys_fwcom_cfg_init to then be clamp'ed to 1..max_streams. It
seems like they are the same, and thus a lot of code could be removed.

/Andreas