| 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
| ||
|
Message-ID: <91af7a0d-de36-4839-a52e-3bc64836824a@CH1EHSMHS023.ehs.local>
Date: Tue, 1 Apr 2014 14:24:36 -0700
From: Sören Brinkmann <soren.brinkmann@...inx.com>
To: Wolfram Sang <wsa@...-dreams.de>
CC: 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>,
Rob Landley <rob@...dley.net>,
Russell King <linux@....linux.org.uk>,
Michal Simek <michal.simek@...inx.com>,
Grant Likely <grant.likely@...aro.org>,
Mike Looijmans <mike.looijmans@...ic.nl>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-doc@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-i2c@...r.kernel.org>, Harini Katakam <harinik@...inx.com>
Subject: Re: [PATCH v3 1/2] i2c: Add driver for Cadence I2C controller
On Tue, 2014-04-01 at 02:50AM +0200, Wolfram Sang wrote:
> On Tue, Mar 11, 2014 at 09:50:12AM -0700, Soren Brinkmann wrote:
> > Add a driver for the Cadence I2C controller. This controller is for
> > example found in Xilinx Zynq.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@...inx.com>
>
> ...
>
> > +static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
> > +{
> > + unsigned int isr_status, avail_bytes;
> > + unsigned int bytes_to_recv, bytes_to_send;
> > + struct cdns_i2c *id = ptr;
> > + /* Signal completion only after everything is updated */
> > + int done_flag = 0;
> > +
> > + isr_status = cdns_i2c_readreg(CDNS_I2C_ISR_OFFSET);
> > +
> > + /* Handling nack and arbitration lost interrupt */
> > + if (isr_status & (CDNS_I2C_IXR_NACK | CDNS_I2C_IXR_ARB_LOST))
> > + done_flag = 1;
> > +
> > + /* Handling Data interrupt */
> > + if ((isr_status & CDNS_I2C_IXR_DATA) &&
> > + (id->recv_count >= CDNS_I2C_DATA_INTR_DEPTH)) {
> > + /* Always read data interrupt threshold bytes */
> > + bytes_to_recv = CDNS_I2C_DATA_INTR_DEPTH;
> > + id->recv_count = id->recv_count - CDNS_I2C_DATA_INTR_DEPTH;
>
> Use '-='
>
> ...
>
> > +static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
> > + struct i2c_adapter *adap)
> > +{
> > + int ret;
> > + u32 reg;
> > + bool retry = false;
> > + unsigned retries = adap->retries;
> > +
> > + id->p_msg = msg;
> > + do {
> > + id->err_status = 0;
> > + init_completion(&id->xfer_done);
>
> reinit_completion. And add init_completion in probe.
>
> > +
> > + /* Check for the TEN Bit mode on each msg */
> > + reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
> > + if (msg->flags & I2C_M_TEN) {
> > + if (reg & CDNS_I2C_CR_NEA)
> > + cdns_i2c_writereg(reg & ~CDNS_I2C_CR_NEA,
> > + CDNS_I2C_CR_OFFSET);
> > + } else {
> > + if (!(reg & CDNS_I2C_CR_NEA))
> > + cdns_i2c_writereg(reg | CDNS_I2C_CR_NEA,
> > + CDNS_I2C_CR_OFFSET);
> > + }
> > +
> > + /* Check for the R/W flag on each msg */
> > + if (msg->flags & I2C_M_RD)
> > + cdns_i2c_mrecv(id);
> > + else
> > + cdns_i2c_msend(id);
> > +
> > + /* Wait for the signal of completion */
> > + ret = wait_for_completion_timeout(&id->xfer_done, HZ);
> > + if (!ret) {
> > + cdns_i2c_master_reset(adap);
> > + dev_err(id->adap.dev.parent,
> > + "timeout waiting on completion\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + cdns_i2c_writereg(CDNS_I2C_IXR_ALL_INTR_MASK,
> > + CDNS_I2C_IDR_OFFSET);
> > +
> > + /* If it is bus arbitration error, try again */
> > + if (id->err_status & CDNS_I2C_IXR_ARB_LOST) {
>
> Just return -EAGAIN here...
>
> > + dev_dbg(id->adap.dev.parent,
> > + "Lost ownership on bus, trying again\n");
> > + if (retries--) {
> > + mdelay(2);
> > + retry = true;
> > + } else {
> > + dev_err(id->adap.dev.parent,
> > + "Retries completed, exit\n");
> > + return -EREMOTEIO;
> > + }
> > + }
>
> ... and you can skip this block since the core will do the retries.
>
> > + } while (retry);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * cdns_i2c_master_xfer - The main i2c transfer function
> > + * @adap: pointer to the i2c adapter driver instance
> > + * @msgs: pointer to the i2c message structure
> > + * @num: the number of messages to transfer
> > + *
> > + * Return: number of msgs processed on success, negative error otherwise
> > + *
> > + * This function waits for the bus idle condition and updates the timeout if
> > + * modified by user. Then initiates the send/recv activity based on the
> > + * transfer message received.
> > + */
> > +static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > + int num)
> > +{
> > + struct cdns_i2c *id = adap->algo_data;
> > + unsigned long timeout;
> > + int ret, count;
> > + u32 reg;
> > +
> > + /* Waiting for bus-ready. If bus not ready, it returns after timeout */
> > + timeout = jiffies + CDNS_I2C_TIMEOUT;
> > + while (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
> > + if (time_after(jiffies, timeout)) {
> > + dev_warn(id->adap.dev.parent,
> > + "timedout waiting for bus ready\n");
> > + cdns_i2c_master_reset(adap);
> > + return -ETIMEDOUT;
> > + }
> > + schedule_timeout(1);
> > + }
> > +
> > + /* The bus is free. Set the new timeout value if updated */
> > + if (id->adap.timeout != id->cur_timeout) {
> > + cdns_i2c_writereg(id->adap.timeout & CDNS_I2C_TIME_OUT_TO_MASK,
> > + CDNS_I2C_TIME_OUT_OFFSET);
> > + id->cur_timeout = id->adap.timeout;
> > + }
> > +
> > + /*
> > + * Set the flag to one when multiple messages are to be
> > + * processed with a repeated start.
> > + */
> > + if (num > 1) {
> > + id->bus_hold_flag = 1;
> > + reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
> > + reg |= CDNS_I2C_CR_HOLD;
> > + cdns_i2c_writereg(reg, CDNS_I2C_CR_OFFSET);
> > + } else {
> > + id->bus_hold_flag = 0;
> > + }
> > +
> > + /* Process the msg one by one */
> > + for (count = 0; count < num; count++, msgs++) {
> > + if (count == (num - 1))
> > + id->bus_hold_flag = 0;
> > +
> > + ret = cdns_i2c_process_msg(id, msgs, adap);
> > + if (ret)
> > + return ret;
> > +
> > + /* Report the other error interrupts to application as EIO */
> > + if (id->err_status & 0xE4) {
>
> Please replace this magic hex value with something readable.
>
> > + cdns_i2c_master_reset(adap);
> > + return -EIO;
> > + }
> > + }
> > +
> > + return num;
> > +}
> > +
>
> ...
>
> > +/**
> > + * cdns_i2c_probe - Platform registration call
> > + * @pdev: Handle to the platform device structure
> > + *
> > + * Return: 0 on success, negative error otherwise
> > + *
> > + * This function does all the memory allocation and registration for the i2c
> > + * device. User can modify the address mode to 10 bit address mode using the
> > + * ioctl call with option I2C_TENBIT.
> > + */
> > +static int cdns_i2c_probe(struct platform_device *pdev)
> > +{
> > + struct resource *r_mem;
> > + struct cdns_i2c *id;
> > + int ret;
> > +
> > + id = devm_kzalloc(&pdev->dev, sizeof(*id), GFP_KERNEL);
> > + if (!id)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, id);
> > +
> > + r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + id->membase = devm_ioremap_resource(&pdev->dev, r_mem);
> > + if (IS_ERR(id->membase))
> > + return PTR_ERR(id->membase);
> > +
> > + id->irq = platform_get_irq(pdev, 0);
> > +
> > + id->adap.nr = pdev->id;
>
> Drop this line and use i2c_add_adapter() later to let the core handle
> the numbering. This will also take aliases into account.
>
> > + id->adap.dev.of_node = pdev->dev.of_node;
> > + id->adap.algo = &cdns_i2c_algo;
> > + id->adap.timeout = 0x1F; /* Default timeout value */
>
> This value is in jiffies, so you probably want to use msecs_to_jiffies
> or alike.
>
> > + id->adap.retries = 3; /* Default retry value. */
> > + id->adap.algo_data = id;
> > + id->adap.dev.parent = &pdev->dev;
> > + snprintf(id->adap.name, sizeof(id->adap.name),
> > + "Cadence I2C at %08lx", (unsigned long)r_mem->start);
> > +
> > + id->cur_timeout = id->adap.timeout;
> > + id->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(id->clk)) {
> > + dev_err(&pdev->dev, "input clock not found.\n");
> > + return PTR_ERR(id->clk);
> > + }
> > + ret = clk_prepare_enable(id->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Unable to enable clock.\n");
> > + return ret;
> > + }
> > + id->clk_rate_change_nb.notifier_call = cdns_i2c_clk_notifier_cb;
> > + if (clk_notifier_register(id->clk, &id->clk_rate_change_nb))
> > + dev_warn(&pdev->dev, "Unable to register clock notifier.\n");
> > + id->input_clk = clk_get_rate(id->clk);
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> > + &id->i2c_clk);
> > + if (ret || (id->i2c_clk > CDNS_I2C_SPEED_MAX))
> > + id->i2c_clk = CDNS_I2C_SPEED_MAX;
> > +
> > + cdns_i2c_writereg(0xE, CDNS_I2C_CR_OFFSET);
>
> Remove magic value. Please check the driver for more, in case I missed
> some.
Thanks for reviewing. I'll work on the next iteration.
Sören
--
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