On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian
<kramasub@xxxxxxxxxxxxxx> wrote:
> This bus driver supports the GENI based i2c hardware controller in the
> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
> module supporting a wide range of serial interfaces including I2C. The
> driver supports FIFO mode and DMA mode of transfer and switches modes
> dynamically depending on the size of the transfer.
>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@xxxxxxxxxxxxxx>
> Signed-off-by: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
> Signed-off-by: Girish Mahadevan <girishm@xxxxxxxxxxxxxx>
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-qcom-geni.c | 656 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 667 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c
Newbie again, throwing in my two cents.
> +static inline void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c, int dfs)
> +{
> + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map + gi2c->clk_fld_idx;
> +
> + geni_write_reg(dfs, gi2c->base, SE_GENI_CLK_SEL);
> +
> + geni_write_reg((itr->clk_div << 4) | 1, gi2c->base, GENI_SER_M_CLK_CFG);
> + geni_write_reg(((itr->t_high << 20) | (itr->t_low << 10) |
> + itr->t_cycle), gi2c->base, SE_I2C_SCL_COUNTERS);
It would be great to get these register field shifts defined, as
they're otherwise fairly opaque.
> +static irqreturn_t geni_i2c_irq(int irq, void *dev)
> +{
> + struct geni_i2c_dev *gi2c = dev;
> + int i, j;
> + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS);
> + u32 rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS);
> + u32 dm_tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT);
> + u32 dm_rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT);
> + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN);
> + struct i2c_msg *cur = gi2c->cur;
> +
> + if (!cur || (m_stat & M_CMD_FAILURE_EN) ||
> + (dm_rx_st & (DM_I2C_CB_ERR)) ||
> + (m_stat & M_CMD_ABORT_EN)) {
> +
> + if (m_stat & M_GP_IRQ_1_EN)
> + geni_i2c_err(gi2c, I2C_NACK);
> + if (m_stat & M_GP_IRQ_3_EN)
> + geni_i2c_err(gi2c, I2C_BUS_PROTO);
> + if (m_stat & M_GP_IRQ_4_EN)
> + geni_i2c_err(gi2c, I2C_ARB_LOST);
> + if (m_stat & M_CMD_OVERRUN_EN)
> + geni_i2c_err(gi2c, GENI_OVERRUN);
> + if (m_stat & M_ILLEGAL_CMD_EN)
> + geni_i2c_err(gi2c, GENI_ILLEGAL_CMD);
> + if (m_stat & M_CMD_ABORT_EN)
> + geni_i2c_err(gi2c, GENI_ABORT_DONE);
> + if (m_stat & M_GP_IRQ_0_EN)
> + geni_i2c_err(gi2c, GP_IRQ0);
> +
> + if (!dma)
> + writel_relaxed(0, (gi2c->base +
> + SE_GENI_TX_WATERMARK_REG));
The writes to the TX_WATERMARK_REG here and a little further down in
the irq handler stick out to me. A comment as to why poking at the
watermark register is necessary here would be very helpful.
-Evan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
