Do we have people interested in device tree janitoring / cleanup?

Do we have people interested in device tree janitoring / cleanup?

[Olof Johansson]

Every now and then I come across a binding that's just done Wrong(tm),
merged through a submaintainer tree and hasn't seen proper review --
if it had, it wouldn't look the way it does. It's something we're
starting to address now since there's more people stepping up to be
maintainers, but there's a backlog of bad bindings already merged.

Often they are produced by translating the platform_data structures
directly over into device-tree properties without consideration to
describing the hardware or usual conventions, using key/value pairs
instead of boolean properties, etc.

Getting involved in cleaning up these kind of bindings is a great way
to learn "the ways of device tree" for someone that has interest in
that.

Latest find in this area is the Maxim 8925 bindings, that I came
across since they caused a compile warning on some defconfig. I'll
post a patch to address the warning but if someone else feels like
fixing the bindings on top of it that would be appreciated!


-Olof

Do we have people interested in device tree janitoring / cleanup?

[Rob Herring]

On 07/24/2013 10:27 AM, Olof Johansson wrote:
> Every now and then I come across a binding that's just done Wrong(tm),
> merged through a submaintainer tree and hasn't seen proper review --
> if it had, it wouldn't look the way it does. It's something we're
> starting to address now since there's more people stepping up to be
> maintainers, but there's a backlog of bad bindings already merged.
> 
> Often they are produced by translating the platform_data structures
> directly over into device-tree properties without consideration to
> describing the hardware or usual conventions, using key/value pairs
> instead of boolean properties, etc.
> 
> Getting involved in cleaning up these kind of bindings is a great way
> to learn "the ways of device tree" for someone that has interest in
> that.
> 
> Latest find in this area is the Maxim 8925 bindings, that I came
> across since they caused a compile warning on some defconfig. I'll
> post a patch to address the warning but if someone else feels like
> fixing the bindings on top of it that would be appreciated!

Are they documented typically? Can we at a minimum update the
documentation with a big fat warning to not use or propagate the crap.
Or move the binding doc file to a fixme directory.

Rob

Do we have people interested in device tree janitoring / cleanup?

[Jason Cooper]

On Wed, Jul 24, 2013 at 01:31:00PM -0500, Rob Herring wrote:
> On 07/24/2013 10:27 AM, Olof Johansson wrote:
> > Every now and then I come across a binding that's just done Wrong(tm),
> > merged through a submaintainer tree and hasn't seen proper review --
> > if it had, it wouldn't look the way it does. It's something we're
> > starting to address now since there's more people stepping up to be
> > maintainers, but there's a backlog of bad bindings already merged.
> > 
> > Often they are produced by translating the platform_data structures
> > directly over into device-tree properties without consideration to
> > describing the hardware or usual conventions, using key/value pairs
> > instead of boolean properties, etc.
> > 
> > Getting involved in cleaning up these kind of bindings is a great way
> > to learn "the ways of device tree" for someone that has interest in
> > that.
> > 
> > Latest find in this area is the Maxim 8925 bindings, that I came
> > across since they caused a compile warning on some defconfig. I'll
> > post a patch to address the warning but if someone else feels like
> > fixing the bindings on top of it that would be appreciated!
> 
> Are they documented typically? Can we at a minimum update the
> documentation with a big fat warning to not use or propagate the crap.
> Or move the binding doc file to a fixme directory.

I agree, in order to do the janitorial work (which I'm not opposed to
helping with), we need a way to mark stable bindings.

fwiw, we could do a separate commit (like the kernel version commit)
where the only change is marking a binding as stable, and is obvious
from a 'git log --oneline'. eg:

---->8------

>From 65c069678cdbd5aaa6aca0d4062dab6eb9f9904c Mon Sep 17 00:00:00 2001
From: Jason Cooper <jason@lakedaemon.net>
Date: Wed, 24 Jul 2013 18:49:28 +0000
Subject: [PATCH] DT: binding: stable: gpio-regulator

A whole bunch of folks reviewed this and think it kicks ass.

List-of-people-responsible-for-this-travesty: ...
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 Documentation/devicetree/bindings/regulator/gpio-regulator.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
index 63c6598..35ec635 100644
--- a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
@@ -1,3 +1,5 @@
+Binding status: Stable
+
 GPIO controlled regulators
 
 Required properties:
