linux-kernel - Re: [PATCH v6] media: i2c: add support for omnivision's ov2659 sensor

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5506A13F.9060608@xs4all.nl>
Date:	Mon, 16 Mar 2015 10:24:15 +0100
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Lad Prabhakar <prabhakar.csengg@...il.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Hans Verkuil <hans.verkuil@...co.com>
CC:	LMML <linux-media@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH v6] media: i2c: add support for omnivision's ov2659 sensor

Hi Prabhakar,

On 03/15/2015 11:34 AM, Lad Prabhakar wrote:
> From: Benoit Parrot <bparrot@...com>
> 
> this patch adds support for omnivision's ov2659
> sensor, the driver supports following features:
> 1: Asynchronous probing
> 2: DT support
> 3: Media controller support
> 
> Signed-off-by: Benoit Parrot <bparrot@...com>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@...il.com>
> Acked-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
> ---
>  Changes for v6:
>  a: fixed V4L2_CID_PIXEL_RATE control to use link_frequency
>     instead of xvclk_frequency.
>  b: Included Ack from Sakari
>  
>  v5: https://patchwork.kernel.org/patch/6000161/
>  v4: https://patchwork.kernel.org/patch/5961661/
>  v3: https://patchwork.kernel.org/patch/5959401/
>  v2: https://patchwork.kernel.org/patch/5859801/
>  v1: https://patchwork.linuxtv.org/patch/27919/
> 
>  .../devicetree/bindings/media/i2c/ov2659.txt       |   38 +
>  MAINTAINERS                                        |   10 +
>  drivers/media/i2c/Kconfig                          |   11 +
>  drivers/media/i2c/Makefile                         |    1 +
>  drivers/media/i2c/ov2659.c                         | 1510 ++++++++++++++++++++
>  include/media/ov2659.h                             |   33 +
>  6 files changed, 1603 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
>  create mode 100644 drivers/media/i2c/ov2659.c
>  create mode 100644 include/media/ov2659.h
> 
> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> new file mode 100644
> index 0000000..3ae6629
> --- /dev/null
> +++ b/drivers/media/i2c/ov2659.c

<snip>

> +static const struct ov2659_pixfmt ov2659_formats[] = {
> +	{
> +		.code = MEDIA_BUS_FMT_YUYV8_2X8,
> +		.colorspace = V4L2_COLORSPACE_JPEG,
> +		.format_ctrl_regs = ov2659_format_yuyv,
> +	},
> +	{
> +		.code = MEDIA_BUS_FMT_UYVY8_2X8,
> +		.colorspace = V4L2_COLORSPACE_JPEG,
> +		.format_ctrl_regs = ov2659_format_uyvy,
> +	},
> +	{
> +		.code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> +		.colorspace = V4L2_COLORSPACE_JPEG,
> +		.format_ctrl_regs = ov2659_format_rgb565,
> +	},
> +	{
> +		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
> +		.colorspace = V4L2_COLORSPACE_SMPTE170M,
> +		.format_ctrl_regs = ov2659_format_bggr,
> +	},
> +};

The colorspaces defined here make no sense. Sensors should give you
V4L2_COLORSPACE_SRGB. Certainly not COLORSPACE_JPEG (unless they encode
to a JPEG for you) and SMPTE170M (SDTV) is unlikely as well, unless the
documentation explicitly states that it uses that colorspace.

Unfortunately, the product brief of this sensor does not mention the
colorimetry information at all, nor does it give any information about
the transfer function (aka gamma) used by the sensor. Since this sensor
is advertised as an HDTV sensor I would guess the colorspace should either
be SRGB or REC709, depending on the transfer function used.

I see a lot of sensor drivers that wrongly use the JPEG colorspace. I'm planning
to fix them, since that is really wrong.

Regards,

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