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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201111231618.45951.arnd@arndb.de>
Date:	Wed, 23 Nov 2011 16:18:45 +0000
From:	Arnd Bergmann <arnd@...db.de>
To:	Nikolaus Voss <n.voss@...nmann.de>
Cc:	linux-i2c@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, ben-linux@...ff.org,
	carsten.behling@...z-fricke.com
Subject: Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/