-- 
1.8.3.2

Do we have people interested in device tree janitoring / cleanup?

[Tomasz Figa]

Hi Olof,

On Wednesday 24 of July 2013 08:27:13 Olof Johansson wrote:
> Every now and then I come across a binding that's just done Wrong(tm),
> merged through a submaintainer tree and hasn't seen proper review --
> if it had, it wouldn't look the way it does. It's something we're
> starting to address now since there's more people stepping up to be
> maintainers, but there's a backlog of bad bindings already merged.
> 
> Often they are produced by translating the platform_data structures
> directly over into device-tree properties without consideration to
> describing the hardware or usual conventions, using key/value pairs
> instead of boolean properties, etc.
> 
> Getting involved in cleaning up these kind of bindings is a great way
> to learn "the ways of device tree" for someone that has interest in
> that.
> 
> Latest find in this area is the Maxim 8925 bindings, that I came
> across since they caused a compile warning on some defconfig. I'll
> post a patch to address the warning but if someone else feels like
> fixing the bindings on top of it that would be appreciated!

Care to explain your doubts about max8952 bindings? As far as I remember 
it's just a standard single voltage regulator (= generic regulator 
bindings) + some device specific properties.

Looking at Documentation/devicetree/bindings/regulator/max8952.txt I don't 
really see anything worrying...

Best regards,
Tomasz

Do we have people interested in device tree janitoring / cleanup?

[Domenico Andreoli]

On Wed, Jul 24, 2013 at 08:27:13AM -0700, Olof Johansson wrote:
> Every now and then I come across a binding that's just done Wrong(tm),
> merged through a submaintainer tree and hasn't seen proper review --
> if it had, it wouldn't look the way it does. It's something we're
> starting to address now since there's more people stepping up to be
> maintainers, but there's a backlog of bad bindings already merged.

Where is this backlog?

> 
> Often they are produced by translating the platform_data structures
> directly over into device-tree properties without consideration to
> describing the hardware or usual conventions, using key/value pairs
> instead of boolean properties, etc.
> 
> Getting involved in cleaning up these kind of bindings is a great way
> to learn "the ways of device tree" for someone that has interest in
> that.
> 
> Latest find in this area is the Maxim 8925 bindings, that I came
> across since they caused a compile warning on some defconfig. I'll
> post a patch to address the warning but if someone else feels like
> fixing the bindings on top of it that would be appreciated!
> 
> 
> -Olof
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Do we have people interested in device tree janitoring / cleanup?

[Olof Johansson]

Hi,

On Wed, Jul 24, 2013 at 4:15 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Olof,
>
> On Wednesday 24 of July 2013 08:27:13 Olof Johansson wrote:
>> Every now and then I come across a binding that's just done Wrong(tm),
>> merged through a submaintainer tree and hasn't seen proper review --
>> if it had, it wouldn't look the way it does. It's something we're
>> starting to address now since there's more people stepping up to be
>> maintainers, but there's a backlog of bad bindings already merged.
>>
>> Often they are produced by translating the platform_data structures
>> directly over into device-tree properties without consideration to
>> describing the hardware or usual conventions, using key/value pairs
>> instead of boolean properties, etc.
>>
>> Getting involved in cleaning up these kind of bindings is a great way
>> to learn "the ways of device tree" for someone that has interest in
>> that.
>>
>> Latest find in this area is the Maxim 8925 bindings, that I came
>> across since they caused a compile warning on some defconfig. I'll
>> post a patch to address the warning but if someone else feels like
>> fixing the bindings on top of it that would be appreciated!
>
> Care to explain your doubts about max8952 bindings? As far as I remember
> it's just a standard single voltage regulator (= generic regulator
> bindings) + some device specific properties.
>
> Looking at Documentation/devicetree/bindings/regulator/max8952.txt I don't
> really see anything worrying...

Documentation/devicetree/bindings/video/backlight/max8925-backlight.txt

But yes, the regulator one looks normal.


-Olof

Do we have people interested in device tree janitoring / cleanup?

[Olof Johansson]

