Re: [PATCH 00/13] Updates for QCOM Alpha PLL

[Abhishek Sahu]

On 2017-12-07 11:53, Stephen Boyd wrote:
> On 09/28, Abhishek Sahu wrote:
> > This patch series does the miscellaneous changes in QCOM Alpha PLL
> > operation and structure to support other types of Alpha PLLâs.
> >
> > 1. It adds the pll_type which will be used for determining all
> >    the properties of Alpha PLL.
> > 2. It adds the support for Brammo and Huayra PLLâs for which
> >    the support is not available in existing alpha PLL code.
> > 3. There wonât be any change in existing users of Alpha PLLâs
> >    since all the newly added code will be under flag for the default
> >    PLL operations.
>
> Ok. I took a long look at this today. I rewrote a bunch of stuff.

 Thanks Stephen for reviewing the changes and making the code
 cleaner. I checked all the code changes and everything looks
 good. It will work for all of our requirement. I will check
 with other PLL users also once and then update the patch
 series with all your suggested code changes after complete
 testing.

> Let me know if anything looks wrong. I'm not really interested in
> having a type template design that causes us to jump through one
> clk op to another set of them. I'd rather keep it flatter. I also
> kept around the macros for the offsets and had it use a register
> map in each struct instead. Yes, we have to go modify the PLL
> types to point to the right register offset, but really that's
> fine and I don't really care. We could have a default fallback
> when the reg pointer is NULL, but I'm not sure that is useful.

 The main reason for going with type template design is due to
 different register offsets. Now, if passing the register
 offsets from pll structure is ok, then we can get rid of this
 function indirection approach.

 Adding NULL check seems to be overhead in all our register macros
 since these PLL structure will be populated by QCOM clock drivers
 only and now, we are making this parameter mandatory.

> The alternative is to make a bunch of new ops structures that
> passes it down into the final functions but that seemed like more
> work for the handful of PLLs we have to worry about. You seem to
> agree here. All told, it got cut down by 100 lines so the patches
> got smaller.

 Passing the ops into final function will be lead to again type
 template design for passing the register offsets. Since, now we
 have very few PLL's in current upstream code so adding register
 offsets will be more convenient and maintainable.

 Also, now the register offset is coming from our own array
 so always we can retrieve the PLL type from that. In future,
 if someone want to have code which requires PLL type then,
 it can be retrieved from diff of passed pll structure
 address and array base address.


 Thanks,
 Abhishek