On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
<ard.biesheu...@linaro.org> wrote:
> This is a cleaned up version of the I2C controller driver for
> the Fujitsu F_I2C IP, which was never supported upstream, and
> has now been incorporated into the Socionext SynQuacer SoC.
>

Thanks for an update.
My comments below.

In case you address them, feel free to add

Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>

> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
>  drivers/i2c/busses/Kconfig         |  11 +
>  drivers/i2c/busses/Makefile        |   1 +
>  drivers/i2c/busses/i2c-synquacer.c | 796 ++++++++++++++++++++
>  3 files changed, 808 insertions(+)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index a9805c7cb305..9001dbaeb60a 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -995,6 +995,17 @@ config I2C_SUN6I_P2WI
>           This interface is used to connect to specific PMIC devices (like the
>           AXP221).
>
> +config I2C_SYNQUACER
> +       tristate "Socionext SynQuacer I2C controller"
> +       depends on ARCH_SYNQUACER || COMPILE_TEST

> +       depends on OF || ACPI

Does it make any sense?

> +       help
> +         Say Y here to include support for the I2C controller used in some
> +         Fujitsu and Socionext SoCs.
> +
> +         This driver can also be built as a module. If so, the module
> +         will be called i2c-synquacer.
> +
>  config I2C_TEGRA
>         tristate "NVIDIA Tegra internal I2C controller"
>         depends on ARCH_TEGRA
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 2ce8576540a2..40ab7bfc3458 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_I2C_STM32F4)     += i2c-stm32f4.o
>  obj-$(CONFIG_I2C_STM32F7)      += i2c-stm32f7.o
>  obj-$(CONFIG_I2C_STU300)       += i2c-stu300.o
>  obj-$(CONFIG_I2C_SUN6I_P2WI)   += i2c-sun6i-p2wi.o
> +obj-$(CONFIG_I2C_SYNQUACER)    += i2c-synquacer.o
>  obj-$(CONFIG_I2C_TEGRA)                += i2c-tegra.o
>  obj-$(CONFIG_I2C_TEGRA_BPMP)   += i2c-tegra-bpmp.o
>  obj-$(CONFIG_I2C_UNIPHIER)     += i2c-uniphier.o
> diff --git a/drivers/i2c/busses/i2c-synquacer.c 
> b/drivers/i2c/busses/i2c-synquacer.c
> new file mode 100644
> index 000000000000..25329f65f35f
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-synquacer.c
> @@ -0,0 +1,796 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2012 FUJITSU SEMICONDUCTOR LIMITED
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#define WAIT_PCLK(n, rate)     \
> +       ndelay(DIV_ROUND_UP(DIV_ROUND_UP(1000000000, rate), n) + 10)
> +
> +/* I2C register adress definitions */
> +#define SYNQUACER_I2C_REG_BSR          (0x00 << 2) // Bus Status
> +#define SYNQUACER_I2C_REG_BCR          (0x01 << 2) // Bus Control
> +#define SYNQUACER_I2C_REG_CCR          (0x02 << 2) // Clock Control
> +#define SYNQUACER_I2C_REG_ADR          (0x03 << 2) // Address
> +#define SYNQUACER_I2C_REG_DAR          (0x04 << 2) // Data
> +#define SYNQUACER_I2C_REG_CSR          (0x05 << 2) // Expansion CS
> +#define SYNQUACER_I2C_REG_FSR          (0x06 << 2) // Bus Clock Freq
> +#define SYNQUACER_I2C_REG_BC2R         (0x07 << 2) // Bus Control 2
> +
> +/* I2C register bit definitions */
> +#define SYNQUACER_I2C_BSR_FBT          BIT(0)  // First Byte Transfer
> +#define SYNQUACER_I2C_BSR_GCA          BIT(1)  // General Call Address
> +#define SYNQUACER_I2C_BSR_AAS          BIT(2)  // Address as Slave
> +#define SYNQUACER_I2C_BSR_TRX          BIT(3)  // Transfer/Receive
> +#define SYNQUACER_I2C_BSR_LRB          BIT(4)  // Last Received Bit
> +#define SYNQUACER_I2C_BSR_AL           BIT(5)  // Arbitration Lost
> +#define SYNQUACER_I2C_BSR_RSC          BIT(6)  // Repeated Start Cond.
> +#define SYNQUACER_I2C_BSR_BB           BIT(7)  // Bus Busy
> +
> +#define SYNQUACER_I2C_BCR_INT          BIT(0)  // Interrupt
> +#define SYNQUACER_I2C_BCR_INTE         BIT(1)  // Interrupt Enable
> +#define SYNQUACER_I2C_BCR_GCAA         BIT(2)  // Gen. Call Access Ack.
> +#define SYNQUACER_I2C_BCR_ACK          BIT(3)  // Acknowledge
> +#define SYNQUACER_I2C_BCR_MSS          BIT(4)  // Master Slave Select
> +#define SYNQUACER_I2C_BCR_SCC          BIT(5)  // Start Condition Cont.
> +#define SYNQUACER_I2C_BCR_BEIE         BIT(6)  // Bus Error Int Enable
> +#define SYNQUACER_I2C_BCR_BER          BIT(7)  // Bus Error
> +
> +#define SYNQUACER_I2C_CCR_CS_MASK      (0x1f)  // CCR Clock Period Sel.
> +#define SYNQUACER_I2C_CCR_EN           BIT(5)  // Enable
> +#define SYNQUACER_I2C_CCR_FM           BIT(6)  // Speed Mode Select
> +
> +#define SYNQUACER_I2C_CSR_CS_MASK      (0x3f)  // CSR Clock Period Sel.
> +
> +#define SYNQUACER_I2C_BC2R_SCLL                BIT(0)  // SCL Low Drive
> +#define SYNQUACER_I2C_BC2R_SDAL                BIT(1)  // SDA Low Drive
> +#define SYNQUACER_I2C_BC2R_SCLS                BIT(4)  // SCL Status
> +#define SYNQUACER_I2C_BC2R_SDAS                BIT(5)  // SDA Status
> +
> +/* PCLK frequency */
> +#define SYNQUACER_I2C_BUS_CLK_FR(rate) (((rate) / 20000000) + 1)
> +
> +/* STANDARD MODE frequency */
> +#define SYNQUACER_I2C_CLK_MASTER_STD(rate)                     \
> +       DIV_ROUND_UP(DIV_ROUND_UP((rate), 100000) - 2, 2)
> +/* FAST MODE frequency */
> +#define SYNQUACER_I2C_CLK_MASTER_FAST(rate)                    \
> +       DIV_ROUND_UP((DIV_ROUND_UP((rate), 400000) - 2) * 2, 3)
> +
> +/* (clkrate <= 18000000) */
> +/* calculate the value of CS bits in CCR register on standard mode */
> +#define SYNQUACER_I2C_CCR_CS_STD_MAX_18M(rate)                 \
> +          ((SYNQUACER_I2C_CLK_MASTER_STD(rate) - 65)           \
> +                                       & SYNQUACER_I2C_CCR_CS_MASK)
> +
> +/* calculate the value of CS bits in CSR register on standard mode */
> +#define SYNQUACER_I2C_CSR_CS_STD_MAX_18M(rate)         0x00
> +
> +/* calculate the value of CS bits in CCR register on fast mode */
> +#define SYNQUACER_I2C_CCR_CS_FAST_MAX_18M(rate)                        \
> +          ((SYNQUACER_I2C_CLK_MASTER_FAST(rate) - 1)           \
> +                                       & SYNQUACER_I2C_CCR_CS_MASK)
> +
> +/* calculate the value of CS bits in CSR register on fast mode */
> +#define SYNQUACER_I2C_CSR_CS_FAST_MAX_18M(rate)                0x00
> +
> +/* (clkrate > 18000000) */
> +/* calculate the value of CS bits in CCR register on standard mode */
> +#define SYNQUACER_I2C_CCR_CS_STD_MIN_18M(rate)                 \
> +          ((SYNQUACER_I2C_CLK_MASTER_STD(rate) - 1)            \
> +                                       & SYNQUACER_I2C_CCR_CS_MASK)
> +
> +/* calculate the value of CS bits in CSR register on standard mode */
> +#define SYNQUACER_I2C_CSR_CS_STD_MIN_18M(rate)                 \
> +          (((SYNQUACER_I2C_CLK_MASTER_STD(rate) - 1) >> 5)     \
> +                                       & SYNQUACER_I2C_CSR_CS_MASK)
> +
> +/* calculate the value of CS bits in CCR register on fast mode */
> +#define SYNQUACER_I2C_CCR_CS_FAST_MIN_18M(rate)                        \
> +          ((SYNQUACER_I2C_CLK_MASTER_FAST(rate) - 1)           \
> +                                       & SYNQUACER_I2C_CCR_CS_MASK)
> +
> +/* calculate the value of CS bits in CSR register on fast mode */
> +#define SYNQUACER_I2C_CSR_CS_FAST_MIN_18M(rate)                        \
> +          (((SYNQUACER_I2C_CLK_MASTER_FAST(rate) - 1) >> 5)    \
> +                                       & SYNQUACER_I2C_CSR_CS_MASK)
> +
> +/* min I2C clock frequency 14M */
> +#define SYNQUACER_I2C_MIN_CLK_RATE     (14 * 1000000)
> +/* max I2C clock frequency 200M */
> +#define SYNQUACER_I2C_MAX_CLK_RATE     (200 * 1000000)
> +/* I2C clock frequency 18M */
> +#define SYNQUACER_I2C_CLK_RATE_18M     (18 * 1000000)
> +
> +#define SYNQUACER_I2C_SPEED_FM         400     // Fast Mode
> +#define SYNQUACER_I2C_SPEED_SM         100     // Standard Mode
> +
> +enum i2c_state {
> +       STATE_IDLE,
> +       STATE_START,
> +       STATE_READ,
> +       STATE_WRITE
> +};
> +
> +struct synquacer_i2c {
> +       struct completion       completion;
> +
> +       struct i2c_msg          *msg;
> +       u32                     msg_num;
> +       u32                     msg_idx;
> +       u32                     msg_ptr;
> +
> +       u32                     irq;
> +       struct device           *dev;
> +       void __iomem            *base;
> +       struct clk              *clk;
> +       u32                     clkrate;
> +       u32                     speed_khz;
> +       u32                     timeout_ms;
> +       enum i2c_state          state;
> +       struct i2c_adapter      adapter;
> +
> +       bool                    is_suspended;
> +};
> +
> +static inline int is_lastmsg(struct synquacer_i2c *i2c)
> +{
> +       return i2c->msg_idx >= (i2c->msg_num - 1);
> +}
> +
> +static inline int is_msglast(struct synquacer_i2c *i2c)
> +{
> +       return i2c->msg_ptr == (i2c->msg->len - 1);
> +}
> +
> +static inline int is_msgend(struct synquacer_i2c *i2c)
> +{
> +       return i2c->msg_ptr >= i2c->msg->len;
> +}
> +
> +static inline unsigned long calc_timeout_ms(struct synquacer_i2c *i2c,
> +                                           struct i2c_msg *msgs,
> +                                           int num)
> +{
> +       unsigned long bit_count = 0;
> +       int i;
> +
> +       for (i = 0; i < num; i++, msgs++)
> +               bit_count += msgs->len;
> +
> +       return DIV_ROUND_UP((bit_count * 9 + 10 * num) * 3, 200) + 10;

When I suggested to drop parens, I also suggested to swap second pair
arguments (b/c I was thinking that parens to prevent confusion + vs
*), like

9 * bit_count + 10 * num, or
bit_count * 9 + num * 10.

Though, it is up to you, I still consider that + vs. * operator
precedence is quite obvious.

> +}
> +
> +static void synquacer_i2c_stop(struct synquacer_i2c *i2c, int ret)
> +{
> +       /*
> +        * clear IRQ (INT=0, BER=0)
> +        * set Stop Condition (MSS=0)
> +        * Interrupt Disable
> +        */
> +       writeb(0, i2c->base + SYNQUACER_I2C_REG_BCR);
> +
> +       i2c->state = STATE_IDLE;
> +
> +       i2c->msg_ptr = 0;
> +       i2c->msg = NULL;
> +       i2c->msg_idx++;
> +       i2c->msg_num = 0;
> +       if (ret)
> +               i2c->msg_idx = ret;
> +
> +       complete(&i2c->completion);
> +}
> +
> +static void synquacer_i2c_hw_init(struct synquacer_i2c *i2c)
> +{
> +       unsigned char ccr_cs, csr_cs;
> +       u32 rt = i2c->clkrate;
> +
> +       /* Set own Address */
> +       writeb(0, i2c->base + SYNQUACER_I2C_REG_ADR);
> +
> +       /* Set PCLK frequency */
> +       writeb(SYNQUACER_I2C_BUS_CLK_FR(i2c->clkrate),
> +              i2c->base + SYNQUACER_I2C_REG_FSR);
> +
> +       switch (i2c->speed_khz) {
> +       case SYNQUACER_I2C_SPEED_FM:
> +               if (i2c->clkrate <= SYNQUACER_I2C_CLK_RATE_18M) {
> +                       ccr_cs = SYNQUACER_I2C_CCR_CS_FAST_MAX_18M(rt);
> +                       csr_cs = SYNQUACER_I2C_CSR_CS_FAST_MAX_18M(rt);
> +               } else {
> +                       ccr_cs = SYNQUACER_I2C_CCR_CS_FAST_MIN_18M(rt);
> +                       csr_cs = SYNQUACER_I2C_CSR_CS_FAST_MIN_18M(rt);
> +               }
> +
> +               /* Set Clock and enable, Set fast mode */
> +               writeb(ccr_cs | SYNQUACER_I2C_CCR_FM |
> +                      SYNQUACER_I2C_CCR_EN,
> +                      i2c->base + SYNQUACER_I2C_REG_CCR);
> +               writeb(csr_cs, i2c->base + SYNQUACER_I2C_REG_CSR);
> +               break;
> +       case SYNQUACER_I2C_SPEED_SM:
> +               if (i2c->clkrate <= SYNQUACER_I2C_CLK_RATE_18M) {
> +                       ccr_cs = SYNQUACER_I2C_CCR_CS_STD_MAX_18M(rt);
> +                       csr_cs = SYNQUACER_I2C_CSR_CS_STD_MAX_18M(rt);
> +               } else {
> +                       ccr_cs = SYNQUACER_I2C_CCR_CS_STD_MIN_18M(rt);
> +                       csr_cs = SYNQUACER_I2C_CSR_CS_STD_MIN_18M(rt);
> +               }
> +
> +               /* Set Clock and enable, Set standard mode */
> +               writeb(ccr_cs | SYNQUACER_I2C_CCR_EN,
> +                     i2c->base + SYNQUACER_I2C_REG_CCR);
> +               writeb(csr_cs, i2c->base + SYNQUACER_I2C_REG_CSR);
> +               break;
> +       default:
> +               WARN_ON(1);
> +       }
> +
> +       /* clear IRQ (INT=0, BER=0), Interrupt Disable */
> +       writeb(0, i2c->base + SYNQUACER_I2C_REG_BCR);
> +       writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
> +}
> +
> +static void synquacer_i2c_hw_reset(struct synquacer_i2c *i2c)
> +{
> +       /* Disable clock */
> +       writeb(0, i2c->base + SYNQUACER_I2C_REG_CCR);
> +       writeb(0, i2c->base + SYNQUACER_I2C_REG_CSR);
> +
> +       WAIT_PCLK(100, i2c->clkrate);
> +
> +       synquacer_i2c_hw_init(i2c);
> +}
> +
> +static int synquacer_i2c_master_start(struct synquacer_i2c *i2c,
> +                                     struct i2c_msg *pmsg)
> +{
> +       unsigned char bsr, bcr;
> +
> +       if (pmsg->flags & I2C_M_RD)
> +               writeb((pmsg->addr << 1) | 1,
> +                      i2c->base + SYNQUACER_I2C_REG_DAR);
> +       else
> +               writeb(pmsg->addr << 1,
> +                      i2c->base + SYNQUACER_I2C_REG_DAR);
> +
> +       dev_dbg(i2c->dev, "slave:0x%02x\n", pmsg->addr);
> +
> +       /* Generate Start Condition */
> +       bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> +       bcr = readb(i2c->base + SYNQUACER_I2C_REG_BCR);
> +       dev_dbg(i2c->dev, "bsr:0x%02x, bcr:0x%02x\n", bsr, bcr);
> +
> +       if ((bsr & SYNQUACER_I2C_BSR_BB) &&
> +           !(bcr & SYNQUACER_I2C_BCR_MSS)) {
> +               dev_dbg(i2c->dev, "bus is busy");
> +               return -EBUSY;
> +       }
> +
> +       if (bsr & SYNQUACER_I2C_BSR_BB) { /* Bus is busy */
> +               dev_dbg(i2c->dev, "Continuous Start");
> +               writeb(bcr | SYNQUACER_I2C_BCR_SCC,
> +                      i2c->base + SYNQUACER_I2C_REG_BCR);
> +       } else {
> +               if (bcr & SYNQUACER_I2C_BCR_MSS) {
> +                       dev_dbg(i2c->dev, "not in master mode");
> +                       return -EAGAIN;
> +               }
> +               dev_dbg(i2c->dev, "Start Condition");
> +               /* Start Condition + Enable Interrupts */
> +               writeb(bcr | SYNQUACER_I2C_BCR_MSS |
> +                      SYNQUACER_I2C_BCR_INTE | SYNQUACER_I2C_BCR_BEIE,
> +                      i2c->base + SYNQUACER_I2C_REG_BCR);
> +       }
> +
> +       WAIT_PCLK(10, i2c->clkrate);
> +
> +       /* get bsr&bcr register */
> +       bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> +       bcr = readb(i2c->base + SYNQUACER_I2C_REG_BCR);
> +       dev_dbg(i2c->dev, "bsr:0x%02x, bcr:0x%02x\n", bsr, bcr);
> +
> +       if ((bsr & SYNQUACER_I2C_BSR_AL) ||
> +           !(bcr & SYNQUACER_I2C_BCR_MSS)) {
> +               dev_dbg(i2c->dev, "arbitration lost\n");
> +               return -EAGAIN;
> +       }
> +
> +       return 0;
> +}
> +
> +static int synquacer_i2c_master_recover(struct synquacer_i2c *i2c)
> +{
> +       unsigned int count = 0;
> +       unsigned char bc2r;
> +
> +       /* Disable interrupts */
> +       writeb(0, i2c->base + SYNQUACER_I2C_REG_BCR);
> +
> +       /* monitor SDA, SCL */
> +       bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
> +       dev_dbg(i2c->dev, "bc2r:0x%02x\n", bc2r);
> +

> +       while (count <= 100) {
> +               WAIT_PCLK(20, i2c->clkrate);
> +               bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
> +
> +               /* another master is running */
> +               if ((bc2r & SYNQUACER_I2C_BC2R_SDAS) ||
> +                   !(bc2r & SYNQUACER_I2C_BC2R_SCLS)) {
> +                       dev_dbg(i2c->dev, "another master is running?\n");
> +                       return -EAGAIN;
> +               }
> +               count++;
> +       }

I still consider do {} while not only taking less LOCs, but more
readable and understanble

unsigned int count = 100;

do {
...
} while (--count);

> +
> +       /* Force to make one clock pulse */

> +       for (count = 1;; count++) {
> +               /* SCL = L->H */
> +               writeb(SYNQUACER_I2C_BC2R_SCLL,
> +                      i2c->base + SYNQUACER_I2C_REG_BC2R);
> +               WAIT_PCLK(20, i2c->clkrate);
> +               writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
> +
> +               WAIT_PCLK(10, i2c->clkrate);
> +
> +               bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
> +
> +               WAIT_PCLK(5, i2c->clkrate);
> +
> +               if (bc2r & SYNQUACER_I2C_BC2R_SDAS)
> +                       break;
> +               WAIT_PCLK(10, i2c->clkrate);

> +               if (count > 9) {
> +                       dev_err(i2c->dev, "count: %i, bc2r: 0x%x\n", count,
> +                               bc2r);
> +                       return -EIO;
> +               }

(1)

> +       }

Ditto. More over in case (1) it's an invariant to the loop, so, it
shouldn't be there in any case.

> +
> +       /* force to make bus-error phase */
> +       /* SDA = L */
> +       writeb(SYNQUACER_I2C_BC2R_SDAL,
> +              i2c->base + SYNQUACER_I2C_REG_BC2R);
> +       WAIT_PCLK(10, i2c->clkrate);
> +       /* SDA = H */
> +       writeb(0, i2c->base + SYNQUACER_I2C_REG_BC2R);
> +       WAIT_PCLK(10, i2c->clkrate);
> +
> +       /* Both SDA & SDL should be H */
> +       bc2r = readb(i2c->base + SYNQUACER_I2C_REG_BC2R);
> +       if ((bc2r & (SYNQUACER_I2C_BC2R_SDAS | SYNQUACER_I2C_BC2R_SCLS))
> +           != (SYNQUACER_I2C_BC2R_SDAS | SYNQUACER_I2C_BC2R_SCLS)) {
> +               dev_err(i2c->dev, "bc2r: 0x%x\n", bc2r);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
> +                               struct i2c_msg *msgs, int num)
> +{
> +       unsigned char bsr;
> +       unsigned long timeout, bb_timeout;
> +       int ret;
> +
> +       if (i2c->is_suspended)
> +               return -EBUSY;
> +
> +       synquacer_i2c_hw_init(i2c);
> +       bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> +       if (bsr & SYNQUACER_I2C_BSR_BB) {
> +               dev_err(i2c->dev, "cannot get bus (bus busy)\n");
> +               return -EBUSY;
> +       }
> +
> +       init_completion(&i2c->completion);
> +
> +       i2c->msg = msgs;
> +       i2c->msg_num = num;
> +       i2c->msg_ptr = 0;
> +       i2c->msg_idx = 0;
> +       i2c->state = STATE_START;
> +
> +       ret = synquacer_i2c_master_start(i2c, i2c->msg);
> +       if (ret < 0) {
> +               dev_dbg(i2c->dev, "Address failed: (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       timeout = wait_for_completion_timeout(&i2c->completion,
> +                                       msecs_to_jiffies(i2c->timeout_ms));
> +       if (timeout <= 0) {
> +               dev_dbg(i2c->dev, "timeout\n");
> +               return -EAGAIN;
> +       }
> +
> +       ret = i2c->msg_idx;
> +       if (ret != num) {
> +               dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);
> +               return -EAGAIN;
> +       }
> +
> +       /* ensure the stop has been through the bus */
> +       bb_timeout = jiffies + HZ;
> +       do {
> +               bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> +       } while ((bsr & SYNQUACER_I2C_BSR_BB) &&
> +                time_before(jiffies, bb_timeout));

I would rather do split timeout exit path with main condition, i.e.

bsr = ...
if (bsr & ...)
 break; // return 0; ?
} while (...);

return 0; // return -ETIMEDOUT; ?

> +

> +       return ret;

return 0; ?

> +}
> +
> +static irqreturn_t synquacer_i2c_isr(int irq, void *dev_id)
> +{
> +       struct synquacer_i2c *i2c = dev_id;
> +
> +       unsigned char byte;
> +       unsigned char bsr, bcr;
> +       int ret = 0;
> +
> +       bcr = readb(i2c->base + SYNQUACER_I2C_REG_BCR);
> +       bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
> +       dev_dbg(i2c->dev, "bsr:0x%02x, bcr:0x%02x\n", bsr, bcr);
> +
> +       if (bcr & SYNQUACER_I2C_BCR_BER) {
> +               dev_err(i2c->dev, "bus error\n");
> +               synquacer_i2c_stop(i2c, -EAGAIN);
> +               goto out;
> +       }
> +       if ((bsr & SYNQUACER_I2C_BSR_AL) ||
> +           !(bcr & SYNQUACER_I2C_BCR_MSS)) {
> +               dev_dbg(i2c->dev, "arbitration lost\n");
> +               synquacer_i2c_stop(i2c, -EAGAIN);
> +               goto out;
> +       }
> +
> +       switch (i2c->state) {
> +
> +       case STATE_START:
> +               if (bsr & SYNQUACER_I2C_BSR_LRB) {
> +                       dev_dbg(i2c->dev, "ack was not received\n");
> +                       synquacer_i2c_stop(i2c, -EAGAIN);
> +                       goto out;
> +               }
> +
> +               if (i2c->msg->flags & I2C_M_RD)
> +                       i2c->state = STATE_READ;
> +               else
> +                       i2c->state = STATE_WRITE;
> +
> +               if (is_lastmsg(i2c) && i2c->msg->len == 0) {
> +                       synquacer_i2c_stop(i2c, 0);
> +                       goto out;
> +               }
> +
> +               if (i2c->state == STATE_READ)
> +                       goto prepare_read;
> +
> +               /* fallthru */
> +
> +       case STATE_WRITE:
> +               if (bsr & SYNQUACER_I2C_BSR_LRB) {
> +                       dev_dbg(i2c->dev, "WRITE: No Ack\n");
> +                       synquacer_i2c_stop(i2c, -EAGAIN);
> +                       goto out;
> +               }
> +
> +               if (!is_msgend(i2c)) {
> +                       writeb(i2c->msg->buf[i2c->msg_ptr++],
> +                              i2c->base + SYNQUACER_I2C_REG_DAR);
> +
> +                       /* clear IRQ, and continue */
> +                       writeb(SYNQUACER_I2C_BCR_BEIE |
> +                              SYNQUACER_I2C_BCR_MSS |
> +                              SYNQUACER_I2C_BCR_INTE,
> +                              i2c->base + SYNQUACER_I2C_REG_BCR);
> +                       break;
> +               }
> +               if (is_lastmsg(i2c)) {
> +                       synquacer_i2c_stop(i2c, 0);
> +                       break;
> +               }
> +               dev_dbg(i2c->dev, "WRITE: Next Message\n");
> +
> +               i2c->msg_ptr = 0;
> +               i2c->msg_idx++;
> +               i2c->msg++;
> +
> +               /* send the new start */
> +               ret = synquacer_i2c_master_start(i2c, i2c->msg);
> +               if (ret < 0) {
> +                       dev_dbg(i2c->dev, "restart error (%d)\n", ret);
> +                       synquacer_i2c_stop(i2c, -EAGAIN);
> +                       break;
> +               }
> +               i2c->state = STATE_START;
> +               break;
> +
> +       case STATE_READ:
> +               if (!(bsr & SYNQUACER_I2C_BSR_FBT)) { /* data */
> +                       byte = readb(i2c->base + SYNQUACER_I2C_REG_DAR);
> +                       i2c->msg->buf[i2c->msg_ptr++] = byte;
> +               } else /* address */
> +                       dev_dbg(i2c->dev, "address:0x%02x. ignore it.\n",
> +                               readb(i2c->base + SYNQUACER_I2C_REG_DAR));
> +
> +prepare_read:
> +               if (is_msglast(i2c)) {
> +                       writeb(SYNQUACER_I2C_BCR_MSS |
> +                              SYNQUACER_I2C_BCR_BEIE |
> +                              SYNQUACER_I2C_BCR_INTE,
> +                              i2c->base + SYNQUACER_I2C_REG_BCR);
> +                       break;
> +               }
> +               if (!is_msgend(i2c)) {
> +                       writeb(SYNQUACER_I2C_BCR_MSS |
> +                              SYNQUACER_I2C_BCR_BEIE |
> +                              SYNQUACER_I2C_BCR_INTE |
> +                              SYNQUACER_I2C_BCR_ACK,
> +                              i2c->base + SYNQUACER_I2C_REG_BCR);
> +                       break;
> +               }
> +               if (is_lastmsg(i2c)) {
> +                       /* last message, send stop and complete */
> +                       dev_dbg(i2c->dev, "READ: Send Stop\n");
> +                       synquacer_i2c_stop(i2c, 0);
> +                       break;
> +               }
> +               dev_dbg(i2c->dev, "READ: Next Transfer\n");
> +
> +               i2c->msg_ptr = 0;
> +               i2c->msg_idx++;
> +               i2c->msg++;
> +
> +               ret = synquacer_i2c_master_start(i2c, i2c->msg);
> +               if (ret < 0) {
> +                       dev_dbg(i2c->dev, "restart error (%d)\n", ret);
> +                       synquacer_i2c_stop(i2c, -EAGAIN);
> +               } else
> +                       i2c->state = STATE_START;
> +               break;
> +       default:
> +               dev_err(i2c->dev, "called in err STATE (%d)\n", i2c->state);
> +               break;
> +       }
> +
> +out:
> +       WAIT_PCLK(10, i2c->clkrate);
> +       return IRQ_HANDLED;
> +}
> +
> +static int synquacer_i2c_xfer(struct i2c_adapter *adap,
> +                             struct i2c_msg *msgs, int num)
> +{
> +       struct synquacer_i2c *i2c;
> +       int retry;

> +       int ret = 0;

Redundant assignment.

> +
> +       if (!msgs)
> +               return -EINVAL;
> +       if (num <= 0)
> +               return -EINVAL;
> +
> +       i2c = i2c_get_adapdata(adap);
> +       i2c->timeout_ms = calc_timeout_ms(i2c, msgs, num);
> +
> +       dev_dbg(i2c->dev, "calculated timeout %d ms\n",
> +               i2c->timeout_ms);
> +
> +       for (retry = 0; retry < adap->retries; retry++) {
> +
> +               ret = synquacer_i2c_doxfer(i2c, msgs, num);
> +               if (ret != -EAGAIN)
> +                       return ret;
> +
> +               dev_dbg(i2c->dev, "Retrying transmission (%d)\n",
> +                       retry);
> +
> +               synquacer_i2c_master_recover(i2c);
> +               synquacer_i2c_hw_reset(i2c);
> +       }
> +       return -EIO;
> +}
> +
> +static u32 synquacer_i2c_functionality(struct i2c_adapter *adap)
> +{
> +       return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm synquacer_i2c_algo = {
> +       .master_xfer    = synquacer_i2c_xfer,
> +       .functionality  = synquacer_i2c_functionality,
> +};
> +
> +static struct i2c_adapter synquacer_i2c_ops = {
> +       .owner          = THIS_MODULE,
> +       .name           = "synquacer_i2c-adapter",
> +       .algo           = &synquacer_i2c_algo,
> +       .retries        = 5,
> +};
> +
> +static int synquacer_i2c_probe(struct platform_device *pdev)
> +{
> +       struct synquacer_i2c *i2c;
> +       struct resource *r;
> +       int speed_khz;
> +       int ret;
> +
> +       ret = device_property_read_u32(&pdev->dev, "clock-frequency",
> +                                      &speed_khz);
> +       if (ret) {
> +               dev_err(&pdev->dev,
> +                       "Missing clock-frequency property\n");
> +               return -EINVAL;
> +       }
> +       speed_khz /= 1000;
> +
> +       i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> +       if (!i2c)
> +               return -ENOMEM;
> +

> +       if (dev_of_node(&pdev->dev)) {
> +               i2c->clk = devm_clk_get(&pdev->dev, "pclk");
> +               if (IS_ERR(i2c->clk)) {
> +                       dev_err(&pdev->dev, "cannot get clock\n");
> +                       return PTR_ERR(i2c->clk);
> +               }
> +               dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
> +
> +               i2c->clkrate = clk_get_rate(i2c->clk);
> +               dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
> +               clk_prepare_enable(i2c->clk);
> +       } else {
> +               ret = device_property_read_u32(&pdev->dev,
> +                                              "socionext,pclk-rate",
> +                                              &i2c->clkrate);
> +               if (ret)
> +                       return ret;
> +       }

Okay, I got this case. It's more likely the one in 8250_dw.c.

Can you do the similar way?

> +
> +       if (i2c->clkrate < SYNQUACER_I2C_MIN_CLK_RATE ||
> +           i2c->clkrate > SYNQUACER_I2C_MAX_CLK_RATE) {
> +               dev_err(&pdev->dev, "PCLK rate out of range (%d)\n",
> +                       i2c->clkrate);
> +               return -EINVAL;
> +       }
> +
> +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       i2c->base = devm_ioremap_resource(&pdev->dev, r);
> +       if (IS_ERR(i2c->base))
> +               return PTR_ERR(i2c->base);
> +

> +       dev_dbg(&pdev->dev, "registers %p (%p)\n", i2c->base, r);

This one is achievable via /proc/iomem, isn't it?

> +
> +       i2c->irq = platform_get_irq(pdev, 0);
> +       if (i2c->irq <= 0) {

< 0 ?

On some platforms IRQ == 0 might be valid.

> +               dev_err(&pdev->dev, "no IRQ resource found\n");
> +               return -ENODEV;
> +       }
> +
> +       ret = devm_request_irq(&pdev->dev, i2c->irq, synquacer_i2c_isr,
> +                              0, dev_name(&pdev->dev), i2c);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "cannot claim IRQ %d\n", i2c->irq);
> +               return ret;
> +       }
> +
> +       i2c->state = STATE_IDLE;
> +       i2c->dev = &pdev->dev;
> +       i2c->speed_khz = SYNQUACER_I2C_SPEED_SM;
> +       if (speed_khz == SYNQUACER_I2C_SPEED_FM)
> +               i2c->speed_khz = SYNQUACER_I2C_SPEED_FM;
> +
> +       synquacer_i2c_hw_init(i2c);
> +
> +       i2c->adapter = synquacer_i2c_ops;
> +       i2c_set_adapdata(&i2c->adapter, i2c);
> +       i2c->adapter.dev.parent = &pdev->dev;
> +       i2c->adapter.nr = pdev->id;
> +
> +       ret = i2c_add_numbered_adapter(&i2c->adapter);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to add bus to i2c core\n");
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, i2c);
> +
> +       dev_info(&pdev->dev, "%s: synquacer_i2c adapter\n",
> +                               dev_name(&i2c->adapter.dev));
> +
> +       return 0;
> +}
> +
> +static int synquacer_i2c_remove(struct platform_device *pdev)
> +{
> +       struct synquacer_i2c *i2c = platform_get_drvdata(pdev);
> +
> +       platform_set_drvdata(pdev, NULL);
> +       i2c_del_adapter(&i2c->adapter);
> +       clk_disable_unprepare(i2c->clk);
> +
> +       return 0;
> +};
> +
> +static int __maybe_unused synquacer_i2c_suspend(struct device *dev)
> +{
> +       struct synquacer_i2c *i2c = dev_get_drvdata(dev);
> +
> +       i2c_lock_adapter(&i2c->adapter);
> +       i2c->is_suspended = true;
> +       i2c_unlock_adapter(&i2c->adapter);
> +
> +       clk_disable_unprepare(i2c->clk);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused synquacer_i2c_resume(struct device *dev)
> +{
> +       struct synquacer_i2c *i2c = dev_get_drvdata(dev);
> +       int ret;
> +
> +       i2c_lock_adapter(&i2c->adapter);
> +
> +       ret = clk_prepare_enable(i2c->clk);
> +       if (!ret)
> +               i2c->is_suspended = false;
> +
> +       i2c_unlock_adapter(&i2c->adapter);
> +
> +       return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(synquacer_i2c_pm, synquacer_i2c_suspend,
> +                        synquacer_i2c_resume);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id synquacer_i2c_dt_ids[] = {
> +       { .compatible = "socionext,synquacer-i2c" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, synquacer_i2c_dt_ids);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id synquacer_i2c_acpi_ids[] = {
> +       { "SCX0003" },
> +       { /* sentinel */ }
> +};
> +#endif
> +
> +static struct platform_driver synquacer_i2c_driver = {
> +       .probe  = synquacer_i2c_probe,
> +       .remove = synquacer_i2c_remove,
> +       .driver = {
> +               .name = "synquacer_i2c",
> +               .of_match_table = of_match_ptr(synquacer_i2c_dt_ids),
> +               .acpi_match_table = ACPI_PTR(synquacer_i2c_acpi_ids),
> +               .pm = &synquacer_i2c_pm,
> +       },
> +};
> +module_platform_driver(synquacer_i2c_driver);
> +
> +MODULE_AUTHOR("Fujitsu Semiconductor Ltd");
> +MODULE_DESCRIPTION("Socionext SynQuacer I2C Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.11.0
>



-- 
With Best Regards,
Andy Shevchenko

Reply via email to