lkml.org 
[lkml]   [2017]   [Jul]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCHv4 3/3] i2c: altera: Add Altera I2C Controller driver
On Mon, Jun 19, 2017 at 11:36 PM,  <thor.thayer@linux.intel.com> wrote:

Either...
> +#include <linux/init.h>

...or...
> +#include <linux/module.h>

...choose one.

> +#define ALTR_I2C_THRESHOLD 0 /*IRQ Threshold at 1 element */

Space missed.

> +/**
> + * altr_i2c_empty_rx_fifo - Fetch data from RX FIFO until end of
> + * transfer. Send a Stop bit on the last byte.
> + */
> +static void altr_i2c_empty_rx_fifo(struct altr_i2c_dev *idev)
> +{
> + size_t rx_fifo_avail = readl(idev->base + ALTR_I2C_RX_FIFO_LVL);
> + int bytes_to_transfer = min(rx_fifo_avail, idev->msg_len);
> +

> + while (bytes_to_transfer-- > 0) {
> + *idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
> + if (idev->msg_len == 1)
> + altr_i2c_stop(idev);
> + else
> + writel(0, idev->base + ALTR_I2C_TFR_CMD);
> +
> + idev->msg_len--;
> + }

Move out invariant from the loop (and I see a bug, you might go over
boundaries).

while (bytes_to_transfer-- > 0) {
*idev->buf++ = readl(idev->base + ALTR_I2C_RX_DATA);
if (idev->msg_len-- == 1)
break;
writel(0, idev->base + ALTR_I2C_TFR_CMD);
}

altr_i2c_stop(idev);

> +}
> +
> +/**
> + * altr_i2c_fill_tx_fifo - Fill TX FIFO from current message buffer.
> + * @return: Number of bytes left to transfer.
> + */
> +static int altr_i2c_fill_tx_fifo(struct altr_i2c_dev *idev)
> +{
> + size_t tx_fifo_avail = idev->fifo_size - readl(idev->base +
> + ALTR_I2C_TC_FIFO_LVL);
> + int bytes_to_transfer = min(tx_fifo_avail, idev->msg_len);
> + int ret = idev->msg_len - bytes_to_transfer;
> +
> + while (bytes_to_transfer-- > 0) {
> + if (idev->msg_len == 1)
> + writel(ALTR_I2C_TFR_CMD_STO | *idev->buf++,
> + idev->base + ALTR_I2C_TFR_CMD);
> + else
> + writel(*idev->buf++, idev->base + ALTR_I2C_TFR_CMD);
> + idev->msg_len--;
> + }

Ditto.

> +
> + return ret;
> +}

> +/**
> + * altr_i2c_wait_for_core_idle - After TX, check core idle for all bytes TX.
> + * @return: 0 on success or -ETIMEDOUT on timeout.
> + */
> +static int altr_i2c_wait_for_core_idle(struct altr_i2c_dev *idev)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(ALTR_I2C_TIMEOUT);
> +
> + do {
> + if (time_after(jiffies, timeout)) {
> + dev_err(idev->dev, "Core Idle timeout\n");
> + return -ETIMEDOUT;
> + }
> + } while (readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE);
> +
> + return 0;
> +}

readl_poll_timeout[_atomic]() please.

> +static irqreturn_t altr_i2c_isr(int irq, void *_dev)
> +{
> + struct altr_i2c_dev *idev = _dev;

> + /* Read IRQ status but only interested in Enabled IRQs. */
> + u32 status = readl(idev->base + ALTR_I2C_ISR) &
> + readl(idev->base + ALTR_I2C_ISER);

Don't you have cached value for mask?

> +}

> +
> +static int altr_i2c_xfer_msg(struct altr_i2c_dev *idev, struct i2c_msg *msg)
> +{
> + u32 int_mask = ALTR_I2C_ISR_RXOF | ALTR_I2C_ISR_ARB | ALTR_I2C_ISR_NACK;

> + u32 addr = (msg->addr << 1) & 0xFF;

i2c_8bit_addr_from_msg() ?

> + unsigned long time_left;
> +

> + if (i2c_m_rd(msg)) {
> + /* Dummy read to ensure RX FIFO is empty */
> + readl(idev->base + ALTR_I2C_RX_DATA);
> + addr |= ALTR_I2C_TFR_CMD_RW_D;
> + }
> +
> + writel(ALTR_I2C_TFR_CMD_STA | addr, idev->base + ALTR_I2C_TFR_CMD);
> +
> + if (i2c_m_rd(msg)) {
> + /* write the first byte to start the RX */
> + writel(0, idev->base + ALTR_I2C_TFR_CMD);
> + int_mask |= ALTR_I2C_ISER_RXOF_EN | ALTR_I2C_ISER_RXRDY_EN;
> + } else {
> + altr_i2c_fill_tx_fifo(idev);
> + int_mask |= ALTR_I2C_ISR_TXRDY;
> + }

It's hard to follow. Perhaps

if (read) {
...do for read...
} else {
...do for write...
}

> + if (readl(idev->base + ALTR_I2C_STATUS) & ALTR_I2C_STAT_CORE)
> + dev_err(idev->dev, "%s: Core Status not IDLE...\n", __func__);

Better to

val = readl();
if (val)
...

> +static int
> +altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{

Why [] ?!

> + struct altr_i2c_dev *idev = i2c_get_adapdata(adap);

> + int i;
> + int ret = 0;
> +
> + for (i = 0; ret == 0 && i < num; ++i)
> + ret = altr_i2c_xfer_msg(idev, &msgs[i]);
> +
> + return ret ? : i;

Oy vey...
Perhaps

static int altr_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
*msgs, int num)
{
struct altr_i2c_dev *idev = i2c_get_adapdata(adap);
int ret;

while (num--) {
ret = altr_i2c_xfer_msg(idev, msgs++);
if (ret)
return ret;
}
return 0;
}

> +static u32 altr_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C;
> +}

Useless. Use value in place.

> +static int altr_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct altr_i2c_dev *idev = NULL;
> + struct resource *res;
> + int irq;

> + int ret = 0;

Redundant assignment.

> + if (of_property_read_u32(np, "fifo-size", &idev->fifo_size))
> + idev->fifo_size = ALTR_I2C_DFLT_FIFO_SZ;

Shouldn't be possible to auto detect?

> + if (of_property_read_u32(np, "clock-frequency", &idev->bus_clk_rate)) {

device_property_*() ?

> + dev_err(&pdev->dev, "Default to 100kHz\n");
> + idev->bus_clk_rate = 100000; /* default clock rate */
> + }

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2017-07-07 18:27    [W:0.080 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site