Re: [PATCH v3] clk: keystone: sci-clk: Adding support for non contiguous clocks

[Kamlesh Gurudasani]

Nishanth Menon <nm@xxxxxx> writes:

> On 00:25-20240210, Kamlesh Gurudasani wrote:
>> >> > > 
>> >> > > diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>> >> > > index 35fe197dd303..31b7df05d7bb 100644
>> >> > > --- a/drivers/clk/keystone/sci-clk.c
>> >> > > +++ b/drivers/clk/keystone/sci-clk.c
>> >> > > @@ -516,6 +516,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>> >> > >   	struct sci_clk *sci_clk, *prev;
>> >> > >   	int num_clks = 0;
>> >> > >   	int num_parents;
>> >> > > [..]					/* Check if this clock id is valid */
>> >> > > +					ret = provider->ops->is_auto(provider->sci,
>> >> > > +						sci_clk->dev_id, ++clk_id, &state);
>> >> > A bit too nice coding ;) => I had been confused momentarily by clk_id = args.args[1]
>> >> > change just above till I saw that you are pre-incrementing
>> >> > clk_id - Is there a harm in leaving the original clk_id increment logic
>> >> > alone (it was much simpler to read up)?
>> >> 
>> >> No warm in using original code but want to avoid, two statement for
>> >> increment in case of failure and success.
>> >> 
>> >> Let me know, if i need to add few comments around this
>> >> 
>> >> or if you think, code is confusing I can move to original one
>> >
>> > Yes, please drop the un-necessary changes. In this case, original
>> > increment code should work just fine.
>> I wouldn't call it unnecessary, If I have to track increment/addition at
>> 3 different places just to understand the loop, it is hard. On other
>> hand, pre-increment code is solving the problem by having increment at
>> only one place(easier to track). On the plus side, every clk_id belonging to
>> parent is handled completely inside the loop.
>> 
>> For a new person looking at this code, pre-increment code would be
>> actually easier to undertsand.
>> 
>> Also, Udit feels the same.
>> 
>> Would you please explain why do you think the original increment code
>> make more sense? It's not simple to understand or track, that's for sure.
>
> the context of the fix is the is_auto call to know what parent options
> are valid or not. Do the absolutely what is necessary in the change. if
> you want to beautify etc, move it to some other patch and debate about
> it. So, this is un-necessary change in this patch.
The context of the fix i.e. handling non contiguous parents is making
the loop logic complex. Before this patch, i.e. contiguous clock
handling was simple.

In this fix, we are solving the problem as well as keeping the loop
simple. clk_id is basically the part of the same loop and it's affecting
nothing but the loop count. 

This is not just beautification, this is also simplifying the logic and
improving the readibility. 

If the patch can provide the solution and avoid the complexity, then I don't
understand why we need a patch that introduce the complexity and another
patch to solve the complexity.

My original question below is still not answered so I guess there is no
debate here actually.
"Would you please explain why do you think the original increment code
 make more sense? It's not simple to understand or track, that's for
 sure."

Kamlesh
>
> -- 
> Regards,
> Nishanth Menon
> Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D