Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220

[John Stultz <john.stultz@linaro.org>]

On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, July 6, 2016 12:20:15 AM CEST John Stultz wrote:
>> On Wed, Jul 6, 2016 at 12:04 AM, Olof Johansson <olof@lixom.net> wrote:
>> > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote:
>> >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote:
>> >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote:
>> >>>> This patchset enables the pl031 RTC on the Hi6220 SoC.
>> >>>>
>> >>>> I'd like to submit it to be merged.
>> >>>>
>> >>>> Wei has acked the second patch (modulo a whitespace fix which
>> >>>> I've included in this v3), so it seems like both could go
>> >>>> through the clk tree.
>> >>>>
>> >>>> But Wei also seemed open to pulling in a clk tree branch
>> >>>> as it goes through arm-soc.
>> >>>>
>> >>>> Michael/Stephen: If there's no other objections, could you
>> >>>> queue the first patch and make it avilable via the branch for
>> >>>> Wei, or just take both patches?
>> >>>
>> >>> I happen to dread these kind of patchsets these days. There's added
>> >>> dependencies across trees just because a defined name for the clock
>> >>> number is added to a header file.
>> >>>
>> >>> I much prefer to use numerical clocks for one release, and then once
>> >>> everything is in, switch over to the defines in the DTS.
>> >>>
>> >>> That way there are no dependencies, no need to setup a shared branch
>> >>> for a simple 3-line patch, etc.
>> >>>
>> >>> So, mind respinning the DTS piece?
>> >>
>> >> Huh..
>> >
>> > Sorry if it appeared random, I've complained about it for a while to
>> > submaintainers.
>>
>> No.. I get it, the cross-maintainer shared branch is complex enough to
>> want to avoid. I figured it would be easier to just take a maintainer
>> acked patch in via the clk tree, but its not my tree, so I'll leave it
>> to you maintainers to resolve.
>
> The question this raises is why that clock was missed the first time
> around. I'd suggest whoever owns the clock driver can go through the
> documentation again and look for others that may have been missed,
> then send a patch to the driver to add *all* the missing ones for the
> merge window, and one release later we add the driver depending on
> previously unknown clocks.

Though this seemingly goes against the otherwise widely recommended
approach of breaking up patches into small obvious chunks.

And personally, and I don't mean to criticize, but the suggestions
here (use numerical values, then later rename to macros; add
everything in one go, then make dts changes a release later) all seem
like non-optimal workarounds for the fact that adding almost any
functionality requires cross subsystem-maintainer negotiations (or two
release steps to get one bit of functionality merged).

It seems like it might even just be clearer to make the
two-release-steps method the widely broadcast rule (ie: no
dependencies on in-flight patches for dts changes), so this doesn't
confuse/dismay new developers.

Anyway... In this case, I don't have the clk documentation, so I'll
ping Zhangfei to check if there is any other clock values that should
be added in the future, but at least for HiKey, while there are still
a few clk patches remaining in the tree, I don't have any more
additions to the clk list.

thanks
-john