Re: [PATCH v3 4/4] DTS: Pandora: fix panel compatibility string

* H. Nikolaus Schaller [off-list ref] [171128 18:35]:

Hi,

quoted

Am 28.11.2017 um 17:18 schrieb Tony Lindgren [off-list ref]:

* H. Nikolaus Schaller [off-list ref] [171128 16:17]:

quoted

Hi Tony,

quoted

Am 28.11.2017 um 17:04 schrieb Tony Lindgren [off-list ref]:

* H. Nikolaus Schaller [off-list ref] [171128 15:52]:

quoted

We can remove the unnecessary "omapdss," prefix because
the omapdrm driver takes care of it when matching with
the driver table.

So is this needed as a fix or is this another clean-up?

So is this is really needed as a fix?

Hm. How do you differentiate between "fix" and "cleanup"?
Maybe it is more a wording than a content issue...

For me it is a "fix" because it is semantically wrong to have
a prefix where it is not needed. And "fixing" it changes the
compiler output by 8 bytes.

How about let's call it a "typo fix" then? :)

Well, it is not really a typo.

Well what if the stable people pick it into earlier stable series
based on the word fix in the subject? That has happened before.

I suggest you update the dts patches to use wording like
"update compatible to use new naming" or something similar.

Regards,

Tony

Hi,

On Thu, Nov 30, 2017 at 07:24:30AM -0800, Tony Lindgren wrote:

* H. Nikolaus Schaller [off-list ref] [171128 18:35]:

quoted

Hi,

quoted

Am 28.11.2017 um 17:18 schrieb Tony Lindgren [off-list ref]:

* H. Nikolaus Schaller [off-list ref] [171128 16:17]:

quoted

Hi Tony,

quoted

Am 28.11.2017 um 17:04 schrieb Tony Lindgren [off-list ref]:

* H. Nikolaus Schaller [off-list ref] [171128 15:52]:

quoted

We can remove the unnecessary "omapdss," prefix because
the omapdrm driver takes care of it when matching with
the driver table.

So is this needed as a fix or is this another clean-up?

So is this is really needed as a fix?

Hm. How do you differentiate between "fix" and "cleanup"?
Maybe it is more a wording than a content issue...

For me it is a "fix" because it is semantically wrong to have
a prefix where it is not needed. And "fixing" it changes the
compiler output by 8 bytes.

How about let's call it a "typo fix" then? :)

Well, it is not really a typo.

Well what if the stable people pick it into earlier stable series
based on the word fix in the subject? That has happened before.

I suggest you update the dts patches to use wording like
"update compatible to use new naming" or something similar.

Patch 4/4 is a Fix and should be applied to stable trees. "omapdss,"
prefix was never supposed to be in the DTS files, is not supposed to
be in there now and will break some time in the future.

Explanation: The early init of omapdss adds the prefix at runtime,
so that the binding can use generic properties and the kernel can
use omapdss specific drivers until the generic ones can be used.

-- Sebastian

Am 30.11.2017 um 16:24 schrieb Tony Lindgren [off-list ref]:

* H. Nikolaus Schaller [off-list ref] [171128 18:35]:

quoted

Hi,

quoted

Am 28.11.2017 um 17:18 schrieb Tony Lindgren [off-list ref]:

* H. Nikolaus Schaller [off-list ref] [171128 16:17]:

quoted

Hi Tony,

quoted

Am 28.11.2017 um 17:04 schrieb Tony Lindgren [off-list ref]:

* H. Nikolaus Schaller [off-list ref] [171128 15:52]:

quoted

We can remove the unnecessary "omapdss," prefix because
the omapdrm driver takes care of it when matching with
the driver table.

So is this needed as a fix or is this another clean-up?

So is this is really needed as a fix?

Hm. How do you differentiate between "fix" and "cleanup"?
Maybe it is more a wording than a content issue...

For me it is a "fix" because it is semantically wrong to have
a prefix where it is not needed. And "fixing" it changes the
compiler output by 8 bytes.

How about let's call it a "typo fix" then? :)

Well, it is not really a typo.

Well what if the stable people pick it into earlier stable series
based on the word fix in the subject? That has happened before.

Well, that may happen but IMHO *every* such backport must be checked
for reasonability and compatibility and sometimes even modified to apply.

And I think the author of the original patch receives a notification
from the stable maintainers and can then veto.

For Example I received "[PATCH 4.7 118/186] w1:omap_hdq: fix regression"
"4.7-stable review patch.  If anyone has any objections, please let me know."