On Wed, Jul 24, 2013 at 4:17 PM, Domenico Andreoli <cavokz@gmail.com> wrote:
> On Wed, Jul 24, 2013 at 08:27:13AM -0700, Olof Johansson wrote:
>> Every now and then I come across a binding that's just done Wrong(tm),
>> merged through a submaintainer tree and hasn't seen proper review --
>> if it had, it wouldn't look the way it does. It's something we're
>> starting to address now since there's more people stepping up to be
>> maintainers, but there's a backlog of bad bindings already merged.
>
> Where is this backlog?

There is no list of them, if that is what you are asking for. If
people are interested in cleaning them up one could be started. Which
is why I was gauging interest.


-Olof

Do we have people interested in device tree janitoring / cleanup?

[Tomasz Figa]

On Wednesday 24 of July 2013 16:20:02 Olof Johansson wrote:
> Hi,
> 
> On Wed, Jul 24, 2013 at 4:15 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > Hi Olof,
> > 
> > On Wednesday 24 of July 2013 08:27:13 Olof Johansson wrote:
> >> Every now and then I come across a binding that's just done
> >> Wrong(tm),
> >> merged through a submaintainer tree and hasn't seen proper review --
> >> if it had, it wouldn't look the way it does. It's something we're
> >> starting to address now since there's more people stepping up to be
> >> maintainers, but there's a backlog of bad bindings already merged.
> >> 
> >> Often they are produced by translating the platform_data structures
> >> directly over into device-tree properties without consideration to
> >> describing the hardware or usual conventions, using key/value pairs
> >> instead of boolean properties, etc.
> >> 
> >> Getting involved in cleaning up these kind of bindings is a great way
> >> to learn "the ways of device tree" for someone that has interest in
> >> that.
> >> 
> >> Latest find in this area is the Maxim 8925 bindings, that I came
> >> across since they caused a compile warning on some defconfig. I'll
> >> post a patch to address the warning but if someone else feels like
> >> fixing the bindings on top of it that would be appreciated!
> > 
> > Care to explain your doubts about max8952 bindings? As far as I
> > remember it's just a standard single voltage regulator (= generic
> > regulator bindings) + some device specific properties.
> > 
> > Looking at Documentation/devicetree/bindings/regulator/max8952.txt I
> > don't really see anything worrying...
> 
> Documentation/devicetree/bindings/video/backlight/max8925-backlight.txt
> 
> But yes, the regulator one looks normal.

Oh gosh, it's time to sleep. I confused 8925 with 8952... Sorry for the 
noise and thanks for quick reply. (I'm writing separate reply about 
janitoring/cleanup itself.)

Best regards,
Tomasz

Do we have people interested in device tree janitoring / cleanup?

[Tomasz Figa]

On Wednesday 24 of July 2013 15:03:15 Jason Cooper wrote:
> On Wed, Jul 24, 2013 at 01:31:00PM -0500, Rob Herring wrote:
> > On 07/24/2013 10:27 AM, Olof Johansson wrote:
> > > Every now and then I come across a binding that's just done
> > > Wrong(tm),
> > > merged through a submaintainer tree and hasn't seen proper review --
> > > if it had, it wouldn't look the way it does. It's something we're
> > > starting to address now since there's more people stepping up to be
> > > maintainers, but there's a backlog of bad bindings already merged.
> > > 
> > > Often they are produced by translating the platform_data structures
> > > directly over into device-tree properties without consideration to
> > > describing the hardware or usual conventions, using key/value pairs
> > > instead of boolean properties, etc.
> > > 
> > > Getting involved in cleaning up these kind of bindings is a great
> > > way
> > > to learn "the ways of device tree" for someone that has interest in
> > > that.
> > > 
> > > Latest find in this area is the Maxim 8925 bindings, that I came
> > > across since they caused a compile warning on some defconfig. I'll
> > > post a patch to address the warning but if someone else feels like
> > > fixing the bindings on top of it that would be appreciated!
> > 
> > Are they documented typically? Can we at a minimum update the
> > documentation with a big fat warning to not use or propagate the crap.
> > Or move the binding doc file to a fixme directory.
> 
> I agree, in order to do the janitorial work (which I'm not opposed to
> helping with), we need a way to mark stable bindings.
> 
> fwiw, we could do a separate commit (like the kernel version commit)
> where the only change is marking a binding as stable, and is obvious
> from a 'git log --oneline'. eg:
> 
> ---->8------
> 
> From 65c069678cdbd5aaa6aca0d4062dab6eb9f9904c Mon Sep 17 00:00:00 2001
> From: Jason Cooper <jason@lakedaemon.net>
> Date: Wed, 24 Jul 2013 18:49:28 +0000
> Subject: [PATCH] DT: binding: stable: gpio-regulator
> 
> A whole bunch of folks reviewed this and think it kicks ass.
> 
> List-of-people-responsible-for-this-travesty: ...
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> ---
>  Documentation/devicetree/bindings/regulator/gpio-regulator.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
> b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt index
> 63c6598..35ec635 100644
> --- a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
> @@ -1,3 +1,5 @@
> +Binding status: Stable
> +
>  GPIO controlled regulators
> 
>  Required properties:

