| 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: <CAHp75Vc1AGzdgTB281XVo=FmZerkU1vijLih3JUO=_wNPjxgfA@mail.gmail.com>
Date: Fri, 23 Feb 2018 14:27:41 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc: Wolfram Sang <wsa@...-dreams.de>, Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
linux-i2c <linux-i2c@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
Jassi Brar <jassisinghbrar@...il.com>
Subject: Re: [PATCH v2 2/2] i2c: add support for Socionext SynQuacer I2C controller
On Thu, Feb 22, 2018 at 9:16 PM, Ard Biesheuvel
<ard.biesheuvel@...aro.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.shevchenko@...il.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.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
Powered by blists - more mailing lists