Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@xxxxxxx> writes:
> 
>>> Feel free to add another patch, while I don't see a need I have nothing
>>> against :-)
>> No, you fix the patch.
> 
> Look, I don't feel my patch needs such "fix". So if you think it does,
> you have to do it.

Eventually, I'm the one who's going to maintain the code, most
of the drivers in the video directory are practically abandoned. 
Additionally, this is mentioned in Documentation/CodingStyle.

2) #ifdefs are ugly

Code cluttered with ifdefs is difficult to read and maintain.  Don't do
it.  Instead, put your ifdefs in a header, and conditionally define
'static inline' functions, or macros, which are used in the code.
Let the compiler optimize away the "no-op" case.

Simple example, of poor code:

	dev = alloc_etherdev (sizeof(struct funky_private));
	if (!dev)
		return -ENODEV;
	#ifdef CONFIG_NET_FUNKINESS
	init_funky_net(dev);
	#endif

> 
>> And while your at it, check your Kconfig
>> dependencies, ie check for impossible combinations such as CONFIG_I2C=m,
>> CONFIG_FB_CIRRUS=y.
> 
> You're right here, I don't know why I assumed DEPENDS does it
> automatically.

Use select.

Tony
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/