Re: [PATCH v3 06/13] rockchip: rk3399: Add Orangepi RK3399 support

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

Hi,

Le vendredi 26 avril 2019 à 22:23 +0530, Jagan Teki a écrit :
> > > > > First series, I get introduced rk3399-u-boot.dtsi, and only the new
> > > > > boards are using this file and next series rest of the boards are
> > > > > using which indeed a valid step. what is inconsistent here, I don't
> > > > > understand.
> > > > 
> > > > Yes, what you are describing is exactly the issue. It does not make
> > > > sense to introduce a new common u-boot dtsi for rk3399 and add new
> > > > devices that use this file *before* switching existing devices to the
> > > > new common u-boot dtsi for rk3399 in the same series.
> > > 
> > > How come? existing boards doesn't use rk3399-u-boot.dtsi at all? as
> > > patch says it is an initial one and it is bit hard to add all at once.
> > > ie what I did for i.MX6. having said that rk3399-u-boot.dtsi is
> > > unaffected to any of the existing dts files.
> > 
> > Again, it's not a technical issue. Your proposal works and has no
> > technical issue. The issue is in how the commits are grouped together.
> > Patch series need to make logical sense. One patch series should
> > accomplish one change, and each patch represents a step of that change.
> 
> It's about how you think. say if I wouldn't send the binman, I'm sure
> this kind of discussion may not happen. In first mail you said
> "something broken and how it can repair next" that indeed doesn't make
> any sense of without understanding the whole series of changes.

One part to that is that I had misunderstood a few things, but I really
should not have to look at each patch before I can have an idea of what
the series does. Your series is called "rockchip: Add new rk3399
boards" and with the v3.1 change you made, the title is no longer true.
You are doing that + introducing a new rk3399 u-boot dtsi without
switching existing boards to it before adding new ones.

From that moment, you needed to split your changes into two series.
Instead of that, you made another series with mostly unrelated changes
where you included an unrelated commit adding the pre-reloc entries to
the dtsi created in the previous series.

That's an issue for communication with the community.

> Any new changes would come up with initial version, that what we
> usually supported, ie what we did -u-boot.dtsi changes to i.MX6 and
> developers are using the same is adding one after another.

But you already have boards in the tree that needs the tree. You can
make individual commits for switching boards to the new dtsi, but you
need to do that in the same series as introducing the dtsi if you are
submitting both things at the same time. If you want to have them
merged separately, then you can send device dtsi patches individually
for that, but not as part of another unrelated series.

> > This is how upstream contributions work, and it's a powerful way to
> > allow both efficient code review and good code quality. We want to keep
> > things as simple, explicit and well-described as possible, so that
> > things are easy for reviewers and as many people as possible can
> > understand the issue and share their thoughts about it.
> 
> Yes, we adopt these principles and that what we are really working.
> 
> > It is all part of communication with others as part of a community.
> > It is definitely an implicit rule that is not written down somewhere
> > precisely, but that's the social contract between developers that work
> > on a common software project.
> > 
> > In that, contributing to upstream is different than baseline tech
> > company standards, because you have to take extra steps to describe
> > your work and explain it to others. You must make sure you give them
> > all the comfort they need to painlessly understand what you did.
> 
> I hope all my communication was relevant to the topics, I can even
> given the example how things were moved.

Clearly there was a major communication issue between us. You only
answered on technical topics and never about how your patch series is
organized and what logical changes you are making.

To be honest with you, I feel like we have a general communication
issue where you only seem to focus on technical aspects, when the
issues are about the meta-data of the changes such as commit messages,
code comments and how changes are organized together.

> > It plays a great deal in having series accepted without going through
> > very long discussions and numerous iterations. The less questions
> > maintainers will have about your work, the better. They may disagree
> > with the decisions you took, but these discussions are purely technical
> > and can be resolved quickly.
> 
> Pure technical, yes ie what I thought off. no problem for me for long
> discussion atleast people can understand how things went.

Community discussions can not, and must not be purely technical. This
is not a personal opinion of mine, this is how all free software
communities work. Some care more than others about it, but in very
technical projects like U-Boot and Linux, people need to care about it
a lot.

The more technical the things you are dealing with are, the more you
need to do on the non-technical side to make sure what you are doing is
clear, understood by everyone and makes sense on its own.

Cheers,

Paul

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot