[Letux-kernel] omap3 isp -- media-ctl

Javier Martinez Canillas javier at osg.samsung.com
Thu Sep 8 13:53:22 CEST 2016


Hello Nikolaus,

On 09/08/2016 05:58 AM, H. Nikolaus Schaller wrote:
> Hi Javier,
> 
>> Am 08.09.2016 um 00:20 schrieb Javier Martinez Canillas <javier at osg.samsung.com>:
>>
>> On 09/07/2016 06:04 PM, Javier Martinez Canillas wrote:
>>
>> [snip]
>>
>>>>
>>>>> Does the omap3isp driver
>>>>> probes? and does probe the driver for your attached camera sensor?
>>>>
>>>> Yes, both. Here an excerpt of the boot log and lsmod
>>>>
>>>
>>> Yes, both are probed but your camera driver (drivers/media/i2c/ov9650.c) is
>>> missing a call to v4l2_async_register_subdev() in its probe function to be
>>> registered async with the bridge driver (omap3isp). You need something like
>>>
>>> commit c7d97499cc8a ("[media] tvp5150: add support for asynchronous probing")
>>>
>>
>> In fact, I went and wrote the patch since is pretty trivial. Could you please
>> test it and if that's the fixes your problem then I can post it to the list?
> 
> Yes!!! It makes the significant difference (I would have searched some more weeks
> for it without your help :):
>

Great, I'm glad that it helped.
 
> root at letux:~# ls -l /dev/media*
> crw------- 1 root root 245, 0 Jan  1  2000 /dev/media0
> root at letux:~# ls -l /dev/video*
> crw-rw---- 1 root video 81, 0 Jan  1  2000 /dev/video0
> crw-rw---- 1 root video 81, 1 Jan  1  2000 /dev/video1
> crw-rw---- 1 root video 81, 2 Jan  1  2000 /dev/video2
> crw-rw---- 1 root video 81, 3 Jan  1  2000 /dev/video3
> crw-rw---- 1 root video 81, 4 Jan  1  2000 /dev/video4
> crw-rw---- 1 root video 81, 5 Jan  1  2000 /dev/video5
> crw-rw---- 1 root video 81, 6 Jan  1  2000 /dev/video6
> root at letux:~# 
> 
> I still get a "green" image in mplayer but that could be bugs or missing pieces
> in our camera module setup, which I wasn't able to debug so far.
>

That's an unrelated issue indeed.
 
> We use an ov9655 chip and the original platform data based driver is for ov9650
> and ov9652 only. And these sensors have a slightly different register set which
> we have to account for.
>

Yes, I noticed that you had a define for OV9655 that wasn't present in the upstream
driver.
 
> So we currently disable I2C writing to the camera so that the module comes up
> in default settings.
> 
> 
> Regarding your patch I could also submit it together with the full set of driver
> patches (we have added DT support) if you agree. But I don't see any issues if you
> submit it separately.
>

Since adding async probe is orthogonal to the ov9655 support, I think that's
better to post it separately. And in any case mention in your patch series
that you need a previous posted patch to make things to work (but I expect
this patch to be merged very quick since is trivial and not controversial).

> The latest patch set (including it) is here:
> 
> http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hns/video/ov9655
>

Great, I can't comment on the contents since I'm not familiar with the ov965x
family. But I noticed that you are missing the [media] prefix in the subjects.

> This also includes one patch for the isp driver which silences a bad of_node_put() warning.
> This could also be submitted separately. Should I?
>

I think so, this is a bug fix for omap3isp so I think should be post it ASAP.

> 
> For the DT style topic we currently have this style to have an endpoint
> reference at some separate location in the DT source file.
> 
> The reason is that we merge several patch sets into a single one and multiple of
> them touch the DTS. This construction reduces the risk of difficult to solve
> git merge conflicts and patch set dependencies because changes to &parallel_ep
> are not within the code line range of i2c devices. So git merge can more easily
> identify hunks with inexact line number references.
>

Got it. Makes sense.

> So indeed it is more difficult to read, but easier to maintain and debug
> (during development). But if it becomes stable I think it can be inlined
> as you suggest.
>

Ok, I don't even know if it would be an issue for DT reviewers so maybe
it can be left at it is.

> 
> BR and many thanks for the help,
> Nikolaus
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America


More information about the Letux-kernel mailing list