linux-kernel - Re: [PATCH v8 2/3] i2c: iproc: Add Broadcom iProc I2C 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: <20150207175039.GB6263@katana>
Date:	Sat, 7 Feb 2015 18:50:39 +0100
From:	Wolfram Sang <wsa@...-dreams.de>
To:	Ray Jui <rjui@...adcom.com>
Cc:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Arend van Spriel <arend@...adcom.com>,
	Kevin Cernekee <cernekee@...omium.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Grant Likely <grant.likely@...aro.org>,
	Christian Daudt <bcm@...thebug.org>,
	Matt Porter <mporter@...aro.org>,
	Florian Fainelli <f.fainelli@...il.com>,
	Russell King <linux@....linux.org.uk>,
	Scott Branden <sbranden@...adcom.com>,
	Dmitry Torokhov <dtor@...gle.com>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	bcm-kernel-feedback-list@...adcom.com, devicetree@...r.kernel.org
Subject: Re: [PATCH v8 2/3] i2c: iproc: Add Broadcom iProc I2C Driver

Hi Ray,

On Fri, Feb 06, 2015 at 05:28:26PM -0800, Ray Jui wrote:
> Add initial support to the Broadcom iProc I2C controller found in the
> iProc family of SoCs.
> 
> The iProc I2C controller has separate internal TX and RX FIFOs, each has
> a size of 64 bytes. The iProc I2C controller supports two bus speeds
> including standard mode (100kHz) and fast mode (400kHz)

Mostly looking good.

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>

Please sort the includes.

> +static bool bcm_iproc_i2c_bus_busy(struct bcm_iproc_i2c_dev *iproc_i2c)
> +{
> +	if (readl(iproc_i2c->base + M_CMD_OFFSET) &
> +	    (1 << M_CMD_START_BUSY_SHIFT))
> +		return true;
> +	else
> +		return false;
> +}

Minor: return !!(readl(...))? You decide.

> +
> +static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c,
> +				     struct i2c_msg *msg, u8 *addr)
> +{
> +	*addr = msg->addr << 1;
> +
> +	if (msg->flags & I2C_M_RD)
> +		*addr |= 1;
> +
> +	return 0;
> +}

I'd suggest a oneliner.

*addr = msg->addr << 1 | (msg->flags & I2C_M_RD ? 1 : 0)

Or use !! like above.

Don't do an extra function for that. It is only used once and it also
doesn't need to be int since it can't fail anyhow.

(Note to self: I should make a macro for that in i2c.h)

> +	/* need to reserve one byte in the FIFO for the slave address */
> +	if (msg->len > M_TX_RX_FIFO_SIZE - 1) {
> +		dev_err(iproc_i2c->device,
> +			"only support data length up to %u bytes\n",
> +			M_TX_RX_FIFO_SIZE - 1);
> +		return -EINVAL;

-EOPNOTSUPP

Is it really a HW limitation? Could the driver later be extended to
continue filling the FIFO if a certain threshold is reached?

> +	dev_dbg(iproc_i2c->device, "xfer %c, addr=0x%02x, len=%d\n",
> +		(msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr,
> +		msg->len);
> +	dev_dbg(iproc_i2c->device, "*** data: %*ph\n", msg->len, msg->buf);

Not really needed. We have tracing for that.

> +	if (bus_speed < 100000) {
> +		dev_err(iproc_i2c->device, "%d Hz bus speed not supported\n",
> +			bus_speed);
> +		dev_err(iproc_i2c->device,
> +			"valid speeds are 100khz and 400khz\n");
> +		return -EINVAL;
> +	} else if (bus_speed < 400000) {
> +		bus_speed = 100000;
> +		speed_bit = 0;
> +	} else {
> +		bus_speed = 400000;
> +		speed_bit = 1;
> +	}
> +
> +	val = readl(iproc_i2c->base + TIM_CFG_OFFSET);
> +	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
> +	val |= speed_bit << TIM_CFG_MODE_400_SHIFT;

val |= (bus_speed == 400000) ...

and skip speed_bit? You decide.

> +static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *iproc_i2c)
> +{
> +	u32 val;
> +
> +	val = readl(iproc_i2c->base + CFG_OFFSET);
> +	val |= 1 << CFG_EN_SHIFT;
> +	writel(val, iproc_i2c->base + CFG_OFFSET);
> +}
> +
> +static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *iproc_i2c)
> +{
> +	u32 val;
> +
> +	val = readl(iproc_i2c->base + CFG_OFFSET);
> +	val &= ~(1 << CFG_EN_SHIFT);
> +	writel(val, iproc_i2c->base + CFG_OFFSET);
> +}

Extra functions? They are self explaining and only used once. You
decide.

Rest looks fine, thanks!


Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)