This looks fine for me, but what about adopting the scheme used for 
drivers and having a staging subdirectory inside bindings/ directory where 
a whole tree of staging bindings would be located? Then marking a binding 
as stable would mean moving it out of this directory.

As for janitoring/cleanup itself, we should start some witch^Wbinding-hunt 
to check all the existing binding for sanity and probably make a list of 
bindings that are not acceptable and possibly contact people responsible 
for them as they are primary candidates to work on fixing bindings they 
introduced.

I also think we should start the periodic binding review meetings and get 
some bindings stabilized, so they could be used as good examples for 
people working on new bindings or fixing existing ones.

Best regards,
Tomasz

P.S. Is it just me or something is wrong with devicetree at vger.kernel.org 
mailing list? I don't receive any messages from it.

Do we have people interested in device tree janitoring / cleanup?

[Jason Cooper]

On Thu, Jul 25, 2013 at 01:29:28AM +0200, Tomasz Figa wrote:
> On Wednesday 24 of July 2013 15:03:15 Jason Cooper wrote:
> > On Wed, Jul 24, 2013 at 01:31:00PM -0500, Rob Herring wrote:
> > > On 07/24/2013 10:27 AM, Olof Johansson wrote:
> > > > Every now and then I come across a binding that's just done
> > > > Wrong(tm),
> > > > merged through a submaintainer tree and hasn't seen proper review --
> > > > if it had, it wouldn't look the way it does. It's something we're
> > > > starting to address now since there's more people stepping up to be
> > > > maintainers, but there's a backlog of bad bindings already merged.
> > > > 
> > > > Often they are produced by translating the platform_data structures
> > > > directly over into device-tree properties without consideration to
> > > > describing the hardware or usual conventions, using key/value pairs
> > > > instead of boolean properties, etc.
> > > > 
> > > > Getting involved in cleaning up these kind of bindings is a great
> > > > way
> > > > to learn "the ways of device tree" for someone that has interest in
> > > > that.
> > > > 
> > > > Latest find in this area is the Maxim 8925 bindings, that I came
> > > > across since they caused a compile warning on some defconfig. I'll
> > > > post a patch to address the warning but if someone else feels like
> > > > fixing the bindings on top of it that would be appreciated!
> > > 
> > > Are they documented typically? Can we at a minimum update the
> > > documentation with a big fat warning to not use or propagate the crap.
> > > Or move the binding doc file to a fixme directory.
> > 
> > I agree, in order to do the janitorial work (which I'm not opposed to
> > helping with), we need a way to mark stable bindings.
> > 
> > fwiw, we could do a separate commit (like the kernel version commit)
> > where the only change is marking a binding as stable, and is obvious
> > from a 'git log --oneline'. eg:
> > 
> > ---->8------
> > 
> > From 65c069678cdbd5aaa6aca0d4062dab6eb9f9904c Mon Sep 17 00:00:00 2001
> > From: Jason Cooper <jason@lakedaemon.net>
> > Date: Wed, 24 Jul 2013 18:49:28 +0000
> > Subject: [PATCH] DT: binding: stable: gpio-regulator
> > 
> > A whole bunch of folks reviewed this and think it kicks ass.
> > 
> > List-of-people-responsible-for-this-travesty: ...
> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> > ---
> >  Documentation/devicetree/bindings/regulator/gpio-regulator.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
> > b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt index
> > 63c6598..35ec635 100644
> > --- a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
> > +++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
> > @@ -1,3 +1,5 @@
> > +Binding status: Stable
> > +
> >  GPIO controlled regulators
> > 
> >  Required properties:
> 
> This looks fine for me, but what about adopting the scheme used for 
> drivers and having a staging subdirectory inside bindings/ directory where 
> a whole tree of staging bindings would be located? Then marking a binding 
> as stable would mean moving it out of this directory.

