Re: [2/3][PATCH][upstream] TDM Framework

[Timur Tabi <timur@freescale.com>]

Michael Ellerman wrote:
> And the bible, K & R, includes an example of an enum which explicitly
> specifies all its values. It goes on to say "enumeration variables offer
> the chance of [type] checking and so are often better than #defines".

I don't want to beat a dead horse here, but if the driver doesn't do enum
type checking, then it's hard to justify using an enum.  Now you said,
"Yes, if you're going to define an enum you should use it, which this
patch doesn't, but that's just a bug in this patch."  It could be argued
that there's no real place to incorporate enum type checking in this code.

IMHO, this is pointless:

enum mytype {
	VALUE1 = 0x10,
	VALUE2 = 0x20
};
	

int func(void)
{
	if (some-condition)
		return VALUE1;
	else
		return VALUE2;
}

> And the bible, K & R, includes an example of an enum which explicitly
> specifies all its values.

First, K&R is pretty old, and there are a lot of style issues in it that
are no longer in favor.  I don't think anyone would advocate this:

int func(x, y)
int x;
int y;
{
	...
}

Secondly, one of the benefits of an enum (which again, is not actually
being used in this patch), is to be able to specify multiple constants
that are given unique values, but you don't actually care what the values
are, like this:

enum radeon_family {
	CHIP_FAMILY_UNKNOW,
	CHIP_FAMILY_LEGACY,
	CHIP_FAMILY_RADEON,
	CHIP_FAMILY_RV100,
	CHIP_FAMILY_RS100,    /* U1 (IGP320M) or A3 (IGP320)*/
	CHIP_FAMILY_RV200,
	...
};

The code will still work if I swap CHIP_FAMILY_RV100 with
CHIP_FAMILY_RS100.  As long as each value is unique, everything works.

-- 
Timur Tabi
Linux kernel developer at Freescale