Em Mon, 01 Nov 2021 22:38:55 +0900
Tsuchiya Yuto <[email protected]> escreveu:
> On Thu, 2021-10-28 at 12:39 +0100, Mauro Carvalho Chehab wrote:
> > Em Thu, 28 Oct 2021 13:12:45 +0900
> > Tsuchiya Yuto <[email protected]> escreveu:
> >
> > > <Adding back people/list to Cc>
> > >
> > > On Tue, 2021-10-26 at 09:26 +0100, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 18 Oct 2021 01:19:44 +0900
> > > > Tsuchiya Yuto <[email protected]> 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.
> > >
> > > Yes, indeed, 2400 and 2401 are different hardware. When they (I mean who
> > > originally wrote atomisp driver non-upstream) needed to distinguish
> > > between ISP2400 and ISP2401, they used the ifdefs like the following:
> > >
> > > - USE_INPUT_SYSTEM_VERSION_2 (for both ISP2400/ISP2401)
> > > - USE_INPUT_SYSTEM_VERSION_2401 (for ISP2401)
> > > ...
> > >
> > > I think this is a sign that the atomisp driver supports both
> > > ISP2400/ISP2401 in a single version.
> >
> > Actually, supporting both on a single version is part of Alan's work.
> >
> > It seems he used the generation tool to produce a version for 2400, and
> > then re-used it to generate for 2401. It then used some scripting tool
> > to convert the differences on #ifdef ISP2401. See:
> >
> > a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
> >
> > There are things there like:
> >
> > +#ifdef ISP2401
> > +
> > +#endif
> >
> > I did a large cleanup work to get rid of those ifdefs, replacing them
> > by runtime logic.
> >
> > The end goal is to have a single compile-time driver that works for
> > both 2400 and 2401.
> >
> > This is not possible yet, as there are some registers that are mapped
> > on different addresses, depending on the hardware version, and making
> > it generic requires a lot of work and tests. So, for now, we need to
> > have a compile-time option to select between both.
> >
> > > Indeed, the upstreamed atomisp uses irci_stable_candrpv_0415_20150521_0458
> > > for ISP2400 and IIUC it was working on Bay Trail. On the other hand,
> > > intel-aero is a kernel for Cherry Trail and uses the same version
> > > irci_stable_candrpv_0415_20150521_0458.
> > >
> > > So, both ISP version ISP2400/ISP2401 can be supported by a single
> > > driver version.
> >
> > I See. OK!
> >
> > > > > 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.
> > >
> > > Yeah, I'm not sure what the "legacy" is. It might be a reference of
> > > `ISP2401_NEW_INPUT_SYSTEM` (css_2401_csi2p_system) and
> > > non-`ISP2401_NEW_INPUT_SYSTEM` (css_2401_system).
> > >
> > > > -
> > > >
> > > > On other words, this patch requires some rework, as otherwise it will break
> > > > support for Baytrail.
> > >
> > > You mean "this patch"? then, I intended this patch is rather a fix for
> > > ISP2400 case! The err variable for ISP2400 case is always true because
> > > it is not used before the error check:
> > >
> > > int
> > > allocate_mipi_frames(struct ia_css_pipe *pipe,
> > > struct ia_css_stream_info *info)
> > > {
> > > int err = -EINVAL;
> > > [...]
> > > if (!IS_ISP2401)
> > > port = (unsigned int)pipe->stream->config.source.port.port;
> > > else
> > > err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > >
> > > assert(port < N_CSI_PORTS);
> > >
> > > if (port >= N_CSI_PORTS || err) {
> > > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> > > "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> > > pipe, port);
> > > return -EINVAL;
> > > }
> > >
> > > The first usage of err variable is ia_css_mipi_is_source_port_valid()
> > > for IS_ISP2401 case, but it's not used for ISP2400 case. This causes
> > > the evaluation `port >= N_CSI_PORTS || err` always true for ISP2400 case,
> > > meaning it will be always treated as a error.
> >
> > Ok. Had you test the driver with Baytrail?
>
> Unfortunately, no. I don't have a Baytrail device to test (yet?). I
> noticed this issue anyway when I tried removing `#ifdef ISP2401`.
>
> This is not directly related to this series, but how we should reduce
> the ifdef usage in the future? Here are my two ideas:
>
> 1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> part completely irci_stable_candrpv_0415_20150521_0458
>
> this way does not require (relatively) much human work I think.
>
> But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> is basically an improved version.
No. What I said is that the if (ISP2401) and the remaining ifdefs are because
of BYT x CHT. The worse part of them are related to those files
(See Makefile):
obj-byt = \
pci/css_2400_system/hive/ia_css_isp_configs.o \
pci/css_2400_system/hive/ia_css_isp_params.o \
pci/css_2400_system/hive/ia_css_isp_states.o \
obj-cht = \
pci/css_2401_system/hive/ia_css_isp_configs.o \
pci/css_2401_system/hive/ia_css_isp_params.o \
pci/css_2401_system/hive/ia_css_isp_states.o \
pci/css_2401_system/host/csi_rx.o \
pci/css_2401_system/host/ibuf_ctrl.o \
pci/css_2401_system/host/isys_dma.o \
pci/css_2401_system/host/isys_irq.o \
pci/css_2401_system/host/isys_stream2mmio.o
Those define regmaps for 2400 and 2401. I was able to remove a lot
of things from the old css_2400/css_2401 directories, but the ones
there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
require some mapping functions to allow the same driver to work with
both BYT and CHT.
The better would be to test the driver first at BYT, fix issues (if any) and
then write the mapping code.
> So, we may also:
>
> 2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
>
> but this way needs more human work I think though. And if we go this
> way, I also need to rewrite this patch as mentioned in the commit
> message.
>
> > >
> > > > Also, patch 13 should be dropped, as the firmware versions for 2400 are
> > > > different
> > >
> > > The firmware version for 2400 on the upstreamed atomisp is
> > > irci_stable_candrpv_0415_20150521_0458 :-)
> > >
> > > static const char *isp2400_release_version = STR(irci_stable_candrpv_0415_20150521_0458);
> > > static const char *isp2401_release_version = STR(irci_ecr - master_20150911_0724);
> > >
> > > The intention of that patch is rather, it clarifies ISP2401 is now using
> > > the same driver (css) version as ISP2400.
> >
> > Ok.
> >
> > > > - and maybe patches 8 to 12 may need more work in order to not
> > > > touch 2400.
> > >
> > > Those patches do not break ISP2400, because what they do for ISP2400
> > > is that, they remove members from `struct`s which were initially inside
> > > of `ifdef ISP2401`. And because these removed members were initially
> > > inside of the ifdefs, the usage was also inside the ifdefs.
> >
> > Did you test on Baytrail (ISP2400), and with the compile-time option
> > enabled/disabled?
>
> Sorry, I should have clarified on the cover later. For ISP2400, I did
> compile test only (CONFIG_VIDEO_ATOMISP_ISP2401 enabled/disabled).
Maybe Hans could help us on that. I guess he has an Asus T100 device,
which is BYT based.
Hans, if you're willing to do the tests, you could either use nvt
or v4l2grab to test it.
It seems that BYT has an additional issue, though: the ISP seems to
require 12 non-visible lines/columns (in addition to 16 ones required
by CHT?) for it to work.
So, you may need to tweak the resolution a bit, as otherwise the
driver will return an -EINVAL.
See:
https://git.linuxtv.org/media_stage.git/commit/?id=dcbc4f570495dbd490975979471687cbe2246f99
For the workaround I had to add for CHT to properly report the
visible resolution.
Regards,
Mauro
Regards,
Mauro
Hi,
On 11/1/21 15:10, Mauro Carvalho Chehab wrote:
<snip>
>>> Did you test on Baytrail (ISP2400), and with the compile-time option
>>> enabled/disabled?
>>
>> Sorry, I should have clarified on the cover later. For ISP2400, I did
>> compile test only (CONFIG_VIDEO_ATOMISP_ISP2401 enabled/disabled).
>
> Maybe Hans could help us on that. I guess he has an Asus T100 device,
> which is BYT based.
>
> Hans, if you're willing to do the tests, you could either use nvt
> or v4l2grab to test it.
>
> It seems that BYT has an additional issue, though: the ISP seems to
> require 12 non-visible lines/columns (in addition to 16 ones required
> by CHT?) for it to work.
>
> So, you may need to tweak the resolution a bit, as otherwise the
> driver will return an -EINVAL.
>
> See:
>
> https://git.linuxtv.org/media_stage.git/commit/?id=dcbc4f570495dbd490975979471687cbe2246f99
>
> For the workaround I had to add for CHT to properly report the
> visible resolution.
Testing BYT support definitely is on my radar. Note that BYT
also has the additional issue that the atomisp2 on BYT can be
enumerated as either a PCI dev (which may work) or an ACPI/platform
dev which is unsupported in the original atomisp2 code-drop and
seems non trivial (because of pci config space writes) to get to
work.
On the T100TA the device is actually enumerated as an ACPI/platform
device and the BIOS option to change this is hidden. In the mean
time I've gained quite a lot of experience with changing hidden
BIOS options though, so I can easily put it in PCI mode for
testing. But eventually we also need to tackle ACPI enumeration
support...
Anyways I've let me self get distracted (hobby time wise) by
looking into PMIC/charger/fuel-gauge issues on the Xiaomi Mi Pad 2.
I've made a list of 3 (small-ish) loose ends which I want to
tie up there and then I plan to start looking into atomisp2
support in my hobby time. ATM my plan is:
-Test on T101HA to reproduce Mauro's work
-Try to get things to work on the T116
-Patch to not load atomisp_foo sensor drivers on !BYT && !CHT
And I've just added:
-Try out BYT support
As 4th item. Eventually I want to look into writing a proper
regulator driver for the PMICs and then try to make the atomisp2
code work with the non "atomisp_xxx" versions of the sensor drivers.
Regards,
Hans
On Mon, Nov 1, 2021 at 9:07 PM Hans de Goede <[email protected]> wrote:
> On 11/1/21 15:10, Mauro Carvalho Chehab wrote:
...
> Testing BYT support definitely is on my radar. Note that BYT
> also has the additional issue that the atomisp2 on BYT can be
> enumerated as either a PCI dev (which may work) or an ACPI/platform
> dev which is unsupported in the original atomisp2 code-drop and
> seems non trivial (because of pci config space writes) to get to
> work.
>
> On the T100TA the device is actually enumerated as an ACPI/platform
> device and the BIOS option to change this is hidden. In the mean
> time I've gained quite a lot of experience with changing hidden
> BIOS options though, so I can easily put it in PCI mode for
> testing. But eventually we also need to tackle ACPI enumeration
> support...
Not sure if you saw my very far from being even tested patch...
Share it here just in case. I believe you will have a better idea on
how to implement it.
--
With Best Regards,
Andy Shevchenko
Hi,
On 11/1/21 20:27, Andy Shevchenko wrote:
> On Mon, Nov 1, 2021 at 9:07 PM Hans de Goede <[email protected]> wrote:
>> On 11/1/21 15:10, Mauro Carvalho Chehab wrote:
>
> ...
>
>> Testing BYT support definitely is on my radar. Note that BYT
>> also has the additional issue that the atomisp2 on BYT can be
>> enumerated as either a PCI dev (which may work) or an ACPI/platform
>> dev which is unsupported in the original atomisp2 code-drop and
>> seems non trivial (because of pci config space writes) to get to
>> work.
>>
>> On the T100TA the device is actually enumerated as an ACPI/platform
>> device and the BIOS option to change this is hidden. In the mean
>> time I've gained quite a lot of experience with changing hidden
>> BIOS options though, so I can easily put it in PCI mode for
>> testing. But eventually we also need to tackle ACPI enumeration
>> support...
>
> Not sure if you saw my very far from being even tested patch...
> Share it here just in case. I believe you will have a better idea on
> how to implement it.
I don't remember seeing that, it is great to at least have
a starting point when I get around to looking into this, thank you!
Regards,
Hans
Em Mon, 1 Nov 2021 20:06:52 +0100
Hans de Goede <[email protected]> escreveu:
> Hi,
>
> On 11/1/21 15:10, Mauro Carvalho Chehab wrote:
>
> <snip>
>
> >>> Did you test on Baytrail (ISP2400), and with the compile-time option
> >>> enabled/disabled?
> >>
> >> Sorry, I should have clarified on the cover later. For ISP2400, I did
> >> compile test only (CONFIG_VIDEO_ATOMISP_ISP2401 enabled/disabled).
> >
> > Maybe Hans could help us on that. I guess he has an Asus T100 device,
> > which is BYT based.
> >
> > Hans, if you're willing to do the tests, you could either use nvt
> > or v4l2grab to test it.
> >
> > It seems that BYT has an additional issue, though: the ISP seems to
> > require 12 non-visible lines/columns (in addition to 16 ones required
> > by CHT?) for it to work.
> >
> > So, you may need to tweak the resolution a bit, as otherwise the
> > driver will return an -EINVAL.
> >
> > See:
> >
> > https://git.linuxtv.org/media_stage.git/commit/?id=dcbc4f570495dbd490975979471687cbe2246f99
> >
> > For the workaround I had to add for CHT to properly report the
> > visible resolution.
>
> Testing BYT support definitely is on my radar. Note that BYT
> also has the additional issue that the atomisp2 on BYT can be
> enumerated as either a PCI dev (which may work) or an ACPI/platform
> dev which is unsupported in the original atomisp2 code-drop and
> seems non trivial (because of pci config space writes) to get to
> work.
>
> On the T100TA the device is actually enumerated as an ACPI/platform
> device and the BIOS option to change this is hidden. In the mean
> time I've gained quite a lot of experience with changing hidden
> BIOS options though, so I can easily put it in PCI mode for
> testing. But eventually we also need to tackle ACPI enumeration
> support...
>
> Anyways I've let me self get distracted (hobby time wise) by
> looking into PMIC/charger/fuel-gauge issues on the Xiaomi Mi Pad 2.
> I've made a list of 3 (small-ish) loose ends which I want to
> tie up there and then I plan to start looking into atomisp2
> support in my hobby time. ATM my plan is:
>
> -Test on T101HA to reproduce Mauro's work
Yeah, it would be great to have a second test on it. I suspect that it
should work just fine with USERPTR (with v4l2grab or nvt).
> -Try to get things to work on the T116
Didn't test it here either, but won't be able to do in the next
couple of weeks.
> -Patch to not load atomisp_foo sensor drivers on !BYT && !CHT
Not sure if it is worth doing it, as there are a lot more to be
done before being able to use a generic sensor driver.
> And I've just added:
>
> -Try out BYT support
Thanks!
>
> As 4th item. Eventually I want to look into writing a proper
> regulator driver for the PMICs
Yeah, a proper regulator driver would be a lot better than the
PMIC ones.
> and then try to make the atomisp2
> code work with the non "atomisp_xxx" versions of the sensor drivers.
With a regulator driver, part of the problem will be solved. However,
as the ISP driver "eats" 16 lines and 16 columns. It means that the sensor
needs to be adjusted for it to provide those extra data. So, the atomisp
sensor resolutions are (X + 16) x (Y + 16), e. g. in the case of
ov2680, it is set to 1616x1216, while the upstream one is 1600x1200.
Not sure if the selection API currently allows changing that to
satisfy atomisp, or if something else would be needed.
Regards,
Mauro
Hi,
On 11/1/21 21:03, Mauro Carvalho Chehab wrote:
> Em Mon, 1 Nov 2021 20:06:52 +0100
> Hans de Goede <[email protected]> escreveu:
<snip>
>> -Patch to not load atomisp_foo sensor drivers on !BYT && !CHT
>
> Not sure if it is worth doing it, as there are a lot more to be
> done before being able to use a generic sensor driver.
As you may know, I'm also working on IPU3 support for $dayjob atm
actually :)
So the drivers for e.g. the ov5693 sensor conflict, by adding
a small (one line) check to atomisp_ov5693.c to not register
the driver at all when not on BYT/CHT we can avoid the conflict
on most devices for now. And when actually on BYT/CHT the user
will need to blacklist the non atomisp sensor-modules which, well
sucks, but atomisp is in staging for a reason ...
So the idea here is that with some small added ugliness to the
atomisp_foo.c sensor drivers we can make the 2 drivers co-exist
a bit more, allowing e.g. generic distro kernels to (maybe) enable
the atomisp2 stuff without regressing the IPU3 support.
###
Since we are discussing this now anyways, the atomisp_foo.c
patches would look like this:
#include <linux/platform_data/x86/soc.h>
if (!soc_intel_is_byt() && !soc_intel_is_cht())
return -ENODEV;
In the probe() function and change driver.name from
e.g. "ov5693" to "atom_ov5693".
Before I spend time on writing patches for this, would patches doing
this for conflicting drivers be acceptable ?
Regards,
Hans
Em Mon, 1 Nov 2021 22:06:12 +0100
Hans de Goede <[email protected]> escreveu:
> Hi,
>
> On 11/1/21 21:03, Mauro Carvalho Chehab wrote:
> > Em Mon, 1 Nov 2021 20:06:52 +0100
> > Hans de Goede <[email protected]> escreveu:
>
> <snip>
>
> >> -Patch to not load atomisp_foo sensor drivers on !BYT && !CHT
> >
> > Not sure if it is worth doing it, as there are a lot more to be
> > done before being able to use a generic sensor driver.
>
> As you may know, I'm also working on IPU3 support for $dayjob atm
> actually :)
Didn't know that... Btw, I have one Dell device with IPU3. It would
be great to have it working there ;-)
> So the drivers for e.g. the ov5693 sensor conflict, by adding
> a small (one line) check to atomisp_ov5693.c to not register
> the driver at all when not on BYT/CHT we can avoid the conflict
> on most devices for now. And when actually on BYT/CHT the user
> will need to blacklist the non atomisp sensor-modules which, well
> sucks, but atomisp is in staging for a reason ...
>
> So the idea here is that with some small added ugliness to the
> atomisp_foo.c sensor drivers we can make the 2 drivers co-exist
> a bit more, allowing e.g. generic distro kernels to (maybe) enable
> the atomisp2 stuff without regressing the IPU3 support.
>
> ###
>
> Since we are discussing this now anyways, the atomisp_foo.c
> patches would look like this:
>
> #include <linux/platform_data/x86/soc.h>
>
> if (!soc_intel_is_byt() && !soc_intel_is_cht())
> return -ENODEV;
>
> In the probe() function and change driver.name from
> e.g. "ov5693" to "atom_ov5693".
>
> Before I spend time on writing patches for this, would patches doing
> this for conflicting drivers be acceptable ?
Surely. Yeah, it makes sense to me. Feel free to submit such
patches.
Regards,
Mauro
Sorry for a little late reply. This is hard to explain...
On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> Em Mon, 01 Nov 2021 22:38:55 +0900
> Tsuchiya Yuto <[email protected]> escreveu:
[...]
>
> > This is not directly related to this series, but how we should reduce
> > the ifdef usage in the future? Here are my two ideas:
> >
> > 1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> > part completely irci_stable_candrpv_0415_20150521_0458
> >
> > this way does not require (relatively) much human work I think.
> >
> > But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> > is basically an improved version.
>
> No. What I said is that the if (ISP2401) and the remaining ifdefs are because
> of BYT x CHT.
I need to elaborate on this. Indeed some of them are really because of
BYT x CHT, but others are stuff from irci_ecr-master_20150911_0724.
What I meant when I mentioned "remove `#ifdef ISP2401` part" is that,
removing things which was _initially_ inside the `#ifdef ISP2401` on the
initial commit of atomisp.
Also I believe we can remove more `if (IS_ISP2401)` and `/* ISP2401 */`
things as well as the some remaining `#ifdef ISP2401` things.
I added about this below and hope it clarifies things...
> The worse part of them are related to those files
> (See Makefile):
>
> obj-byt = \
> pci/css_2400_system/hive/ia_css_isp_configs.o \
> pci/css_2400_system/hive/ia_css_isp_params.o \
> pci/css_2400_system/hive/ia_css_isp_states.o \
>
> obj-cht = \
> pci/css_2401_system/hive/ia_css_isp_configs.o \
> pci/css_2401_system/hive/ia_css_isp_params.o \
> pci/css_2401_system/hive/ia_css_isp_states.o \
> pci/css_2401_system/host/csi_rx.o \
> pci/css_2401_system/host/ibuf_ctrl.o \
> pci/css_2401_system/host/isys_dma.o \
> pci/css_2401_system/host/isys_irq.o \
> pci/css_2401_system/host/isys_stream2mmio.o
>
> Those define regmaps for 2400 and 2401. I was able to remove a lot
> of things from the old css_2400/css_2401 directories, but the ones
> there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
> require some mapping functions to allow the same driver to work with
> both BYT and CHT.
>
> The better would be to test the driver first at BYT, fix issues (if any) and
> then write the mapping code.
>
> > So, we may also:
> >
> > 2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> >
> > but this way needs more human work I think though. And if we go this
> > way, I also need to rewrite this patch as mentioned in the commit
> > message.
What the idea #1 want to say is, let's remove things _initially_ within
`ifdef ISP2401` (so, except things which were added inside it later
upstream) including formerly within `ifdef ISP2401` things, i.e.,
`if (IS_ISP2401)` and things commented with `/* ISP2401 */`.
However, I don't say we can remove all the ifdefs like things formerly
within USE_INPUT_SYSTEM_VERSION_2, USE_INPUT_SYSTEM_VERSION_2401, etc.,
which later removed/integrated into `ifdef ISP2401` on some commits [1].
We may temporarily revert those commits when we want to distinguish
between what were formerly within there and what were not.
Such ifdefs were added by them as a real hardware difference. Thus, I
agree that we still need the CONFIG_VIDEO_ATOMISP_ISP2401 stuff to support
both ISP2400/ISP2401 at the same time.
This is what I meant "reduce the ifdef usage" in a previous mail. So,
I'm not talking about if dropping CONFIG_VIDEO_ATOMISP_ISP2401 is doable,
but talking about just how to reduce the code.
[1] 641c2292bf19 ("media: atomisp: get rid of version-dependent globals")
bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
Anyway, if you agree or not on what I'm saying, can I send this patch
without code changes in v2, i.e., looks OK for you regarding the code?
I'll remove the commit message about
irci_stable_candrpv_0415_20150521_0458 vs irci_ecr-master_20150911_0724
in v2 anyway, which needs to be discussed further later.
The following notes are about what I have done until now for removing
such tests. (More elaborated version than cover letter). You don't have
to see them, but I hope it might clarify things...
## `ifdef ISP2401` added in the initial commit of atomisp
The `ifdef ISP2401` was the result of merging two different version of
driver, added on the initial commit of upstreamed atomisp. And for the
`ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
the initial commit of atomisp [2][3]
[1] here are the three exceptions:
("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
[2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
[3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f
Here is the kernel tree if someone is interested:
https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@a49d25364dfb_first
Especially, here is one of the part where this patch is touching
for example:
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
@@ -416,26 +362,16 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info)
[...]
-#ifndef ISP2401
port = (unsigned int) pipe->stream->config.source.port.port;
assert(port < N_CSI_PORTS);
if (port >= N_CSI_PORTS) {
-#else
- if (!ia_css_mipi_is_source_port_valid(pipe, &port)) {
-#endif
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
"allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
pipe, port);
By removing (almost) all of the `#ifdef ISP2401` things, (although we
still can't remove like former USE_INPUT_SYSTEM_VERSION_2,
USE_INPUT_SYSTEM_VERSION_2401) we can reduce the number of ifdefs.
## `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */` in the recent
atomisp
That is for the initial commit of atomisp. For the recent version of
atomisp, we can still remove `ifdef ISP2401` things (again, except things
which were added inside it later upstream) as well as the former
`ifdef ISP2401` things, i.e., `if (IS_ISP2401)` [1] and things commented
with [2] `/* ISP2401 */`.
[1] ("atomisp: pci: remove IS_ISP2401 test")
https://github.com/kitakar5525/linux-kernel/commit/397e543e493dfd60d91e2b5cc164da342b26906c
[2] ("atomisp: pci: remove `/* ISP2401 */` comments and its contents")
https://github.com/kitakar5525/linux-kernel/commit/b3928e3c1a709853971715ce35459b9b79e708f2
These commits were made against
bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
where I randomely picked.
Here is the kernel tree if someone is interested:
https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@bd674b5a413c_before_get_rid_ver_globals
I confirmed capture is still working here on surface3 (ISP2401). Compile
tested for ISP2400. As you can see, there are some WIP and FIXME commits
on top of removing such tests though. (The others are backports).
Especially, here is one of the part where this patch is touching
for example:
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -553,10 +548,7 @@ free_mipi_frames(struct ia_css_pipe *pipe) {
return err;
}
- if (!IS_ISP2401)
- port = (unsigned int)pipe->stream->config.source.port.port;
- else
- err = ia_css_mipi_is_source_port_valid(pipe, &port);
+ port = (unsigned int)pipe->stream->config.source.port.port;
assert(port < N_CSI_PORTS);
So, we can also remove a lot of `if (IS_ISP2401)` and `/* ISP2401 */`
things as well as the remaining `ifdef ISP2401`.
## WIP: removing `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */`
against the latest atomisp
And here is the branch where I'm working on removing such tests against
the latest atomisp:
https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp+remove_unneeded_tests
It'd be the best if I can show you working one, but it currently has
seemingly include issues on ISP2400 vs ISP2401 (as well as many WIP
commits I added):
drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c: In function ‘ia_css_isys_init’:
drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:29:9: error: unknown type name ‘backend_channel_cfg_t’
29 | backend_channel_cfg_t backend_ch0;
| ^~~~~~~~~~~~~~~~~~~~~
drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:30:9: error: unknown type name ‘backend_channel_cfg_t’
30 | backend_channel_cfg_t backend_ch1;
| ^~~~~~~~~~~~~~~~~~~~~
drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:31:9: error: unknown type name ‘target_cfg2400_t’
31 | target_cfg2400_t targetB;
| ^~~~~~~~~~~~~~~~
drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:32:9: error: unknown type name ‘target_cfg2400_t’
32 | target_cfg2400_t targetC;
| ^~~~~~~~~~~~~~~~
[...]
The full output of the make error is here:
("NOTE: issue: some undeclared errors")
https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
Regards,
Tsuchiya Yuto
> > >
On Thu, Nov 11, 2021 at 11:34:23PM +0900, Tsuchiya Yuto wrote:
> On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > Em Mon, 01 Nov 2021 22:38:55 +0900
> > Tsuchiya Yuto <[email protected]> escreveu:
...
> The full output of the make error is here:
>
> ("NOTE: issue: some undeclared errors")
> https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
I just realize that we may do at some point
cflags-y += -Werror
to avoid changes that breaks build (with warnings). And also I would suggest
to run build with `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`
--
With Best Regards,
Andy Shevchenko
Em Thu, 11 Nov 2021 23:34:23 +0900
Tsuchiya Yuto <[email protected]> escreveu:
> Sorry for a little late reply. This is hard to explain...
>
> On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > Em Mon, 01 Nov 2021 22:38:55 +0900
> > Tsuchiya Yuto <[email protected]> escreveu:
>
> [...]
> >
>
> > > This is not directly related to this series, but how we should reduce
> > > the ifdef usage in the future? Here are my two ideas:
> > >
> > > 1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> > > part completely irci_stable_candrpv_0415_20150521_0458
> > >
> > > this way does not require (relatively) much human work I think.
> > >
> > > But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> > > is basically an improved version.
> >
> > No. What I said is that the if (ISP2401) and the remaining ifdefs are because
> > of BYT x CHT.
>
> I need to elaborate on this. Indeed some of them are really because of
> BYT x CHT, but others are stuff from irci_ecr-master_20150911_0724.
>
> What I meant when I mentioned "remove `#ifdef ISP2401` part" is that,
> removing things which was _initially_ inside the `#ifdef ISP2401` on the
> initial commit of atomisp.
>
> Also I believe we can remove more `if (IS_ISP2401)` and `/* ISP2401 */`
> things as well as the some remaining `#ifdef ISP2401` things.
>
> I added about this below and hope it clarifies things...
It is clearer now. Yeah, we can touch on whatever is inside the
ISP2401 ifs, as we can now test them. Touching things for ISP2400
is harder, as we depend on a test platform.
> > The worse part of them are related to those files
> > (See Makefile):
> >
> > obj-byt = \
> > pci/css_2400_system/hive/ia_css_isp_configs.o \
> > pci/css_2400_system/hive/ia_css_isp_params.o \
> > pci/css_2400_system/hive/ia_css_isp_states.o \
> >
> > obj-cht = \
> > pci/css_2401_system/hive/ia_css_isp_configs.o \
> > pci/css_2401_system/hive/ia_css_isp_params.o \
> > pci/css_2401_system/hive/ia_css_isp_states.o \
> > pci/css_2401_system/host/csi_rx.o \
> > pci/css_2401_system/host/ibuf_ctrl.o \
> > pci/css_2401_system/host/isys_dma.o \
> > pci/css_2401_system/host/isys_irq.o \
> > pci/css_2401_system/host/isys_stream2mmio.o
> >
> > Those define regmaps for 2400 and 2401. I was able to remove a lot
> > of things from the old css_2400/css_2401 directories, but the ones
> > there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
> > require some mapping functions to allow the same driver to work with
> > both BYT and CHT.
> >
> > The better would be to test the driver first at BYT, fix issues (if any) and
> > then write the mapping code.
> >
> > > So, we may also:
> > >
> > > 2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> > >
> > > but this way needs more human work I think though. And if we go this
> > > way, I also need to rewrite this patch as mentioned in the commit
> > > message.
>
> What the idea #1 want to say is, let's remove things _initially_ within
> `ifdef ISP2401` (so, except things which were added inside it later
> upstream) including formerly within `ifdef ISP2401` things, i.e.,
> `if (IS_ISP2401)` and things commented with `/* ISP2401 */`.
>
> However, I don't say we can remove all the ifdefs like things formerly
> within USE_INPUT_SYSTEM_VERSION_2, USE_INPUT_SYSTEM_VERSION_2401, etc.,
> which later removed/integrated into `ifdef ISP2401` on some commits [1].
> We may temporarily revert those commits when we want to distinguish
> between what were formerly within there and what were not.
>
> Such ifdefs were added by them as a real hardware difference. Thus, I
> agree that we still need the CONFIG_VIDEO_ATOMISP_ISP2401 stuff to support
> both ISP2400/ISP2401 at the same time.
>
> This is what I meant "reduce the ifdef usage" in a previous mail. So,
> I'm not talking about if dropping CONFIG_VIDEO_ATOMISP_ISP2401 is doable,
> but talking about just how to reduce the code.
>
> [1] 641c2292bf19 ("media: atomisp: get rid of version-dependent globals")
> bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
>
> Anyway, if you agree or not on what I'm saying, can I send this patch
> without code changes in v2, i.e., looks OK for you regarding the code?
> I'll remove the commit message about
> irci_stable_candrpv_0415_20150521_0458 vs irci_ecr-master_20150911_0724
> in v2 anyway, which needs to be discussed further later.
No need for a v2. The /17 patch series was merged already, plus some
patches from the /5 that made sense to apply.
Ok, there are some followup patches that could be added, but please
send those in separate.
> The following notes are about what I have done until now for removing
> such tests. (More elaborated version than cover letter). You don't have
> to see them, but I hope it might clarify things...
>
> ## `ifdef ISP2401` added in the initial commit of atomisp
>
> The `ifdef ISP2401` was the result of merging two different version of
> driver, added on the initial commit of upstreamed atomisp. And for the
> `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> the initial commit of atomisp [2][3]
>
> [1] here are the three exceptions:
> ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
>
> [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f
Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
driver running with the firmware we're using, I'm all for it. Just send
the patches ;-)
>
> Here is the kernel tree if someone is interested:
>
> https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@a49d25364dfb_first
>
> Especially, here is one of the part where this patch is touching
> for example:
>
> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> @@ -416,26 +362,16 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info)
> [...]
> -#ifndef ISP2401
> port = (unsigned int) pipe->stream->config.source.port.port;
> assert(port < N_CSI_PORTS);
> if (port >= N_CSI_PORTS) {
> -#else
> - if (!ia_css_mipi_is_source_port_valid(pipe, &port)) {
> -#endif
> ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> pipe, port);
>
> By removing (almost) all of the `#ifdef ISP2401` things, (although we
> still can't remove like former USE_INPUT_SYSTEM_VERSION_2,
> USE_INPUT_SYSTEM_VERSION_2401) we can reduce the number of ifdefs.
Sounds good to me.
>
>
> ## `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */` in the recent
> atomisp
>
> That is for the initial commit of atomisp. For the recent version of
> atomisp, we can still remove `ifdef ISP2401` things (again, except things
> which were added inside it later upstream) as well as the former
> `ifdef ISP2401` things, i.e., `if (IS_ISP2401)` [1] and things commented
> with [2] `/* ISP2401 */`.
>
> [1] ("atomisp: pci: remove IS_ISP2401 test")
> https://github.com/kitakar5525/linux-kernel/commit/397e543e493dfd60d91e2b5cc164da342b26906c
> [2] ("atomisp: pci: remove `/* ISP2401 */` comments and its contents")
> https://github.com/kitakar5525/linux-kernel/commit/b3928e3c1a709853971715ce35459b9b79e708f2
> These commits were made against
> bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> where I randomely picked.
>
> Here is the kernel tree if someone is interested:
>
> https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@bd674b5a413c_before_get_rid_ver_globals
>
> I confirmed capture is still working here on surface3 (ISP2401). Compile
> tested for ISP2400. As you can see, there are some WIP and FIXME commits
> on top of removing such tests though. (The others are backports).
>
> Especially, here is one of the part where this patch is touching
> for example:
>
> --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> @@ -553,10 +548,7 @@ free_mipi_frames(struct ia_css_pipe *pipe) {
> return err;
> }
>
> - if (!IS_ISP2401)
> - port = (unsigned int)pipe->stream->config.source.port.port;
> - else
> - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> + port = (unsigned int)pipe->stream->config.source.port.port;
>
> assert(port < N_CSI_PORTS);
>
>
> So, we can also remove a lot of `if (IS_ISP2401)` and `/* ISP2401 */`
> things as well as the remaining `ifdef ISP2401`.
>
>
>
> ## WIP: removing `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */`
> against the latest atomisp
>
> And here is the branch where I'm working on removing such tests against
> the latest atomisp:
>
> https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp+remove_unneeded_tests
>
> It'd be the best if I can show you working one,
Well, send the ones that were already tested, and won't cause
regressions to v4l2grab and camorama (e. g. it shouldn't cause
generic V4L2 generic apps to break).
It would be nice to also not break nvt and other original apps for
this device, as it could be useful later, in order to be able to
test the other pipelines, as currently only the preview one seems
to be working properly, at least with generic apps.
> but it currently has
> seemingly include issues on ISP2400 vs ISP2401 (as well as many WIP
> commits I added):
>
> drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c: In function ‘ia_css_isys_init’:
> drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:29:9: error: unknown type name ‘backend_channel_cfg_t’
> 29 | backend_channel_cfg_t backend_ch0;
> | ^~~~~~~~~~~~~~~~~~~~~
> drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:30:9: error: unknown type name ‘backend_channel_cfg_t’
> 30 | backend_channel_cfg_t backend_ch1;
> | ^~~~~~~~~~~~~~~~~~~~~
> drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:31:9: error: unknown type name ‘target_cfg2400_t’
> 31 | target_cfg2400_t targetB;
> | ^~~~~~~~~~~~~~~~
> drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:32:9: error: unknown type name ‘target_cfg2400_t’
> 32 | target_cfg2400_t targetC;
> | ^~~~~~~~~~~~~~~~
> [...]
>
> The full output of the make error is here:
>
> ("NOTE: issue: some undeclared errors")
> https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
>
>
>
> Regards,
> Tsuchiya Yuto
> > > >
Em Thu, 11 Nov 2021 18:09:49 +0200
Andy Shevchenko <[email protected]> escreveu:
> On Thu, Nov 11, 2021 at 11:34:23PM +0900, Tsuchiya Yuto wrote:
> > On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > > Em Mon, 01 Nov 2021 22:38:55 +0900
> > > Tsuchiya Yuto <[email protected]> escreveu:
>
> ...
>
> > The full output of the make error is here:
> >
> > ("NOTE: issue: some undeclared errors")
> > https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
>
> I just realize that we may do at some point
>
> cflags-y += -Werror
>
> to avoid changes that breaks build (with warnings).
No need. Upstream already added Werror. It is just a matter of
adding:
CONFIG_WERROR=y
to .config.
> And also I would suggest
> to run build with `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`
Yeah, that's good. On media, we also require no (unjustified) sparse
and smatch warnings.
Regards,
Mauro
On Thu, Nov 11, 2021 at 9:41 PM Mauro Carvalho Chehab
<[email protected]> wrote:
> Em Thu, 11 Nov 2021 18:09:49 +0200
> Andy Shevchenko <[email protected]> escreveu:
> > On Thu, Nov 11, 2021 at 11:34:23PM +0900, Tsuchiya Yuto wrote:
...
> > I just realize that we may do at some point
> >
> > cflags-y += -Werror
> >
> > to avoid changes that breaks build (with warnings).
>
> No need. Upstream already added Werror. It is just a matter of
> adding:
>
> CONFIG_WERROR=y
>
> to .config.
Unfortunately this is not gonna work with `make W=1`.
https://lore.kernel.org/lkml/CAHp75Vef8QW3Y0yA702KUqPDHNRLN0kCv06=cgPpgPbUeAb-dw@mail.gmail.com/T/#u
> > And also I would suggest
> > to run build with `make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...`
>
> Yeah, that's good. On media, we also require no (unjustified) sparse
> and smatch warnings.
--
With Best Regards,
Andy Shevchenko
Hi
Em Thu, 11 Nov 2021 18:38:12 +0000
Mauro Carvalho Chehab <[email protected]> escreveu:
> > The `ifdef ISP2401` was the result of merging two different version of
> > driver, added on the initial commit of upstreamed atomisp. And for the
> > `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> > the initial commit of atomisp [2][3]
> >
> > [1] here are the three exceptions:
> > ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> > https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> >
> > [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> > https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> > [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> > https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f
>
> Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
> driver running with the firmware we're using, I'm all for it. Just send
> the patches ;-)
I went ahead and solved several INPUT_SYSTEM related ifdefs on a way
that it is compatible with Intel Aero firmware for the sh_css* files.
Except if I made any mistake, the ifdefs that are related to the
input system were already addressed.
I didn't notice any changes when running camorama on the PREVIEW
node.
Please test. Feel free to submit fixup patches if needed.
Regards,
Mauro
I'm really sorry about this delay recently. I can't spare much time now...
On Wed, 2021-11-17 at 22:24 +0000, Mauro Carvalho Chehab wrote:
> Hi
> Em Thu, 11 Nov 2021 18:38:12 +0000
> Mauro Carvalho Chehab <[email protected]> escreveu:
>
> > > The `ifdef ISP2401` was the result of merging two different version of
> > > driver, added on the initial commit of upstreamed atomisp. And for the
> > > `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> > > the initial commit of atomisp [2][3]
> > >
> > > [1] here are the three exceptions:
> > > ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> > > https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> > >
> > > [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> > > https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> > > [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> > > https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f
> >
> > Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
> > driver running with the firmware we're using, I'm all for it. Just send
> > the patches ;-)
>
> I went ahead and solved several INPUT_SYSTEM related ifdefs on a way
> that it is compatible with Intel Aero firmware for the sh_css* files.
> Except if I made any mistake, the ifdefs that are related to the
> input system were already addressed.
>
> I didn't notice any changes when running camorama on the PREVIEW
> node.
>
> Please test. Feel free to submit fixup patches if needed.
Thank you for your work. Just tried now and I also don't notice any
issues so far with the latest media_stage tree.
Regards,
Tsuchiya Yuto
On Thu, 2021-11-11 at 18:38 +0000, Mauro Carvalho Chehab wrote:
> Em Thu, 11 Nov 2021 23:34:23 +0900
> Tsuchiya Yuto <[email protected]> escreveu:
>
> > Sorry for a little late reply. This is hard to explain...
> >
> > On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > > Em Mon, 01 Nov 2021 22:38:55 +0900
> > > Tsuchiya Yuto <[email protected]> escreveu:
> >
> > [...]
> > >
> >
> > > > This is not directly related to this series, but how we should reduce
> > > > the ifdef usage in the future? Here are my two ideas:
> > > >
> > > > 1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> > > > part completely irci_stable_candrpv_0415_20150521_0458
> > > >
> > > > this way does not require (relatively) much human work I think.
> > > >
> > > > But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> > > > is basically an improved version.
> > >
> > > No. What I said is that the if (ISP2401) and the remaining ifdefs are because
> > > of BYT x CHT.
> >
> > I need to elaborate on this. Indeed some of them are really because of
> > BYT x CHT, but others are stuff from irci_ecr-master_20150911_0724.
> >
> > What I meant when I mentioned "remove `#ifdef ISP2401` part" is that,
> > removing things which was _initially_ inside the `#ifdef ISP2401` on the
> > initial commit of atomisp.
> >
> > Also I believe we can remove more `if (IS_ISP2401)` and `/* ISP2401 */`
> > things as well as the some remaining `#ifdef ISP2401` things.
> >
> > I added about this below and hope it clarifies things...
>
> It is clearer now. Yeah, we can touch on whatever is inside the
> ISP2401 ifs, as we can now test them. Touching things for ISP2400
> is harder, as we depend on a test platform.
>
> > > The worse part of them are related to those files
> > > (See Makefile):
> > >
> > > obj-byt = \
> > > pci/css_2400_system/hive/ia_css_isp_configs.o \
> > > pci/css_2400_system/hive/ia_css_isp_params.o \
> > > pci/css_2400_system/hive/ia_css_isp_states.o \
> > >
> > > obj-cht = \
> > > pci/css_2401_system/hive/ia_css_isp_configs.o \
> > > pci/css_2401_system/hive/ia_css_isp_params.o \
> > > pci/css_2401_system/hive/ia_css_isp_states.o \
> > > pci/css_2401_system/host/csi_rx.o \
> > > pci/css_2401_system/host/ibuf_ctrl.o \
> > > pci/css_2401_system/host/isys_dma.o \
> > > pci/css_2401_system/host/isys_irq.o \
> > > pci/css_2401_system/host/isys_stream2mmio.o
> > >
> > > Those define regmaps for 2400 and 2401. I was able to remove a lot
> > > of things from the old css_2400/css_2401 directories, but the ones
> > > there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
> > > require some mapping functions to allow the same driver to work with
> > > both BYT and CHT.
> > >
> > > The better would be to test the driver first at BYT, fix issues (if any) and
> > > then write the mapping code.
> > >
> > > > So, we may also:
> > > >
> > > > 2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> > > >
> > > > but this way needs more human work I think though. And if we go this
> > > > way, I also need to rewrite this patch as mentioned in the commit
> > > > message.
> >
> > What the idea #1 want to say is, let's remove things _initially_ within
> > `ifdef ISP2401` (so, except things which were added inside it later
> > upstream) including formerly within `ifdef ISP2401` things, i.e.,
> > `if (IS_ISP2401)` and things commented with `/* ISP2401 */`.
> >
> > However, I don't say we can remove all the ifdefs like things formerly
> > within USE_INPUT_SYSTEM_VERSION_2, USE_INPUT_SYSTEM_VERSION_2401, etc.,
> > which later removed/integrated into `ifdef ISP2401` on some commits [1].
> > We may temporarily revert those commits when we want to distinguish
> > between what were formerly within there and what were not.
> >
> > Such ifdefs were added by them as a real hardware difference. Thus, I
> > agree that we still need the CONFIG_VIDEO_ATOMISP_ISP2401 stuff to support
> > both ISP2400/ISP2401 at the same time.
> >
> > This is what I meant "reduce the ifdef usage" in a previous mail. So,
> > I'm not talking about if dropping CONFIG_VIDEO_ATOMISP_ISP2401 is doable,
> > but talking about just how to reduce the code.
> >
> > [1] 641c2292bf19 ("media: atomisp: get rid of version-dependent globals")
> > bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> >
> > Anyway, if you agree or not on what I'm saying, can I send this patch
> > without code changes in v2, i.e., looks OK for you regarding the code?
> > I'll remove the commit message about
> > irci_stable_candrpv_0415_20150521_0458 vs irci_ecr-master_20150911_0724
> > in v2 anyway, which needs to be discussed further later.
>
> No need for a v2. The /17 patch series was merged already, plus some
> patches from the /5 that made sense to apply.
>
> Ok, there are some followup patches that could be added, but please
> send those in separate.
OK, thanks. I'll prepare the followup patches as soon as I can.
> > The following notes are about what I have done until now for removing
> > such tests. (More elaborated version than cover letter). You don't have
> > to see them, but I hope it might clarify things...
> >
> > ## `ifdef ISP2401` added in the initial commit of atomisp
> >
> > The `ifdef ISP2401` was the result of merging two different version of
> > driver, added on the initial commit of upstreamed atomisp. And for the
> > `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> > the initial commit of atomisp [2][3]
> >
> > [1] here are the three exceptions:
> > ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> > https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> >
> > [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> > https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> > [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> > https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f
>
> Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
> driver running with the firmware we're using, I'm all for it. Just send
> the patches ;-)
>
> >
> > Here is the kernel tree if someone is interested:
> >
> > https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@a49d25364dfb_first
> >
> > Especially, here is one of the part where this patch is touching
> > for example:
> >
> > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> > @@ -416,26 +362,16 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info)
> > [...]
> > -#ifndef ISP2401
> > port = (unsigned int) pipe->stream->config.source.port.port;
> > assert(port < N_CSI_PORTS);
> > if (port >= N_CSI_PORTS) {
> > -#else
> > - if (!ia_css_mipi_is_source_port_valid(pipe, &port)) {
> > -#endif
> > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> > "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> > pipe, port);
> >
> > By removing (almost) all of the `#ifdef ISP2401` things, (although we
> > still can't remove like former USE_INPUT_SYSTEM_VERSION_2,
> > USE_INPUT_SYSTEM_VERSION_2401) we can reduce the number of ifdefs.
>
> Sounds good to me.
>
> >
> >
> > ## `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */` in the recent
> > atomisp
> >
> > That is for the initial commit of atomisp. For the recent version of
> > atomisp, we can still remove `ifdef ISP2401` things (again, except things
> > which were added inside it later upstream) as well as the former
> > `ifdef ISP2401` things, i.e., `if (IS_ISP2401)` [1] and things commented
> > with [2] `/* ISP2401 */`.
> >
> > [1] ("atomisp: pci: remove IS_ISP2401 test")
> > https://github.com/kitakar5525/linux-kernel/commit/397e543e493dfd60d91e2b5cc164da342b26906c
> > [2] ("atomisp: pci: remove `/* ISP2401 */` comments and its contents")
> > https://github.com/kitakar5525/linux-kernel/commit/b3928e3c1a709853971715ce35459b9b79e708f2
> > These commits were made against
> > bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> > where I randomely picked.
> >
> > Here is the kernel tree if someone is interested:
> >
> > https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@bd674b5a413c_before_get_rid_ver_globals
> >
> > I confirmed capture is still working here on surface3 (ISP2401). Compile
> > tested for ISP2400. As you can see, there are some WIP and FIXME commits
> > on top of removing such tests though. (The others are backports).
> >
> > Especially, here is one of the part where this patch is touching
> > for example:
> >
> > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > @@ -553,10 +548,7 @@ free_mipi_frames(struct ia_css_pipe *pipe) {
> > return err;
> > }
> >
> > - if (!IS_ISP2401)
> > - port = (unsigned int)pipe->stream->config.source.port.port;
> > - else
> > - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > + port = (unsigned int)pipe->stream->config.source.port.port;
> >
> > assert(port < N_CSI_PORTS);
> >
> >
> > So, we can also remove a lot of `if (IS_ISP2401)` and `/* ISP2401 */`
> > things as well as the remaining `ifdef ISP2401`.
> >
> >
> >
> > ## WIP: removing `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */`
> > against the latest atomisp
> >
> > And here is the branch where I'm working on removing such tests against
> > the latest atomisp:
> >
> > https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp+remove_unneeded_tests
> >
> > It'd be the best if I can show you working one,
>
> Well, send the ones that were already tested, and won't cause
> regressions to v4l2grab and camorama (e. g. it shouldn't cause
> generic V4L2 generic apps to break).
>
> It would be nice to also not break nvt and other original apps for
> this device, as it could be useful later, in order to be able to
> test the other pipelines, as currently only the preview one seems
> to be working properly, at least with generic apps.
Got it :-)
> > but it currently has
> > seemingly include issues on ISP2400 vs ISP2401 (as well as many WIP
> > commits I added):
> >
> > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c: In function ‘ia_css_isys_init’:
> > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:29:9: error: unknown type name ‘backend_channel_cfg_t’
> > 29 | backend_channel_cfg_t backend_ch0;
> > | ^~~~~~~~~~~~~~~~~~~~~
> > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:30:9: error: unknown type name ‘backend_channel_cfg_t’
> > 30 | backend_channel_cfg_t backend_ch1;
> > | ^~~~~~~~~~~~~~~~~~~~~
> > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:31:9: error: unknown type name ‘target_cfg2400_t’
> > 31 | target_cfg2400_t targetB;
> > | ^~~~~~~~~~~~~~~~
> > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:32:9: error: unknown type name ‘target_cfg2400_t’
> > 32 | target_cfg2400_t targetC;
> > | ^~~~~~~~~~~~~~~~
> > [...]
> >
> > The full output of the make error is here:
> >
> > ("NOTE: issue: some undeclared errors")
> > https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
> >
> >
> >
> > Regards,
> > Tsuchiya Yuto
> > > > >
On Wed, 2021-12-01 at 20:35 +0900, Tsuchiya Yuto wrote:
> On Thu, 2021-11-11 at 18:38 +0000, Mauro Carvalho Chehab wrote:
> > Em Thu, 11 Nov 2021 23:34:23 +0900
> > Tsuchiya Yuto <[email protected]> escreveu:
> >
> > > Sorry for a little late reply. This is hard to explain...
> > >
> > > On Mon, 2021-11-01 at 14:10 +0000, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 01 Nov 2021 22:38:55 +0900
> > > > Tsuchiya Yuto <[email protected]> escreveu:
> > >
> > > [...]
> > > >
> > >
> > > > > This is not directly related to this series, but how we should reduce
> > > > > the ifdef usage in the future? Here are my two ideas:
> > > > >
> > > > > 1. (my initial idea) remove `#ifdef ISP2401` part and make ISP2401
> > > > > part completely irci_stable_candrpv_0415_20150521_0458
> > > > >
> > > > > this way does not require (relatively) much human work I think.
> > > > >
> > > > > But as Mauro says, the `#ifdef ISP2401` part (irci_ecr-master_20150911_0724)
> > > > > is basically an improved version.
> > > >
> > > > No. What I said is that the if (ISP2401) and the remaining ifdefs are because
> > > > of BYT x CHT.
> > >
> > > I need to elaborate on this. Indeed some of them are really because of
> > > BYT x CHT, but others are stuff from irci_ecr-master_20150911_0724.
> > >
> > > What I meant when I mentioned "remove `#ifdef ISP2401` part" is that,
> > > removing things which was _initially_ inside the `#ifdef ISP2401` on the
> > > initial commit of atomisp.
> > >
> > > Also I believe we can remove more `if (IS_ISP2401)` and `/* ISP2401 */`
> > > things as well as the some remaining `#ifdef ISP2401` things.
> > >
> > > I added about this below and hope it clarifies things...
> >
> > It is clearer now. Yeah, we can touch on whatever is inside the
> > ISP2401 ifs, as we can now test them. Touching things for ISP2400
> > is harder, as we depend on a test platform.
> >
> > > > The worse part of them are related to those files
> > > > (See Makefile):
> > > >
> > > > obj-byt = \
> > > > pci/css_2400_system/hive/ia_css_isp_configs.o \
> > > > pci/css_2400_system/hive/ia_css_isp_params.o \
> > > > pci/css_2400_system/hive/ia_css_isp_states.o \
> > > >
> > > > obj-cht = \
> > > > pci/css_2401_system/hive/ia_css_isp_configs.o \
> > > > pci/css_2401_system/hive/ia_css_isp_params.o \
> > > > pci/css_2401_system/hive/ia_css_isp_states.o \
> > > > pci/css_2401_system/host/csi_rx.o \
> > > > pci/css_2401_system/host/ibuf_ctrl.o \
> > > > pci/css_2401_system/host/isys_dma.o \
> > > > pci/css_2401_system/host/isys_irq.o \
> > > > pci/css_2401_system/host/isys_stream2mmio.o
> > > >
> > > > Those define regmaps for 2400 and 2401. I was able to remove a lot
> > > > of things from the old css_2400/css_2401 directories, but the ones
> > > > there at pci/*/css*/ia_css_isp_*.c are a lot more complex, and would
> > > > require some mapping functions to allow the same driver to work with
> > > > both BYT and CHT.
> > > >
> > > > The better would be to test the driver first at BYT, fix issues (if any) and
> > > > then write the mapping code.
> > > >
> > > > > So, we may also:
> > > > >
> > > > > 2. continue unifying `#ifdef ISP2401` and `#ifndef ISP2401` parts
> > > > >
> > > > > but this way needs more human work I think though. And if we go this
> > > > > way, I also need to rewrite this patch as mentioned in the commit
> > > > > message.
> > >
> > > What the idea #1 want to say is, let's remove things _initially_ within
> > > `ifdef ISP2401` (so, except things which were added inside it later
> > > upstream) including formerly within `ifdef ISP2401` things, i.e.,
> > > `if (IS_ISP2401)` and things commented with `/* ISP2401 */`.
> > >
> > > However, I don't say we can remove all the ifdefs like things formerly
> > > within USE_INPUT_SYSTEM_VERSION_2, USE_INPUT_SYSTEM_VERSION_2401, etc.,
> > > which later removed/integrated into `ifdef ISP2401` on some commits [1].
> > > We may temporarily revert those commits when we want to distinguish
> > > between what were formerly within there and what were not.
> > >
> > > Such ifdefs were added by them as a real hardware difference. Thus, I
> > > agree that we still need the CONFIG_VIDEO_ATOMISP_ISP2401 stuff to support
> > > both ISP2400/ISP2401 at the same time.
> > >
> > > This is what I meant "reduce the ifdef usage" in a previous mail. So,
> > > I'm not talking about if dropping CONFIG_VIDEO_ATOMISP_ISP2401 is doable,
> > > but talking about just how to reduce the code.
> > >
> > > [1] 641c2292bf19 ("media: atomisp: get rid of version-dependent globals")
> > > bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> > >
> > > Anyway, if you agree or not on what I'm saying, can I send this patch
> > > without code changes in v2, i.e., looks OK for you regarding the code?
> > > I'll remove the commit message about
> > > irci_stable_candrpv_0415_20150521_0458 vs irci_ecr-master_20150911_0724
> > > in v2 anyway, which needs to be discussed further later.
> >
> > No need for a v2. The /17 patch series was merged already, plus some
> > patches from the /5 that made sense to apply.
> >
> > Ok, there are some followup patches that could be added, but please
> > send those in separate.
>
> OK, thanks. I'll prepare the followup patches as soon as I can.
Oh, I spoke too soon. The issues pointed out by code review are already
fixed/gone by rebase and later patches. Thanks!
> > > The following notes are about what I have done until now for removing
> > > such tests. (More elaborated version than cover letter). You don't have
> > > to see them, but I hope it might clarify things...
> > >
> > > ## `ifdef ISP2401` added in the initial commit of atomisp
> > >
> > > The `ifdef ISP2401` was the result of merging two different version of
> > > driver, added on the initial commit of upstreamed atomisp. And for the
> > > `ifdef ISP2401`, I confirmed I can remove (almost [1]) all of them against
> > > the initial commit of atomisp [2][3]
> > >
> > > [1] here are the three exceptions:
> > > ("NOTE: ifdef ISP2400/ISP2401 usage in aero-atomisp")
> > > https://github.com/kitakar5525/linux-kernel/commit/1a8488cdd31ad38a3805824700b29d1e5213d3f2
> > >
> > > [2] ("atomisp: pci: css2400: remove ISP2401 ifdefs")
> > > https://github.com/kitakar5525/linux-kernel/commit/dd6723fc5b9fe040e33b227b509a7e004243edce
> > > [3] ("atomisp: pci: remove ISP2401 ifdefs for main pci driver")
> > > https://github.com/kitakar5525/linux-kernel/commit/1734341f84a96945af7635f6fff061db910f746f
> >
> > Ok, if there are more if/ifdef ISP2401 that, if reverted will keep the
> > driver running with the firmware we're using, I'm all for it. Just send
> > the patches ;-)
> >
> > >
> > > Here is the kernel tree if someone is interested:
> > >
> > > https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@a49d25364dfb_first
> > >
> > > Especially, here is one of the part where this patch is touching
> > > for example:
> > >
> > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c
> > > @@ -416,26 +362,16 @@ allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info)
> > > [...]
> > > -#ifndef ISP2401
> > > port = (unsigned int) pipe->stream->config.source.port.port;
> > > assert(port < N_CSI_PORTS);
> > > if (port >= N_CSI_PORTS) {
> > > -#else
> > > - if (!ia_css_mipi_is_source_port_valid(pipe, &port)) {
> > > -#endif
> > > ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> > > "allocate_mipi_frames(%p) exit: error: port is not correct (port=%d).\n",
> > > pipe, port);
> > >
> > > By removing (almost) all of the `#ifdef ISP2401` things, (although we
> > > still can't remove like former USE_INPUT_SYSTEM_VERSION_2,
> > > USE_INPUT_SYSTEM_VERSION_2401) we can reduce the number of ifdefs.
> >
> > Sounds good to me.
> >
> > >
> > >
> > > ## `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */` in the recent
> > > atomisp
> > >
> > > That is for the initial commit of atomisp. For the recent version of
> > > atomisp, we can still remove `ifdef ISP2401` things (again, except things
> > > which were added inside it later upstream) as well as the former
> > > `ifdef ISP2401` things, i.e., `if (IS_ISP2401)` [1] and things commented
> > > with [2] `/* ISP2401 */`.
> > >
> > > [1] ("atomisp: pci: remove IS_ISP2401 test")
> > > https://github.com/kitakar5525/linux-kernel/commit/397e543e493dfd60d91e2b5cc164da342b26906c
> > > [2] ("atomisp: pci: remove `/* ISP2401 */` comments and its contents")
> > > https://github.com/kitakar5525/linux-kernel/commit/b3928e3c1a709853971715ce35459b9b79e708f2
> > > These commits were made against
> > > bd674b5a413c ("media: atomisp: cleanup ifdefs from ia_css_debug.c")
> > > where I randomely picked.
> > >
> > > Here is the kernel tree if someone is interested:
> > >
> > > https://github.com/kitakar5525/linux-kernel/tree/mainline+upst_atomisp@bd674b5a413c_before_get_rid_ver_globals
> > >
> > > I confirmed capture is still working here on surface3 (ISP2401). Compile
> > > tested for ISP2400. As you can see, there are some WIP and FIXME commits
> > > on top of removing such tests though. (The others are backports).
> > >
> > > Especially, here is one of the part where this patch is touching
> > > for example:
> > >
> > > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > > @@ -553,10 +548,7 @@ free_mipi_frames(struct ia_css_pipe *pipe) {
> > > return err;
> > > }
> > >
> > > - if (!IS_ISP2401)
> > > - port = (unsigned int)pipe->stream->config.source.port.port;
> > > - else
> > > - err = ia_css_mipi_is_source_port_valid(pipe, &port);
> > > + port = (unsigned int)pipe->stream->config.source.port.port;
> > >
> > > assert(port < N_CSI_PORTS);
> > >
> > >
> > > So, we can also remove a lot of `if (IS_ISP2401)` and `/* ISP2401 */`
> > > things as well as the remaining `ifdef ISP2401`.
> > >
> > >
> > >
> > > ## WIP: removing `ifdef ISP2401`, `if (IS_ISP2401)` and `/* ISP2401 */`
> > > against the latest atomisp
> > >
> > > And here is the branch where I'm working on removing such tests against
> > > the latest atomisp:
> > >
> > > https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp+remove_unneeded_tests
> > >
> > > It'd be the best if I can show you working one,
> >
> > Well, send the ones that were already tested, and won't cause
> > regressions to v4l2grab and camorama (e. g. it shouldn't cause
> > generic V4L2 generic apps to break).
> >
> > It would be nice to also not break nvt and other original apps for
> > this device, as it could be useful later, in order to be able to
> > test the other pipelines, as currently only the preview one seems
> > to be working properly, at least with generic apps.
>
> Got it :-)
>
> > > but it currently has
> > > seemingly include issues on ISP2400 vs ISP2401 (as well as many WIP
> > > commits I added):
> > >
> > > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c: In function ‘ia_css_isys_init’:
> > > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:29:9: error: unknown type name ‘backend_channel_cfg_t’
> > > 29 | backend_channel_cfg_t backend_ch0;
> > > | ^~~~~~~~~~~~~~~~~~~~~
> > > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:30:9: error: unknown type name ‘backend_channel_cfg_t’
> > > 30 | backend_channel_cfg_t backend_ch1;
> > > | ^~~~~~~~~~~~~~~~~~~~~
> > > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:31:9: error: unknown type name ‘target_cfg2400_t’
> > > 31 | target_cfg2400_t targetB;
> > > | ^~~~~~~~~~~~~~~~
> > > drivers/staging/media/atomisp/pci/runtime/isys/src/isys_init.c:32:9: error: unknown type name ‘target_cfg2400_t’
> > > 32 | target_cfg2400_t targetC;
> > > | ^~~~~~~~~~~~~~~~
> > > [...]
> > >
> > > The full output of the make error is here:
> > >
> > > ("NOTE: issue: some undeclared errors")
> > > https://github.com/kitakar5525/linux-kernel/commit/a932d16681f941161385659b9d0316a3a4975e86
> > >
> > >
> > >
> > > Regards,
> > > Tsuchiya Yuto
> > > > > >
>