I'm not opposed to the concept of a staging directory per-se, I just
don't see a need for it.  We aren't coaxing horrible code into workable
form suitable for mainline.  I doubt the fixes to any given binding will
be a large patch series, much less span multiple merge windows.  It just
seems like churn to me to move stuff to 'staging', apply one patch, and
move it back.

The only advantage I see is that users would need to type 'staging' to
get to it, thus cluing them in to the tenuous state of the binding.
That is a definite advantage.  I'm at a loss to find a better
clue-hammer than this to make it clear as to the state of the binding.
To that end, I'd prefer 'unstable'.

Of course, the key question to ask is, how long will this take?  If we
think it will span multiple merge windows, then we should probably do
it.

> As for janitoring/cleanup itself, we should start some witch^Wbinding-hunt 
> to check all the existing binding for sanity and probably make a list of 
> bindings that are not acceptable and possibly contact people responsible 
> for them as they are primary candidates to work on fixing bindings they 
> introduced.
> 
> I also think we should start the periodic binding review meetings and get 
> some bindings stabilized, so they could be used as good examples for 
> people working on new bindings or fixing existing ones.

Once the witch-hunt is completed, there shouldn't be a need to do periodic
review.  It should be reviewed *before* merging.  That'll be easier once
it's a separate tree.

thx,

Jason.

Do we have people interested in device tree janitoring / cleanup?

[Domenico Andreoli]

On Wed, Jul 24, 2013 at 04:20:48PM -0700, Olof Johansson wrote:
> On Wed, Jul 24, 2013 at 4:17 PM, Domenico Andreoli <cavokz@gmail.com> wrote:
> > On Wed, Jul 24, 2013 at 08:27:13AM -0700, Olof Johansson wrote:
> >> Every now and then I come across a binding that's just done Wrong(tm),
> >> merged through a submaintainer tree and hasn't seen proper review --
> >> if it had, it wouldn't look the way it does. It's something we're
> >> starting to address now since there's more people stepping up to be
> >> maintainers, but there's a backlog of bad bindings already merged.
> >
> > Where is this backlog?
> 
> There is no list of them, if that is what you are asking for. If
> people are interested in cleaning them up one could be started. Which
> is why I was gauging interest.

Wanted to see the kind of issues and find something to start with. I expect
some hints because I clearly cannot find what/where is wrong on my own.

> 
> -Olof

Domenico

Do we have people interested in device tree janitoring / cleanup?

[Kyle Spaans (CSC)]

2013/7/24 Olof Johansson <olof@lixom.net>:
> On Wed, Jul 24, 2013 at 4:17 PM, Domenico Andreoli <cavokz@gmail.com> wrote:
>> On Wed, Jul 24, 2013 at 08:27:13AM -0700, Olof Johansson wrote:
>>> Every now and then I come across a binding that's just done Wrong(tm),
>>> merged through a submaintainer tree and hasn't seen proper review --
>>> if it had, it wouldn't look the way it does. It's something we're
>>> starting to address now since there's more people stepping up to be
>>> maintainers, but there's a backlog of bad bindings already merged.
>>
>> Where is this backlog?
>
> There is no list of them, if that is what you are asking for. If
> people are interested in cleaning them up one could be started. Which
> is why I was gauging interest.

I'm a new contributor, but I'm interesting in helping with the
janitorial work. I need to learn more about device trees to help get
support for a board I'm working with (Freescale iMX6q SD) into
mainline.

Do we have people interested in device tree janitoring / cleanup?

[Dave Martin]

