[Gta04-owner] Updates for gta04 DTS file.

[Dr. H. Nikolaus Schaller]

Hi,

Am 06.01.2015 um 20:54 schrieb NeilBrown <neilb at suse.de>:

> On Mon, 5 Jan 2015 23:02:32 +0100 "Dr. H. Nikolaus Schaller"
> <hns at goldelico.com> wrote:
> 
>> Hi,
>> 
>> Am 05.01.2015 um 21:01 schrieb NeilBrown <neilb at suse.de>:
>> 
>>> On Sun, 4 Jan 2015 12:23:38 +0100 "Dr. H. Nikolaus Schaller"
>>> <hns at goldelico.com> wrote:
>>> 
>>>> Hi Neil,
>>>> 
>>>> Am 03.01.2015 um 20:27 schrieb NeilBrown <neilb at suse.de>:
>>>> 
>>>>> 
>>>>> Hi Marek, Nikolaus,
>>>>> 
>>>>> I'm wondering what your plans are for pushing more updates for the gta04 dts
>>>>> file upstream?
>>>> 
>>>> The plan: as fast as we can with our small team. Rome wasn’t built in a day :)
>>> 
>>> Understood.  So I would like to help build Rome and am trying to find the
>>> easiest way to collaborate on this.
>> 
>> That is welcome.
>> 
>>> 
>>>> 
>>>>> There are quite a few fixes that both of us have in our
>>>>> different trees and I'm hoping they can go upstream soon.
>>>> 
>>>> Well, we could think about having one tree to avoid this situation...
>>> 
>>> We have one tree - Linus maintains it for us.  
>> 
>> No, I see three trees:
>> * Linus'
>> * yours
>> * gta04-kernel
>> 
>> which are not in sync and both, the second and third, are almost independently developed and
>> have the goal to get merged into Linus'. The question is how (by which collaboration model) things
>> are synced.
> 
> There is a well established model.  Patches go from developer to maintainer to
> Linus (possibly though intermediate maintainers).
> 
>> 
>> At the moment I think they are synced through Linus. I.e. the one who is faster in submitting and
>> convincing Linus (and his submaintainers) wins. And the second one was waste of energy (because
>> there was no discussion during development - just about the results).
> 
> Duplicated effort does not equate to wasted effort.  If two people work on
> one thing then in the end, two people understand it.  That is better (for the
> community) than only one.  Please be assured that if you get something
> upstream which means that something I was working on is no longer needed, I
> will not feel that my work has been wasted.  I will be happy that upstream
> has improved.
> 
> Where do you suppose this “discussion during development" might happen?  

here on this list.

> As
> I've said before I think the best place for discussion is on the lists
> relevant to the specific subsystem. e.g. linux-mmc for mmc related things.

But do they know the GTA04 and what the GTA04 users want?

> I try to remember to CC things here, but really there just aren't that many
> kernel developers on this list, and we seem to be interested in quite
> separate parts  of the kernel.
> 
> Certainly we could discuss changes to the dts file...
> 
> On that topic, I had a look through some of your changes.  I note that you
> tried to add pinctrl settings for the hdq pin:
> 
>        hdq_pins: hdq_pins {
>                pinctrl-single,pins = <
>                        OMAP3_CORE1_IOPAD(0x21c4, PIN_OUTPUT_PULLDOWN | MUX_MODE0) /* i2c3_sda.hdq */
>> ;
>        };
> 
> but that it didn't work.
> I think the problem is that "0x21c4" should be "0x21c6".
> 0x21c4, bits 0-15 is i2c3_sda and gpio_185.
> 0x21c4, bits 16-31 is hdq_sio, various others, and gpio_170.
> To get the top two bytes you add 2, so 0x21c6.

Yes, that is a common pitfall and I remember there is even one in linus/master.
But since it is not GTA04 related (but OMAP5) I have not mentioned.

> 
> Also, 
> commit 4807b3a4b338c7d59fff61c0b75cace5d411f236
> Author: H. Nikolaus Schaller <hns at goldelico.com>
> Date:   Wed Oct 29 22:21:21 2014 +0100
> 
>    GTA04 DT: set default brightness of PWM backlight to 80%
> 
>                brightness-levels = <0 11 20 30 40 50 60 70 80 90 100>;
> -               default-brightness-level = <10>;
> +               default-brightness-level = <80>;
> 
> This is wrong.  The "brightness-levels" map an index (0-10 in this case) to
> percent duty cycle.
> The default-brightness-level is an index into the array of brightness-levels,
> so 10 is the maximum value.

Ok. So maybe I misunderstood the word “-level”. It should be “-index”…

