Messages in this thread |  | | From | Jerome Brunet <> | | Subject | Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX | | Date | Tue, 03 Dec 2024 12:15:41 +0100 |
| |
On Mon 02 Dec 2024 at 18:53, Stephen Boyd <sboyd@kernel.org> wrote:
> Happy Thanksgiving! > > Quoting Arnd Bergmann (2024-11-28 07:34:46) >> 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? > > I think the best approach is to add the reset auxilary device with a > function that creates the auxiliary device directly by string name and > does nothing else. Maybe we can have some helper in the auxiliary > layer that does that all for us, because it's quite a bit of boiler > plate that we need to write over and over again. Something like: > > int devm_auxiliary_device_create(struct device *parent, const char *name) > > that does the whole kzalloc() + ida dance that > devm_meson_rst_aux_register() is doing today and wraps it all up so that > the device is removed when the parent driver unbinds. Then this clk > driver can register the reset device with a single call and not need to > do anything besides select AUXILIARY_BUS.
I think this is fairly close to what I proposed in the inital RFC, but generic instead of specific.
I suspect the the generic path is likely to trigger more discussion. I'd like to be able to finish this migration, instead of leaving half finished like it is now.
May I add back the boiler plate code in drivers/clk/meson, similar to what was proposed in the RFC [1] and propose the generic implementation in parallel ? It will just be a matter of switching when/if it is approved.
[1] https://lore.kernel.org/r/20240516150842.705844-9-jbrunet@baylibre.com
> The regmap can be acquired > from the parent device in the auxiliary driver probe with > dev_get_regmap(adev->parent).
Did not think about that, I'll check, Thanks
-- Jerome
|  |