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: 2298 bytes --]

On Mon, Nov 25, 2013 at 12:10:35PM +0000, Lee Jones wrote:
> > > > > > +	/* 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.
> 
> +1
> 
> > > 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.
> 	
> This would obviously be my preference. I'd be surprised if anyone was
> using this with platform data, but as you say, you never know. Perhaps
> it would be worth obtaining for clarification from the Google guys? 

Bernie, Andrew, Rhyland, can anyone of you comment. Are you aware of
this driver being used with a non-DT setup? I suspect maybe on Intel
Chromebooks it might be, but I couldn't find any evidence of that in
the upstream kernel.

In fact the only upstream user of cros-ec seems to be exynos5250-snow,
which I think is one of the Chromebooks, but it uses DT as well.

Are there any objections to removing non-DT support from the driver?

Thierry

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