> 
> 
>> 
>> Is such a competition approach efficient? Is this producing good solutions? I don’t know myself
>> and would be interested in opinions of other gta04-owners how they would like to see the gta04
>> kernel development been done: competitive or cooperatively?
> 
> I don't see our work as competitive.
> I agree that it isn't particularly cooperative.  Partly that is because I
> don't like the way you manage your kernel tree, and you don't want to
> change.  Partly it is because we seem to be interested in quite different
> specifics.
> 
> To be honest, I'm quite happy working by myself.  I do what I can and then
> present it to the upstream maintainer and any interested parties.  I
> incorporate responses and hopefully ultimately get things included upstream.
> 
> If I find that there is someone else working in a similar area to me I would
> certainly discuss issues with them, but for the most part that doesn't
> happen.  I occasionally post here about what I have been working on in case
> someone else is interested.  I hope others who might be working on code might
> also post in case I or others are interested.
> 
> 
>> 
>> 
>>> As soon as patches land there
>>> they are equally available to everyone.
>> 
>> which is too late for doing coordinated work upfront to shape what is being upstreamed…
>> 
>> I mean: we (the gta04 users and project) should have only one tree where we
>> develop things until they are mature to hand them over to Linus for maintaining.
> 
> Why?
> 
> I think we are working on different projects, you and I.
> 
> My project it “make the mainline kernel work on the GTA04".

>  This means
> working with various parts of the kernel community to improve functionality
> of mainline linux so that functionality is available to the GTA04.  It is
> very important to have the discussion in the broader forums as that more
> perspective and more expertise is available and so more general and well
> rounded results can be obtained.
> 
> Your project seems to be “create a kernel that works well on the GTA04".

Using your formulations I would say the goal is

“make the mainline kernel work well on the GTA04”

> That leads you to think that the GTA04 community should be doing something.
> To me that is a very small vision.  I'd rather think "the Linux kernel
> community should be doing something”.

Yes, sure. But they neither own a GTA04 nor can test anything. And sitting there
and waiting that “the Linux kernel community” is doing something is also wrong.

BTW: I understand myself also as a member of “the Linux kernel community”

> To be honest, "we" the "gta04 project" have no hope at all of producing
> something mature and ready to hand over to Linus.  We just don't have the
> skills and the experience.  

Yes.

> We *need* to regularly interact with relevant
> parts of the broader community - to see ourselves as simply a part of that
> community and not something separate - to have any hope of producing anything
> “mature".

That is not excluded and I have had many discussions with Tomi Valkainen to
fix issues with the display subsystem.

So this all is not excluded by the development model or the gta04-kernel project.

> 
> 
>> 
>>> 
>>> 
>>>> 
>>>>> 
>>>>> I have the following 11 patches which I could send upstream with your
>>>>> Acked-by, or which I could send you to be forwarded to Tony, or which could
>>>>> simply become irrelevant if you'll be sending your own patches upstream soon.
>>>> 
>>>> Marek as the DT maintainer should send them to lkml adding his ack and proper
>>>> attribution of the original author(s).
>>> 
>>> OK, I'll send him the patches, though as your small team is apparently short
>>> of time it might be easier for him to just Ack the patches and then I can
>>> send them upstream - there is  plenty of precedent for maintainers not
>>> actually handing the patches, just Acking them.
>> 
>> Acking is the critical piece of work. Not formatting and sending patches. So your offer
>> does not reduce significant work.
> 
> I'm offering to do all the non-critical bits so that you have time to focus
> on that critical stuff.
> 
>> 
>> I understand your urgency but please give us time to do the quality checks your work
>> deserves.
>> 
>>> 
>>>> 
>>>>> 
>>>>> I've included the list of patch titles and the combined diff below.  The
>>>>> individual patches are in
>>>>> git://neil.brown.name/gta04 dts
>>>>> or at
>>>>> http://git.neil.brown.name/?p=gta04.git;a=shortlog;h=refs/heads/dts
>>>>> 
>>>>> or I am happy to post them here.
>>>> 
>>>> So if easily possible please (re)format them as a patch-set against
>>>> 
>>>> http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/3.19-rc2
>>>> 
>>>> and submit here.
>>> 
>>> That doesn't make any sense.  Patches against your tree would not apply
>>> upstream,
>> 
>> Why not? We are based on top of upstream. Just some patches ahead. The 3-way
>> git merge should resolve them.
> 
> "should" != "does".
> 
> I just tried.

Ok, we might be too far away in this case. One obvious reason appears to be reordering
of DT nodes within the file.

