| 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
| ||
|
Date: Sat, 7 Feb 2015 21:08:13 -0800
From: Ray Jui <rjui@...adcom.com>
To: Wolfram Sang <wsa@...-dreams.de>
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
On 2/7/2015 9:50 AM, Wolfram Sang wrote:
> 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.
>
Will do.
>> +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.
>
Okay will do that. Will also remove this function since now it becomes
one line and is used only once.
>> +
>> +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)
>
Yes will change. Thanks.
>> + /* 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?
>
Will return -EOPNOTSUPP. This really depends on whether or not we expect
one sequence of START + SLV ADDR + DATA + STOP per i2c message. I can
later extend the driver to refill/re-drain the FIFO for data size >= 64
bytes, if one sequence of SATRT...STOP per message is not a requirement.
>> + 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.
>
Will remove.
>> + 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.
>
Okay, I'll get rid of speed_bit.
>> +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.
In fact I'll keep the function, since it will likely be needed later
when we add suspend/resume support to the driver. But I'll combine the
two functions and make it a single function called
bcm_iproc_i2c_enable_disable.
>
> Rest looks fine, thanks!
>
Thanks for the review!
Ray
--
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/
Powered by blists - more mailing lists