Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Tuesday 08 November 2011, Nikolaus Voss wrote:


> +
> +static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
> +{
> +	return __raw_readl(dev->base + reg);
> +}
> +
> +static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
> +{
> +	__raw_writel(val, dev->base + reg);
> +}

Better use readl/writel or readl_relaxed/writel_relaxed.

> +/*
> + * Calculate and set symmetric clock as stated in datasheet:
> + * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
> + */
> +static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int twi_clk)
> +{
> +	const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
> +	int ckdiv = fls(div >> 8);
> +	const int cdiv = div >> ckdiv;
> +
> +	if (cpu_is_at91rm9200() && (ckdiv > 5)) {
> +		dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
> +		ckdiv = 5;
> +	}

Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.

> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> new file mode 100644
> index 0000000..a898159
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -0,0 +1,80 @@
> +
> +#ifndef AT91_TWI_H
> +#define AT91_TWI_H
> +

No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c

> +#define	AT91_TWI_MMR		0x04		/* Master Mode Register */
> +#define	AT91_TWI_IADRSZ		(3    <<  8)	/* Internal Device Address
> +						 *  Size */
> +#define	AT91_TWI_IADRSZ_NO	(0 << 8)
> +#define	AT91_TWI_IADRSZ_1	(1 << 8)
> +#define	AT91_TWI_IADRSZ_2	(2 << 8)
> +#define	AT91_TWI_IADRSZ_3	(3 << 8)
> +#define	AT91_TWI_MREAD		(1    << 12)	/* Master Read Direction */
> +#define	AT91_TWI_DADR		(0x7f << 16)	/* Device Address */

These are harder to read than just using hexadecimal bitmasks:

#define	AT91_TWI_MMR		0x00000004
#define	AT91_TWI_IADRSZ		0x00000300
#define	AT91_TWI_IADRSZ_NO	0x00000000
#define	AT91_TWI_IADRSZ_1	0x00000100
...

	Arnd