linux-kernel - Re: [PATCH] Add VIDEO IN driver for OKI SEMICONDUCTOR ML7213/ML7223 IOHs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <201105122223.01369.hverkuil@xs4all.nl>
Date:	Thu, 12 May 2011 22:23:01 +0200
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Tomoya MORINAGA <tomoya-linux@....okisemi.com>
Cc:	Mauro Carvalho Chehab <mchehab@...radead.org>,
	Guennadi Liakhovetski <g.liakhovetski@....de>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	qi.wang@...el.com, yong.y.wang@...el.com, joel.clark@...el.com,
	kok.howg.ewe@...el.com, toshiharu-linux@....okisemi.com
Subject: Re: [PATCH] Add VIDEO IN driver for OKI SEMICONDUCTOR ML7213/ML7223 IOHs

Hi Tomoya,

This is a pretty big patch and it will take some time to review properly.
However, I can give some high-level comments already based on an initial
scan of the code.

1) New drivers that use controls (G/S_CTRL et al) must use the new control
framework. See Documentation/video4linux/v4l2-controls.txt. We are slowly
converting existing drivers to this framework, and I don't want to add to that
workload :-)

2) The sensor drivers should be named 'ov7620', 'ncm13j', etc. and be
independent of (and not refer to) the IOH driver. After all, these sensor
drivers can be used in other devices as well.

3) ioh_video_in.c is definitely too large with 4704 lines. I would strongly
suggest splitting it in multiple files.

4) There are a bunch of IOH_VIDEO_ private ioctls. I haven't had time to analyse
them, but it would help a lot if you could write some sort of overview of what
they do and why you need them. I suspect that some can be replaced by private
controls, some might already be available in V4L2 and some might need extensions
to V4L2 (I hope not, though).

5) The OH_VIDEO_IN_DMA_CONTIG define makes a mess of the code. Why do you need it?
I have never seen a driver that can support either DMA_CONTIG or VMALLOC and I would
like to know some background information here. BTW, 2.6.39 merges the new videobuf2
framework (the successor to videobuf), which I would recommend that you use. It is
much easier to work with and understand than videobuf.

Regards,

	Hans

On Thursday, May 12, 2011 10:16:04 Tomoya MORINAGA wrote:
> This patch is for Video IN driver of OKI SEMICONDUCTOR ML7213/ML7223 IOHs
> (Input/Output Hub).
> These ML7213/ML7223 IOHs are companion chip for Intel Atom E6xx series.
> ML7213 IOH is for IVI(In-Vehicle Infotainment) use and ML7223 IOH is for
> MP(Media Phone) use.
> 
> Signed-off-by: Tomoya MORINAGA <tomoya-linux@....okisemi.com>
> ---
>  drivers/media/video/Kconfig                   |   79 +
>  drivers/media/video/Makefile                  |   15 +
>  drivers/media/video/ioh_video_in.c            | 4704 +++++++++++++++++++++++++
>  drivers/media/video/ioh_video_in_main.h       | 1058 ++++++
>  drivers/media/video/ioh_video_in_ml86v76651.c |  620 ++++
>  drivers/media/video/ioh_video_in_ncm13j.c     |  584 +++
>  drivers/media/video/ioh_video_in_ov7620.c     |  637 ++++
>  drivers/media/video/ioh_video_in_ov9653.c     |  818 +++++
>  8 files changed, 8515 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/ioh_video_in.c
>  create mode 100644 drivers/media/video/ioh_video_in_main.h
>  create mode 100644 drivers/media/video/ioh_video_in_ml86v76651.c
>  create mode 100644 drivers/media/video/ioh_video_in_ncm13j.c
>  create mode 100644 drivers/media/video/ioh_video_in_ov7620.c
>  create mode 100644 drivers/media/video/ioh_video_in_ov9653.c
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 00f51dd..11a96a8 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -928,6 +928,85 @@ config VIDEO_MX2
>  	  Interface
>  
>  
> +config IOH_VIDEOIN
> +        tristate "OKI SEMICONDUCTOR ML7213/ML7223 IOH VIDEO IN"
> +        depends on PCI && DMADEVICES
> +	select PCH_DMA
> +        help
> +	  This driver is for Video IN of OKI SEMICONDUCTOR ML7213/ML7223 IOHs
> +	  (Input/Output Hub).
> +	  These ML7213/ML7223 IOHs are companion chip for Intel Atom E6xx
> +	  series.
> +	  ML7213 IOH is for IVI(In-Vehicle Infotainment) use and ML7223 IOH is
> +	  for MP(Media Phone) use.
> +
> +config  IOH_VIDEO_DEVICE_SELECT
> +        boolean
> +
> +choice
> +        prompt "Select IOH VIDEO IN Device"
> +        depends on IOH_VIDEOIN
> +        help
> +           This is a selection of used device of the IOH VIDEO.
> +
> +config IOH_ML86V76651
> +        boolean "IOH VIDEO IN(ML86V76651)"
> +        depends on PCI && IOH_VIDEOIN && I2C_EG20T
> +        help
> +          If you say yes to this option, support will be included for the
> +          IOH VIDEO ON Driver(ML86V76651).
> +
> +config IOH_ML86V76653
> +        boolean "IOH VIDEO IN(ML86V76653)"
> +        depends on PCI && IOH_VIDEOIN && I2C_EG20T
> +        help
> +          If you say yes to this option, support will be included for the
> +          IOH VIDEO ON Driver(ML86V76653).
> +
> +config IOH_OV7620
> +        boolean "IOH VIDEO IN(OV7620)"
> +        depends on PCI && IOH_VIDEOIN && I2C_EG20T
> +        help
> +          If you say yes to this option, support will be included for the
> +          IOH VIDEO ON Driver(OV7620).
> +
> +config IOH_OV9653
> +        boolean "IOH VIDEO IN(OV9653)"
> +        depends on PCI && IOH_VIDEOIN && I2C_EG20T
> +        help
> +          If you say yes to this option, support will be included for the
> +          IOH VIDEO ON Driver(OV9653).
> +
> +config IOH_NCM13J
> +        boolean "IOH VIDEO IN(NCM13-J)"
> +        depends on PCI && IOH_VIDEOIN && I2C_EG20T
> +        help
> +          If you say yes to this option, support will be included for the
> +          IOH VIDEO ON Driver(NCM13-J).
> +endchoice
> +
> +config  IOH_VIDEO_FRAMEWORK_SELECT
> +        boolean
> +
> +choice
> +        prompt "Select IOH VIDEO IN Videobuf Freamework"
> +        depends on IOH_VIDEOIN
> +        help
> +           This is a selection of used the method of video buffer framework.
> +
> +config IOH_VIDEO_IN_VMALLOC
> +        boolean "VMALLOC Framework"
> +	select VIDEOBUF_VMALLOC
> +        help
> +          If you say yes to this option, VMALLOC framework is used.
> +
> +config IOH_VIDEO_IN_DMA_CONTIG
> +        boolean "DMA-CONTIG Framework"
> +	select VIDEOBUF_DMA_CONTIG
> +        help
> +          If you say yes to this option, DMA-CONTIG framework is used.
> +endchoice
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/