Re: [RESUBMIT][PATCH][RFC] OMAP4: I2C Support for OMAP4430

[Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>]

On Tue, Jul 21, 2009 at 08:49:46PM +0530, Syed Rafiuddin wrote:
> This patch adds OMAP4 support to the I2C driver. All I2C register addresses
> are different between OMAP1/2/3 and OMAP4. In order to not have #ifdef's at
> various places in code, as well as to support multi-OMAP build, Array's are
> created to hold the register addresses.

Hmm, some comments follow.

> @@ -156,6 +162,12 @@
>  #define SYSC_IDLEMODE_SMART		0x2
>  #define SYSC_CLOCKACTIVITY_FCLK		0x2
> 
> +#define maxvalue(x, y)	(x > y ? x : y)

We have a max() and max_t() functions in the kernel which are both
typesafe.  Please don't reintroduce the above buggy construct.

> +
> +struct reg_type {
> +	u32 reg;
> +	u32 offset;
> +};

I'm not sure what use 'reg' is here - since it's always identical to
the index into the array.  Just make this an array.

> 
>  struct omap_i2c_dev {
>  	struct device		*dev;
> @@ -165,6 +177,7 @@ struct omap_i2c_dev {
>  	struct clk		*fclk;		/* Functional clock */
>  	struct completion	cmd_complete;
>  	struct resource		*ioarea;
> +	struct reg_type		*regs;

This could be const...

>  	u32			speed;		/* Speed of bus in Khz */
>  	u16			cmd_err;
>  	u8			*buf;
> @@ -180,15 +193,61 @@ struct omap_i2c_dev {
>  	u16			iestate;	/* Saved interrupt register */
>  };
> 
> +static struct  __initdata reg_type reg_map[] = {
> +	{OMAP_I2C_REV_REG, 0x00},
> +	{OMAP_I2C_IE_REG, 0x04},
> +	{OMAP_I2C_STAT_REG, 0x08},
> +	{OMAP_I2C_IV_REG, 0x0c},
> +	{OMAP_I2C_WE_REG, 0x0c},
> +	{OMAP_I2C_SYSS_REG, 0x10},
> +	{OMAP_I2C_BUF_REG, 0x14},
> +	{OMAP_I2C_CNT_REG, 0x18},
> +	{OMAP_I2C_DATA_REG, 0x1c},
> +	{OMAP_I2C_SYSC_REG, 0x20},
> +	{OMAP_I2C_CON_REG, 0x24},
> +	{OMAP_I2C_OA_REG, 0x28},
> +	{OMAP_I2C_SA_REG, 0x2c},
> +	{OMAP_I2C_PSC_REG, 0x30},
> +	{OMAP_I2C_SCLL_REG, 0x34},
> +	{OMAP_I2C_SCLH_REG, 0x38},
> +	{OMAP_I2C_SYSTEST_REG, 0x3C},
> +	{OMAP_I2C_BUFSTAT_REG, 0x40},
> +};
> +
> +static struct  __initdata reg_type omap4_reg_map[] = {
> +	{OMAP_I2C_REV_REG, 0x04},
> +	{OMAP_I2C_IE_REG, 0x2c},
> +	{OMAP_I2C_STAT_REG, 0x28},
> +	{OMAP_I2C_IV_REG, 0x34},
> +	{OMAP_I2C_WE_REG, 0x34},
> +	{OMAP_I2C_SYSS_REG, 0x90},
> +	{OMAP_I2C_BUF_REG, 0x94},
> +	{OMAP_I2C_CNT_REG, 0x98},
> +	{OMAP_I2C_DATA_REG, 0x9c},
> +	{OMAP_I2C_SYSC_REG, 0x20},
> +	{OMAP_I2C_CON_REG, 0xa4},
> +	{OMAP_I2C_OA_REG, 0xa8},
> +	{OMAP_I2C_SA_REG, 0xac},
> +	{OMAP_I2C_PSC_REG, 0xb0},
> +	{OMAP_I2C_SCLL_REG, 0xb4},
> +	{OMAP_I2C_SCLH_REG, 0xb8},
> +	{OMAP_I2C_SYSTEST_REG, 0xbC},
> +	{OMAP_I2C_BUFSTAT_REG, 0xc0},
> +	{OMAP_I2C_REVNB_LO, 0x00},
> +	{OMAP_I2C_IRQSTATUS_RAW, 0x24},
> +	{OMAP_I2C_IRQENABLE_SET, 0x2c},
> +	{OMAP_I2C_IRQENABLE_CLR, 0x30},
> +};

These arrays could be of a well defined size (enough to hold all enum
values).  They're not going to be huge, and I suspect that the cost of
this code:

> +	if (dev->regs == NULL) {
> +		dev->regs = kmalloc(maxvalue(sizeof(omap4_reg_map),
> +				sizeof(reg_map)), GFP_KERNEL);
> +		if (dev->regs == NULL) {
> +			r = -ENOMEM;
> +			goto err_free_mem;
> +		}
> +	}
> +
> +	if (cpu_is_omap44xx())
> +		memcpy(dev->regs, omap4_reg_map, sizeof(omap4_reg_map));
> +	else
> +		memcpy(dev->regs, reg_map, sizeof(reg_map));
> +

is higher than just having the above be const arrays of 'u8' or maybe
'u16'.

Note that you can explicitly initialize array entries as follows:

static u8 omap4_reg_map[OMAP_I2C_MAX_REG] = {
	[OMAP_I2C_REV_REG] = 0x04,
	[OMAP_I2C_IE_REG]  = 0x2c,
...
};