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

H. Nikolaus Schaller hns at goldelico.com
Thu Sep 8 17:34:46 CEST 2016


Hi,

> Am 08.09.2016 um 13:53 schrieb Javier Martinez Canillas <javier at osg.samsung.com>:
> 
> 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.

Ok, then we keep it in our patch set (for testing) until it arrives in linus/master
but mark it in a way that we don't post it.

And you can add a tested-by: <me>

> 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).

Yes, indeed.

> 
>> 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.

Ok, thanks for this hint. Every subsystem has its own conventions and this is the
first media driver...

> 
>> 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.

Ok, I post it separately.

> 
>> 
>> 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

BR and thanks again,
Nikolaus



More information about the Letux-kernel mailing list