| Message ID | 1543291261-26174-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: unify some sony camera sensors pattern naming | expand |
Hi Bingbu, On 27/11/18 05:01, bingbu.cao@intel.com wrote: > From: Bingbu Cao <bingbu.cao@intel.com> > > Some Sony camera sensors have same test pattern > definitions, this patch unify the pattern naming > to make it more clear to the userspace. > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > --- > drivers/media/i2c/imx258.c | 8 ++++---- > drivers/media/i2c/imx319.c | 8 ++++---- > drivers/media/i2c/imx355.c | 8 ++++---- > 3 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > index 31a1e2294843..a8a2880c6b4e 100644 > --- a/drivers/media/i2c/imx258.c > +++ b/drivers/media/i2c/imx258.c > @@ -504,10 +504,10 @@ struct imx258_mode { > > static const char * const imx258_test_pattern_menu[] = { > "Disabled", > - "Color Bars", > - "Solid Color", > - "Grey Color Bars", > - "PN9" > + "Solid Colour", > + "Eight Vertical Colour Bars", > + "Colour Bars With Fade to Grey", > + "Pseudorandom Sequence (PN9)", I had a look at imx274, it has many more values but definitely some could be unified too. However I noticed something strange in that driver: The "Horizontal Color Bars" pattern has vertical bars, side-by-side, as in ||||. "Vertical Color Bars" are one on top of the other, as in ==. Is it just me crazy, or are they swapped? Only one minor nitpick about your patch. The USA spelling "color" seems a lot more frequent in the kernel sources than the UK "colour", so it's probably better to be consistent.
On 11/27/2018 04:05 PM, Luca Ceresoli wrote: > Hi Bingbu, > > On 27/11/18 05:01, bingbu.cao@intel.com wrote: >> From: Bingbu Cao <bingbu.cao@intel.com> >> >> Some Sony camera sensors have same test pattern >> definitions, this patch unify the pattern naming >> to make it more clear to the userspace. >> >> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> --- >> drivers/media/i2c/imx258.c | 8 ++++---- >> drivers/media/i2c/imx319.c | 8 ++++---- >> drivers/media/i2c/imx355.c | 8 ++++---- >> 3 files changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c >> index 31a1e2294843..a8a2880c6b4e 100644 >> --- a/drivers/media/i2c/imx258.c >> +++ b/drivers/media/i2c/imx258.c >> @@ -504,10 +504,10 @@ struct imx258_mode { >> >> static const char * const imx258_test_pattern_menu[] = { >> "Disabled", >> - "Color Bars", >> - "Solid Color", >> - "Grey Color Bars", >> - "PN9" >> + "Solid Colour", >> + "Eight Vertical Colour Bars", >> + "Colour Bars With Fade to Grey", >> + "Pseudorandom Sequence (PN9)", > I had a look at imx274, it has many more values but definitely some > could be unified too. > > However I noticed something strange in that driver: The "Horizontal > Color Bars" pattern has vertical bars, side-by-side, as in ||||. > "Vertical Color Bars" are one on top of the other, as in ==. Is it just > me crazy, or are they swapped? Luca, thanks for your review. I do not have the manual of imx274. |||| should be the 'Vertical Color Bars' without any rotation process. If not, I think the definitions there are swapped. > Only one minor nitpick about your patch. The USA spelling "color" seems > a lot more frequent in the kernel sources than the UK "colour", so it's > probably better to be consistent. Ack, I will update in next version. Thanks! >
Hi Luca, On Tue, Nov 27, 2018 at 09:05:34AM +0100, Luca Ceresoli wrote: > Hi Bingbu, > > On 27/11/18 05:01, bingbu.cao@intel.com wrote: > > From: Bingbu Cao <bingbu.cao@intel.com> > > > > Some Sony camera sensors have same test pattern > > definitions, this patch unify the pattern naming > > to make it more clear to the userspace. > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > > --- > > drivers/media/i2c/imx258.c | 8 ++++---- > > drivers/media/i2c/imx319.c | 8 ++++---- > > drivers/media/i2c/imx355.c | 8 ++++---- > > 3 files changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > > index 31a1e2294843..a8a2880c6b4e 100644 > > --- a/drivers/media/i2c/imx258.c > > +++ b/drivers/media/i2c/imx258.c > > @@ -504,10 +504,10 @@ struct imx258_mode { > > > > static const char * const imx258_test_pattern_menu[] = { > > "Disabled", > > - "Color Bars", > > - "Solid Color", > > - "Grey Color Bars", > > - "PN9" > > + "Solid Colour", > > + "Eight Vertical Colour Bars", > > + "Colour Bars With Fade to Grey", > > + "Pseudorandom Sequence (PN9)", > > I had a look at imx274, it has many more values but definitely some > could be unified too. > > However I noticed something strange in that driver: The "Horizontal > Color Bars" pattern has vertical bars, side-by-side, as in ||||. > "Vertical Color Bars" are one on top of the other, as in ==. Is it just > me crazy, or are they swapped? > > Only one minor nitpick about your patch. The USA spelling "color" seems > a lot more frequent in the kernel sources than the UK "colour", so it's > probably better to be consistent. This has been around for some seven or so years in the smiapp driver, and changing strings in uAPI isn't something we prefer to do in general. I wonder what others think. If that's changed, it should be a separate patch.
Hi Bingbu, On 27/11/18 09:55, Bingbu Cao wrote: > > > On 11/27/2018 04:05 PM, Luca Ceresoli wrote: >> Hi Bingbu, >> >> On 27/11/18 05:01, bingbu.cao@intel.com wrote: >>> From: Bingbu Cao <bingbu.cao@intel.com> >>> >>> Some Sony camera sensors have same test pattern >>> definitions, this patch unify the pattern naming >>> to make it more clear to the userspace. >>> >>> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >>> --- >>> drivers/media/i2c/imx258.c | 8 ++++---- >>> drivers/media/i2c/imx319.c | 8 ++++---- >>> drivers/media/i2c/imx355.c | 8 ++++---- >>> 3 files changed, 12 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c >>> index 31a1e2294843..a8a2880c6b4e 100644 >>> --- a/drivers/media/i2c/imx258.c >>> +++ b/drivers/media/i2c/imx258.c >>> @@ -504,10 +504,10 @@ struct imx258_mode { >>> static const char * const imx258_test_pattern_menu[] = { >>> "Disabled", >>> - "Color Bars", >>> - "Solid Color", >>> - "Grey Color Bars", >>> - "PN9" >>> + "Solid Colour", >>> + "Eight Vertical Colour Bars", >>> + "Colour Bars With Fade to Grey", >>> + "Pseudorandom Sequence (PN9)", >> I had a look at imx274, it has many more values but definitely some >> could be unified too. >> >> However I noticed something strange in that driver: The "Horizontal >> Color Bars" pattern has vertical bars, side-by-side, as in ||||. >> "Vertical Color Bars" are one on top of the other, as in ==. Is it just >> me crazy, or are they swapped? > Luca, thanks for your review. > I do not have the manual of imx274. > |||| should be the 'Vertical Color Bars' without any rotation process. > If not, I think the definitions there are swapped. Definitely. I'll send a patch for imx274.
Hi Bingbu, On Mon, Nov 26, 2018 at 7:56 PM <bingbu.cao@intel.com> wrote: > > From: Bingbu Cao <bingbu.cao@intel.com> > > Some Sony camera sensors have same test pattern > definitions, this patch unify the pattern naming > to make it more clear to the userspace. > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > --- > drivers/media/i2c/imx258.c | 8 ++++---- > drivers/media/i2c/imx319.c | 8 ++++---- > drivers/media/i2c/imx355.c | 8 ++++---- > 3 files changed, 12 insertions(+), 12 deletions(-) > Thanks for the patch! One comment inline. > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > index 31a1e2294843..a8a2880c6b4e 100644 > --- a/drivers/media/i2c/imx258.c > +++ b/drivers/media/i2c/imx258.c > @@ -504,10 +504,10 @@ struct imx258_mode { > > static const char * const imx258_test_pattern_menu[] = { > "Disabled", > - "Color Bars", > - "Solid Color", > - "Grey Color Bars", > - "PN9" > + "Solid Colour", > + "Eight Vertical Colour Bars", Is it just me or "solid color" and "color bars" are being swapped here? Did the driver had the names mixed up before or the order of modes is different between these sensors? Best regards, Tomasz
On 12/01/2018 02:08 AM, Tomasz Figa wrote: > Hi Bingbu, > > On Mon, Nov 26, 2018 at 7:56 PM <bingbu.cao@intel.com> wrote: >> From: Bingbu Cao <bingbu.cao@intel.com> >> >> Some Sony camera sensors have same test pattern >> definitions, this patch unify the pattern naming >> to make it more clear to the userspace. >> >> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> --- >> drivers/media/i2c/imx258.c | 8 ++++---- >> drivers/media/i2c/imx319.c | 8 ++++---- >> drivers/media/i2c/imx355.c | 8 ++++---- >> 3 files changed, 12 insertions(+), 12 deletions(-) >> > Thanks for the patch! One comment inline. > >> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c >> index 31a1e2294843..a8a2880c6b4e 100644 >> --- a/drivers/media/i2c/imx258.c >> +++ b/drivers/media/i2c/imx258.c >> @@ -504,10 +504,10 @@ struct imx258_mode { >> >> static const char * const imx258_test_pattern_menu[] = { >> "Disabled", >> - "Color Bars", >> - "Solid Color", >> - "Grey Color Bars", >> - "PN9" >> + "Solid Colour", >> + "Eight Vertical Colour Bars", > Is it just me or "solid color" and "color bars" are being swapped > here? Did the driver had the names mixed up before or the order of > modes is different between these sensors? The test pattern value order of the 3 camera sensors should be same. All are: 0 - Disabled 1 - Solid Colour 2 - Eight Vertical Colour Bars ... This patch swapped the first 2 item for imx258 (wrong order before) and use unified name for all 3 sensors. > > Best regards, > Tomasz >
Hi Bing Bu, Tomasz, On Mon, Dec 03, 2018 at 10:53:34AM +0800, Bingbu Cao wrote: > > > On 12/01/2018 02:08 AM, Tomasz Figa wrote: > > Hi Bingbu, > > > > On Mon, Nov 26, 2018 at 7:56 PM <bingbu.cao@intel.com> wrote: > > > From: Bingbu Cao <bingbu.cao@intel.com> > > > > > > Some Sony camera sensors have same test pattern > > > definitions, this patch unify the pattern naming > > > to make it more clear to the userspace. > > > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > > > --- > > > drivers/media/i2c/imx258.c | 8 ++++---- > > > drivers/media/i2c/imx319.c | 8 ++++---- > > > drivers/media/i2c/imx355.c | 8 ++++---- > > > 3 files changed, 12 insertions(+), 12 deletions(-) > > > > > Thanks for the patch! One comment inline. > > > > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > > > index 31a1e2294843..a8a2880c6b4e 100644 > > > --- a/drivers/media/i2c/imx258.c > > > +++ b/drivers/media/i2c/imx258.c > > > @@ -504,10 +504,10 @@ struct imx258_mode { > > > > > > static const char * const imx258_test_pattern_menu[] = { > > > "Disabled", > > > - "Color Bars", > > > - "Solid Color", > > > - "Grey Color Bars", > > > - "PN9" > > > + "Solid Colour", > > > + "Eight Vertical Colour Bars", > > Is it just me or "solid color" and "color bars" are being swapped > > here? Did the driver had the names mixed up before or the order of > > modes is different between these sensors? > The test pattern value order of the 3 camera sensors should be same. > All are: > 0 - Disabled > 1 - Solid Colour > 2 - Eight Vertical Colour Bars > ... > > This patch swapped the first 2 item for imx258 (wrong order before) and use unified > name for all 3 sensors. I guess this isn't based on Jason's patch (now merged) that fixed the issue. I'll rebase this; it's trivial. commit 53f6f81da7db96557fe2bff9b15bd6b83d301f9f Author: Chen, JasonX Z <jasonx.z.chen@intel.com> Date: Wed Nov 7 21:47:34 2018 -0500 media: imx258: remove test pattern map from driver change bayer order when using test pattern mode. remove test pattern mapping method [Sakari Ailus: Drop extra added newline] Signed-off-by: Chen, JasonX Z <jasonx.z.chen@intel.com> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Hi Sakari, Bingbu, On Mon, Dec 3, 2018 at 7:25 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Bing Bu, Tomasz, > > On Mon, Dec 03, 2018 at 10:53:34AM +0800, Bingbu Cao wrote: > > > > > > On 12/01/2018 02:08 AM, Tomasz Figa wrote: > > > Hi Bingbu, > > > > > > On Mon, Nov 26, 2018 at 7:56 PM <bingbu.cao@intel.com> wrote: > > > > From: Bingbu Cao <bingbu.cao@intel.com> > > > > > > > > Some Sony camera sensors have same test pattern > > > > definitions, this patch unify the pattern naming > > > > to make it more clear to the userspace. > > > > > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > > > > --- > > > > drivers/media/i2c/imx258.c | 8 ++++---- > > > > drivers/media/i2c/imx319.c | 8 ++++---- > > > > drivers/media/i2c/imx355.c | 8 ++++---- > > > > 3 files changed, 12 insertions(+), 12 deletions(-) > > > > > > > Thanks for the patch! One comment inline. > > > > > > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > > > > index 31a1e2294843..a8a2880c6b4e 100644 > > > > --- a/drivers/media/i2c/imx258.c > > > > +++ b/drivers/media/i2c/imx258.c > > > > @@ -504,10 +504,10 @@ struct imx258_mode { > > > > > > > > static const char * const imx258_test_pattern_menu[] = { > > > > "Disabled", > > > > - "Color Bars", > > > > - "Solid Color", > > > > - "Grey Color Bars", > > > > - "PN9" > > > > + "Solid Colour", > > > > + "Eight Vertical Colour Bars", > > > Is it just me or "solid color" and "color bars" are being swapped > > > here? Did the driver had the names mixed up before or the order of > > > modes is different between these sensors? > > The test pattern value order of the 3 camera sensors should be same. > > All are: > > 0 - Disabled > > 1 - Solid Colour > > 2 - Eight Vertical Colour Bars > > ... > > > > This patch swapped the first 2 item for imx258 (wrong order before) and use unified > > name for all 3 sensors. > > I guess this isn't based on Jason's patch (now merged) that fixed the > issue. I'll rebase this; it's trivial. Thanks for clarifying. Best regards, Tomasz
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c index 31a1e2294843..a8a2880c6b4e 100644 --- a/drivers/media/i2c/imx258.c +++ b/drivers/media/i2c/imx258.c @@ -504,10 +504,10 @@ struct imx258_mode { static const char * const imx258_test_pattern_menu[] = { "Disabled", - "Color Bars", - "Solid Color", - "Grey Color Bars", - "PN9" + "Solid Colour", + "Eight Vertical Colour Bars", + "Colour Bars With Fade to Grey", + "Pseudorandom Sequence (PN9)", }; static const int imx258_test_pattern_val[] = { diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c index acd988d2d7f1..17c2e4b41221 100644 --- a/drivers/media/i2c/imx319.c +++ b/drivers/media/i2c/imx319.c @@ -1648,10 +1648,10 @@ struct imx319 { static const char * const imx319_test_pattern_menu[] = { "Disabled", - "Solid color", - "100% color bars", - "Fade to gray color bars", - "PN9" + "Solid Colour", + "Eight Vertical Colour Bars", + "Colour Bars With Fade to Grey", + "Pseudorandom Sequence (PN9)", }; /* supported link frequencies */ diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c index 9c9559dfd3dd..bed293b60e50 100644 --- a/drivers/media/i2c/imx355.c +++ b/drivers/media/i2c/imx355.c @@ -876,10 +876,10 @@ struct imx355 { static const char * const imx355_test_pattern_menu[] = { "Disabled", - "Solid color", - "100% color bars", - "Fade to gray color bars", - "PN9" + "Solid Colour", + "Eight Vertical Colour Bars", + "Colour Bars With Fade to Grey", + "Pseudorandom Sequence (PN9)", }; /* supported link frequencies */