On Wed, Jul 24, 2013 at 08:27:13AM -0700, Olof Johansson wrote:
> Every now and then I come across a binding that's just done Wrong(tm),
> merged through a submaintainer tree and hasn't seen proper review --
> if it had, it wouldn't look the way it does. It's something we're
> starting to address now since there's more people stepping up to be
> maintainers, but there's a backlog of bad bindings already merged.
> 
> Often they are produced by translating the platform_data structures
> directly over into device-tree properties without consideration to
> describing the hardware or usual conventions, using key/value pairs
> instead of boolean properties, etc.
> 
> Getting involved in cleaning up these kind of bindings is a great way
> to learn "the ways of device tree" for someone that has interest in
> that.
> 
> Latest find in this area is the Maxim 8925 bindings, that I came
> across since they caused a compile warning on some defconfig. I'll
> post a patch to address the warning but if someone else feels like
> fixing the bindings on top of it that would be appreciated!

DT bindings (even poorly conceived, ad-hoc and/or undocumented ones)
are ABI.

A review/merge process which _allows_ junk into an ABI is the real
problem we need to solve here, but once it's there we can't just magic
it away.

Do we plan on having a proper deprecation path for the junk, so that
the old, superseded bindings continue to work for a limited time,
preferably with a big fat warning somewhere?

Cheers
---Dave

Do we have people interested in device tree janitoring / cleanup?

[Russell King - ARM Linux]

On Thu, Jul 25, 2013 at 12:06:34PM +0100, Dave Martin wrote:
> On Wed, Jul 24, 2013 at 08:27:13AM -0700, Olof Johansson wrote:
> > Every now and then I come across a binding that's just done Wrong(tm),
> > merged through a submaintainer tree and hasn't seen proper review --
> > if it had, it wouldn't look the way it does. It's something we're
> > starting to address now since there's more people stepping up to be
> > maintainers, but there's a backlog of bad bindings already merged.
> > 
> > Often they are produced by translating the platform_data structures
> > directly over into device-tree properties without consideration to
> > describing the hardware or usual conventions, using key/value pairs
> > instead of boolean properties, etc.
> > 
> > Getting involved in cleaning up these kind of bindings is a great way
> > to learn "the ways of device tree" for someone that has interest in
> > that.
> > 
> > Latest find in this area is the Maxim 8925 bindings, that I came
> > across since they caused a compile warning on some defconfig. I'll
> > post a patch to address the warning but if someone else feels like
> > fixing the bindings on top of it that would be appreciated!
> 
> DT bindings (even poorly conceived, ad-hoc and/or undocumented ones)
> are ABI.
> 
> A review/merge process which _allows_ junk into an ABI is the real
> problem we need to solve here, but once it's there we can't just magic
> it away.
> 
> Do we plan on having a proper deprecation path for the junk, so that
> the old, superseded bindings continue to work for a limited time,
> preferably with a big fat warning somewhere?

This is the exact problem, and if you've seen how Linus rants about
the ARM ecosystem breaking ABI stuff all the time, you will start to
appreciate why ARM is not highly valued in the Linux communities.

We need to stop this crap getting in.

We need subsystem people to refuse to merge DT stuff if it hasn't been
reviewed by DT people - subsystem people are _not_ qualified to accept
those patches, even if they're responsible for the files which they're
being applied to. Yes, they can review them, but they should _not_
accept them without them having the bindings reviewed by folk who (a)
know the hardware and (b) know the implications of creating a DT binding.

That means whoever is doing the review of DT bindings probably needs to
have access to documentation on the hardware - or they need to be educated
by the submitter.  Or, we need submitters to individually justify why
each and every property is required in the description.

As for those already there, I don't think there's anything which can be
done about them in the short term (short = quite a few years) - we can't
go removing any properties that are already there, because that will
break the ABI.  We're basically stuck with that crap for ever now.

That's the point that people have to understand: if they accept a DT
description into the kernel, it's basically saying at that point that
the DT description is the official and definitive description for that
hardware.  It's just the same as adding a new syscall or something like
that which the entire world starts to use immediately.  You can't go
removing or changing it after the fact.

And this is also why a _hell_ of a lot of thought should go into each
and every property _before_ it is even proposed by the patch submitter.

Do we have people interested in device tree janitoring / cleanup?

[Mark Brown]

On Thu, Jul 25, 2013 at 12:25:58PM +0100, Russell King - ARM Linux wrote:

