On Thu, May 24, 2018 at 6:01 AM Vijay Viswanath <vvisw...@codeaurora.org> wrote:
> On 5/22/2018 11:42 PM, Evan Green wrote: > > Hi Vijay. Thanks for this patch. > > > > On Thu, May 17, 2018 at 3:30 AM Vijay Viswanath <vvisw...@codeaurora.org > > wrote: > > > >> From: Sayali Lokhande <saya...@codeaurora.org> > > ... > > Nit: host->ioaddr + msm_offset->core_dll_config might benefit from having > > its own local, since you use it so much in this function. Same goes for > > where I've noted below... > > > core_dll_config is very much used. But having a local for it feels like > a bad idea. As different versions come up, the most used register may > change. So it would be better to stick to a consistent approach to > accessing every register. I generally optimize for readability, rather than find/replace-ability. In my opinion, it's distracting to see that expression copy/pasted so many times in the same function. But ultimately this is a style preference, so if you decide not to do it, I'll live. > > > >> + msm_offset->core_pwrctl_status), > >> + msm_host->var_ops->msm_readl_relaxed(host, > >> + msm_offset->core_pwrctl_mask), > >> + msm_host->var_ops->msm_readl_relaxed(host, > >> + msm_offset->core_pwrctl_ctl)); > > > > I think the idea of function pointers is fine, but overall the use of them > > everywhere sure is ugly. It makes it really hard to actually see what's > > happening. I wonder if things might look a lot cleaner with a helper > > function here. Then instead of: > > > > msm_host->var_ops->msm_readl_relaxed(host, msm_offset->core_pwrctl_ctl); > > > > You could have > > > > msm_core_read(host, msm_offset->core_pwrctl_ctl); > > > if we use a helper function, then we will have to pass msm_host into it > as well. Otherwise there would be the hassle of deriving msm_host > address from sdhci_host. > How about using a MACRO here instead for readability ? The deriving part in the helper would likely get inlined and shared by the compiler among all call-sites within a function. But yes, a macro would work too. Thanks Vijay, Evan
