Re: [PATCH] mfd: cros ec: spi: Add delay for raising CS

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3470 bytes --]

On Mon, Nov 25, 2013 at 10:45:32AM +0000, Lee Jones wrote:
> > > > From: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > 
> > > > The EC has specific timing it requires. Add support for an optional delay
> > > > after raising CS to fix timing issues. This is configurable based on
> > > > a DT property "google,cros-ec-spi-msg-delay".
> > > > 
> > > > If this property isn't set, then no delay will be added. However, if set
> > > > it will cause a delay equal to the value passed to it to be inserted at
> > > > the end of a transaction.
> > > > 
> > > > Signed-off-by: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Bernie Thompson <bhthompson-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > > Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > > > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > > > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > > Changes in v2:
> > > > - make property description more verbose
> > > > 
> > > >  Documentation/devicetree/bindings/mfd/cros-ec.txt |  9 +++++++
> > > 
> > > We need a DT dude to look over this.
> > 
> > I think Mark Rutland looked at this last week and I think I've addressed
> > all his comments. Hopefully he'll find the time to review this.
> 
> Right, I just need his (or one of the other guy's) Ack(s) before I can
> apply the patch.

Sure, no problem.

> > > > +	/* Check for any DT properties */
> > > > +	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> > > 
> > > No need for the first check.
> > 
> > Why not? While it is true that dev->of_node would be enough to determine
> > that the device was instantiated from a device tree, the IS_ENABLED()
> > will allow the compiler to throw away cros_ec_spi_dt_probe() if OF isn't
> > enabled. At the same time it's nicer than #ifdeffery sprinkled across
> > the file and it actually compile-tests all the code. Win-win-win, isn't
> > it?
> 
> I agree that it's better than #ifdeffery, but I didn't know that if
> this check tested negative that the subordinate method would be
> optimised out by the compiler. Are you sure that happens?

I'm pretty sure that's what happens. If you have code like this (which
is pretty much what the above evaluates to if !OF):

	static void foo(void)
	{
		...
	}

	void bar(void)
	{
		if (0)
			foo();
	}

Then the compiler would be actively stupid if it kept foo around. Note
that this is only thrown away because foo() is static and therefore it
cannot be used by anything else except that dead branch. While I have
never checked this personally, I remember it being highlighted as a
feature of the IS_ENABLED() macro back when it was introduced. It is
also quite commonly used throughout the kernel.

> Also, how often is this used without DT?

Well, it doesn't have a dependency on OF, so it can be used without it.
In-tree there seems to be no indication that it is used without DT, but
given that there's no dependency it makes sense to keep it optional.

Of course we could also add the dependency if it really isn't used
outside of a DT context.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]