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@xxxxxxxxx wrote:
> > From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
>
> 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
