Hello Maxime, On Wed, Dec 13, 2023 at 09:36:49AM +0100, Maxime Ripard wrote: > On Wed, Dec 13, 2023 at 08:43:00AM +0100, Uwe Kleine-König wrote: > > On Wed, Dec 13, 2023 at 08:16:04AM +0100, Maxime Ripard wrote: > > > On Tue, Dec 12, 2023 at 06:26:37PM +0100, Uwe Kleine-König wrote: > > > > clk_rate_exclusive_get() returns zero unconditionally. Most users "know" > > > > that and don't check the return value. This series fixes the four users > > > > that do error checking on the returned value and then makes function > > > > return void. > > > > > > > > Given that the changes to the drivers are simple and so merge conflicts > > > > (if any) should be easy to handle, I suggest to merge this complete > > > > series via the clk tree. > > > > > > I don't think it's the right way to go about it. > > > > > > clk_rate_exclusive_get() should be expected to fail. For example if > > > there's another user getting an exclusive rate on the same clock. > > > > > > If we're not checking for it right now, then it should probably be > > > fixed, but the callers checking for the error are right to do so if they > > > rely on an exclusive rate. It's the ones that don't that should be > > > modified. > > > > If some other consumer has already "locked" a clock that I call > > clk_rate_exclusive_get() for, this isn't an error. In my bubble I call > > this function because I don't want the rate to change e.g. because I > > setup some registers in the consuming device to provide a fixed UART > > baud rate or i2c bus frequency (and that works as expected). > > [a long text of mostly right things (Uwe's interpretation) that are > however totally unrelated to the patches under discussion.] The clk API works with and without my patches in exactly the same way. It just makes more explicit that clk_rate_exclusive_get() cannot fail today and removes the error handling from consumers that is never used. Yes, my series doesn't fix any race conditions that are there without doubt in some consumers. It also doesn't make the situation any worse. It also doesn't fix other problems that are orthogonal to the intention of this patch series (neither makes it any of them any worse). It's just dead code removal and making sure no new dead code of the same type is introduced in the future. Is there anyone working on improving the clk framework regarding how clk rate exclusivity works? I'd probably not notice, but I guess there is noone that I need to consider for. And if in the future someone redesigns how all that works *and* clk_rate_exclusive_get() stays around *and* that makes it possible that clk_rate_exclusive_get() fails (and thus breaking all consumers that don't care for the actual clk rate and only want it to not change), then they'll have to review all users anyhow and reintroduce error handling. I can live with that and suggest until then we're happy that we have a few drivers with less dead code than before. Cheers! Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
