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

[Jerome Brunet]

On Mon 02 Dec 2024 at 18:53, Stephen Boyd <sboyd@xxxxxxxxxx> 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@xxxxxxxx> wrote:
>> >> On Thu, Nov 28, 2024, at 15:39, Jerome Brunet wrote:
>> >>> On Thu 28 Nov 2024 at 15:11, "Arnd Bergmann" <arnd@xxxxxxxx> 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@xxxxxxxxxx
>> >
>> > 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@xxxxxxxxxxxx

> 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