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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Mon, Jan 15, 2024 at 02:36:43PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 1/11/24 07:55, 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>
> 
> Just one small thing I noticed, not a full review.
> 
> <snip>
> 
> > +static u32 ipu6se_csi_offsets[] = {
> > +	IPU6_CSI_PORT_A_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_B_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_C_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_D_ADDR_OFFSET
> > +};
> > +
> > +/*
> > + * IPU6 on TGL support maximum 8 csi2 ports
> > + * IPU6SE on JSL and IPU6EP on ADL support maximum 4 csi2 ports
> > + * IPU6EP on MTL support maximum 6 csi2 ports
> > + */
> > +static u32 ipu6_tgl_csi_offsets[] = {
> > +	IPU6_CSI_PORT_A_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_B_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_C_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_D_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_E_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_F_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_G_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_H_ADDR_OFFSET
> > +};
> > +
> > +static u32 ipu6ep_mtl_csi_offsets[] = {
> > +	IPU6_CSI_PORT_A_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_B_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_C_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_D_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_E_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_F_ADDR_OFFSET
> > +};
> > +
> > +static u32 ipu6_csi_offsets[] = {
> > +	IPU6_CSI_PORT_A_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_B_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_C_ADDR_OFFSET,
> > +	IPU6_CSI_PORT_D_ADDR_OFFSET
> > +};
> 
> These 4 arrays all are the same, except for their lengths.
> 
> Please just use a single array wuth the full 8 A-H
> entries and then just directly code the amount of ports
> here:
> 
> > +static void ipu6_internal_pdata_init(struct ipu6_device *isp)
> > +{
> <snip>
> 
> > +	isys_ipdata.csi2.nports = ARRAY_SIZE(ipu6_csi_offsets);
> 
> So put the default 4 here instead of ARRAY_SIZE(),
> 
> <snip>
> 
> > +	if (is_ipu6_tgl(hw_ver)) {
> > +		isys_ipdata.csi2.nports = ARRAY_SIZE(ipu6_tgl_csi_offsets);
> 
> and then put 8 here instead of ARRAY_SIZE(), etc.
> 
> Then there no longer is a need to have 4 different arrays just
> to have them a different lenght.

Instead of having these arrays at all, you could directly use CSI_REG_BASE
+ CSI_REG_BASE_PORT(port_number) as the base address. There's only need to
store nr_of_ports in that case.

I'd prefer adding a macro for the numbers of ports on various devices, if
the information remains embedded in a function (rather than the big device
description elsewhere).

> 
> And this line:
> 
> > +		isys_ipdata.csi2.offsets = ipu6_tgl_csi_offsets;
> 
> Can then be fully dropped as well as similar lines below.
> 
> 
> > +	}
> > +
> > +	if (is_ipu6ep_mtl(hw_ver)) {
> > +		isys_ipdata.csi2.nports = ARRAY_SIZE(ipu6ep_mtl_csi_offsets);
> > +		isys_ipdata.csi2.offsets = ipu6ep_mtl_csi_offsets;
> 
> E.g. this one can also be dropped.

-- 
Regards,

Sakari Ailus