[PATCH] media: unify some sony camera sensors pattern naming

[PATCH] media: unify some sony camera sensors pattern naming

[bingbu.cao]

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)",
 };
 
 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 */
-- 
1.9.1

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Luca Ceresoli]

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.

-- 
Luca

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Bingbu Cao]

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

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Sakari Ailus]

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.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Luca Ceresoli]

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.

-- 
Luca

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Tomasz Figa]

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

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Bingbu Cao]

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
>

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Sakari Ailus]

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>

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Tomasz Figa]

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