> That means whoever is doing the review of DT bindings probably needs to
> have access to documentation on the hardware - or they need to be educated
> by the submitter.  Or, we need submitters to individually justify why
> each and every property is required in the description.

My general strategy on that is to complain about any binding which isn't
comprehensible just from the binding document, plus pushing back on
anything that the driver really ought to be able to work out
automatically at runtime.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130725/34e215b4/attachment.sig>

Do we have people interested in device tree janitoring / cleanup?

[Domenico Andreoli]

On Thu, Jul 25, 2013 at 12:25:58PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 25, 2013 at 12:06:34PM +0100, Dave Martin wrote:
> > On Wed, Jul 24, 2013 at 08:27:13AM -0700, Olof Johansson wrote:
> > > Every now and then I come across a binding that's just done Wrong(tm),
> > > merged through a submaintainer tree and hasn't seen proper review --
> > > if it had, it wouldn't look the way it does. It's something we're
> > > starting to address now since there's more people stepping up to be
> > > maintainers, but there's a backlog of bad bindings already merged.
> > > 
> > > Often they are produced by translating the platform_data structures
> > > directly over into device-tree properties without consideration to
> > > describing the hardware or usual conventions, using key/value pairs
> > > instead of boolean properties, etc.
> > > 
> > > Getting involved in cleaning up these kind of bindings is a great way
> > > to learn "the ways of device tree" for someone that has interest in
> > > that.
> > > 
> > > Latest find in this area is the Maxim 8925 bindings, that I came
> > > across since they caused a compile warning on some defconfig. I'll
> > > post a patch to address the warning but if someone else feels like
> > > fixing the bindings on top of it that would be appreciated!
> > 
> > DT bindings (even poorly conceived, ad-hoc and/or undocumented ones)
> > are ABI.
> > 
> > A review/merge process which _allows_ junk into an ABI is the real
> > problem we need to solve here, but once it's there we can't just magic
> > it away.
> > 
> > Do we plan on having a proper deprecation path for the junk, so that
> > the old, superseded bindings continue to work for a limited time,
> > preferably with a big fat warning somewhere?
> 
> This is the exact problem, and if you've seen how Linus rants about
> the ARM ecosystem breaking ABI stuff all the time, you will start to
> appreciate why ARM is not highly valued in the Linux communities.
> 
> We need to stop this crap getting in.
> 
> We need subsystem people to refuse to merge DT stuff if it hasn't been
> reviewed by DT people - subsystem people are _not_ qualified to accept
> those patches, even if they're responsible for the files which they're
> being applied to. Yes, they can review them, but they should _not_
> accept them without them having the bindings reviewed by folk who (a)
> know the hardware and (b) know the implications of creating a DT binding.
> 
> That means whoever is doing the review of DT bindings probably needs to
> have access to documentation on the hardware - or they need to be educated
> by the submitter.  Or, we need submitters to individually justify why
> each and every property is required in the description.
> 
> As for those already there, I don't think there's anything which can be
> done about them in the short term (short = quite a few years) - we can't
> go removing any properties that are already there, because that will
> break the ABI.  We're basically stuck with that crap for ever now.
> 
> That's the point that people have to understand: if they accept a DT
> description into the kernel, it's basically saying at that point that
> the DT description is the official and definitive description for that
> hardware.  It's just the same as adding a new syscall or something like
> that which the entire world starts to use immediately.  You can't go
> removing or changing it after the fact.
> 
> And this is also why a _hell_ of a lot of thought should go into each
> and every property _before_ it is even proposed by the patch submitter.

So this is the true price for having a nice HW agnostic kernel. I'm seeing
the urgency for an impedence adapter.

Would it be anyhow different with ACPI?

Domenico

Do we have people interested in device tree janitoring / cleanup?

[Russell King - ARM Linux]