If that process still fails, we simply have to revert it... Has also
happened before. So it will not be end of mankind :)

I suggest you update the dts patches to use wording like
"update compatible to use new naming" or something similar.

I have applied s/fix/improve/ for patch 3/4 and left 4/4 as
suggested by Sebastian since it is really a strongly suggested
correction.

Since Tomi has already accepted the underlaying driver patch,
[PATCH v4] will only have the remaining DTS patches.

BR and thanks,
Nikolaus

On 01/12/17 00:06, Sebastian Reichel wrote:

quoted

quoted

quoted

How about let's call it a "typo fix" then? :)

Well, it is not really a typo.

Well what if the stable people pick it into earlier stable series
based on the word fix in the subject? That has happened before.

I suggest you update the dts patches to use wording like
"update compatible to use new naming" or something similar.

Patch 4/4 is a Fix and should be applied to stable trees. "omapdss,"
prefix was never supposed to be in the DTS files, is not supposed to
be in there now and will break some time in the future.

Explanation: The early init of omapdss adds the prefix at runtime,
so that the binding can use generic properties and the kernel can
use omapdss specific drivers until the generic ones can be used

This is true, but the extra "omapdss" does not cause any issues at the
moment, and the time when it causes issues is still many kernel versions
in the future.

But, yes, thinking about this, I agree, it's better to pick this one
separately as a fix (after rebasing it on top of current maineline so
that it doesn't depend on the toppoly name change), and leaving the
toppoly->tpo change as a cleanup.

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Hi Tomi,

Am 01.12.2017 um 09:13 schrieb Tomi Valkeinen [off-list ref]:

On 01/12/17 00:06, Sebastian Reichel wrote:

quoted

quoted

quoted

quoted

How about let's call it a "typo fix" then? :)

Well, it is not really a typo.

Well what if the stable people pick it into earlier stable series
based on the word fix in the subject? That has happened before.

I suggest you update the dts patches to use wording like
"update compatible to use new naming" or something similar.

Patch 4/4 is a Fix and should be applied to stable trees. "omapdss,"
prefix was never supposed to be in the DTS files, is not supposed to
be in there now and will break some time in the future.

Explanation: The early init of omapdss adds the prefix at runtime,
so that the binding can use generic properties and the kernel can
use omapdss specific drivers until the generic ones can be used

This is true, but the extra "omapdss" does not cause any issues at the
moment, and the time when it causes issues is still many kernel versions
in the future.

But, yes, thinking about this, I agree, it's better to pick this one
separately as a fix (after rebasing it on top of current maineline so
that it doesn't depend on the toppoly name change), and leaving the
toppoly->tpo change as a cleanup.

Just a note: there is no toppoly->tpo change for *this* panel and
Pandora board. Just omapdss removal.

The GTA04 needs a toppoly->tpo change but no omapdss, removal.

So they solve different problems and are independent of each other.

GTA04: change vendor string
Pandora: remove omapdss, prefix

BR and thanks,
Nikolaus

On 01/12/17 11:48, H. Nikolaus Schaller wrote:

Just a note: there is no toppoly->tpo change for *this* panel and
Pandora board. Just omapdss removal.

The GTA04 needs a toppoly->tpo change but no omapdss, removal.

So they solve different problems and are independent of each other.

GTA04: change vendor string
Pandora: remove omapdss, prefix

Oh, right, I totally missed that. I thought they were changes to the
same file...

In that case, Tony, can you pick this one as a fix? I'll pick the
toppoly->tpo patch and merge via drm tree, if you give the ack.

For this:

Acked-by: Tomi Valkeinen <redacted>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

* Tomi Valkeinen [off-list ref] [171201 02:03]:

On 01/12/17 11:48, H. Nikolaus Schaller wrote:

quoted

Just a note: there is no toppoly->tpo change for *this* panel and
Pandora board. Just omapdss removal.

The GTA04 needs a toppoly->tpo change but no omapdss, removal.

So they solve different problems and are independent of each other.

GTA04: change vendor string
Pandora: remove omapdss, prefix

Oh, right, I totally missed that. I thought they were changes to the
same file...

In that case, Tony, can you pick this one as a fix? I'll pick the
toppoly->tpo patch and merge via drm tree, if you give the ack.

For this:

Acked-by: Tomi Valkeinen <redacted>

Sorry I've lost track of this thread and what if anything is really
needed as a fix for v4.16. And now there's newer version of the two
dts patches in thread "[PATCH v4 0/2] Fixes for omapdrm on
OpenPandora and GTA04" so let's move the discussion there.

Regards,

Tony