Re: atomisp kernel driver(s)

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

Em Sat, 2 May 2020 16:29:51 +0200
Patrik Gfeller <patrik.gfeller@gmail.com> escreveu:

> On Sat, 2 May 2020 11:34:14 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Em Sat, 2 May 2020 10:15:42 +0200
> > Patrik Gfeller <patrik.gfeller@gmail.com> escreveu:
> > 
> >  [...]  
> >  [...]  
> >   
> > > > Ok. Btw, there is a driver for Atomisp on an yocto tree:
> > > > 
> > > > 	https://github.com/intel-aero/meta-intel-aero.git
> > > > 
> > > > It got removed back in 2018, but if you checkout this changeset:
> > > > 
> > > > 	Merge: db1df368eb58 08f476112708
> > > > 	Author: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > 	Date:   Tue Apr 4 11:51:42 2017 -0700
> > > > 
> > > > 	    Merge pull request #70 from zehortigoza/jam
> > > >           
> > > 
> > > Cool, the code might give additional information.    
> > 
> > Yes. And, as it was released from Intel for a specific device,
> > it should very likely work for the model supported there (with
> > the Yocto 4.4 Kernel). So, it could be valuable to help identifying
> > if some of the cleanup patches would have broken something.
> > 
> > ON a quick look, it sounds that such build was is targeted for
> > ISP2401:
> > 
> > 	+++ b/drivers/media/pci/atomisp/Makefile
> > 	@@ -0,0 +1 @@
> > 	+obj-$(CONFIG_VIDEO_ATOMISP) += css2401a0_v21_build/
> >   
> > > I've also send a
> > > request regarding the firmware and HW documentation to the author and
> > > the engineers that signed the patch. The firmware in the patch has a
> > > different version string and the size is surprisingly a few MB bigger
> > > than the one I used for the first experiment.     
> > 
> > There are some vendors that generate a firmware together with a source
> > code. This could be the case here. That's my feeling when looking into
> > a file like:
> > 
> > 	drivers/staging/media/atomisp/pci/css_2401_system/spmem_dump.c
> > 
> > Which contains lots of addresses that it is different betwen 2401 and
> > 2400.
> > 
> > If so, using a different firmware version will likely cause at least
> > some parts of the driver to fail.
> > 
> > 
> >  [...]  
> >  [...]    
> > > 
> > > It looks a this firmware is for the 2400 variant; I could also not
> > > extract the irci version string. Thus I'll not try them for the moment
> > > to perform experiments.    
> > 
> > Ok.
> > 
> >  [...]    
> > > 
> > > It identifies itself as irci_stable_bw10p_0518_20150801_0537;     
> > 
> > Same year as what we had. Just a little older. Yeah, some things there
> > could work.  
> 
> Below the result from the test with
> irci_stable_bw10p_0518_20150801_0537 - looks like it loaded the
> firmare; I got an a message when the file was not present, or the
> version did not add up. I tried to cleanup the dmesg output a little
> (removed what was unrelated to atom-isp):

> [   10.089196] ------------[ cut here ]------------
> [   10.093225] WARNING: CPU: 1 PID: 503 at drivers/media/v4l2-core/v4l2-dev.c:885 __video_register_device+0x93e/0x1120 [videodev]

That's due to a change at the media core that added this test:

	/* the device_caps field MUST be set for all but subdevs */
	if (WARN_ON(type != VFL_TYPE_SUBDEV && !vdev->device_caps))
		return -EINVAL;

Added on this patch:

	commit 3c1350501c21db8e3b1a38d9e97db29694305c3b
	Author: Hans Verkuil <hverkuil-cisco@xs4all.nl>
	Date:   Tue Jul 23 04:21:25 2019 -0400

	    media: v4l2-dev/ioctl: require non-zero device_caps, verify sane querycap results
    
	    Now that all V4L2 drivers set device_caps in struct video_device, we can add
	    a check for this to ensure all future drivers fill this in.

Fixing it is simple. Just sent a patch.

> > I'm suspecting that, before being able of calling regulator_get(),
> > the code should use some match logic to get the regulators on this
> > board and call regulator_register().
> > 
> > Please run this command on your tablet:
> > 
> > 	$ sudo cat /sys/kernel/debug/regulator/supply_map
> > 
> > If this returns nothing - as I suspect - then calling regulator_get()
> > won't be doing anything.   
> 
> The statement to read the supply_map did return nothing, as you'd
> expected.

Ok. That explains why register_get() failed ;-)

If this time the probing part works, I guess the next step would
be to use some tools from https://git.linuxtv.org/v4l-utils.git/,
in order to test the stuff that doesn't depend on the sensors,
as, without the regulator settings, it won't be turned on.

The simplest test would be to run:

	$ v4l2-ctl --all -d /dev/video0

(and the same for the other /dev/video? nodes created by the driver)

-

A more complete test would be to run v4l2-compliance (without
enabling streaming), but let's first check if v4l2-ctl won't
hit any Kernel bugs.

Thanks,
Mauro