> 
> If you were "based on top of upstream", then
>  git log --follow v3.19-rc2.. arch/arm/boot/dts/omap3-gta04.dtsi
> 
> would show my just the changes you have made to that file against upstream.
> It doesn't.  It also shows lots of things that are already upstream.  It
> is very hard to see which of the ...
> 
> $ git log --follow --oneline  v3.19-rc2.. arch/arm/boot/dts/omap3-gta04.dtsi | wc -l
> 100
> 
> 100 patches listed are still relevant and which have gone upstream and which
> have been replaced by subsequent patches and ….

Do a

git diff linus/master 3.19-rc3 — arch/arm/boot/dts/omap3-gta04.dtsi

It shows what differences there are to be upstreamed. Then one can browse through
them and make a nice clean patch.

For example I see the RFID EEPROM. It should be easy to find the patch(es) that
introduced it to the gta04-kernel tree and reformat it to a single patch with 5 lines
to get it upstream. This will trigger discussion with kernel maintainers.

On some future merge with linus/master it will no longer appear in the diff. No work left over.

The problem is that you appear to think in “patches” while I think in “results” (i.e.
flat files in a tree).

Another approach could be to checkout the linus/master version and commit.
This would be a “set back to mainline”. And then copy&paste each of the original
diffs and commit with a nice message. Then you have your “clean patches”.

This is how the “cleanup” process could work. Without breaking the linus/master
repo and the gta04-kernel history. And it can be done if someone wants to spend
the time to do it.

> 
> 
>> 
>>> so Marek would not be able to send them upstream without
>>> re-formatting them to apply to upstream.  So it would be a waste of
>>> everyone's time.
>> 
>> But how do you expect that we test the patches? Just from looking at them?
> 
> That is up to the maintainer.  Sometimes a visual inspection is all that is
> required and all that is possible.
> 
> Generally a maintainer will have a tree in which they put patches that are
> ready to go upstream.  Some maintainers will have several for different
> levels of readiness.
> This (or one of these) will normally be automatically included in linux-next
> where it gets some basic integration (compile) testing.
> The maintainer will normally test this tree before upstream submission, and
> will expect developers to develop against this tree.
> So for example my mmc patches are against the 'next' branch of 
>   git://git.linaro.org/people/ulf.hansson/mmc
> 
> which makes it easy for Ulf to apply and test them.
> 
> If Marek had such a tree I would certainly provide patches against that and
> he would be able to test them.
> Testing against a tree that differs significantly from mainline is
> something that Linus has very explicitly said he doesn't like.  To some
> extent we have to do that with the GTA04 as so much doesn't work, but it is
> best to keep it to a minimum.
> 
> I think we are that the stage were you can boot a mainline kernel on the
> GTA04, providing you have a serial console attached. So it would be possible
> to test against a plain upstream kernel before submission.
> 
> But as I said, it is really up to the maintainer.  Different circumstances
> suggest different compromises.

Exactly. This is why I compromised on the “cleanness” of patches in the gta04-kernel
project. Because the benefits in my view of doing the cleanup just before upstreaming
outweight the benefits of having clean patches only.

BTW: initially I tried to work this way - but failed. It did not run smoothly for me. Maybe
I did not understand some git commands completely.

BR,
Nikolaus

> 
> Thanks,
> NeilBrown
> 
> 
>> 
>>> 
>>> 
>>>> 
>>>>> 
>>>>> Please let me know how you would like to proceed.
>>>> 
>>>> Marek will go through them and check if anything is missing/problematic and we
>>>> will discuss here (if necessary). Finally he submits it upstream and we all will see
>>>> what we get as a response.
>>>> 
>>>> I have only one diff in doubt (without cross-checking with code): the patch for mmc2
>>>> appears to assume driver-features that are not yet in mainline. Or are these new
>>>> features of 3.19-rc2?
>>> 
>>> I assume you are referring to the declaration of a second interrupt line
>>> and the pinctrl settings?  This enables the SDIO card to interrupt the CPU.
>>> 
>>> Support for this went upstream in v3.17:
>>> 
>>> commit 2cd3a2a54656f9c480b1c7272fc07635d575841b
>>> Author: Andreas Fenkart <afenkart at gmail.com>
>>> Date:   Thu May 29 10:28:00 2014 +0200
>>> 
>>>   mmc: omap_hsmmc: Enable SDIO interrupt
>> 
>> Ah, thanks. We have not noticed this nice sdio feature. There are so many changes in
>> upstream hidden in tons of commit messages so that you don’t find all the gold nuggets…
>> 
>>> 
>>> 
>>> 
>>>> 
>>>> What we have learned is that the policy by Tony (and others) is that they reject
>>>> DT patches where the driver is not yet capable of using the attributes (and they
>>>> expect a bindings document as well).
>>> 
>>> Of course.  I kept back all the patches which use features that aren't
>>> upstream yet.
>>> 
>>> Thanks,
>>> NeilBrown
>> 
>> BR,
>> Nikolaus
>