Re: [PATCH V1 3/3] mmc: host: Register changes for sdcc V5

[Evan Green]

On Thu, May 24, 2018 at 6:01 AM Vijay Viswanath <vviswana@xxxxxxxxxxxxxx>
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 <vviswana@xxxxxxxxxxxxxx

> > wrote:
> >
> >> From: Sayali Lokhande <sayalil@xxxxxxxxxxxxxx>
> >
...
> > 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