On Thu, Jul 25, 2013 at 03:56:45PM +0200, Domenico Andreoli wrote:
> On Thu, Jul 25, 2013 at 12:25:58PM +0100, Russell King - ARM Linux wrote:
> > This is the exact problem, and if you've seen how Linus rants about
> > the ARM ecosystem breaking ABI stuff all the time, you will start to
> > appreciate why ARM is not highly valued in the Linux communities.
> > 
> > We need to stop this crap getting in.
> > 
> > We need subsystem people to refuse to merge DT stuff if it hasn't been
> > reviewed by DT people - subsystem people are _not_ qualified to accept
> > those patches, even if they're responsible for the files which they're
> > being applied to. Yes, they can review them, but they should _not_
> > accept them without them having the bindings reviewed by folk who (a)
> > know the hardware and (b) know the implications of creating a DT binding.
> > 
> > That means whoever is doing the review of DT bindings probably needs to
> > have access to documentation on the hardware - or they need to be educated
> > by the submitter.  Or, we need submitters to individually justify why
> > each and every property is required in the description.
> > 
> > As for those already there, I don't think there's anything which can be
> > done about them in the short term (short = quite a few years) - we can't
> > go removing any properties that are already there, because that will
> > break the ABI.  We're basically stuck with that crap for ever now.
> > 
> > That's the point that people have to understand: if they accept a DT
> > description into the kernel, it's basically saying at that point that
> > the DT description is the official and definitive description for that
> > hardware.  It's just the same as adding a new syscall or something like
> > that which the entire world starts to use immediately.  You can't go
> > removing or changing it after the fact.
> > 
> > And this is also why a _hell_ of a lot of thought should go into each
> > and every property _before_ it is even proposed by the patch submitter.
> 
> So this is the true price for having a nice HW agnostic kernel.

Of course it is, and if you thought otherwise, then you haven't thought
about the problem enough.

If anyone went walking into DT thinking that the DT bindings can be hacked
around any old way, then they went into DT with a blindfold on, and did
not *think*/have not thought about the consequences.

Don't go thinking that ACPI would be any different, because it isn't.
It's no different from any _other_ interface that the kernel may ever
use to the outside world, be that the boot loader interface or the user
interfaces.  All those interfaces are all part of "the ABI" to code
outside of the kernel, and those are all interfaces which must be
thought out REAL carefully, and any changes to them must be backed up
with a way for the existing interfaces to _continue_ working.

What that means is that any DT representation created for any device which
has ever appeared in a mainline kernel is part of the kernel ABI and must
remain working in that state.  You can supplement it with new ways of doing
things, but the original bindings must continue to work.

Remember the stated assertion when DT was first added to the kernel: the
DT descriptions _will_ become a separately maintained project which is
independent of the kernel.  They will _not_ be kernel version specific.

Linus wants the platform specific crap like clocks and the like _out_ of
the kernel tree.  He doesn't want the stuff merely transformed into a
different representation - he wants it out of the kernel tree and
entirely separate.

Think about that for a moment with regard to churn in DT descriptions
and you will soon realise that if you change the DT descriptions and
then have to update the kernel inline with those updates, you're on to
creating one hell of a mess.

DT done correctly with care and thought works fine.  DT done without that
is a disaster, one which I had hoped people had learnt from with the
problems that PPC has had with bad bindings.

ABI compatibility is VERY important.  If you disagree with that, send
Linus an email justifying why you think you can change the ABI with no
compatibility.

Do we have people interested in device tree janitoring / cleanup?

[Catalin Marinas]

On Thu, Jul 25, 2013 at 03:27:02PM +0100, Russell King - ARM Linux wrote:
> Remember the stated assertion when DT was first added to the kernel: the
> DT descriptions _will_ become a separately maintained project which is
> independent of the kernel.  They will _not_ be kernel version specific.

I'm looking forward to this.

Question for the DT guys: what are the plans here? Are we going to get
rid of the .dts files inside the kernel tree?

-- 
Catalin

Do we have people interested in device tree janitoring / cleanup?

[Grant Likely]

On Thu, 25 Jul 2013 15:38:15 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Thu, Jul 25, 2013 at 03:27:02PM +0100, Russell King - ARM Linux wrote:
> > Remember the stated assertion when DT was first added to the kernel: the
> > DT descriptions _will_ become a separately maintained project which is
> > independent of the kernel.  They will _not_ be kernel version specific.
> 
> I'm looking forward to this.
> 
> Question for the DT guys: what are the plans here? Are we going to get
> rid of the .dts files inside the kernel tree?

Short answer: yes

Slightly longer answer: we're working out the plan now. There's lots of
discussion going on right now and we'll be talking about it face-to-face
in Edinburgh in October.

g.