Re: [PATCH v5 08/10] interconnect: qcom: msm8996: Specify no bus clock scaling on A0NoC

[Dmitry Baryshkov]

On 17/02/2023 22:28, Konrad Dybcio wrote:
> On 17.02.2023 21:26, Dmitry Baryshkov wrote:
> > On Fri, 17 Feb 2023 at 21:53, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
> > > On 17.02.2023 20:27, Konrad Dybcio wrote:
> > > > On 17.02.2023 11:46, Konrad Dybcio wrote:
> > > > > A0NoC only does bus scaling through RPM votes and does not have any
> > > > > ICC clocks. Describe this.
> > > > >
> > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> > > > > ---
> > > > This is bad, as devm_clk_get_bulk{"", _optional} doesn't
> > > > check if num_clocks makes sense and passes "-1" down the
> > > > devres alloc chain..
> > > >
> > > > I'll rework this for the next revision by simply assigning
> > > > the common "bus", "bus_a" set everywhere instead of relying
> > > > on it being there by default..
> > > Or maybe I shouldn't, as that will require redefining the array
> > > over and over again.. Perhaps just passing <&xo_board>, <&xo_board>
> > > to a0noc's "bus", "bus_a", similar to what's been done on SDM630's
> > > GNoC would be less messy?
> >
> > What about simply skipping a call to devm_clk_get if num_bus_clocks is negative?
> I tested that locally before reporting the mistake on my side and
> while it works, I just consider it.. ugly, because:
>
> num_clocks =
> > 0 => use the externally specified num_/clocks (logical)
> =0 => use the default 2
> <0 => consider there's zero
>
> ..but maybe that's just me.. if you don't find it ugly, I may just
> go with that.

Would 'lesser ugliness' count? Maybe add a define 'ICC_RPM_NO_CLOCKS = 
-1'? I think that spawning default bus & bus_a everywhere would be worse.

When this driver is rewritten in Rust, we'll have a clear distinction 
between None (meaning default two clocks) and Some(0) (meaning no 
clocks). Wait, does that sound even more ugly?

--
With best wishes
Dmitry