| 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: Thu, 15 Jan 2015 09:41:19 +0100
From: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
To: Ray Jui <rjui@...adcom.com>
Cc: Wolfram Sang <wsa@...-dreams.de>, 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>,
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 v4 2/3] i2c: iproc: Add Broadcom iProc I2C Driver
Hello,
On Wed, Jan 14, 2015 at 02:23:32PM -0800, Ray Jui wrote:
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
some of them are not needed. I tested on amd64 and efm32 and could drop
linux/device.h, linux/sched.h, linux/clk.h. (BTW, I wonder that you
don't need clk handling.)
> +#define TIM_CFG_OFFSET 0x04
> +#define TIME_CFG_MODE_400_SHIFT 31
Is the register name and the bit name prefix really different or is this
a typo?
> +static int __wait_for_bus_idle(struct bcm_iproc_i2c_dev *iproc_i2c)
A bcm_iproc_i2c prefix would be nice here.
> +static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c,
> + struct i2c_msg *msg, u8 *addr)
> +{
> +
> + if (msg->flags & I2C_M_TEN) {
> + dev_err(iproc_i2c->device, "no support for 10-bit address\n");
> + return -EINVAL;
> + }
> +
> + *addr = (msg->addr << 1);
You can also drop the parentheses.
> + switch (val) {
> + case M_CMD_STATUS_SUCCESS:
> + return 0;
> +
> + case M_CMD_STATUS_LOST_ARB:
> + dev_err(iproc_i2c->device, "lost bus arbitration\n");
> + return -EREMOTEIO;
> +
> + case M_CMD_STATUS_NACK_ADDR:
> + dev_err(iproc_i2c->device, "NAK addr:0x%02x\n",
> + iproc_i2c->msg->addr);
> + return -EREMOTEIO;
> +
> + case M_CMD_STATUS_NACK_DATA:
> + dev_err(iproc_i2c->device, "NAK data\n");
> + return -EREMOTEIO;
> +
> + case M_CMD_STATUS_TIMEOUT:
> + dev_err(iproc_i2c->device, "bus timeout\n");
> + return -ETIMEDOUT;
> +
> + default:
> + dev_err(iproc_i2c->device, "unknown error code=%d\n", val);
> + return -EREMOTEIO;
> + }
> +
> + return -EREMOTEIO;
This is not reached.
> +}
> +
> +static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
> + struct i2c_msg *msg)
> +{
> + int ret, i;
> + u8 addr;
> + u32 val;
> + unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC);
> +
> + if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) {
Is the < 1 a hardware or a software limitation? That means your driver
doesn't support I2C_SMBUS_QUICK which is used for example by i2cdetect.
> + dev_err(iproc_i2c->device,
> + "supported data length is 1 - %u bytes\n",
> + M_TX_RX_FIFO_SIZE - 1);
> + return -EINVAL;
> + }
> +
> + iproc_i2c->msg = msg;
Can it happen that iproc_i2c->msg still holds an uncompleted message
here or is this serialized by the core? Wolfram? Either here something
like:
if (iproc_i2c->msg)
return -EBUSY;
and
iproc_i2c->msg = NULL;
when a transfer is completed is needed, or the respective code can be
dropped from other drivers (e.g. i2c-efm32).
On the other hand .msg is only used in bcm_iproc_i2c_check_status() to
give a diagnostic message. Maybe you can drop .msg and instead give it
as an additional parameter to bcm_iproc_i2c_check_status().
> + ret = __wait_for_bus_idle(iproc_i2c);
> + if (ret)
> + return ret;
I would still prefer to have something like:
if (bcm_iproc_i2c_bus_busy())
return -EBUSY;
instead of a tight loop here.
> + ret = bcm_iproc_i2c_format_addr(iproc_i2c, msg, &addr);
> + if (ret)
> + return ret;
> +
> + /* load slave address into the TX FIFO */
> + writel(addr, iproc_i2c->base + M_TX_OFFSET);
> +
> + /* for a write transaction, load data into the TX FIFO */
> + if (!(msg->flags & I2C_M_RD)) {
> + for (i = 0; i < msg->len; i++) {
> + val = msg->buf[i];
> +
> + /* mark the last byte */
> + if (i == msg->len - 1)
> + val |= 1 << M_TX_WR_STATUS_SHIFT;
What happens if you don't mark this last byte? Could this be used to
support transfers bigger than the fifo size?
> + /*
> + * Enable the "start busy" interrupt, which will be triggered after
> + * the transaction is done, i.e., the internal start_busy bit
s/\.,/./ I think
> + * transitions from 1 to 0
s/$/./
> + */
> + writel(1 << IE_M_START_BUSY_SHIFT, iproc_i2c->base + IE_OFFSET);
> +
> + /*
> + * Now we can activate the transfer. For a read operation, specify the
> + * number of bytes to read
s/$/./
> + */
> + val = 1 << M_CMD_START_BUSY_SHIFT;
> + if (msg->flags & I2C_M_RD) {
> + val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
> + (msg->len << M_CMD_RD_CNT_SHIFT);
> + } else {
> + val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
> + }
> + writel(val, iproc_i2c->base + M_CMD_OFFSET);
> +
> + time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left);
When the interrupt fires here after the complete timed out and before
you disable the irq you still throw the result away.
> +
> + /* disable all interrupts */
> + writel(0, iproc_i2c->base + IE_OFFSET);
> +
> + if (!time_left) {
> + dev_err(iproc_i2c->device, "transaction times out\n");
s/times/timed/
> +static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
Note that I2C_FUNC_SMBUS_EMUL includes I2C_FUNC_SMBUS_QUICK, so your
driver claims to support transfers of length 0.
> +static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
> +{
> + unsigned int bus_speed, speed_bit;
> + u32 val;
> + int ret = of_property_read_u32(iproc_i2c->device->of_node,
> + "clock-frequency", &bus_speed);
> + if (ret < 0) {
> + dev_err(iproc_i2c->device,
> + "missing clock-frequency property\n");
> + return -ENODEV;
Is a missing property the only situation where of_property_read_u32
returns an error? Would it be sane to default to 100 kHz?
> +static int bcm_iproc_i2c_remove(struct platform_device *pdev)
> +{
> + struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&iproc_i2c->adapter);
You need to free the irq before i2c_del_adapter.
> + free_irq(iproc_i2c->irq, iproc_i2c);
> + bcm_iproc_i2c_disable(iproc_i2c);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id bcm_iproc_i2c_of_match[] = {
> + {.compatible = "brcm,iproc-i2c",},
Not sure this is specified to be a must, but I'd add spaces after { and
before }.
> + {},
It's a good habit to write this as
{ /* sentinel */ }
without trailing comma.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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