[PATCH] simplefb: add support for a8b8g8r8 pixel format

[PATCH] simplefb: add support for a8b8g8r8 pixel format

[Alexandre Courbot]

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
 drivers/video/simplefb.c                                       | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 3ea4605..70c26f3 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -12,6 +12,7 @@ Required properties:
 - stride: The number of bytes in each line of the framebuffer.
 - format: The format of the framebuffer surface. Valid values are:
   - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+  - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
 
 Example:
 
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index e2e9e3e..d7041aa 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -84,6 +84,7 @@ struct simplefb_format {
 
 static struct simplefb_format simplefb_formats[] = {
 	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
 };
 
 struct simplefb_params {
-- 
1.8.3

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Jean-Christophe PLAGNIOL-VILLARD]

On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:

> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
> drivers/video/simplefb.c                                       | 1 +
> 2 files changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 3ea4605..70c26f3 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -12,6 +12,7 @@ Required properties:
> - stride: The number of bytes in each line of the framebuffer.
> - format: The format of the framebuffer surface. Valid values are:
>   - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
> +  - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
> 
> Example:
> 
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index e2e9e3e..d7041aa 100644
> --- a/drivers/video/simplefb.c
> +++ b/drivers/video/simplefb.c
> @@ -84,6 +84,7 @@ struct simplefb_format {
> 
> static struct simplefb_format simplefb_formats[] = {
> 	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
> +	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },

why don't you parse the string?

so you will a real generic bindings

Best Regards,
J.
> };
> 
> struct simplefb_params {
> -- 
> 1.8.3
> 

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Alex Courbot]

On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
>> drivers/video/simplefb.c                                       | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 3ea4605..70c26f3 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -12,6 +12,7 @@ Required properties:
>> - stride: The number of bytes in each line of the framebuffer.
>> - format: The format of the framebuffer surface. Valid values are:
>>    - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>> +  - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>>
>> Example:
>>
>> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
>> index e2e9e3e..d7041aa 100644
>> --- a/drivers/video/simplefb.c
>> +++ b/drivers/video/simplefb.c
>> @@ -84,6 +84,7 @@ struct simplefb_format {
>>
>> static struct simplefb_format simplefb_formats[] = {
>> 	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
>> +	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
>
> why don't you parse the string?
>
> so you will a real generic bindings

Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330

The list of modes of this driver should not grow too big. Even in terms 
of footprint I'd say the list should remain smaller than the parsing code.

What we can discuss though is whether we want to keep this a8b8g8r8 
syntax or switch to something more standard, say "rgba8888".

Alex.

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Jean-Christophe PLAGNIOL-VILLARD]

On Jun 6, 2013, at 10:12 AM, Alex Courbot <acourbot@nvidia.com> wrote:

> On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> 
>> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> 
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>> ---
>>> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
>>> drivers/video/simplefb.c                                       | 1 +
>>> 2 files changed, 2 insertions(+)
>>> 
>>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> index 3ea4605..70c26f3 100644
>>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> @@ -12,6 +12,7 @@ Required properties:
>>> - stride: The number of bytes in each line of the framebuffer.
>>> - format: The format of the framebuffer surface. Valid values are:
>>>   - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>>> +  - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>>> 
>>> Example:
>>> 
>>> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
>>> index e2e9e3e..d7041aa 100644
>>> --- a/drivers/video/simplefb.c
>>> +++ b/drivers/video/simplefb.c
>>> @@ -84,6 +84,7 @@ struct simplefb_format {
>>> 
>>> static struct simplefb_format simplefb_formats[] = {
>>> 	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
>>> +	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
>> 
>> why don't you parse the string?
>> 
>> so you will a real generic bindings
> 
> Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
> 
> The list of modes of this driver should not grow too big. Even in terms of footprint I'd say the list should remain smaller than the parsing code.
> 
> What we can discuss though is whether we want to keep this a8b8g8r8 syntax or switch to something more standard, say "rgba8888".

I'm going to be very honest I do not like the simplefb driver from the beginning
but I do found it useful. And as said in it's name it need to be *SIMPLE*

Then a huge list of compatible no way
otherwise we drop this from the simplefb and make it a generic helper

I do not want to see format parser in every drivers this need to handle at video framework level

If I see that we start to increase again and again the simplefb I will not accept
to merge the code as we must keep it simple

Best Regards,
J.

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Alex Courbot]

On 06/06/2013 05:24 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> On Jun 6, 2013, at 10:12 AM, Alex Courbot <acourbot@nvidia.com> wrote:
>
>> On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>
>>> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>>
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> ---
>>>> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
>>>> drivers/video/simplefb.c                                       | 1 +
>>>> 2 files changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> index 3ea4605..70c26f3 100644
>>>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> @@ -12,6 +12,7 @@ Required properties:
>>>> - stride: The number of bytes in each line of the framebuffer.
>>>> - format: The format of the framebuffer surface. Valid values are:
>>>>    - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>>>> +  - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>>>>
>>>> Example:
>>>>
>>>> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
>>>> index e2e9e3e..d7041aa 100644
>>>> --- a/drivers/video/simplefb.c
>>>> +++ b/drivers/video/simplefb.c
>>>> @@ -84,6 +84,7 @@ struct simplefb_format {
>>>>
>>>> static struct simplefb_format simplefb_formats[] = {
>>>> 	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
>>>> +	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
>>>
>>> why don't you parse the string?
>>>
>>> so you will a real generic bindings
>>
>> Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
>>
>> The list of modes of this driver should not grow too big. Even in terms of footprint I'd say the list should remain smaller than the parsing code.
>>
>> What we can discuss though is whether we want to keep this a8b8g8r8 syntax or switch to something more standard, say "rgba8888".
>
> I'm going to be very honest I do not like the simplefb driver from the beginning
> but I do found it useful. And as said in it's name it need to be *SIMPLE*
>
> Then a huge list of compatible no way
> otherwise we drop this from the simplefb and make it a generic helper
>
> I do not want to see format parser in every drivers this need to handle at video framework level
>
> If I see that we start to increase again and again the simplefb I will not accept
> to merge the code as we must keep it simple

In that case it's probably better to maintain a "simple" list of 
supported modes, which is what this patch does.

Alex.

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:27 Thu 06 Jun     , Alex Courbot wrote:
> On 06/06/2013 05:24 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >
> >On Jun 6, 2013, at 10:12 AM, Alex Courbot <acourbot@nvidia.com> wrote:
> >
> >>On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>
> >>>On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> >>>
> >>>>Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> >>>>---
> >>>>Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
> >>>>drivers/video/simplefb.c                                       | 1 +
> >>>>2 files changed, 2 insertions(+)
> >>>>
> >>>>diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> >>>>index 3ea4605..70c26f3 100644
> >>>>--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> >>>>+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> >>>>@@ -12,6 +12,7 @@ Required properties:
> >>>>- stride: The number of bytes in each line of the framebuffer.
> >>>>- format: The format of the framebuffer surface. Valid values are:
> >>>>   - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
> >>>>+  - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
> >>>>
> >>>>Example:
> >>>>
> >>>>diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> >>>>index e2e9e3e..d7041aa 100644
> >>>>--- a/drivers/video/simplefb.c
> >>>>+++ b/drivers/video/simplefb.c
> >>>>@@ -84,6 +84,7 @@ struct simplefb_format {
> >>>>
> >>>>static struct simplefb_format simplefb_formats[] = {
> >>>>	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
> >>>>+	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
> >>>
> >>>why don't you parse the string?
> >>>
> >>>so you will a real generic bindings
> >>
> >>Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
> >>
> >>The list of modes of this driver should not grow too big. Even in terms of footprint I'd say the list should remain smaller than the parsing code.
> >>
> >>What we can discuss though is whether we want to keep this a8b8g8r8 syntax or switch to something more standard, say "rgba8888".
> >
> >I'm going to be very honest I do not like the simplefb driver from the beginning
> >but I do found it useful. And as said in it's name it need to be *SIMPLE*
> >
> >Then a huge list of compatible no way
> >otherwise we drop this from the simplefb and make it a generic helper
> >
> >I do not want to see format parser in every drivers this need to handle at video framework level
> >
> >If I see that we start to increase again and again the simplefb I will not accept
> >to merge the code as we must keep it simple
> 
> In that case it's probably better to maintain a "simple" list of
> supported modes, which is what this patch does.

so get out it of the simplefb other drivers can use it

Best Regards,
J.
> Alex.
> 

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Stephen Warren]

On 06/06/2013 08:50 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 17:27 Thu 06 Jun     , Alex Courbot wrote:
>> On 06/06/2013 05:24 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>
>>> On Jun 6, 2013, at 10:12 AM, Alex Courbot <acourbot@nvidia.com> wrote:
>>>
>>>> On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>
>>>>> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
...
>>>>>> static struct simplefb_format simplefb_formats[] = {
>>>>>> 	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
>>>>>> +	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
>>>>>
>>>>> why don't you parse the string?

Jean-Christophe, I'm afraid I can't tell exactly what you're arguing for.

Here, you want code added to parse the string ...

This has already been rejected as being over-engineered, and more than
this simple driver needs. Even if it were shared code, the only
practical use of such a parsing function would be to support this
driver, since presumably any other HW-specific driver already knows
exactly which format the FB is in, and hence wouldn't need to share this
code.

>>>>> so you will a real generic bindings
>>>>
>>>> Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
>>>>
>>>> The list of modes of this driver should not grow too big. Even in terms of footprint I'd say the list should remain smaller than the parsing code.
>>>>
>>>> What we can discuss though is whether we want to keep this a8b8g8r8 syntax or switch to something more standard, say "rgba8888".
>>>
>>> I'm going to be very honest I do not like the simplefb driver from the beginning
>>> but I do found it useful. And as said in it's name it need to be *SIMPLE*
>>>
>>> Then a huge list of compatible no way
>>> otherwise we drop this from the simplefb and make it a generic helper
>>>
>>> I do not want to see format parser in every drivers this need to handle at video framework level

... yet here you appear to be arguing against using a format parser, or
including a format parser ...

Note that a lookup table isn't any kind of shared parser; it's just a
very tiny and simple table of static data. It seems quite unlikely that
this could be a maintenance issue, even if over time a few more entries
get added to the table.

>>> If I see that we start to increase again and again the simplefb I will not accept
>>> to merge the code as we must keep it simple
>>
>> In that case it's probably better to maintain a "simple" list of
>> supported modes, which is what this patch does.
> 
> so get out it of the simplefb other drivers can use it

... yet here you appear to want to move the list of modes into some
central location ...

I don't think that's useful for the reason I mentioned above: presumably
any other HW-specific driver already knows exactly which format the FB
is in, and hence wouldn't need to share this code/table.

Why don't we simply take this patch to extend this table, and then *if*
any other FB driver needs to parse a format from DT, we can move the
code(table) to a common location at that time. That will be a trivial
change, and one this patch does nothing to make any harder. Making the
code/table common before then seems like over-engineering.

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:17 Thu 06 Jun     , Stephen Warren wrote:
> On 06/06/2013 08:50 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 17:27 Thu 06 Jun     , Alex Courbot wrote:
> >> On 06/06/2013 05:24 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>
> >>> On Jun 6, 2013, at 10:12 AM, Alex Courbot <acourbot@nvidia.com> wrote:
> >>>
> >>>> On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>>>
> >>>>> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> ...
> >>>>>> static struct simplefb_format simplefb_formats[] = {
> >>>>>> 	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
> >>>>>> +	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
> >>>>>
> >>>>> why don't you parse the string?
> 
> Jean-Christophe, I'm afraid I can't tell exactly what you're arguing for.
> 
> Here, you want code added to parse the string ...
> 
> This has already been rejected as being over-engineered, and more than
> this simple driver needs. Even if it were shared code, the only
> practical use of such a parsing function would be to support this
> driver, since presumably any other HW-specific driver already knows
> exactly which format the FB is in, and hence wouldn't need to share this
> code.
> 
> >>>>> so you will a real generic bindings
> >>>>
> >>>> Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
> >>>>
> >>>> The list of modes of this driver should not grow too big. Even in terms of footprint I'd say the list should remain smaller than the parsing code.
> >>>>
> >>>> What we can discuss though is whether we want to keep this a8b8g8r8 syntax or switch to something more standard, say "rgba8888".
> >>>
> >>> I'm going to be very honest I do not like the simplefb driver from the beginning
> >>> but I do found it useful. And as said in it's name it need to be *SIMPLE*
> >>>
> >>> Then a huge list of compatible no way
> >>> otherwise we drop this from the simplefb and make it a generic helper
> >>>
> >>> I do not want to see format parser in every drivers this need to handle at video framework level
> 
> ... yet here you appear to be arguing against using a format parser, or
> including a format parser ...
> 
> Note that a lookup table isn't any kind of shared parser; it's just a
> very tiny and simple table of static data. It seems quite unlikely that
> this could be a maintenance issue, even if over time a few more entries
> get added to the table.
> 
> >>> If I see that we start to increase again and again the simplefb I will not accept
> >>> to merge the code as we must keep it simple
> >>
> >> In that case it's probably better to maintain a "simple" list of
> >> supported modes, which is what this patch does.
> > 
> > so get out it of the simplefb other drivers can use it
> 
> ... yet here you appear to want to move the list of modes into some
> central location ...
> 
> I don't think that's useful for the reason I mentioned above: presumably
> any other HW-specific driver already knows exactly which format the FB
> is in, and hence wouldn't need to share this code/table.
> 
> Why don't we simply take this patch to extend this table, and then *if*
> any other FB driver needs to parse a format from DT, we can move the
> code(table) to a common location at that time. That will be a trivial
> change, and one this patch does nothing to make any harder. Making the
> code/table common before then seems like over-engineering.

that why I said *if I see that we start ....*

I did reject this patch but put a warning I do not want to see a huge table
if so => factorisation or parser

Best Regards,
J.

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Stephen Warren]

On 06/06/2013 10:29 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
...
> I did reject this patch but put a warning I do not want to see a huge table
> if so => factorisation or parser

Did you mean "accept" here not "reject"?

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Stephen Warren]

On 06/06/2013 02:12 AM, Alex Courbot wrote:
> On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>
>> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com>
>> wrote:
>>
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

No commit description? It'd be useful to at least justify this by
mentioning that some platform will actually use this...

...
>>> static struct simplefb_format simplefb_formats[] = {
>>>     { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
>>> +    { "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
>>
>> why don't you parse the string?
>>
>> so you will a real generic bindings
> 
> Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
> 
> The list of modes of this driver should not grow too big. Even in terms
> of footprint I'd say the list should remain smaller than the parsing code.
> 
> What we can discuss though is whether we want to keep this a8b8g8r8
> syntax or switch to something more standard, say "rgba8888".

I would prefer to keep the syntax of the new formats consistent, so that
if we ever do add format-parsing code rather than table-based lookup,
all the existing formats will continue to work unchanged, without any
kind of fallback lookup table.

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:11 Thu 06 Jun     , Stephen Warren wrote:
> On 06/06/2013 02:12 AM, Alex Courbot wrote:
> > On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>
> >> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com>
> >> wrote:
> >>
> >>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> No commit description? It'd be useful to at least justify this by
> mentioning that some platform will actually use this...
> 
> ...
> >>> static struct simplefb_format simplefb_formats[] = {
> >>>     { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
> >>> +    { "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
> >>
> >> why don't you parse the string?
> >>
> >> so you will a real generic bindings
> > 
> > Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
> > 
> > The list of modes of this driver should not grow too big. Even in terms
> > of footprint I'd say the list should remain smaller than the parsing code.
> > 
> > What we can discuss though is whether we want to keep this a8b8g8r8
> > syntax or switch to something more standard, say "rgba8888".
> 
> I would prefer to keep the syntax of the new formats consistent, so that
> if we ever do add format-parsing code rather than table-based lookup,
> all the existing formats will continue to work unchanged, without any
> kind of fallback lookup table.

I do prefer a format-parsing than any long lookup table that take time at boot
time

Best Regards,
J.

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Olof Johansson]

On Thu, Jun 6, 2013 at 9:20 AM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
> On 10:11 Thu 06 Jun     , Stephen Warren wrote:
>> On 06/06/2013 02:12 AM, Alex Courbot wrote:
>> > On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> >>
>> >> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com>
>> >> wrote:
>> >>
>> >>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>
>> No commit description? It'd be useful to at least justify this by
>> mentioning that some platform will actually use this...
>>
>> ...
>> >>> static struct simplefb_format simplefb_formats[] = {
>> >>>     { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
>> >>> +    { "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
>> >>
>> >> why don't you parse the string?
>> >>
>> >> so you will a real generic bindings
>> >
>> > Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
>> >
>> > The list of modes of this driver should not grow too big. Even in terms
>> > of footprint I'd say the list should remain smaller than the parsing code.
>> >
>> > What we can discuss though is whether we want to keep this a8b8g8r8
>> > syntax or switch to something more standard, say "rgba8888".
>>
>> I would prefer to keep the syntax of the new formats consistent, so that
>> if we ever do add format-parsing code rather than table-based lookup,
>> all the existing formats will continue to work unchanged, without any
>> kind of fallback lookup table.
>
> I do prefer a format-parsing than any long lookup table that take time at boot
> time

We're talking about adding a bunch of code instead of one line in a
table. Sorry, I NACK your NACK. If we get more entries we'll add the
parser, but not just for this.

If you want to make a framebuffer-subsystem generic and shared helper,
go ahead. I'm sure simplefb will move over to it when it's available.

Until then:

Acked-by: Olof Johansson <olof@lixom.net>



-Olof

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:33 Thu 06 Jun     , Olof Johansson wrote:
> On Thu, Jun 6, 2013 at 9:20 AM, Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj@jcrosoft.com> wrote:
> > On 10:11 Thu 06 Jun     , Stephen Warren wrote:
> >> On 06/06/2013 02:12 AM, Alex Courbot wrote:
> >> > On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >> >>
> >> >> On Jun 6, 2013, at 9:20 AM, Alexandre Courbot <acourbot@nvidia.com>
> >> >> wrote:
> >> >>
> >> >>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> >>
> >> No commit description? It'd be useful to at least justify this by
> >> mentioning that some platform will actually use this...
> >>
> >> ...
> >> >>> static struct simplefb_format simplefb_formats[] = {
> >> >>>     { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
> >> >>> +    { "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
> >> >>
> >> >> why don't you parse the string?
> >> >>
> >> >> so you will a real generic bindings
> >> >
> >> > Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
> >> >
> >> > The list of modes of this driver should not grow too big. Even in terms
> >> > of footprint I'd say the list should remain smaller than the parsing code.
> >> >
> >> > What we can discuss though is whether we want to keep this a8b8g8r8
> >> > syntax or switch to something more standard, say "rgba8888".
> >>
> >> I would prefer to keep the syntax of the new formats consistent, so that
> >> if we ever do add format-parsing code rather than table-based lookup,
> >> all the existing formats will continue to work unchanged, without any
> >> kind of fallback lookup table.
> >
> > I do prefer a format-parsing than any long lookup table that take time at boot
> > time
> 
> We're talking about adding a bunch of code instead of one line in a
> table. Sorry, I NACK your NACK. If we get more entries we'll add the
> parser, but not just for this.
as there is no NACK so far I do take as a NACK

Best Regards,
J.
> 
> If you want to make a framebuffer-subsystem generic and shared helper,
> go ahead. I'm sure simplefb will move over to it when it's available.
> 
> Until then:
> 
> Acked-by: Olof Johansson <olof@lixom.net>
> 
> 
> 
> -Olof

Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format

[Alex Courbot]

On 06/07/2013 01:32 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> We're talking about adding a bunch of code instead of one line in a
>> table. Sorry, I NACK your NACK. If we get more entries we'll add the
>> parser, but not just for this.
> as there is no NACK so far I do take as a NACK

... which means?

Anyway, I have sent a new version of this patch that includes a summary 
motivating why we need it. Please explain clearly whether you accept it 
or not, and if not, what we need to do to add this mode in a 
satisfactory way.

Thanks,
Alex.