Re: [PATCHv4 3/3] i2c: altera: Add Altera I2C Controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Andy,

On 07/07/2017 11:25 AM, Andy Shevchenko wrote:
On Mon, Jun 19, 2017 at 11:36 PM,  <thor.thayer@xxxxxxxxxxxxxxx> 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.

Got it. Thanks!

+/**
+ * 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);

I see your point on the boundary. However your change is slightly different from what I'm trying to do.

I think you assumed the alt_i2c_stop() call can cause a stop condition. This soft IP can't send just a start or a stop condition by itself - both of these conditions need to be paired with a byte.

The other subtle side effect is the start condition + byte write is the first write which is why the last write is skipped.

I need to send a byte with a stop condition on the last expected byte (idev->msg_len == 1) while this change would send it after the FIFO is empty or after (msg_len == 1).


Your version is cleaner so I'll just add the alt_i2c_stop(idev) call inside the (msg_len == 1) condition and before the break.

+}
+
+/**
+ * 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.

See above but I will move the msg_len-- inside the condition check like you had.

+
+       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?

Not right now, but it may be good to add that to the altr_i2c_dev structure.

+}

+
+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() ?

Nice! I missed that function when writing this but I like it since it also simplifies the code below.

+       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...
}

Will do. This will be much cleaner, especially with the i2c_8bit_addr_from_msg() function you pointed out. Thanks!


+       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)
  ...

Got it. Thanks!

+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;
}

Yes, I just copied this from the axxia driver but I'll clean this up for my re-write.

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

Useless. Use value in place.

Got it. Thanks!


+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?

I agree. That would have been SO much better but the hardware designers released this without capturing the size in a register - they come from a bare-metal project perspective. Since the FIFO size is configurable in the FPGA from 4 to 256 levels deep, I need to capture this with the device tree.

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

device_property_*() ?

OK. This is another function I wasn't aware of but I see the i2c-designware-platdrv.c uses it too.

IIUC, it seems like this falls back to the device tree if the device properties aren't defined.


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

Great comments! Thanks for reviewing!

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux