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

[Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

> > > > > +	/* 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? 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html