Re: [PATCH 1/6] dt-bindings: clocks: add binding for generic clock-generators

From: "Heiko Stübner" <heiko@sntech.de>
To: mturquette@baylibre.com, Stephen Boyd <sboyd@kernel.org>,
	Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
	quentin.schulz@cherry.de, linux-clk@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH 1/6] dt-bindings: clocks: add binding for generic clock-generators
Date: Mon, 15 Jul 2024 12:59:25 +0200	[thread overview]
Message-ID: <3268030.0WQXIW03uk@diego> (raw)
In-Reply-To: <1899010.tdWV9SEqCh@steina-w>

Am Donnerstag, 11. Juli 2024, 07:27:40 CEST schrieb Alexander Stein:
> Am Donnerstag, 11. Juli 2024, 01:21:15 CEST schrieb Stephen Boyd:
> > Quoting Heiko Stübner (2024-07-10 00:45:17)
> > > Am Mittwoch, 10. Juli 2024, 09:02:34 CEST schrieb Alexander Stein:
> > > > 
> > > > So essentially only enable-gpios and vdd-supply is added in comparison to
> > > > fixed-clock. Does it make sense to add that to the fixed-clocks instead?
> > > > Similar to fixed-regulator.
> > > 
> > > I wasn't that sure which way to go in the first place.
> > > The deciding point was reading that line about the fixed clock not
> > > being gateable, so I opted to not touch the fixed-clock.
> > > 
> > > But you're definitly right, this _could_ live inside the fixed-clock
> > > as well, if we decide to get rid of the not-gateable thing above.
> > 
> > It's probably more complicated to combine it with the fixed-clock
> > binding after making properties required like vdd-supply. I'd just make
> > a new binding and look at combining later.
> 
> Maybe I am missing something IMHO adding optional vdd-supply and
> enable-gpios doesn't seem a big deal.
> Anyway I don't have a hard opinion here. To me fixed-clocks still
> seems very appropriate for having a controlling GPIO and power supply.
> I just would get rid of the (comment only) hint they are ungatable.

I think the main issue is that the fixed-rate code is not limited to the
actual fixed-rate clock.

The clk_fixed_rate_ops is exported and used itself in a number of
completely different clock drivers. The same is true for the
clk_register_fixed_rate function, also exported and used in even more
places throughout the kernel while implicitly using clk_fixed_rate_ops.

For just being a simple always-on fixed rate clock, the clk-fixed-rate.c is
already pretty complex and adding supply handling will entail modifying
the shared clk-ops, or defining a separate clk-ops and clk-register
function, which is what we're doing here already.

Heiko