Re: [PATCH v2 1/1]sdhci-pxa: support tune_timming for various cards

[Chris Ball <cjb@laptop.org>]

Hi Zhangfei,

On Thu, Nov 11, 2010 at 08:19:12AM -0500, zhangfei gao wrote:
> Hi, Chirs & Eric
> 
> Thanks for review, here is updated version.
> 1. After clk_gating is enabled, set_clock will transfer clock=0, so
> clk_disable will be called, currently set_clock will never transfer
> clock=0.
> Later tune_timing only occurs once clock is started, currently it will
> happen when clock is changed.

There are still some review comments that haven't been replied to yet:

* Eric asked whether you really need to tune on every clock change, or
  if once at initialization time would be enough.

* I asked why we shouldn't just inline tune_timing() at its callsite,
  since it's a void function that's only called from one place.

* Philip points out that SD_CLOCK_AND_BURST_SIZE_SETUP is an MMP2-only
  register and should be marked as such, and I agree.  Even *if*
  sdhci-pxa wasn't going to support non-MMP2 SoCs¹, you should still
  mark hardware-specific registers with the hardware they're used on.

Speaking generally, please always reply to review comments -- even if
it's just to explain why you considered a suggested change and aren't
going to make it.

Thanks,

- Chris.

¹: (.. which doesn't seem to be the case, since you've since posted a
   patchset that generalizes the driver.)
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child