Re: [PATCH 04/17] media: atomisp: pci: do not use err var when checking port validity for ISP2400

[Mauro Carvalho Chehab <mchehab@kernel.org>]

Em Mon, 18 Oct 2021 01:19:44 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> Currently, the `port >= N_CSI_PORTS || err` checks for ISP2400 are always
> evaluated as true because the err variable is set to `-EINVAL` on
> declaration but the variable is never used until the evaluation.
> 
> Looking at the diff of commit 3c0538fbad9f ("media: atomisp: get rid of
> most checks for ISP2401 version"), the `port >= N_CSI_PORTS` check is
> for ISP2400 and the err variable check is for ISP2401. Fix this issue
> by adding ISP version test there accordingly.
> 
> Yes, there are other better ways to fix this issue, like adding support
> for ISP2400 to ia_css_mipi_is_source_port_valid(). In this way, we can
> unify the following test:
> 
> 	if (!IS_ISP2401)
> 		port = (unsigned int)pipe->stream->config.source.port.port;
> 	else
> 		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> 
> However, the IS_ISP2401 test here (formerly `ifdef ISP2401`) is not
> a result of real hardware difference, but just a result of the following
> two different versions of driver merged by tools [1]:
> 
>   - ISP2400: irci_stable_candrpv_0415_20150521_0458
>   - ISP2401: irci_ecr-master_20150911_0724

No.

While I don't have any internal information from the hardware manufacturer,
I guess you misinterpreted things here. 2400 and 2401 are different
hardware versions. See atomisp_pci_probe() logic.

Basically, Cherrytail and Anniedale comes with 2401. Older Atom CPUs
(Merrifield and Baytrail) comes with 2400.

> We should eventually remove (not unify) such tests caused by just a
> driver version difference and use just one version of driver. So, for
> now, let's avoid further unification.
> 
> [1] The function ia_css_mipi_is_source_port_valid() and its usage is
>     added on updating css version to irci_master_20150701_0213
>     https://raw.githubusercontent.com/intel/ProductionKernelQuilts/cht-m1stable-2016_ww31/uefi/cht-m1stable/patches/cam-0439-atomisp2-css2401-and-2401_legacy-irci_master_2015070.patch
>     ("atomisp2: css2401 and 2401_legacy-irci_master_20150701_0213")

What happens is that there is a 2401 and a 2401 "legacy". It sounds
that this due to some different software stacks that are reflected both
at the firmware and at the driver.

-

On other words, this patch requires some rework, as otherwise it will break
support for Baytrail.

Also, patch 13 should be dropped, as the firmware versions for 2400 are
different - and maybe patches 8 to 12 may need more work in order to not
touch 2400.

Regards,
Mauro