Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

From: Dongchun Zhu <dongchun.zhu@mediatek.com>
To: Tomasz Figa <tfiga@chromium.org>
Cc: "Linus Walleij" <linus.walleij@linaro.org>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Nicolas Boichat" <drinkcat@chromium.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"Cao Bing Bu" <bingbu.cao@intel.com>,
	srv_heupstream <srv_heupstream@mediatek.com>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
	Joerg  Roedel <joro@8bytes.org>,"
	<linux-arm-kernel@lists.infradead.org>,
	"Sj Huang" <sj.huang@mediatek.com>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	linux-devicetree <devicetree@vger.kernel.org>,
	"Louis Kuo" <louis.kuo@mediatek.com>,
	"Shengnan Wang (王圣男)" <shengnan.wang@mediatek.com>,
	dongchun.zhu@mediatek.com
Subject: Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver
Date: Mon, 15 Jun 2020 13:54:31 +0800	[thread overview]
Message-ID: <1592200471.8804.681.camel@mhfsdcap03> (raw)
In-Reply-To: <CAAFQd5Bcb4A+HAivA-jrczK+OMxwZk3w0GYoh-DU=6gmTZBWnQ@mail.gmail.com>

Hi Tomasz,

On Fri, 2020-06-12 at 20:39 +0200, Tomasz Figa wrote:
> On Fri, Jun 12, 2020 at 12:49 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote:
> > > Hi Dongchun,
> > >
> > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote:
> > > > Add a V4L2 sub-device driver for OV02A10 image sensor.
> > > >
> > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > ---
> > > >  MAINTAINERS                 |    1 +
> > > >  drivers/media/i2c/Kconfig   |   13 +
> > > >  drivers/media/i2c/Makefile  |    1 +
> > > >  drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 1040 insertions(+)
> > > >  create mode 100644 drivers/media/i2c/ov02a10.c
> > > >
> > >
> > > Thank you for the patch. Please see my comments inline.
> > >
> > > [snip]
> > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c
> > > > new file mode 100644
> > > > index 0000000..160a0b5
> > > > --- /dev/null
> > > > +++ b/drivers/media/i2c/ov02a10.c
> > > [snip]
> > > > +static const char * const ov02a10_test_pattern_menu[] = {
> > > > +   "Disabled",
> > > > +   "Color Bar",
> > >
> > > nit: We should normalize this to one of the standard names. What is the
> > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"?
> > >
> >
> > Yes. It is one kind of 'Eight Vertical Colour Bars'.
> > This pattern is called as 'MIPI color bar' per the datasheet.
> > Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'?
> >
> 
> We should try to stick to the names as exposed by existing drivers.
> There was an attempt to unify the naming of some Sony sensors some
> time ago [1]. Perhaps one of the names there matches the pattern of
> this sensor?
> 
> [1] https://patchwork.kernel.org/patch/10711777/
> 

Sounds great.
It is one good idea to summarize test patterns from various sensors.
But one question plaguing me is that it seems even for the same "Eight
Vertical Colour Bars", different sensors may have different RGB color
map.

Moreover, definition standards of color bar style may differ among
different sensor chip vendors.
For instance, Sony often uses "Solid Color", "Color Bars With Fade to
Grey", "PN9" to abstract test pattern output type; while OmniVision
adopts color bar type 1, 2, 3, 4 or "MIPI Color Bar", "ISP Color Bar"
instead.

> Best regards,
> Tomasz