Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

From: "Arnd Bergmann" <arnd@arndb.de>
To: "Jerome Brunet" <jbrunet@baylibre.com>
Cc: "Neil Armstrong" <neil.armstrong@linaro.org>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Kevin Hilman" <khilman@baylibre.com>,
	"Martin Blumenstingl" <martin.blumenstingl@googlemail.com>,
	linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, "Mark Brown" <broonie@kernel.org>
Subject: Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX
Date: Thu, 28 Nov 2024 16:34:46 +0100	[thread overview]
Message-ID: <ce67e512-a15b-4482-8194-b917096f4eeb@app.fastmail.com> (raw)
In-Reply-To: <1jplmf1jqa.fsf@starbuckisacylon.baylibre.com>

On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote:
> On Thu 28 Nov 2024 at 15:51, "Arnd Bergmann" <arnd@arndb.de> wrote:
>> On Thu, Nov 28, 2024, at 15:39, Jerome Brunet wrote:
>>> On Thu 28 Nov 2024 at 15:11, "Arnd Bergmann" <arnd@arndb.de> wrote:
>>> Eventually that will happen for the rest of the reset implemented
>>> under drivers/clk/meson.
>>>
>>> It allows to make some code common between the platform reset
>>> drivers and the aux ones. It also ease maintainance for both
>>> Stephen and Philipp.
>>
>> I don't understand how this helps: the entire point of using
>> an auxiliary device is to separate the lifetime rules of
>> the different bits, but by doing the creation of the device
>> in the same file as the implementation, you are not taking
>> advantage of that at all, but instead get the complexity of
>> a link-time dependency in addition to a lot of extra code
>> for dealing with the additional device.
>
> My initial rework had the creation in clock (note: that is why I
> initially used 'imply', and forgot to update when the creation moved to
> reset).
>
> I was asked to move the creation in reset:
> https://lore.kernel.org/r/217a785212d7c1a5b504c6040b3636e6.sboyd@kernel.org
>
> We are deviating a bit from the initial regression reported by Mark.
> Is Ok with you to proceed with that fix and then continue this discussion
> ?

I really don't want to see those stray 'select' statements
in there, as that leave very little incentive for anyone to
fix it properly.

It sounds like Stephen gave you bad advice for how it should
be structured, so my best suggestion would be to move the
the problem (and the reset driver) back into his subsystem
and leave only a simple 'select RESET_CONTROLLER'.

From the message you cited, I think Stephen had the right
intentions ("so that the clk and reset drivers are decoupled"),
but the end result did not actually do what he intended
even if you did what he asked for.

Stephen, can you please take a look here and see if you
have a better idea for either decoupling the two drivers
enough to avoid the link time dependency, or to reintegrate
the reset controller code into the clk driver and avoid
the complexity?

      Arnd

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic