From: Dmitry Osipenko <digetx@gmail.com>
To: Jon Hunter <jonathanh@nvidia.com>
Cc: linux-pm@vger.kernel.org,
Peter De Schrijver <pdeschrijver@nvidia.com>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
linux-kernel@vger.kernel.org,
Thierry Reding <thierry.reding@gmail.com>,
linux-tegra@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 1/6] ARM: tegra: Remove cpuidle drivers
Date: Mon, 15 Jul 2019 01:19:19 +0300 [thread overview]
Message-ID: <0a04f2f3-a2bd-b750-d6db-06dae481fb0b@gmail.com> (raw)
In-Reply-To: <9d79ed3e-a37f-af9c-0696-31dc33bbdefd@gmail.com>
12.07.2019 19:23, Dmitry Osipenko пишет:
> 12.07.2019 12:39, Jon Hunter пишет:
>>
>> On 11/07/2019 18:03, Dmitry Osipenko wrote:
>>> 11.07.2019 12:26, Jon Hunter пишет:
>>>>
>>>> On 11/07/2019 04:13, Dmitry Osipenko wrote:
>>>>> Remove the old drivers to replace them cleanly with a new one later on.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>> arch/arm/mach-tegra/Makefile | 13 --
>>>>> arch/arm/mach-tegra/cpuidle-tegra114.c | 89 -----------
>>>>> arch/arm/mach-tegra/cpuidle-tegra20.c | 212 -------------------------
>>>>> arch/arm/mach-tegra/cpuidle-tegra30.c | 132 ---------------
>>>>> arch/arm/mach-tegra/cpuidle.c | 50 ------
>>>>> arch/arm/mach-tegra/cpuidle.h | 21 ---
>>>>> arch/arm/mach-tegra/irq.c | 18 ---
>>>>> arch/arm/mach-tegra/irq.h | 11 --
>>>>> arch/arm/mach-tegra/pm.c | 7 -
>>>>> arch/arm/mach-tegra/pm.h | 1 -
>>>>> arch/arm/mach-tegra/reset-handler.S | 11 --
>>>>> arch/arm/mach-tegra/reset.h | 9 +-
>>>>> arch/arm/mach-tegra/sleep-tegra20.S | 190 +---------------------
>>>>> arch/arm/mach-tegra/sleep.h | 12 --
>>>>> arch/arm/mach-tegra/tegra.c | 3 -
>>>>> drivers/soc/tegra/Kconfig | 1 -
>>>>> include/soc/tegra/cpuidle.h | 4 -
>>>>> 17 files changed, 5 insertions(+), 779 deletions(-)
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra114.c
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra20.c
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra30.c
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle.c
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle.h
>>>>> delete mode 100644 arch/arm/mach-tegra/irq.h
>>>>
>>>> By removing all the above, it is really hard to review the diff. Is
>>>> there any way you could first consolidate the cpuidle drivers into say
>>>> the existing arch/arm/mach-tegra/cpuidle-tegra20.c and then move to
>>>> drivers/cpuidle?
>>>
>>> I'm afraid that it will make reviewing even more difficult because
>>> everything that is removed here is not returned in the further patches.
>>> The new driver is based on the older ones, but I wrote it from scratch
>>> and it's not only looks different, but also works a bit different as you
>>> may see.
>>>
>>> Could you please clarify what exactly makes it hard to review? The diff
>>> looks pretty clean to me, while squashing everything into existing
>>> driver should be quite a mess.
>>
>> Ideally a patch should standalone and can be reviewed by itself.
>> However, to review this, we need to review patches 1, 2 and 3 at the
>> same time. So IMO it is not that convenient from a reviewers
>> perspective. Furthermore, patches 1 and 3 are large and so easy to miss
>> something.
>>
>> Is there really no way to have a patch to combined the existing drivers,
>> then a patch to convert them into the newer rewritten version you have
>> implemented, then move the driver?
>
> Probably I spent a bit too much time with that code, so now yours
> suggestion looks to me like an unnecessary step. But I will try and see
> how it goes, at least it should be possible to break down the patch 1 a
> bit more, hopefully it will help to better understand what's going on in
> the further patches if you're not familiar or don't remember how it all
> works.
I tried (in several attempts) and couldn't find a way how to
meaningfully squash the old drivers. It's a lot of unnecessary churning
that won't help with reviewing of the code at all because new driver is
structured differently and beating of old drivers until they resemble
the new one just not worth it. I'll write a bit more detailed commit
message, emphasizing the details, hope it will be helpful. The old
drivers are ~200 LOC each and the new one will be ~300 LOC + 50 lines of
comments, I don't think that it's really very hard to review as it it
may initially looked like to you, Jon.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Osipenko <digetx@gmail.com>
To: Jon Hunter <jonathanh@nvidia.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
Peter De Schrijver <pdeschrijver@nvidia.com>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/6] ARM: tegra: Remove cpuidle drivers
Date: Mon, 15 Jul 2019 01:19:19 +0300 [thread overview]
Message-ID: <0a04f2f3-a2bd-b750-d6db-06dae481fb0b@gmail.com> (raw)
In-Reply-To: <9d79ed3e-a37f-af9c-0696-31dc33bbdefd@gmail.com>
12.07.2019 19:23, Dmitry Osipenko пишет:
> 12.07.2019 12:39, Jon Hunter пишет:
>>
>> On 11/07/2019 18:03, Dmitry Osipenko wrote:
>>> 11.07.2019 12:26, Jon Hunter пишет:
>>>>
>>>> On 11/07/2019 04:13, Dmitry Osipenko wrote:
>>>>> Remove the old drivers to replace them cleanly with a new one later on.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>> arch/arm/mach-tegra/Makefile | 13 --
>>>>> arch/arm/mach-tegra/cpuidle-tegra114.c | 89 -----------
>>>>> arch/arm/mach-tegra/cpuidle-tegra20.c | 212 -------------------------
>>>>> arch/arm/mach-tegra/cpuidle-tegra30.c | 132 ---------------
>>>>> arch/arm/mach-tegra/cpuidle.c | 50 ------
>>>>> arch/arm/mach-tegra/cpuidle.h | 21 ---
>>>>> arch/arm/mach-tegra/irq.c | 18 ---
>>>>> arch/arm/mach-tegra/irq.h | 11 --
>>>>> arch/arm/mach-tegra/pm.c | 7 -
>>>>> arch/arm/mach-tegra/pm.h | 1 -
>>>>> arch/arm/mach-tegra/reset-handler.S | 11 --
>>>>> arch/arm/mach-tegra/reset.h | 9 +-
>>>>> arch/arm/mach-tegra/sleep-tegra20.S | 190 +---------------------
>>>>> arch/arm/mach-tegra/sleep.h | 12 --
>>>>> arch/arm/mach-tegra/tegra.c | 3 -
>>>>> drivers/soc/tegra/Kconfig | 1 -
>>>>> include/soc/tegra/cpuidle.h | 4 -
>>>>> 17 files changed, 5 insertions(+), 779 deletions(-)
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra114.c
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra20.c
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra30.c
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle.c
>>>>> delete mode 100644 arch/arm/mach-tegra/cpuidle.h
>>>>> delete mode 100644 arch/arm/mach-tegra/irq.h
>>>>
>>>> By removing all the above, it is really hard to review the diff. Is
>>>> there any way you could first consolidate the cpuidle drivers into say
>>>> the existing arch/arm/mach-tegra/cpuidle-tegra20.c and then move to
>>>> drivers/cpuidle?
>>>
>>> I'm afraid that it will make reviewing even more difficult because
>>> everything that is removed here is not returned in the further patches.
>>> The new driver is based on the older ones, but I wrote it from scratch
>>> and it's not only looks different, but also works a bit different as you
>>> may see.
>>>
>>> Could you please clarify what exactly makes it hard to review? The diff
>>> looks pretty clean to me, while squashing everything into existing
>>> driver should be quite a mess.
>>
>> Ideally a patch should standalone and can be reviewed by itself.
>> However, to review this, we need to review patches 1, 2 and 3 at the
>> same time. So IMO it is not that convenient from a reviewers
>> perspective. Furthermore, patches 1 and 3 are large and so easy to miss
>> something.
>>
>> Is there really no way to have a patch to combined the existing drivers,
>> then a patch to convert them into the newer rewritten version you have
>> implemented, then move the driver?
>
> Probably I spent a bit too much time with that code, so now yours
> suggestion looks to me like an unnecessary step. But I will try and see
> how it goes, at least it should be possible to break down the patch 1 a
> bit more, hopefully it will help to better understand what's going on in
> the further patches if you're not familiar or don't remember how it all
> works.
I tried (in several attempts) and couldn't find a way how to
meaningfully squash the old drivers. It's a lot of unnecessary churning
that won't help with reviewing of the code at all because new driver is
structured differently and beating of old drivers until they resemble
the new one just not worth it. I'll write a bit more detailed commit
message, emphasizing the details, hope it will be helpful. The old
drivers are ~200 LOC each and the new one will be ~300 LOC + 50 lines of
comments, I don't think that it's really very hard to review as it it
may initially looked like to you, Jon.
next prev parent reply other threads:[~2019-07-14 22:19 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-11 3:13 [PATCH v2 0/6] Consolidate and improve NVIDIA Tegra CPUIDLE driver(s) Dmitry Osipenko
2019-07-11 3:13 ` Dmitry Osipenko
2019-07-11 3:13 ` [PATCH v2 1/6] ARM: tegra: Remove cpuidle drivers Dmitry Osipenko
2019-07-11 3:13 ` Dmitry Osipenko
2019-07-11 9:26 ` Jon Hunter
2019-07-11 9:26 ` Jon Hunter
2019-07-11 9:26 ` Jon Hunter
2019-07-11 17:03 ` Dmitry Osipenko
2019-07-11 17:03 ` Dmitry Osipenko
2019-07-12 9:39 ` Jon Hunter
2019-07-12 9:39 ` Jon Hunter
2019-07-12 9:39 ` Jon Hunter
2019-07-12 16:23 ` Dmitry Osipenko
2019-07-12 16:23 ` Dmitry Osipenko
2019-07-14 22:19 ` Dmitry Osipenko [this message]
2019-07-14 22:19 ` Dmitry Osipenko
2019-07-11 3:13 ` [PATCH v2 2/6] ARM: tegra: Expose functions required for cpuidle driver Dmitry Osipenko
2019-07-11 3:13 ` Dmitry Osipenko
2019-07-11 12:42 ` Jon Hunter
2019-07-11 12:42 ` Jon Hunter
2019-07-11 12:42 ` Jon Hunter
2019-07-11 17:25 ` Dmitry Osipenko
2019-07-11 17:25 ` Dmitry Osipenko
2019-07-11 3:13 ` [PATCH v2 3/6] cpuidle: Add unified driver for NVIDIA Tegra SoCs Dmitry Osipenko
2019-07-11 3:13 ` Dmitry Osipenko
2019-07-11 18:35 ` Dmitry Osipenko
2019-07-11 18:35 ` Dmitry Osipenko
2019-07-12 16:41 ` Dmitry Osipenko
2019-07-12 16:41 ` Dmitry Osipenko
2019-07-11 3:13 ` [PATCH v2 4/6] cpuidle: tegra: Support CPU cluster power-down on Tegra30 Dmitry Osipenko
2019-07-11 3:13 ` Dmitry Osipenko
2019-07-11 3:13 ` [PATCH v2 5/6] ARM: multi_v7_defconfig: Enable Tegra cpuidle driver Dmitry Osipenko
2019-07-11 3:13 ` Dmitry Osipenko
2019-07-11 3:13 ` [PATCH v2 6/6] ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig Dmitry Osipenko
2019-07-11 3:13 ` Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0a04f2f3-a2bd-b750-d6db-06dae481fb0b@gmail.com \
--to=digetx@gmail.com \
--cc=daniel.lezcano@linaro.org \
--cc=jonathanh@nvidia.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=pdeschrijver@nvidia.com \
--cc=rjw@rjwysocki.net \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.