* Re: [PATCH] mfd: axp20x-i2c: Add i2c-ids to fix module auto-loading
From: Wolfram Sang @ 2016-10-24 10:47 UTC (permalink / raw)
To: Lee Jones
Cc: Hans de Goede, Chen-Yu Tsai, Sebastian Reichel, Dennis Gilmore,
Maxime Ripard, open list:THERMAL, linux-arm-kernel, linux-i2c
In-Reply-To: <20161024103428.GA8574@dell>
[-- Attachment #1: Type: text/plain, Size: 193 bytes --]
> I was under the impression it was all but ready.
Then, I would have applied it.
> What are you waiting on?
Lee, I don't want to explain it *again*. Please re-read Kieran's last
attempt.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] mfd: axp20x-i2c: Add i2c-ids to fix module auto-loading
From: Lee Jones @ 2016-10-24 10:34 UTC (permalink / raw)
To: Wolfram Sang
Cc: Hans de Goede, Chen-Yu Tsai, Sebastian Reichel, Dennis Gilmore,
Maxime Ripard, open list:THERMAL, linux-arm-kernel, linux-i2c
In-Reply-To: <20161024101659.GA1440@katana>
On Mon, 24 Oct 2016, Wolfram Sang wrote:
>
> > > > Even though axp20x-i2c seems to be the only "DT only" i2c client,
> > > > would it make sense to add DT-based module autoloading to the i2c
> > > > core?
> > >
> > > If it is not too invasive, then yes that would be a sensible addition IMHO.
> >
> > If I understand you correctly, I already have a patch-set on the ML
> > that does this.
>
> Incomplete, though. But we will see if Kieran finds time somewhen...
I was under the impression it was all but ready.
What are you waiting on?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] mfd: axp20x-i2c: Add i2c-ids to fix module auto-loading
From: Wolfram Sang @ 2016-10-24 10:17 UTC (permalink / raw)
To: Lee Jones
Cc: Hans de Goede, Chen-Yu Tsai, Sebastian Reichel, Dennis Gilmore,
Maxime Ripard, open list:THERMAL, linux-arm-kernel, linux-i2c
In-Reply-To: <20161024100938.GI14477@dell>
[-- Attachment #1: Type: text/plain, Size: 416 bytes --]
> > > Even though axp20x-i2c seems to be the only "DT only" i2c client,
> > > would it make sense to add DT-based module autoloading to the i2c
> > > core?
> >
> > If it is not too invasive, then yes that would be a sensible addition IMHO.
>
> If I understand you correctly, I already have a patch-set on the ML
> that does this.
Incomplete, though. But we will see if Kieran finds time somewhen...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] mfd: axp20x-i2c: Add i2c-ids to fix module auto-loading
From: Lee Jones @ 2016-10-24 10:09 UTC (permalink / raw)
To: Hans de Goede
Cc: Chen-Yu Tsai, Sebastian Reichel, Dennis Gilmore, Maxime Ripard,
open list:THERMAL, linux-arm-kernel, Wolfram Sang, linux-i2c
In-Reply-To: <bbd16837-a260-b574-1b1a-1bede038e3de@redhat.com>
On Tue, 18 Oct 2016, Hans de Goede wrote:
> On 18-10-16 07:25, Chen-Yu Tsai wrote:
> > On Wed, Oct 5, 2016 at 11:51 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> > > The i2c subsys does not load modules by compatible, only by
> > > i2c-id, with e.g. a modalias of: "i2c:axp209".
> > >
> > > Populate the axp20x_i2c_id[] table with supported ids, so that
> > > module auto-loading will work.
> > >
> > > Reported-by: Dennis Gilmore <dennis@ausil.us>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >
> > Acked-by: Chen-Yu Tsai <wens@csie.org>
> >
> > Even though axp20x-i2c seems to be the only "DT only" i2c client,
> > would it make sense to add DT-based module autoloading to the i2c
> > core?
>
> If it is not too invasive, then yes that would be a sensible addition IMHO.
If I understand you correctly, I already have a patch-set on the ML
that does this.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH] i2c: hix5hd2: allow build with ARCH_HISI
From: Ruqiang Ju @ 2016-10-24 8:39 UTC (permalink / raw)
To: wsa; +Cc: linux-i2c, linux-kernel, xuejiancheng, yanhaifeng, juruqiang
This driver should be buildable with ARCH_HISI,
because some of other HiSilicon SoCs also use it.
Signed-off-by: Ruqiang Ju <juruqiang@huawei.com>
---
drivers/i2c/busses/Kconfig | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 6d94e2e..41abb20 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -79,12 +79,12 @@ config I2C_AMD8111
config I2C_HIX5HD2
tristate "Hix5hd2 high-speed I2C driver"
- depends on ARCH_HIX5HD2 || COMPILE_TEST
+ depends on ARCH_HISI || ARCH_HIX5HD2 || COMPILE_TEST
help
- Say Y here to include support for high-speed I2C controller in the
- Hisilicon based hix5hd2 SoCs.
+ Say Y here to include support for the high-speed I2C controller
+ used in HiSilicon hix5hd2 SoCs.
- This driver can also be built as a module. If so, the module
+ This driver can also be built as a module. If so, the module
will be called i2c-hix5hd2.
config I2C_I801
--
1.9.1
^ permalink raw reply related
* RE: [Patch V3] i2c: imx: add low power i2c bus driver
From: Pandy Gao @ 2016-10-24 5:51 UTC (permalink / raw)
To: Pandy Gao, wsa@the-dreams.de, u.kleine-koenig@pengutronix.de,
cmo@melexis.com
Cc: linux-i2c@vger.kernel.org, Frank Li, Andy Duan
In-Reply-To: <VI1PR0401MB1791A799C3E1447559F436F7F6EC0@VI1PR0401MB1791.eurprd04.prod.outlook.com>
Ping...
> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org [mailto:linux-i2c-
> owner@vger.kernel.org] On Behalf Of Pan Gao
> Sent: Friday, August 26, 2016 3:37 PM
> To: wsa@the-dreams.de; u.kleine-koenig@pengutronix.de; cmo@melexis.com
> Cc: linux-i2c@vger.kernel.org; Frank Li <frank.li@nxp.com>; Fugang Duan
> <fugang.duan@nxp.com>
> Subject: RE: [Patch V3] i2c: imx: add low power i2c bus driver
>
> Ping...
>
> > -----Original Message-----
> > From: Pan Gao
> > Sent: Wednesday, August 17, 2016 4:00 PM
> > To: wsa@the-dreams.de; u.kleine-koenig@pengutronix.de;
> cmo@melexis.com
> > Cc: linux-i2c@vger.kernel.org; Frank Li <frank.li@nxp.com>; Fugang
> > Duan <fugang.duan@nxp.com>; Pan Gao <pandy.gao@nxp.com>
> > Subject: [Patch V3] i2c: imx: add low power i2c bus driver
> >
> > This patch adds low power i2c bus driver to support new i.MX products
> > which use low power i2c instead of the old imx i2c.
> >
> > The low power i2c can continue operating in stop mode when an
> > appropriate clock is available. It is also designed for low CPU
> > overhead with DMA offloading of FIFO register accesses.
> >
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Reviewed-by: Fugang Duan <B38611@freescale.com>
> > ---
> > V2:
> > -stop i2c transfer under the wrong condition -add timeout check in
> > while() domain
> >
> > V3:
> > -fix typo inside commit message and the driver.
> >
> > .../devicetree/bindings/i2c/i2c-imx-lpi2c.txt | 25 +
> > drivers/i2c/busses/Kconfig | 10 +
> > drivers/i2c/busses/Makefile | 1 +
> > drivers/i2c/busses/i2c-imx-lpi2c.c | 667 +++++++++++++++++++++
> > 4 files changed, 703 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
> > b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
> > new file mode 100644
> > index 0000000..1f10cbf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.txt
> > @@ -0,0 +1,25 @@
> > +* Freescale Low Power Inter IC (LPI2C) for i.MX
> > +
> > +Required properties:
> > +- compatible :
> > + - "fsl,imx8dv-lpi2c" for LPI2C compatible with the one integrated
> > +on i.MX8DV soc
> > + - "fsl,imx7ulp-lpi2c" for LPI2C compatible with the one integrated
> > +on i.MX7ULP soc
> > +- reg : Should contain LPI2C registers location and length
> > +- interrupts : Should contain LPI2C interrupt
> > +- clocks : Should contain LPI2C clock specifier
> > +- power-domains : should contain LPI2C power domain
> > +
> > +Optional properties:
> > +- clock-frequency : Constains desired LPI2C bus clock frequency in Hz.
> > + The absence of the property indicates the default frequency 100 kHz.
> > +
> > +Examples:
> > +
> > +i2c1: i2c@5e110000 { /* LPI2C on i.MX8DV */
> > + compatible = "fsl,imx8dv-lpi2c";
> > + reg = <0x0 0x5e110000 0x0 0x4000>;
> > + interrupts = <0 88 4>;
> > + clocks = <&clk IMX8DV_I2C1_CLK>;
> > + clock-names = "per";
> > + power-domains = <&pd_lsio_i2c1>;
> > +};
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index
> > efa3d9b..1fc7a10 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -596,6 +596,16 @@ config I2C_IMX
> > This driver can also be built as a module. If so, the module
> > will be called i2c-imx.
> >
> > +config I2C_IMX_LPI2C
> > + tristate "IMX Low Power I2C interface"
> > + depends on ARCH_MXC || COMPILE_TEST
> > + help
> > + Say Y here if you want to use the Low Power IIC bus controller
> > + on the Freescale i.MX processors.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called i2c-imx-lpi2c.
> > +
> > config I2C_IOP3XX
> > tristate "Intel IOPx3xx and IXP4xx on-chip I2C interface"
> > depends on ARCH_IOP32X || ARCH_IOP33X || ARCH_IXP4XX ||
> ARCH_IOP13XX
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 37f2819..cc93457 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -56,6 +56,7 @@ obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o
> > obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o
> > obj-$(CONFIG_I2C_IMG) += i2c-img-scb.o
> > obj-$(CONFIG_I2C_IMX) += i2c-imx.o
> > +obj-$(CONFIG_I2C_IMX_LPI2C) += i2c-imx-lpi2c.o
> > obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o
> > obj-$(CONFIG_I2C_JZ4780) += i2c-jz4780.o
> > obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > new file mode 100644
> > index 0000000..308ecf5
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -0,0 +1,667 @@
> > +/*
> > + * This is i.MX low power i2c controller driver.
> > + *
> > + * Copyright 2016 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sched.h>
> > +#include <linux/slab.h>
> > +
> > +#define DRIVER_NAME "imx-lpi2c"
> > +
> > +#define LPI2C_PARAM 0x04 /* i2c RX/TX FIFO size */
> > +#define LPI2C_MCR 0x10 /* i2c contrl register */
> > +#define LPI2C_MSR 0x14 /* i2c status register */
> > +#define LPI2C_MIER 0x18 /* i2c interrupt enable */
> > +#define LPI2C_MCFGR0 0x20 /* i2c master configuration */
> > +#define LPI2C_MCFGR1 0x24 /* i2c master configuration */
> > +#define LPI2C_MCFGR2 0x28 /* i2c master configuration */
> > +#define LPI2C_MCFGR3 0x2C /* i2c master configuration */
> > +#define LPI2C_MCCR0 0x48 /* i2c master clk configuration */
> > +#define LPI2C_MCCR1 0x50 /* i2c master clk configuration */
> > +#define LPI2C_MFCR 0x58 /* i2c master FIFO control */
> > +#define LPI2C_MFSR 0x5C /* i2c master FIFO status */
> > +#define LPI2C_MTDR 0x60 /* i2c master TX data register */
> > +#define LPI2C_MRDR 0x70 /* i2c master RX data register */
> > +
> > +/* i2c command */
> > +#define TRAN_DATA 0X00
> > +#define RECV_DATA 0X01
> > +#define GEN_STOP 0X02
> > +#define RECV_DISCARD 0X03
> > +#define GEN_START 0X04
> > +#define START_NACK 0X05
> > +#define START_HIGH 0X06
> > +#define START_HIGH_NACK 0X07
> > +
> > +#define MCR_MEN (1 << 0)
> > +#define MCR_RST (1 << 1)
> > +#define MCR_DOZEN (1 << 2)
> > +#define MCR_DBGEN (1 << 3)
> > +#define MCR_RTF (1 << 8)
> > +#define MCR_RRF (1 << 9)
> > +#define MSR_TDF (1 << 0)
> > +#define MSR_RDF (1 << 1)
> > +#define MSR_SDF (1 << 9)
> > +#define MSR_NDF (1 << 10)
> > +#define MSR_ALF (1 << 11)
> > +#define MSR_MBF (1 << 24)
> > +#define MSR_BBF (1 << 25)
> > +#define MIER_TDIE (1 << 0)
> > +#define MIER_RDIE (1 << 1)
> > +#define MIER_SDIE (1 << 9)
> > +#define MIER_NDIE (1 << 10)
> > +#define MCFGR1_AUTOSTOP (1 << 8)
> > +#define MCFGR1_IGNACK (1 << 9)
> > +#define MRDR_RXEMPTY (1 << 14)
> > +
> > +#define I2C_CLK_RATIO 2
> > +#define CHUNK_DATA 256
> > +
> > +#define LPI2C_RX_FIFOSIZE 4
> > +#define LPI2C_TX_FIFOSIZE 4
> > +
> > +#define LPI2C_DEFAULT_RATE 100000
> > +#define STARDARD_MAX_BITRATE 400000
> > +#define FAST_MAX_BITRATE 1000000
> > +#define FAST_PLUS_MAX_BITRATE 3400000
> > +#define HIGHSPEED_MAX_BITRATE 5000000
> > +
> > +
> > +enum lpi2c_imx_mode {
> > + STANDARD, /* 100+Kbps */
> > + FAST, /* 400+Kbps */
> > + FAST_PLUS, /* 1.0+Mbps */
> > + ULTRA_FAST, /* 5.0+Mbps */
> > + HS, /* 3.4+Mbps */
> > +};
> > +
> > +enum lpi2c_imx_pincfg {
> > + TWO_PIN_OD, /* 2-pin open drain mode */
> > + TWO_PIN_OO, /* 2-pin output only mode (utra-fast mode) */
> > + TWO_PIN_PP, /* 2-pin push-pull mode */
> > + FOUR_PIN_PP, /* 4-pin push-pull mode */
> > + TWO_PIN_OD_SS, /* 2-pin open drain mode with separate slave
> */
> > + TWO_PIN_OO_SS, /* 2-pin output only mode with separate slave
> */
> > + TWO_PIN_PP_SS, /* 2-pin push-pull mode with separate slave */
> > + FOUR_PIN_PP_IO, /* 4-pin push-pull mode (inverted output) */
> > +};
> > +
> > +struct lpi2c_imx_clkcfg {
> > + u8 prescale;
> > + u8 filtscl;
> > + u8 filtsda;
> > + u8 sethold;
> > + u8 clklo;
> > + u8 clkhi;
> > + u8 datavd;
> > +};
> > +
> > +struct lpi2c_imx_struct {
> > + struct i2c_adapter adapter;
> > + struct clk *per_clk;
> > + void __iomem *base;
> > + __u8 *rx_buf;
> > + __u8 *tx_buf;
> > + struct completion complete;
> > + unsigned int msglen;
> > + unsigned int delivered;
> > + unsigned int block_data;
> > + unsigned int bitrate;
> > + enum lpi2c_imx_mode mode;
> > +};
> > +
> > +static void lpi2c_imx_intctrl(
> > + struct lpi2c_imx_struct *lpi2c_imx, unsigned int enable) {
> > + writel(enable, lpi2c_imx->base + LPI2C_MIER); }
> > +
> > +static int lpi2c_imx_bus_busy(struct lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned long orig_jiffies = jiffies;
> > + unsigned int temp;
> > +
> > + while (1) {
> > + temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > +
> > + /* check for arbitration lost, clear if set */
> > + if (temp & MSR_ALF) {
> > + writel(temp, lpi2c_imx->base + LPI2C_MSR);
> > + return -EAGAIN;
> > + }
> > +
> > + if ((temp & MSR_BBF) && (temp & MSR_MBF))
> > + break;
> > +
> > + if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> > + dev_dbg(&lpi2c_imx->adapter.dev, "bus not work\n");
> > + return -ETIMEDOUT;
> > + }
> > + schedule();
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void lpi2c_imx_set_mode(struct lpi2c_imx_struct *lpi2c_imx) {
> > + enum lpi2c_imx_mode mode;
> > + unsigned int bitrate = lpi2c_imx->bitrate;
> > +
> > + if (bitrate < STARDARD_MAX_BITRATE)
> > + mode = STANDARD;
> > + else if (bitrate < FAST_MAX_BITRATE)
> > + mode = FAST;
> > + else if (bitrate < FAST_PLUS_MAX_BITRATE)
> > + mode = FAST_PLUS;
> > + else if (bitrate < HIGHSPEED_MAX_BITRATE)
> > + mode = HS;
> > + else
> > + mode = ULTRA_FAST;
> > +
> > + lpi2c_imx->mode = mode;
> > +}
> > +
> > +static int lpi2c_imx_start(struct lpi2c_imx_struct *lpi2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + u8 read;
> > + unsigned int temp;
> > +
> > + temp = readl(lpi2c_imx->base + LPI2C_MCR);
> > + temp |= MCR_RRF | MCR_RTF;
> > + writel(temp, lpi2c_imx->base + LPI2C_MCR);
> > + writel(0x7f00, lpi2c_imx->base + LPI2C_MSR);
> > +
> > + read = msgs->flags & I2C_M_RD;
> > + temp = (msgs->addr << 1 | read) | (GEN_START << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > +
> > + return lpi2c_imx_bus_busy(lpi2c_imx); }
> > +
> > +static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned int temp;
> > + unsigned long orig_jiffies = jiffies;
> > +
> > + writel(GEN_STOP << 8, lpi2c_imx->base + LPI2C_MTDR);
> > + do {
> > + temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > + if (temp & MSR_SDF)
> > + break;
> > +
> > + if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> > + dev_dbg(&lpi2c_imx->adapter.dev, "stop timeout\n");
> > + break;
> > + }
> > + schedule();
> > +
> > + } while (1);
> > +}
> > +
> > +
> > +/* CLKLO = I2C_CLK_RATIO * CLKHI, SETHOLD = CLKHI, DATAVD = CLKHI/2
> > +*/ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned int temp;
> > + unsigned int per_clk_rate;
> > + unsigned int prescale, clk_high, clk_low, clk_cycle;
> > + enum lpi2c_imx_pincfg pincfg;
> > + struct lpi2c_imx_clkcfg clkcfg;
> > +
> > + lpi2c_imx_set_mode(lpi2c_imx);
> > + per_clk_rate = clk_get_rate(lpi2c_imx->per_clk);
> > +
> > + if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
> > + clkcfg.filtscl = clkcfg.filtsda = 0;
> > + else
> > + clkcfg.filtscl = clkcfg.filtsda = 2;
> > +
> > + for (prescale = 0; prescale <= 7; prescale++) {
> > + clk_cycle = per_clk_rate / ((1 << prescale) * lpi2c_imx->bitrate)
> > + - 3 - (clkcfg.filtscl >> 1);
> > + clk_high = (clk_cycle + I2C_CLK_RATIO) / (I2C_CLK_RATIO + 1);
> > + clk_low = clk_cycle - clk_high;
> > + if (clk_low < 64)
> > + break;
> > + }
> > +
> > + if (prescale > 7)
> > + return -EINVAL;
> > +
> > + clkcfg.prescale = prescale;
> > + clkcfg.sethold = clk_high;
> > + clkcfg.clklo = clk_low;
> > + clkcfg.clkhi = clk_high;
> > + clkcfg.datavd = clk_high >> 1;
> > +
> > + /* set MCFGR1: PINCFG, PRESCALE, IGNACK */
> > + if (lpi2c_imx->mode == ULTRA_FAST)
> > + pincfg = TWO_PIN_OO;
> > + else
> > + pincfg = TWO_PIN_OD;
> > + temp = clkcfg.prescale | pincfg << 24;
> > +
> > + if (lpi2c_imx->mode == ULTRA_FAST)
> > + temp |= MCFGR1_IGNACK;
> > +
> > + writel(temp, lpi2c_imx->base + LPI2C_MCFGR1);
> > +
> > + /* set MCFGR2: FILTSDA, FILTSCL */
> > + temp = (clkcfg.filtscl << 16) | (clkcfg.filtsda << 24);
> > + writel(temp, lpi2c_imx->base + LPI2C_MCFGR2);
> > +
> > +
> > + /* set MCCR: DATAVD, SETHOLD, CLKHI, CLKLO */
> > + temp = clkcfg.datavd << 24 | clkcfg.sethold << 16 |
> > + clkcfg.clkhi << 8 | clkcfg.clklo;
> > +
> > + if (lpi2c_imx->mode == HS)
> > + writel(temp, lpi2c_imx->base + LPI2C_MCCR1);
> > + else
> > + writel(temp, lpi2c_imx->base + LPI2C_MCCR0);
> > +
> > + return 0;
> > +}
> > +
> > +static int lpi2c_imx_master_enable(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + int ret;
> > + unsigned int temp;
> > +
> > + ret = clk_prepare_enable(lpi2c_imx->per_clk);
> > + if (ret)
> > + return ret;
> > +
> > + temp = MCR_RST;
> > + writel(temp, lpi2c_imx->base + LPI2C_MCR);
> > + writel(0, lpi2c_imx->base + LPI2C_MCR);
> > +
> > + ret = lpi2c_imx_config(lpi2c_imx);
> > + if (ret)
> > + return ret;
> > +
> > + temp = readl(lpi2c_imx->base + LPI2C_MCR);
> > + temp |= MCR_MEN;
> > + writel(temp, lpi2c_imx->base + LPI2C_MCR);
> > +
> > + return 0;
> > +}
> > +
> > +static int lpi2c_imx_master_disable(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + unsigned int temp = 0;
> > +
> > + temp = readl(lpi2c_imx->base + LPI2C_MCR);
> > + temp &= ~MCR_MEN;
> > + writel(temp, lpi2c_imx->base + LPI2C_MCR);
> > +
> > + clk_disable_unprepare(lpi2c_imx->per_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int lpi2c_imx_msg_complete(struct lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned int timeout;
> > +
> > + timeout = wait_for_completion_timeout(&lpi2c_imx->complete, HZ);
> > +
> > + return timeout ? 0 : -ETIMEDOUT;
> > +}
> > +
> > +static int lpi2c_imx_txfifo_empty(struct lpi2c_imx_struct *lpi2c_imx) {
> > + u32 txcnt;
> > + unsigned long orig_jiffies = jiffies;
> > +
> > + do {
> > + txcnt = readl(lpi2c_imx->base + LPI2C_MFSR) & 0xff;
> > +
> > + if (readl(lpi2c_imx->base + LPI2C_MSR) & MSR_NDF) {
> > + dev_dbg(&lpi2c_imx->adapter.dev, "NDF detected\n");
> > + return -EIO;
> > + }
> > +
> > + if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
> > + dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty
> > timeout\n");
> > + return -ETIMEDOUT;
> > + }
> > + schedule();
> > +
> > + } while (txcnt);
> > +
> > + return 0;
> > +}
> > +
> > +static void lpi2c_imx_set_tx_watermark(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + unsigned int temp;
> > +
> > + temp = LPI2C_TX_FIFOSIZE >> 1;
> > + writel(temp, lpi2c_imx->base + LPI2C_MFCR); }
> > +
> > +static void lpi2c_imx_set_rx_watermark(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + unsigned int temp, remaining;
> > +
> > + remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
> > +
> > + if (remaining > (LPI2C_RX_FIFOSIZE >> 1))
> > + temp = LPI2C_RX_FIFOSIZE >> 1;
> > + else
> > + temp = 0;
> > +
> > + writel(temp << 16, lpi2c_imx->base + LPI2C_MFCR); }
> > +
> > +static void lpi2c_imx_write_txfifo(struct lpi2c_imx_struct
> > +*lpi2c_imx) {
> > + unsigned int data, txcnt;
> > +
> > + txcnt = readl(lpi2c_imx->base + LPI2C_MFSR) & 0xff;
> > + while (txcnt < LPI2C_TX_FIFOSIZE) {
> > + if (lpi2c_imx->delivered == lpi2c_imx->msglen)
> > + break;
> > + data = lpi2c_imx->tx_buf[lpi2c_imx->delivered++];
> > + writel(data, lpi2c_imx->base + LPI2C_MTDR);
> > + txcnt++;
> > + }
> > +
> > + if (lpi2c_imx->delivered < lpi2c_imx->msglen)
> > + lpi2c_imx_intctrl(lpi2c_imx, MIER_TDIE | MIER_NDIE);
> > + else
> > + complete(&lpi2c_imx->complete);
> > +}
> > +
> > +static void lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned int temp, data;
> > + unsigned int blocklen, remaining;
> > +
> > + do {
> > + data = readl(lpi2c_imx->base + LPI2C_MRDR);
> > + if (data & MRDR_RXEMPTY)
> > + break;
> > + lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
> > + } while (1);
> > +
> > + /*
> > + * First byte is the length of remaining packet in the SMBus block
> > + * data read. Add it to msgs->len.
> > + */
> > + if (lpi2c_imx->block_data) {
> > + blocklen = lpi2c_imx->rx_buf[0];
> > + lpi2c_imx->msglen += blocklen;
> > + }
> > +
> > + remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
> > + /* not finished, still waiting for rx data */
> > + if (remaining) {
> > + lpi2c_imx_set_rx_watermark(lpi2c_imx);
> > + /* multiple receive commands */
> > + if (lpi2c_imx->block_data) {
> > + lpi2c_imx->block_data = 0;
> > + temp = remaining;
> > + temp |= (RECV_DATA << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > + } else if (!(lpi2c_imx->delivered & 0xff)) {
> > + temp = remaining > CHUNK_DATA ?
> > + CHUNK_DATA - 1 : (remaining - 1);
> > + temp |= (RECV_DATA << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > + }
> > +
> > + lpi2c_imx_intctrl(lpi2c_imx, MIER_RDIE);
> > + } else
> > + complete(&lpi2c_imx->complete);
> > +}
> > +
> > +static void lpi2c_imx_write(struct lpi2c_imx_struct *lpi2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + lpi2c_imx->tx_buf = msgs->buf;
> > + lpi2c_imx_set_tx_watermark(lpi2c_imx);
> > + lpi2c_imx_write_txfifo(lpi2c_imx);
> > +}
> > +
> > +static void lpi2c_imx_read(struct lpi2c_imx_struct *lpi2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + unsigned int temp;
> > +
> > + lpi2c_imx->rx_buf = msgs->buf;
> > + lpi2c_imx->block_data = msgs->flags & I2C_M_RECV_LEN;
> > +
> > + lpi2c_imx_set_rx_watermark(lpi2c_imx);
> > + temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
> > + temp |= (RECV_DATA << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > +
> > + lpi2c_imx_intctrl(lpi2c_imx, MIER_RDIE | MIER_NDIE); }
> > +
> > +static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
> > + struct i2c_msg *msgs, int num)
> > +{
> > + int i, result;
> > + unsigned int temp;
> > + struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(adapter);
> > +
> > + result = lpi2c_imx_master_enable(lpi2c_imx);
> > + if (result)
> > + return result;
> > +
> > + for (i = 0; i < num; i++) {
> > + result = lpi2c_imx_start(lpi2c_imx, &msgs[i]);
> > + if (result)
> > + goto disable;
> > +
> > + /* quick smbus */
> > + if (num == 1 && msgs[0].len == 0)
> > + goto stop;
> > +
> > + lpi2c_imx->delivered = 0;
> > + lpi2c_imx->msglen = msgs[i].len;
> > + init_completion(&lpi2c_imx->complete);
> > +
> > + if (msgs[i].flags & I2C_M_RD)
> > + lpi2c_imx_read(lpi2c_imx, &msgs[i]);
> > + else
> > + lpi2c_imx_write(lpi2c_imx, &msgs[i]);
> > +
> > + result = lpi2c_imx_msg_complete(lpi2c_imx);
> > + if (result)
> > + goto stop;
> > +
> > + if (!(msgs[i].flags & I2C_M_RD)) {
> > + result = lpi2c_imx_txfifo_empty(lpi2c_imx);
> > + if (result)
> > + goto stop;
> > + }
> > + }
> > +
> > +stop:
> > + lpi2c_imx_stop(lpi2c_imx);
> > +
> > + temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > + if ((temp & MSR_NDF) && !result)
> > + result = -EIO;
> > +
> > +disable:
> > + lpi2c_imx_master_disable(lpi2c_imx);
> > +
> > + dev_dbg(&lpi2c_imx->adapter.dev, "<%s> exit with: %s: %d\n",
> __func__,
> > + (result < 0) ? "error" : "success msg",
> > + (result < 0) ? result : num);
> > +
> > + return (result < 0) ? result : num;
> > +}
> > +
> > +static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) {
> > + unsigned int temp;
> > + struct lpi2c_imx_struct *lpi2c_imx = dev_id;
> > +
> > + lpi2c_imx_intctrl(lpi2c_imx, 0);
> > + temp = readl(lpi2c_imx->base + LPI2C_MSR);
> > +
> > + if (temp & MSR_RDF) {
> > + lpi2c_imx_read_rxfifo(lpi2c_imx);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + if (temp & MSR_TDF) {
> > + lpi2c_imx_write_txfifo(lpi2c_imx);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + complete(&lpi2c_imx->complete);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static u32 lpi2c_imx_func(struct i2c_adapter *adapter) {
> > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> > + | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> > +}
> > +
> > +static struct i2c_algorithm lpi2c_imx_algo = {
> > + .master_xfer = lpi2c_imx_xfer,
> > + .functionality = lpi2c_imx_func,
> > +};
> > +
> > +static const struct of_device_id lpi2c_imx_of_match[] = {
> > + { .compatible = "fsl,imx8dv-lpi2c" },
> > + { .compatible = "fsl,imx7ulp-lpi2c" },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, lpi2c_imx_of_match)
> > +
> > +static int lpi2c_imx_probe(struct platform_device *pdev) {
> > + int irq, ret;
> > + void __iomem *base;
> > + struct resource *res;
> > + struct lpi2c_imx_struct *lpi2c_imx;
> > +
> > + lpi2c_imx = devm_kzalloc(&pdev->dev,
> > + sizeof(*lpi2c_imx), GFP_KERNEL);
> > + if (!lpi2c_imx)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(&pdev->dev, "can't get irq number\n");
> > + return irq;
> > + }
> > +
> > + lpi2c_imx->adapter.owner = THIS_MODULE;
> > + lpi2c_imx->adapter.algo = &lpi2c_imx_algo;
> > + lpi2c_imx->adapter.dev.parent = &pdev->dev;
> > + lpi2c_imx->adapter.nr = pdev->id;
> > + lpi2c_imx->base = base;
> > + lpi2c_imx->adapter.dev.of_node = pdev->dev.of_node;
> > + strlcpy(lpi2c_imx->adapter.name, pdev->name,
> > + sizeof(lpi2c_imx->adapter.name));
> > +
> > + lpi2c_imx->per_clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(lpi2c_imx->per_clk)) {
> > + dev_err(&pdev->dev, "can't get I2C peripheral clock\n");
> > + return PTR_ERR(lpi2c_imx->per_clk);
> > + }
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node,
> > + "clock-frequency", &lpi2c_imx->bitrate);
> > + if (ret)
> > + lpi2c_imx->bitrate = LPI2C_DEFAULT_RATE;
> > +
> > + ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> > + pdev->name, lpi2c_imx);
> > + if (ret) {
> > + dev_err(&pdev->dev, "can't claim irq %d\n", irq);
> > + goto ret;
> > + }
> > +
> > + i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
> > + platform_set_drvdata(pdev, lpi2c_imx);
> > +
> > + ret = i2c_add_numbered_adapter(&lpi2c_imx->adapter);
> > + if (ret) {
> > + dev_err(&pdev->dev, "registration failed\n");
> > + goto ret;
> > + }
> > +
> > + dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n");
> > +
> > +ret:
> > + return ret;
> > +}
> > +
> > +static int lpi2c_imx_remove(struct platform_device *pdev) {
> > + struct lpi2c_imx_struct *lpi2c_imx = platform_get_drvdata(pdev);
> > +
> > + i2c_del_adapter(&lpi2c_imx->adapter);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver lpi2c_imx_driver = {
> > + .probe = lpi2c_imx_probe,
> > + .remove = lpi2c_imx_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .of_match_table = lpi2c_imx_of_match,
> > + },
> > +};
> > +
> > +static int __init i2c_adap_imx_init(void) {
> > + return platform_driver_register(&lpi2c_imx_driver);
> > +}
> > +module_init(i2c_adap_imx_init);
> > +
> > +static void __exit i2c_adap_imx_exit(void) {
> > + platform_driver_unregister(&lpi2c_imx_driver);
> > +}
> > +module_exit(i2c_adap_imx_exit);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Gao Pan <pandy.gao@nxp.com>");
> > MODULE_DESCRIPTION("I2C
> > +adapter driver for LPI2C bus"); MODULE_ALIAS("platform:"
> > +DRIVER_NAME);
> > --
> > 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body
> of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] i2c: rk3x: Give the tuning value 0 during rk3x_i2c_v0_calc_timings
From: David Wu @ 2016-10-22 8:43 UTC (permalink / raw)
To: heiko-4mtYJXux2i+zQB+pC5nmwQ, wsa-z923LK4zBo2bacvFa/9K2g
Cc: dianders-F7+t8E8rja9g9hUCZPvPmw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Wu
We found a bug that i2c transfer sometimes failed on 3066a board with
stabel-4.8, the con register would be updated by uninitialized tuning
value, it made the i2c transfer failed.
So give the tuning value to be zero during rk3x_i2c_v0_calc_timings.
Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
drivers/i2c/busses/i2c-rk3x.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 50702c7..df22066 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -694,6 +694,8 @@ static int rk3x_i2c_v0_calc_timings(unsigned long clk_rate,
t_calc->div_low--;
t_calc->div_high--;
+ /* Give the tuning value 0, that would not update con register */
+ t_calc->tuning = 0;
/* Maximum divider supported by hw is 0xffff */
if (t_calc->div_low > 0xffff) {
t_calc->div_low = 0xffff;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v2 4/4] Cleaned the code, no functional changes.
From: Andy Shevchenko @ 2016-10-21 10:55 UTC (permalink / raw)
To: Luis.Oliveira-HKixBCOQz3hWk0Htik3J/w,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
wsa-z923LK4zBo2bacvFa/9K2g, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <3df96f37129d81eb2275e0151d1aab6ae43fbaa2.1476462204.git.lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Fri, 2016-10-14 at 17:52 +0100, Luis.Oliveira-HKixBCOQz3hWk0Htik3J/w@public.gmane.org wrote:
> From: Luis Oliveira <lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>
Style issues.
The parts of this patch shouldn't be brought by the others patches in
the series. Consider carefully check your patches before submitting.
> Signed-off-by: Luis Oliveira <lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
> drivers/i2c/busses/i2c-designware-core.c | 113 ++++++++++++++-----
> ---------
> drivers/i2c/busses/i2c-designware-platdrv.c | 24 +++---
> 2 files changed, 68 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.c
> b/drivers/i2c/busses/i2c-designware-core.c
> index 71a377e..4196491 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -90,15 +90,15 @@
> #define DW_IC_INTR_DEFAULT_MASK (DW_IC_INTR_RX_FULL |
> \
> DW_IC_INTR_TX_ABRT | \
> DW_IC_INTR_STOP_DET)
> -
> +
> #define DW_IC_INTR_MASTER_MASK (DW_IC_INTR_DEFAULT_MAS
> K | \
> DW_IC_INTR_TX_EMPTY)
> -
> +
> #define DW_IC_INTR_SLAVE_MASK (DW_IC_INTR_DEFAULT_MASK
> | \
> DW_IC_INTR_RX_DONE | \
> - DW_IC_INTR_RX_UNDER | \
> - DW_IC_INTR_RD_REQ)
> -
> + DW_IC_INTR_RX_UNDER | \
> + DW_IC_INTR_RD_REQ)
> +
> #define DW_IC_STATUS_ACTIVITY 0x1
> #define DW_IC_STATUS_TFE BIT(2)
> #define DW_IC_STATUS_MASTER_ACTIVITY BIT(5)
> @@ -229,7 +229,7 @@ static void i2c_dw_configure_fifo_master(struct
> dw_i2c_dev *dev)
> dw_writel(dev, 0, DW_IC_RX_TL);
>
> /* configure the i2c master */
> - dw_writel(dev, dev->master_cfg , DW_IC_CON);
> + dw_writel(dev, dev->master_cfg, DW_IC_CON);
> dw_writel(dev, DW_IC_INTR_MASTER_MASK,
> DW_IC_INTR_MASK);
> }
>
> @@ -240,7 +240,7 @@ static void i2c_dw_configure_fifo_slave(struct
> dw_i2c_dev *dev)
> dw_writel(dev, 0, DW_IC_RX_TL);
>
> /* configure the i2c slave */
> - dw_writel(dev, dev->slave_cfg , DW_IC_CON);
> + dw_writel(dev, dev->slave_cfg, DW_IC_CON);
> dw_writel(dev, DW_IC_INTR_SLAVE_MASK,
> DW_IC_INTR_MASK);
> }
>
> @@ -386,8 +386,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> /* Configure register access mode 16bit */
> dev->accessor_flags |= ACCESS_16BIT;
> } else if (reg != DW_IC_COMP_TYPE_VALUE) {
> - dev_err(dev->dev, "Unknown Synopsys component type: "
> - "0x%08x\n", reg);
> + dev_err(dev->dev, "Unknown Synopsys component type:
> 0x%08x\n",
> + reg);
> i2c_dw_release_lock(dev);
> return -ENODEV;
> }
> @@ -475,7 +475,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> }
>
> if ((dev->master_cfg & DW_IC_CON_MASTER) &&
> - (dev->master_cfg & DW_IC_CON_SLAVE_DISABLE))
> + (dev->master_cfg & DW_IC_CON_SLAVE_DISABLE))
> i2c_dw_configure_fifo_master(dev);
> else
> i2c_dw_configure_fifo_slave(dev);
> @@ -814,6 +814,7 @@ done_nolock:
> static u32 i2c_dw_func(struct i2c_adapter *adap)
> {
> struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> return dev->functionality;
> }
>
> @@ -821,19 +822,19 @@ static int i2c_dw_reg_slave(struct i2c_client
> *slave)
> {
> struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
>
> - if(dev->slave)
> + if (dev->slave)
> return -EBUSY;
> - if(slave->flags & I2C_CLIENT_TEN)
> + if (slave->flags & I2C_CLIENT_TEN)
> return -EAFNOSUPPORT;
> - /* set slave address in the IC_SAR register,
> - * the address to which the DW_apb_i2c responds */
> + /* set slave address in the IC_SAR register,
> + * the address to which the DW_apb_i2c responds */
>
> __i2c_dw_enable(dev, false);
> -
> +
> dw_writel(dev, slave->addr, DW_IC_SAR);
>
> pm_runtime_get_sync(dev->dev);
> -
> +
> dev->slave = slave;
>
> __i2c_dw_enable(dev, true);
> @@ -929,78 +930,76 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
> * Interrupt service routine. This gets called whenever an I2C
> interrupt
> * occurs.
> */
> -
> -static bool i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev )
> +
> +static bool i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
> {
> u32 raw_stat, stat, enabled;
> u8 val, slave_activity;
> -
> +
> stat = dw_readl(dev, DW_IC_INTR_STAT);
> enabled = dw_readl(dev, DW_IC_ENABLE);
> raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> - slave_activity = ((dw_readl(dev,DW_IC_STATUS) &
> - DW_IC_STATUS_SLAVE_ACTIVITY)>>6);
> -
> - dev_dbg(dev->dev,
> - "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :
> INTR_STAT=%#x\n",
> - __func__, enabled, slave_activity, raw_stat, stat);
> + slave_activity = ((dw_readl(dev, DW_IC_STATUS)
> + & DW_IC_STATUS_SLAVE_ACTIVITY)>>6);
> +
> + dev_dbg(dev->dev,
> + "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :
> INTR_STAT=%#x\n",
> + __func__, enabled, slave_activity, raw_stat, stat);
>
> if (stat & DW_IC_INTR_START_DET)
> dw_readl(dev, DW_IC_CLR_START_DET);
> -
> +
> if (stat & DW_IC_INTR_ACTIVITY)
> dw_readl(dev, DW_IC_CLR_ACTIVITY);
>
> if (stat & DW_IC_INTR_RX_OVER)
> dw_readl(dev, DW_IC_CLR_RX_OVER);
> -
> - if ((stat & DW_IC_INTR_RX_FULL) && (stat &
> DW_IC_INTR_STOP_DET))
> +
> + if ((stat & DW_IC_INTR_RX_FULL) && (stat &
> DW_IC_INTR_STOP_DET))
> i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_REQUESTED, &val);
> -
> - if (slave_activity) {
> - if (stat & DW_IC_INTR_RD_REQ) {
> +
> + if (slave_activity) {
> + if (stat & DW_IC_INTR_RD_REQ) {
> if (stat & DW_IC_INTR_RX_FULL) {
> val = dw_readl(dev, DW_IC_DATA_CMD);
> - if (!i2c_slave_event(dev->slave,
> + if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_RECEIVED,
> &val)) {
> - dev_dbg(dev->dev, "Byte %X
> acked! ",val);
> + dev_dbg(dev->dev, "Byte %X
> acked! ", val);
> }
> dw_readl(dev, DW_IC_CLR_RD_REQ);
> stat =
> i2c_dw_read_clear_intrbits(dev);
> - }
> - else {
> + } else {
> dw_readl(dev, DW_IC_CLR_RD_REQ);
> dw_readl(dev, DW_IC_CLR_RX_UNDER);
> stat =
> i2c_dw_read_clear_intrbits(dev);
> - }
> - if (!i2c_slave_event(dev->slave,
> + }
> + if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_READ_REQUESTED,
> &val))
> dw_writel(dev, val, DW_IC_DATA_CMD);
> }
> - }
> -
> + }
> +
> if (stat & DW_IC_INTR_RX_DONE) {
> -
> - if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_READ_PROCESSED, &val))
> +
> + if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_READ_PROCESSED, &val))
> dw_readl(dev, DW_IC_CLR_RX_DONE);
> -
> - i2c_slave_event(dev->slave, I2C_SLAVE_STOP , &val);
> +
> + i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
> stat = i2c_dw_read_clear_intrbits(dev);
>
> return true;
> }
> -
> - if (stat & DW_IC_INTR_RX_FULL) {
> +
> + if (stat & DW_IC_INTR_RX_FULL) {
> val = dw_readl(dev, DW_IC_DATA_CMD);
> if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_RECEIVED, &val))
> - dev_dbg(dev->dev, "Byte %X acked! ",val);
> - }
> - else {
> - i2c_slave_event(dev->slave, I2C_SLAVE_STOP , &val);
> + dev_dbg(dev->dev, "Byte %X acked! ", val);
> + } else {
> + i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
> stat = i2c_dw_read_clear_intrbits(dev);
> }
>
> - if (stat & DW_IC_INTR_TX_OVER) {
> + if (stat & DW_IC_INTR_TX_OVER) {
> dw_readl(dev, DW_IC_CLR_TX_OVER);
> return true;
> }
> @@ -1008,12 +1007,12 @@ static bool i2c_dw_irq_handler_slave(struct
> dw_i2c_dev *dev )
> return true;
> }
>
> -static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
> +static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
> {
> u32 stat;
> -
> +
> stat = i2c_dw_read_clear_intrbits(dev);
> -
> +
> if (stat & DW_IC_INTR_TX_ABRT) {
> dev->cmd_err |= DW_IC_ERR_TX_ABRT;
> dev->status = STATUS_IDLE;
> @@ -1068,17 +1067,17 @@ static irqreturn_t i2c_dw_isr(int this_irq,
> void *dev_id)
> enabled = dw_readl(dev, DW_IC_ENABLE);
> mode = dw_readl(dev, DW_IC_CON);
> stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> -
> +
> dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__,
> enabled, stat);
> if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
> return IRQ_NONE;
> -
> +
> if (!(mode & DW_IC_CON_MASTER) && !(mode &
> DW_IC_CON_SLAVE_DISABLE)) {
> stat = i2c_dw_read_clear_intrbits(dev);
> if (!i2c_dw_irq_handler_slave(dev))
> return IRQ_NONE;
> } else {
> - if(i2c_dw_irq_handler_master(dev))
> + if (i2c_dw_irq_handler_master(dev))
> return IRQ_HANDLED;
> }
>
> @@ -1148,7 +1147,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
> adap->dev.parent = dev->dev;
> i2c_set_adapdata(adap, dev);
>
> - if (!i2c_check_functionality(adap,I2C_FUNC_SLAVE))
> + if (!i2c_check_functionality(adap, I2C_FUNC_SLAVE))
> i2c_dw_disable_int(dev);
>
> r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index f29e657..035b93f 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -141,13 +141,13 @@ static inline int dw_i2c_acpi_configure(struct
> platform_device *pdev)
> static void i2c_dw_configure_master(struct platform_device *pdev)
> {
> struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> -
> +
> dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE
> |
> DW_IC_CON_RESTART_EN;
> -
> +
> dev->functionality |= I2C_FUNC_10BIT_ADDR;
> dev_info(&pdev->dev, "I am registed as a I2C Master!\n");
> -
> +
> switch (dev->clk_freq) {
> case 100000:
> dev->master_cfg |= DW_IC_CON_SPEED_STD;
> @@ -158,25 +158,25 @@ static void i2c_dw_configure_master(struct
> platform_device *pdev)
> default:
> dev->master_cfg |= DW_IC_CON_SPEED_FAST;
> }
> -
> +
> }
>
> static void i2c_dw_configure_slave(struct platform_device *pdev)
> {
> struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> -
> - dev->slave_cfg = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL |
> - DW_IC_CON_RESTART_EN |
> DW_IC_CON_STOP_DET_IFADDRESSED |
> +
> + dev->slave_cfg = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL |
> + DW_IC_CON_RESTART_EN |
> DW_IC_CON_STOP_DET_IFADDRESSED |
> DW_IC_CON_SPEED_FAST;
> -
> +
> dev->functionality |= I2C_FUNC_SLAVE;
> dev->functionality &= ~I2C_FUNC_10BIT_ADDR;
> dev_info(&pdev->dev, "I am registed as a I2C Slave!\n");
> -
> +
> switch (dev->clk_freq) {
> case 100000:
> dev->slave_cfg |= DW_IC_CON_SPEED_STD;
> -
> +
> case 3400000:
> dev->slave_cfg |= DW_IC_CON_SPEED_HIGH;
> break;
> @@ -272,10 +272,10 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> I2C_FUNC_SMBUS_BYTE_DATA |
> I2C_FUNC_SMBUS_WORD_DATA |
> I2C_FUNC_SMBUS_I2C_BLOCK;
> -
> +
> if (is_slave)
> i2c_dw_configure_slave(pdev);
> - else
> + else
> i2c_dw_configure_master(pdev);
>
> dev->clk = devm_clk_get(&pdev->dev, NULL);
--
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported
From: Andy Shevchenko @ 2016-10-21 10:54 UTC (permalink / raw)
To: Luis Oliveira, Wolfram Sang, Ramiro Oliveira
Cc: Mark Rutland, jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <76ab13e9-9aa5-97c3-2328-928bbcef9877-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Fri, 2016-10-21 at 10:56 +0100, Luis Oliveira wrote:
> Since practically 90% of the code is shared between master and slave,
> I was
> thinking if it will be acceptable to use the same driver for both but
> differentiate the master/slave mode by the compatible strings.
It might be possible to split like other drivers do:
1. Core part (i2c-designware-core.c)
2. Master part (i2c-designware-master.c)
3. Slave part (i2c-designware-slave.c)
4. Glue drivers (like: i2c-designware-platdrv.c)
>
> Thanks,
> Luis
>
> On 10/18/2016 16:17, Wolfram Sang wrote:
> > > This is needed because the configuration is different and the i2c-
> > > designware
> > > cannot be master/slave without a reset. To resolve that I added
> > > this property
> > > to bind it as a slave when needed.
> >
> > Aww, pity that the HW can't do that. Do you have details why?
> >
> > If that is really a HW limitation, then I'd suggest having a
> > seperate
> > driver for slave-only mode so we can differentiate by compatible
> > strings.
> >
>
>
--
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/4] Added I2C_SLAVE as a dependency to I2C_DESIGNWARE_CORE Enable _slave() mode Review of the pm_runtime...() methods and cleaning
From: Andy Shevchenko @ 2016-10-21 10:52 UTC (permalink / raw)
To: Luis.Oliveira, jarkko.nikula, mika.westerberg, wsa, linux-i2c,
linux-kernel, robh+dt, mark.rutland, devicetree
Cc: CARLOS.PALMINHA, Ramiro.Oliveira
In-Reply-To: <ffbbaeace295a28ade33aafde7f287c6bcc901eb.1476462204.git.lolivei@synopsys.com>
On Fri, 2016-10-14 at 17:52 +0100, Luis.Oliveira@synopsys.com wrote:
> From: Luis Oliveira <lolivei@synopsys.com>
>
Same style issues here and in the code itself. Check all your patches
before submitting.
More comments below.
> @@ -785,9 +817,59 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
> return dev->functionality;
> }
>
> +static int i2c_dw_reg_slave(struct i2c_client *slave)
> +{
> + struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
Just '...dev = i2c...'.
> +
> + if(dev->slave)
> + return -EBUSY;
Hmm... Is it possible that function be called twice?
> + if(slave->flags & I2C_CLIENT_TEN)
> + return -EAFNOSUPPORT;
> + /* set slave address in the IC_SAR register,
> + * the address to which the DW_apb_i2c responds */
> +
> + __i2c_dw_enable(dev, false);
> +
> + dw_writel(dev, slave->addr, DW_IC_SAR);
> +
> + pm_runtime_get_sync(dev->dev);
> +
> + dev->slave = slave;
So, you are using this variable to serialize the calls. Assume it's
possible that upper layer calls several time the function how you
protect the potential race here?
No need runtime PM for this line as well.
> +
> + __i2c_dw_enable(dev, true);
> +
> + dev->cmd_err = 0;
> + dev->msg_write_idx = 0;
> + dev->msg_read_idx = 0;
> + dev->msg_err = 0;
> + dev->status = STATUS_IDLE;
> + dev->abort_source = 0;
> + dev->rx_outstanding = 0;
> +
> + return 0;
> +}
> +
> +static int i2c_dw_unreg_slave(struct i2c_client *slave)
> +{
> + struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
> +
> + WARN_ON(!dev->slave);
I'm not sure it's needed. Consider together with comment regarding
_reg_slave().
> +
> + i2c_dw_disable_int(dev);
> + i2c_dw_disable(dev);
> +
> + dev->slave = NULL;
Same comments as for _reg_slave().
> +
> + pm_runtime_put(dev->dev);
> +
> + return 0;
> +}
> +
> static struct i2c_algorithm i2c_dw_algo = {
> .master_xfer = i2c_dw_xfer,
> .functionality = i2c_dw_func,
> + .reg_slave = i2c_dw_reg_slave,
> + .unreg_slave = i2c_dw_unreg_slave,
> };
>
> static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
> @@ -821,8 +903,6 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
> dw_readl(dev, DW_IC_CLR_RX_OVER);
> if (stat & DW_IC_INTR_TX_OVER)
> dw_readl(dev, DW_IC_CLR_TX_OVER);
> - if (stat & DW_IC_INTR_RD_REQ)
> - dw_readl(dev, DW_IC_CLR_RD_REQ);
> if (stat & DW_IC_INTR_TX_ABRT) {
> /*
> * The IC_TX_ABRT_SOURCE register is cleared whenever
> @@ -849,6 +929,84 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
> * Interrupt service routine. This gets called whenever an I2C
> interrupt
> * occurs.
> */
> +
> +static bool i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev )
> +{
> + u32 raw_stat, stat, enabled;
> + u8 val, slave_activity;
> +
> + stat = dw_readl(dev, DW_IC_INTR_STAT);
> + enabled = dw_readl(dev, DW_IC_ENABLE);
> + raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> + slave_activity = ((dw_readl(dev,DW_IC_STATUS) &
> + DW_IC_STATUS_SLAVE_ACTIVITY)>>6);
> +
> + dev_dbg(dev->dev,
> + "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :
> INTR_STAT=%#x\n",
> + __func__, enabled, slave_activity, raw_stat, stat);
> +
> + if (stat & DW_IC_INTR_START_DET)
> + dw_readl(dev, DW_IC_CLR_START_DET);
> +
> + if (stat & DW_IC_INTR_ACTIVITY)
> + dw_readl(dev, DW_IC_CLR_ACTIVITY);
> +
> + if (stat & DW_IC_INTR_RX_OVER)
> + dw_readl(dev, DW_IC_CLR_RX_OVER);
> +
> + if ((stat & DW_IC_INTR_RX_FULL) && (stat &
> DW_IC_INTR_STOP_DET))
> + i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_REQUESTED, &val);
> +
> + if (slave_activity) {
> + if (stat & DW_IC_INTR_RD_REQ) {
> + if (stat & DW_IC_INTR_RX_FULL) {
> + val = dw_readl(dev, DW_IC_DATA_CMD);
> + if (!i2c_slave_event(dev->slave,
> + I2C_SLAVE_WRITE_RECEIVED,
> &val)) {
> + dev_dbg(dev->dev, "Byte %X
> acked! ",val);
> + }
> + dw_readl(dev, DW_IC_CLR_RD_REQ);
> + stat =
> i2c_dw_read_clear_intrbits(dev);
> + }
> + else {
Style is '} else {'.
> + dw_readl(dev, DW_IC_CLR_RD_REQ);
> + dw_readl(dev, DW_IC_CLR_RX_UNDER);
> + stat =
> i2c_dw_read_clear_intrbits(dev);
> + }
> + if (!i2c_slave_event(dev->slave,
> + I2C_SLAVE_READ_REQUESTED,
> &val))
> + dw_writel(dev, val, DW_IC_DATA_CMD);
> + }
> + }
> +
> + if (stat & DW_IC_INTR_RX_DONE) {
> +
> + if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_READ_PROCESSED, &val))
> + dw_readl(dev, DW_IC_CLR_RX_DONE);
> +
> + i2c_slave_event(dev->slave, I2C_SLAVE_STOP , &val);
> + stat = i2c_dw_read_clear_intrbits(dev);
> +
> + return true;
> + }
> +
> + if (stat & DW_IC_INTR_RX_FULL) {
> + val = dw_readl(dev, DW_IC_DATA_CMD);
> + if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_RECEIVED, &val))
> + dev_dbg(dev->dev, "Byte %X acked! ",val);
> + }
> + else {
> + i2c_slave_event(dev->slave, I2C_SLAVE_STOP , &val);
> + stat = i2c_dw_read_clear_intrbits(dev);
> + }
> +
> + if (stat & DW_IC_INTR_TX_OVER) {
> + dw_readl(dev, DW_IC_CLR_TX_OVER);
> + return true;
> + }
> +
> + return true;
> +}
>
> static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
> {
> @@ -910,14 +1068,20 @@ static irqreturn_t i2c_dw_isr(int this_irq,
> void *dev_id)
> enabled = dw_readl(dev, DW_IC_ENABLE);
> mode = dw_readl(dev, DW_IC_CON);
> stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> -
> +
Shouldn't be here.
> dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__,
> enabled, stat);
> if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
> return IRQ_NONE;
> -
> +
Ditto.
> + if (!(mode & DW_IC_CON_MASTER) && !(mode &
> DW_IC_CON_SLAVE_DISABLE)) {
> + stat = i2c_dw_read_clear_intrbits(dev);
> + if (!i2c_dw_irq_handler_slave(dev))
> + return IRQ_NONE;
> + } else {
> if(i2c_dw_irq_handler_master(dev))
> return IRQ_HANDLED;
> -
> + }
> +
Ditto.
> complete(&dev->cmd_complete);
> return IRQ_HANDLED;
> }
> @@ -984,7 +1148,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
> adap->dev.parent = dev->dev;
> i2c_set_adapdata(adap, dev);
>
> - i2c_dw_disable_int(dev);
> + if (!i2c_check_functionality(adap,I2C_FUNC_SLAVE))
> + i2c_dw_disable_int(dev);
> +
> r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
> IRQF_SHARED | IRQF_COND_SUSPEND,
> dev_name(dev->dev), dev);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index 0d44d2a..de5e4a0 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -28,9 +28,13 @@
> #define DW_IC_CON_SPEED_FAST 0x4
> #define DW_IC_CON_SPEED_HIGH 0x6
> #define DW_IC_CON_SPEED_MASK 0x6
> +#define DW_IC_CON_10BITADDR_SLAVE 0x8
> #define DW_IC_CON_10BITADDR_MASTER 0x10
> #define DW_IC_CON_RESTART_EN 0x20
> #define DW_IC_CON_SLAVE_DISABLE 0x40
> +#define DW_IC_CON_STOP_DET_IFADDRESSED 0x80
> +#define DW_IC_CON_TX_EMPTY_CTRL 0x100
> +#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL 0x200
>
Is it possible to split your approach to something like:
1. Add necessary definitions (not actual code).
2. Enable slave.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v2 1/4] Factor out _master() parts of code and identify as much as possible all related with MASTER mode
From: Andy Shevchenko @ 2016-10-21 10:37 UTC (permalink / raw)
To: Luis.Oliveira-HKixBCOQz3hWk0Htik3J/w,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
wsa-z923LK4zBo2bacvFa/9K2g, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <923d41c5a11538963fe5b49d181cfa8d1fec5756.1476462204.git.lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Fri, 2016-10-14 at 17:52 +0100, Luis.Oliveira-HKixBCOQz3hWk0Htik3J/w@public.gmane.org wrote:
> From: Luis Oliveira <lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
This wouldn't be here.
>
Something wrong with your commit message. Perhaps you were into SVN,
here is a bit different format of the commit messages, i.e.
1. Summary / Subject line — short description
2. Empty line
3. Full description in free form
4. Empty line
5. Tags, such as SoB (Signed-off-by), TB (Tested-by), Fixes, etc.
> @@ -201,6 +203,17 @@ static void dw_writel(struct dw_i2c_dev *dev, u32
> b, int offset)
> }
> }
>
> +static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
> +{
> + /* Configure Tx/Rx FIFO threshold levels */
> + dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
> + dw_writel(dev, 0, DW_IC_RX_TL);
> +
> + /* configure the i2c master */
I understand you just moved existing lines, though here is a good chance
to do small style fixes to the comments, i.e. use capital letter.
> + dw_writel(dev, dev->master_cfg , DW_IC_CON);
> + dw_writel(dev, DW_IC_INTR_MASTER_MASK,
> DW_IC_INTR_MASK);
Too much indentation. One tab is enough here.
> +}
> @@ -431,12 +444,9 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> "Hardware too old to adjust SDA hold
> time.\n");
> }
>
> - /* Configure Tx/Rx FIFO threshold levels */
> - dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
> - dw_writel(dev, 0, DW_IC_RX_TL);
> -
> - /* configure the i2c master */
> - dw_writel(dev, dev->master_cfg , DW_IC_CON);
> + if ((dev->master_cfg & DW_IC_CON_MASTER) &&
> + (dev->master_cfg & DW_IC_CON_SLAVE_DISABLE))
Ditto. Align both conditions to be like tabular view.
> + i2c_dw_configure_fifo_master(dev);
>
> i2c_dw_release_lock(dev);
>
> @@ -480,7 +490,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> */
> ic_status = dw_readl(dev, DW_IC_STATUS);
> if (!dev->dynamic_tar_update_enabled ||
> - (ic_status & DW_IC_STATUS_MST_ACTIVITY) ||
> + (ic_status & DW_IC_STATUS_MASTER_ACTIVITY) ||
> !(ic_status & DW_IC_STATUS_TFE)) {
> __i2c_dw_enable_and_wait(dev, false);
> enabled = false;
> @@ -520,7 +530,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
>
> /* Clear and enable interrupts */
> dw_readl(dev, DW_IC_CLR_INTR);
> - dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
> + dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
> }
>
> /*
> @@ -540,7 +550,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> u8 *buf = dev->tx_buf;
> bool need_restart = false;
>
> - intr_mask = DW_IC_INTR_DEFAULT_MASK;
> + intr_mask = DW_IC_INTR_MASTER_MASK;
>
> for (; dev->msg_write_idx < dev->msgs_num; dev-
> >msg_write_idx++) {
> /*
> @@ -839,19 +849,13 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
> * Interrupt service routine. This gets called whenever an I2C
> interrupt
Perhaps '...I2C master interrupt occurs.' Or move this comment towards
common handler function.
> * occurs.
> */
> -static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> -{
> - struct dw_i2c_dev *dev = dev_id;
> - u32 stat, enabled;
> -
> - enabled = dw_readl(dev, DW_IC_ENABLE);
> - stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> - dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__,
> enabled, stat);
> - if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
> - return IRQ_NONE;
>
> +static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
> +{
> + u32 stat;
> +
> stat = i2c_dw_read_clear_intrbits(dev);
> -
> +
This change shouldn't be here.
> if (stat & DW_IC_INTR_TX_ABRT) {
> dev->cmd_err |= DW_IC_ERR_TX_ABRT;
> dev->status = STATUS_IDLE;
> @@ -895,7 +899,26 @@ tx_aborted:
> i2c_dw_disable_int(dev);
> dw_writel(dev, stat, DW_IC_INTR_MASK);
> }
> + return true;
> +}
> +
> +static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> +{
> + struct dw_i2c_dev *dev = dev_id;
> + u32 stat, enabled, mode;
> +
> + enabled = dw_readl(dev, DW_IC_ENABLE);
> + mode = dw_readl(dev, DW_IC_CON);
Hmm... It's defined but not used?
> + stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> +
> + dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__,
> enabled, stat);
> + if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
> + return IRQ_NONE;
>
> + if(i2c_dw_irq_handler_master(dev))
Keep style — 'if ('.
> + return IRQ_HANDLED;
> +
Check for trailing whitespaces. Please, don't add new ones. Good start
is to run checkpatch.pl before send them.
> + complete(&dev->cmd_complete);
Didn't find removal of the line. Is it new code? Shouldn't be here if
so.
> return IRQ_HANDLED;
> }
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..d5986c2 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -138,6 +138,29 @@ static inline int dw_i2c_acpi_configure(struct
> platform_device *pdev)
> }
> #endif
>
> +static void i2c_dw_configure_master(struct platform_device *pdev)
> +{
> + struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> +
> + dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE
> |
> + DW_IC_CON_RESTART_EN;
> +
> + dev->functionality |= I2C_FUNC_10BIT_ADDR;
> + dev_info(&pdev->dev, "I am registed as a I2C Master!\n");
> +
> + switch (dev->clk_freq) {
> + case 100000:
> + dev->master_cfg |= DW_IC_CON_SPEED_STD;
> + break;
> + case 3400000:
> + dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
> + break;
> + default:
> + dev->master_cfg |= DW_IC_CON_SPEED_FAST;
> + }
> +
Again, trailing white space and moreover redundant entire line since
it's empty and doesn't bring any readability.
> +}
> +
> static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool
> prepare)
> {
> if (IS_ERR(i_dev->clk))
> @@ -222,19 +245,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> I2C_FUNC_SMBUS_WORD_DATA |
> I2C_FUNC_SMBUS_I2C_BLOCK;
>
> - dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE
> |
> - DW_IC_CON_RESTART_EN;
> -
> - switch (dev->clk_freq) {
> - case 100000:
> - dev->master_cfg |= DW_IC_CON_SPEED_STD;
> - break;
> - case 3400000:
> - dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
> - break;
> - default:
> - dev->master_cfg |= DW_IC_CON_SPEED_FAST;
> - }
> + i2c_dw_configure_master(pdev);
>
> dev->clk = devm_clk_get(&pdev->dev, NULL);
> if (!i2c_dw_plat_prepare_clk(dev, true)) {
--
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported
From: Luis Oliveira @ 2016-10-21 9:56 UTC (permalink / raw)
To: Wolfram Sang, Ramiro Oliveira
Cc: Mark Rutland, Luis.Oliveira, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, linux-kernel, robh+dt, devicetree,
CARLOS.PALMINHA
In-Reply-To: <20161018151706.GA1433@katana>
Since practically 90% of the code is shared between master and slave, I was
thinking if it will be acceptable to use the same driver for both but
differentiate the master/slave mode by the compatible strings.
Thanks,
Luis
On 10/18/2016 16:17, Wolfram Sang wrote:
>> This is needed because the configuration is different and the i2c-designware
>> cannot be master/slave without a reset. To resolve that I added this property
>> to bind it as a slave when needed.
> Aww, pity that the HW can't do that. Do you have details why?
>
> If that is really a HW limitation, then I'd suggest having a seperate
> driver for slave-only mode so we can differentiate by compatible
> strings.
>
^ permalink raw reply
* Re: [PATCH 06/10] gpio: gpio-i2cs: Document bindings of I2CS FPGA GPIO block
From: Linus Walleij @ 2016-10-21 8:59 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Lee Jones, Alexandre Courbot, Rob Herring, Mark Rutland,
Frank Rowand, Wolfram Sang, Richard Purdie, Jacek Anaszewski,
Jean Delvare, Peter Rosin, Avirup Banerjee, Georgi Vlaev,
Guenter Roeck, JawaharBalaji Thirumalaisamy,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org
In-Reply-To: <1475853669-22480-7-git-send-email-pantelis.antoniou@konsulko.com>
On Fri, Oct 7, 2016 at 5:21 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> +This is virtual gpio driver, that maps each bit of the I2CS FPGA to
> +a gpio. It's used as a compatibility replacement for FRUs that use
> +I2CS FPGA to report presence, control and report power status in
> +the Juniper's driver infra that uses gpios. Compatible with any I2CS.
What does "virtual" mean?
I regularly NACK patches that try to shoehorn stuff into
GPIO not because they are actually GPIO hardware drivers or
general purpose at all, but because it is convenient. Don't do this.
> +- i2c-gpio-map: Map of "I2CS register" and "direction". The registers
> + are 8 bit wide, each bit of the register is mapped to either
> + input or output depending on the bits of the "direction". If
> + the bit in the direction is 1, then that bit from the I2CS
> + register is mapped to gpio input, otherwise to gpio output.
Does this mean that the hardware is synthesized so that some lines
are hardwired as input and some hardwired as output?
That again does not sound like "general purpose" at all.
I think this happens in other hardware and should be a generic
binding like
gpio-lines-input-only = <0>, <4>;
gpio-lines-output-only = <7>, <8>;
And no bitmasking, use the local offset, parse and construct the
mask.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 05/10] gpio: i2cs: Juniper I2CS to GPIO pin mapping driver
From: Linus Walleij @ 2016-10-21 8:41 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Lee Jones, Alexandre Courbot, Rob Herring, Mark Rutland,
Frank Rowand, Wolfram Sang, Richard Purdie, Jacek Anaszewski,
Jean Delvare, Peter Rosin, Avirup Banerjee, Georgi Vlaev,
Guenter Roeck, JawaharBalaji Thirumalaisamy,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org
In-Reply-To: <1475853669-22480-6-git-send-email-pantelis.antoniou@konsulko.com>
On Fri, Oct 7, 2016 at 5:21 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> From: Guenter Roeck <groeck@juniper.net>
>
> This driver maps I2C slave register bits to GPIO pins. Registers
> are supposed to be 8 bit wide. Interrupt support is optional.
>
> The driver is implemented as client of the I2CS MFD driver.
>
> Signed-off-by: Georgi Vlaev <gvlaev@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> Signed-off-by: JawaharBalaji Thirumalaisamy <jawaharb@juniper.net>
> [Ported from Juniper kernel]
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
This driver again has the same review comments as the two first
ones, GPIOLIB_IRQCHIP etc. Revise accordingly and repost
and I will look at it again.
(Interestingly this one uses BIT() properly.)
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 3/4] gpio: ptxpmb-ext-cpld: Add driver for Juniper's PTXPMB extended CPLD
From: Linus Walleij @ 2016-10-21 8:39 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Lee Jones, Alexandre Courbot, Rob Herring, Mark Rutland,
Frank Rowand, Georgi Vlaev, Guenter Roeck,
JawaharBalaji Thirumalaisamy,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1475853574-22339-4-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
On Fri, Oct 7, 2016 at 5:19 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> From: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>
> This IP block is present in the PTXPMB extended CPLD present on
> Junipers PTX series of routers and provides SIB connector status pins
> as GPIO pins for use with other drivers.
>
> Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> Signed-off-by: JawaharBalaji Thirumalaisamy <jawaharb-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> [Ported from Juniper kernel]
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
This driver has pretty much the same issues as the two others
I reviewed yesterday. Please address the same comments
and repost and I will look at it again!
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH resend v4] i2c: hibvt: add Hisilicon BVT I2C controller driver
From: Pan Wen @ 2016-10-21 8:17 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland
Cc: linux-i2c, devicetree, linux-kernel, howell.yang, xuejiancheng,
jalen.hsu, lvkuanliang, suwenping, raojun, kevin.lixu, wenpan
add Hisilicon BVT I2C controller driver support.
Signed-off-by: Pan Wen <wenpan@hisilicon.com>
Acked-by: Rob Herring <robh@kernel.org>
---
change log
v4:
Modify the default frequency to 100KHz.
v3:
Add a SoC specific compatible string.
v2:
1)Fixed a compile error.
2)Dropped the clock-names property.
.../devicetree/bindings/i2c/i2c-hibvt.txt | 24 +
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-hibvt.c | 737 +++++++++++++++++++++
4 files changed, 772 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-hibvt.txt
create mode 100644 drivers/i2c/busses/i2c-hibvt.c
diff --git a/Documentation/devicetree/bindings/i2c/i2c-hibvt.txt b/Documentation/devicetree/bindings/i2c/i2c-hibvt.txt
new file mode 100644
index 0000000..db3d2e2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-hibvt.txt
@@ -0,0 +1,24 @@
+Hisilicon BVT I2C master controller
+
+Required properties:
+- compatible: should be "hisilicon,hibvt-i2c" and one of the following:
+ "hisilicon,hi3516cv300-i2c"
+- reg: physical base address of the controller and length of memory mapped.
+ region.
+- interrupts: interrupt number to the cpu.
+- clocks: phandles to input clocks.
+
+Optional properties:
+- clock-frequency: Desired I2C bus frequency in Hz, otherwise defaults to 100000.
+
+Other properties:
+see Documentation/devicetree/bindings/i2c/i2c.txt.
+
+Examples:
+i2c_bus0: i2c@12110000 {
+ compatible = "hisilicon,hi3516cv300-i2c", "hisilicon,hibvt-i2c";
+ reg = <0x12110000 0x100>;
+ interrupts = <20>;
+ clocks = <&crg_ctrl HI3516CV300_APB_CLK>;
+ clock-frequency = <100000>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5c3993b..fc1b679 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -555,6 +555,16 @@ config I2C_GPIO
This is a very simple bitbanging I2C driver utilizing the
arch-neutral GPIO API to control the SCL and SDA lines.
+config I2C_HIBVT
+ tristate "Hisilicon BVT I2C Controller"
+ depends on ARCH_HISI
+ help
+ Say Y here to include support for Hisilicon BVT I2C controller in the
+ Hisilicon BVT SoCs.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-hibvt.
+
config I2C_HIGHLANDER
tristate "Highlander FPGA SMBus interface"
depends on SH_HIGHLANDER
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 37f2819..42ef2e0 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_I2C_EG20T) += i2c-eg20t.o
obj-$(CONFIG_I2C_EMEV2) += i2c-emev2.o
obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o
obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o
+obj-$(CONFIG_I2C_HIBVT) += i2c-hibvt.o
obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o
obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o
obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o
diff --git a/drivers/i2c/busses/i2c-hibvt.c b/drivers/i2c/busses/i2c-hibvt.c
new file mode 100644
index 0000000..e8af63e
--- /dev/null
+++ b/drivers/i2c/busses/i2c-hibvt.c
@@ -0,0 +1,737 @@
+/*
+ * Hisilicon BVT I2C Controller Driver
+ *
+ * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
+ *
+ * Authors: wenpan@hisilicon.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+/*
+ * I2C Registers offsets
+ */
+#define HIBVT_I2C_GLB 0x0
+#define HIBVT_I2C_SCL_H 0x4
+#define HIBVT_I2C_SCL_L 0x8
+#define HIBVT_I2C_DATA1 0x10
+#define HIBVT_I2C_TXF 0x20
+#define HIBVT_I2C_RXF 0x24
+#define HIBVT_I2C_CMD_BASE 0x30
+#define HIBVT_I2C_LOOP1 0xb0
+#define HIBVT_I2C_DST1 0xb4
+#define HIBVT_I2C_TX_WATER 0xc8
+#define HIBVT_I2C_RX_WATER 0xcc
+#define HIBVT_I2C_CTRL1 0xd0
+#define HIBVT_I2C_STAT 0xd8
+#define HIBVT_I2C_INTR_RAW 0xe0
+#define HIBVT_I2C_INTR_EN 0xe4
+#define HIBVT_I2C_INTR_STAT 0xe8
+
+/*
+ * I2C Global Config Register -- HIBVT_I2C_GLB
+ */
+#define GLB_EN_MASK BIT(0)
+#define GLB_SDA_HOLD_MASK GENMASK(23, 8)
+#define GLB_SDA_HOLD_SHIFT (8)
+
+/*
+ * I2C Timing CMD Register -- HIBVT_I2C_CMD_BASE + n * 4 (n = 0, 1, 2, ... 31)
+ */
+#define CMD_EXIT 0x0
+#define CMD_TX_S 0x1
+#define CMD_TX_D1_2 0x4
+#define CMD_TX_D1_1 0x5
+#define CMD_TX_FIFO 0x9
+#define CMD_RX_FIFO 0x12
+#define CMD_RX_ACK 0x13
+#define CMD_IGN_ACK 0x15
+#define CMD_TX_ACK 0x16
+#define CMD_TX_NACK 0x17
+#define CMD_JMP1 0x18
+#define CMD_UP_TXF 0x1d
+#define CMD_TX_RS 0x1e
+#define CMD_TX_P 0x1f
+
+/*
+ * I2C Control Register 1 -- HIBVT_I2C_CTRL1
+ */
+#define CTRL1_CMD_START_MASK BIT(0)
+
+/*
+ * I2C Status Register -- HIBVT_I2C_STAT
+ */
+#define STAT_RXF_NOE_MASK BIT(16) /* RX FIFO not empty flag */
+#define STAT_TXF_NOF_MASK BIT(19) /* TX FIFO not full flag */
+
+
+/*
+ * I2C Interrupt status and mask Register --
+ * HIBVT_I2C_INTR_RAW, HIBVT_I2C_STAT, HIBVT_I2C_INTR_STAT
+ */
+#define INTR_ABORT_MASK (BIT(0) | BIT(11))
+#define INTR_RX_MASK BIT(2)
+#define INTR_TX_MASK BIT(4)
+#define INTR_CMD_DONE_MASK BIT(12)
+#define INTR_USE_MASK (INTR_ABORT_MASK \
+ |INTR_RX_MASK \
+ | INTR_TX_MASK \
+ | INTR_CMD_DONE_MASK)
+#define INTR_ALL_MASK GENMASK(31, 0)
+
+#define I2C_DEFAULT_FREQUENCY 100000
+#define I2C_TXF_DEPTH 64
+#define I2C_RXF_DEPTH 64
+#define I2C_TXF_WATER 32
+#define I2C_RXF_WATER 32
+#define I2C_WAIT_TIMEOUT 0x10000
+#define I2C_IRQ_TIMEOUT (msecs_to_jiffies(1000))
+
+
+struct hibvt_i2c_dev {
+ struct device *dev;
+ struct i2c_adapter adap;
+ void __iomem *base;
+ struct clk *clk;
+ int irq;
+
+ unsigned int freq;
+ struct i2c_msg *msg;
+ unsigned int msg_num;
+ unsigned int msg_idx;
+ unsigned int msg_buf_ptr;
+ struct completion msg_complete;
+
+ spinlock_t lock;
+ int status;
+};
+
+static inline void hibvt_i2c_disable(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_GLB);
+ val &= ~GLB_EN_MASK;
+ writel(val, i2c->base + HIBVT_I2C_GLB);
+}
+
+static inline void hibvt_i2c_enable(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_GLB);
+ val |= GLB_EN_MASK;
+ writel(val, i2c->base + HIBVT_I2C_GLB);
+}
+
+static inline void hibvt_i2c_cfg_irq(struct hibvt_i2c_dev *i2c,
+ unsigned int flag)
+{
+ writel(flag, i2c->base + HIBVT_I2C_INTR_EN);
+}
+
+static inline void hibvt_i2c_disable_irq(struct hibvt_i2c_dev *i2c,
+ unsigned int flag)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_INTR_EN);
+ val &= ~flag;
+ writel(val, i2c->base + HIBVT_I2C_INTR_EN);
+}
+
+static inline unsigned int hibvt_i2c_clr_irq(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_INTR_STAT);
+ writel(INTR_ALL_MASK, i2c->base + HIBVT_I2C_INTR_RAW);
+
+ return val;
+}
+
+static inline void hibvt_i2c_cmdreg_set(struct hibvt_i2c_dev *i2c,
+ unsigned int cmd, unsigned int *offset)
+{
+ dev_dbg(i2c->dev, "hii2c reg: offset=0x%x, cmd=0x%x...\n",
+ *offset * 4, cmd);
+ writel(cmd, i2c->base + HIBVT_I2C_CMD_BASE + *offset * 4);
+ (*offset)++;
+}
+
+/*
+ * config i2c slave addr
+ */
+static inline void hibvt_i2c_set_addr(struct hibvt_i2c_dev *i2c)
+{
+ struct i2c_msg *msg = i2c->msg;
+ u16 addr;
+
+ if (msg->flags & I2C_M_TEN) {
+ /* First byte is 11110XX0 where XX is upper 2 bits */
+ addr = ((msg->addr & 0x300) << 1) | 0xf000;
+ if (msg->flags & I2C_M_RD)
+ addr |= 1 << 8;
+
+ /* Second byte is the remaining 8 bits */
+ addr |= msg->addr & 0xff;
+ } else {
+ addr = (msg->addr & 0x7f) << 1;
+ if (msg->flags & I2C_M_RD)
+ addr |= 1;
+ }
+
+ writel(addr, i2c->base + HIBVT_I2C_DATA1);
+}
+
+/*
+ * Start command sequence
+ */
+static inline void hibvt_i2c_start_cmd(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_CTRL1);
+ val |= CTRL1_CMD_START_MASK;
+ writel(val, i2c->base + HIBVT_I2C_CTRL1);
+}
+
+static int hibvt_i2c_wait_rx_noempty(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int time_cnt = 0;
+ unsigned int val;
+
+ do {
+ val = readl(i2c->base + HIBVT_I2C_STAT);
+ if (val & STAT_RXF_NOE_MASK)
+ return 0;
+
+ udelay(50);
+ } while (time_cnt++ < I2C_WAIT_TIMEOUT);
+
+ dev_err(i2c->dev, "wait rx no empty timeout, RIS: 0x%x, SR: 0x%x\n",
+ readl(i2c->base + HIBVT_I2C_INTR_RAW), val);
+ return -EIO;
+}
+
+static int hibvt_i2c_wait_tx_nofull(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int time_cnt = 0;
+ unsigned int val;
+
+ do {
+ val = readl(i2c->base + HIBVT_I2C_STAT);
+ if (val & STAT_TXF_NOF_MASK)
+ return 0;
+
+ udelay(50);
+ } while (time_cnt++ < I2C_WAIT_TIMEOUT);
+
+ dev_err(i2c->dev, "wait rx no empty timeout, RIS: 0x%x, SR: 0x%x\n",
+ readl(i2c->base + HIBVT_I2C_INTR_RAW), val);
+ return -EIO;
+}
+
+static int hibvt_i2c_wait_idle(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int time_cnt = 0;
+ unsigned int val;
+
+ do {
+ val = readl(i2c->base + HIBVT_I2C_INTR_RAW);
+ if (val & (INTR_ABORT_MASK)) {
+ dev_err(i2c->dev, "wait idle abort!, RIS: 0x%x\n",
+ val);
+ return -EIO;
+ }
+
+ if (val & INTR_CMD_DONE_MASK)
+ return 0;
+
+ udelay(50);
+ } while (time_cnt++ < I2C_WAIT_TIMEOUT);
+
+ dev_err(i2c->dev, "wait idle timeout, RIS: 0x%x, SR: 0x%x\n",
+ val, readl(i2c->base + HIBVT_I2C_STAT));
+
+ return -EIO;
+}
+
+static void hibvt_i2c_set_freq(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int max_freq, freq;
+ unsigned int clk_rate;
+ unsigned int val, sda_hold;
+
+ freq = i2c->freq;
+ clk_rate = clk_get_rate(i2c->clk);
+ max_freq = clk_rate >> 1;
+
+ if (freq > max_freq) {
+ i2c->freq = max_freq;
+ freq = i2c->freq;
+ }
+
+ if (freq <= 100000) {
+ val = clk_rate / (freq * 2) - 1;
+ writel(val, i2c->base + HIBVT_I2C_SCL_H);
+ writel(val, i2c->base + HIBVT_I2C_SCL_L);
+ } else {
+ val = (clk_rate * 36) / (freq * 100);
+ writel(val, i2c->base + HIBVT_I2C_SCL_H);
+ val = (clk_rate * 64) / (freq * 100);
+ writel(val, i2c->base + HIBVT_I2C_SCL_L);
+ }
+
+ sda_hold = val * 3 / 10;
+ sda_hold = (sda_hold << GLB_SDA_HOLD_SHIFT) & GLB_SDA_HOLD_MASK;
+ val = readl(i2c->base + HIBVT_I2C_GLB);
+ val &= ~GLB_SDA_HOLD_MASK;
+ val |= sda_hold;
+ writel(val, i2c->base + HIBVT_I2C_GLB);
+}
+
+/*
+ * set i2c controller TX and RX FIFO water
+ */
+static inline void hibvt_i2c_set_water(struct hibvt_i2c_dev *i2c)
+{
+ writel(I2C_TXF_WATER, i2c->base + HIBVT_I2C_TX_WATER);
+ writel(I2C_RXF_WATER, i2c->base + HIBVT_I2C_RX_WATER);
+}
+
+/*
+ * initialise the controller, set i2c bus interface freq
+ */
+static void hibvt_i2c_hw_init(struct hibvt_i2c_dev *i2c)
+{
+ hibvt_i2c_disable(i2c);
+ hibvt_i2c_disable_irq(i2c, INTR_ALL_MASK);
+ hibvt_i2c_set_freq(i2c);
+ hibvt_i2c_set_water(i2c);
+}
+
+/*
+ * hibvt_i2c_cfg_cmd - config i2c controller command sequence
+ *
+ * After all the timing command is configured,
+ * and then start the command, you can i2c communication,
+ * and then only need to read and write i2c fifo.
+ */
+static void hibvt_i2c_cfg_cmd(struct hibvt_i2c_dev *i2c)
+{
+ struct i2c_msg *msg = i2c->msg;
+ int offset = 0;
+
+ if (i2c->msg_idx == 0)
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_S, &offset);
+ else
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_RS, &offset);
+
+ if (msg->flags & I2C_M_TEN) {
+ if (i2c->msg_idx == 0) {
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_D1_2, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_D1_1, &offset);
+ } else {
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_D1_2, &offset);
+ }
+ } else {
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_D1_1, &offset);
+ }
+
+ if (msg->flags & I2C_M_IGNORE_NAK)
+ hibvt_i2c_cmdreg_set(i2c, CMD_IGN_ACK, &offset);
+ else
+ hibvt_i2c_cmdreg_set(i2c, CMD_RX_ACK, &offset);
+
+ if (msg->flags & I2C_M_RD) {
+ if (msg->len >= 2) {
+ writel(offset, i2c->base + HIBVT_I2C_DST1);
+ writel(msg->len - 2, i2c->base + HIBVT_I2C_LOOP1);
+ hibvt_i2c_cmdreg_set(i2c, CMD_RX_FIFO, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_ACK, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_JMP1, &offset);
+ }
+ hibvt_i2c_cmdreg_set(i2c, CMD_RX_FIFO, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_NACK, &offset);
+ } else {
+ writel(offset, i2c->base + HIBVT_I2C_DST1);
+ writel(msg->len - 1, i2c->base + HIBVT_I2C_LOOP1);
+ hibvt_i2c_cmdreg_set(i2c, CMD_UP_TXF, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_FIFO, &offset);
+
+ if (msg->flags & I2C_M_IGNORE_NAK)
+ hibvt_i2c_cmdreg_set(i2c, CMD_IGN_ACK, &offset);
+ else
+ hibvt_i2c_cmdreg_set(i2c, CMD_RX_ACK, &offset);
+
+ hibvt_i2c_cmdreg_set(i2c, CMD_JMP1, &offset);
+ }
+
+ if ((i2c->msg_idx == (i2c->msg_num - 1)) || (msg->flags & I2C_M_STOP)) {
+ dev_dbg(i2c->dev, "run to %s %d...TX STOP\n",
+ __func__, __LINE__);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_P, &offset);
+ }
+
+ hibvt_i2c_cmdreg_set(i2c, CMD_EXIT, &offset);
+}
+
+static int hibvt_i2c_polling_xfer_one_msg(struct hibvt_i2c_dev *i2c)
+{
+ int status;
+ unsigned int val;
+ struct i2c_msg *msg = i2c->msg;
+
+ dev_dbg(i2c->dev, "[%s,%d]msg->flags=0x%x, len=0x%x\n",
+ __func__, __LINE__, msg->flags, msg->len);
+
+ hibvt_i2c_enable(i2c);
+ hibvt_i2c_clr_irq(i2c);
+ hibvt_i2c_set_addr(i2c);
+ hibvt_i2c_cfg_cmd(i2c);
+ hibvt_i2c_start_cmd(i2c);
+
+ i2c->msg_buf_ptr = 0;
+
+ if (msg->flags & I2C_M_RD) {
+ while (i2c->msg_buf_ptr < msg->len) {
+ status = hibvt_i2c_wait_rx_noempty(i2c);
+ if (status)
+ goto end;
+
+ val = readl(i2c->base + HIBVT_I2C_RXF);
+ msg->buf[i2c->msg_buf_ptr] = val;
+ i2c->msg_buf_ptr++;
+
+ }
+ } else {
+ while (i2c->msg_buf_ptr < msg->len) {
+ status = hibvt_i2c_wait_tx_nofull(i2c);
+ if (status)
+ goto end;
+
+ val = msg->buf[i2c->msg_buf_ptr];
+ writel(val, i2c->base + HIBVT_I2C_TXF);
+ i2c->msg_buf_ptr++;
+ }
+ }
+
+ status = hibvt_i2c_wait_idle(i2c);
+end:
+ hibvt_i2c_disable(i2c);
+
+ return status;
+}
+
+static irqreturn_t hibvt_i2c_isr(int irq, void *dev_id)
+{
+ struct hibvt_i2c_dev *i2c = dev_id;
+ unsigned int irq_status;
+ struct i2c_msg *msg = i2c->msg;
+
+ spin_lock(&i2c->lock);
+
+ irq_status = hibvt_i2c_clr_irq(i2c);
+ dev_dbg(i2c->dev, "%s RIS: 0x%x\n", __func__, irq_status);
+
+ if (!irq_status) {
+ dev_dbg(i2c->dev, "no irq\n");
+ goto end;
+ }
+
+ if (irq_status & INTR_ABORT_MASK) {
+ dev_err(i2c->dev, "irq handle abort, RIS: 0x%x\n",
+ irq_status);
+ i2c->status = -EIO;
+ hibvt_i2c_disable_irq(i2c, INTR_ALL_MASK);
+
+ complete(&i2c->msg_complete);
+ goto end;
+ }
+
+ if (msg->flags & I2C_M_RD) {
+ while ((readl(i2c->base + HIBVT_I2C_STAT) & STAT_RXF_NOE_MASK)
+ && (i2c->msg_buf_ptr < msg->len)) {
+ msg->buf[i2c->msg_buf_ptr] =
+ readl(i2c->base + HIBVT_I2C_RXF);
+ i2c->msg_buf_ptr++;
+ }
+ } else {
+ while ((readl(i2c->base + HIBVT_I2C_STAT) & STAT_TXF_NOF_MASK)
+ && (i2c->msg_buf_ptr < msg->len)) {
+ writel(msg->buf[i2c->msg_buf_ptr],
+ i2c->base + HIBVT_I2C_TXF);
+ i2c->msg_buf_ptr++;
+ }
+ }
+
+ if (i2c->msg_buf_ptr >= msg->len)
+ hibvt_i2c_disable_irq(i2c, INTR_TX_MASK | INTR_RX_MASK);
+
+ if (irq_status & INTR_CMD_DONE_MASK) {
+ dev_dbg(i2c->dev, "cmd done\n");
+ i2c->status = 0;
+ hibvt_i2c_disable_irq(i2c, INTR_ALL_MASK);
+
+ complete(&i2c->msg_complete);
+ }
+
+end:
+ spin_unlock(&i2c->lock);
+
+ return IRQ_HANDLED;
+}
+
+static int hibvt_i2c_interrupt_xfer_one_msg(struct hibvt_i2c_dev *i2c)
+{
+ int status;
+ struct i2c_msg *msg = i2c->msg;
+ unsigned long timeout;
+ unsigned long flags;
+
+ dev_dbg(i2c->dev, "[%s,%d]msg->flags=0x%x, len=0x%x\n",
+ __func__, __LINE__, msg->flags, msg->len);
+
+ reinit_completion(&i2c->msg_complete);
+ i2c->msg_buf_ptr = 0;
+ i2c->status = -EIO;
+
+ spin_lock_irqsave(&i2c->lock, flags);
+ hibvt_i2c_enable(i2c);
+ hibvt_i2c_clr_irq(i2c);
+ if (msg->flags & I2C_M_RD)
+ hibvt_i2c_cfg_irq(i2c, INTR_USE_MASK & ~INTR_TX_MASK);
+ else
+ hibvt_i2c_cfg_irq(i2c, INTR_USE_MASK & ~INTR_RX_MASK);
+
+ hibvt_i2c_set_addr(i2c);
+ hibvt_i2c_cfg_cmd(i2c);
+ hibvt_i2c_start_cmd(i2c);
+ spin_unlock_irqrestore(&i2c->lock, flags);
+
+ timeout = wait_for_completion_timeout(&i2c->msg_complete,
+ I2C_IRQ_TIMEOUT);
+
+ if (timeout == 0) {
+ hibvt_i2c_disable_irq(i2c, INTR_ALL_MASK);
+ status = -EIO;
+ dev_err(i2c->dev, "%s timeout\n",
+ msg->flags & I2C_M_RD ? "rx" : "tx");
+ } else {
+ status = i2c->status;
+ }
+
+ hibvt_i2c_disable(i2c);
+
+ return status;
+}
+
+/*
+ * Master transfer function
+ */
+static int hibvt_i2c_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct hibvt_i2c_dev *i2c = i2c_get_adapdata(adap);
+ int status;
+
+ if (!msgs) {
+ dev_err(i2c->dev, "msgs == NULL\n");
+ return -EIO;
+ }
+
+ i2c->msg = msgs;
+ i2c->msg_num = num;
+ i2c->msg_idx = 0;
+
+ if (i2c->irq >= 0) {
+ while (i2c->msg_idx < i2c->msg_num) {
+ status = hibvt_i2c_interrupt_xfer_one_msg(i2c);
+ if (status)
+ break;
+
+ i2c->msg++;
+ i2c->msg_idx++;
+ }
+ } else {
+ while (i2c->msg_idx < i2c->msg_num) {
+ status = hibvt_i2c_polling_xfer_one_msg(i2c);
+ if (status)
+ break;
+
+ i2c->msg++;
+ i2c->msg_idx++;
+ }
+ }
+
+ if (!status || i2c->msg_idx > 0)
+ status = i2c->msg_idx;
+
+ return status;
+}
+
+static u32 hibvt_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR
+ | I2C_FUNC_PROTOCOL_MANGLING;
+}
+
+static const struct i2c_algorithm hibvt_i2c_algo = {
+ .master_xfer = hibvt_i2c_xfer,
+ .functionality = hibvt_i2c_func,
+};
+
+static int hibvt_i2c_probe(struct platform_device *pdev)
+{
+ int status;
+ struct hibvt_i2c_dev *i2c;
+ struct i2c_adapter *adap;
+ struct resource *res;
+
+ i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, i2c);
+ i2c->dev = &pdev->dev;
+ spin_lock_init(&i2c->lock);
+ init_completion(&i2c->msg_complete);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ i2c->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(i2c->base)) {
+ dev_err(i2c->dev, "cannot ioremap resource\n");
+ return -ENOMEM;
+ }
+
+ i2c->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(i2c->clk)) {
+ dev_err(i2c->dev, "cannot get clock\n");
+ return -ENOENT;
+ }
+ clk_prepare_enable(i2c->clk);
+
+ if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+ &i2c->freq)) {
+ dev_warn(i2c->dev, "setting default clock-frequency@%dHz\n",
+ I2C_DEFAULT_FREQUENCY);
+ i2c->freq = I2C_DEFAULT_FREQUENCY;
+ }
+
+ /* i2c controller initialization, disable interrupt */
+ hibvt_i2c_hw_init(i2c);
+
+ i2c->irq = platform_get_irq(pdev, 0);
+ status = devm_request_irq(&pdev->dev, i2c->irq, hibvt_i2c_isr,
+ IRQF_SHARED, dev_name(&pdev->dev), i2c);
+ if (status) {
+ dev_dbg(i2c->dev, "falling back to polling mode");
+ i2c->irq = -1;
+ }
+
+ adap = &i2c->adap;
+ i2c_set_adapdata(adap, i2c);
+ adap->owner = THIS_MODULE;
+ strlcpy(adap->name, "hibvt-i2c", sizeof(adap->name));
+ adap->dev.parent = &pdev->dev;
+ adap->dev.of_node = pdev->dev.of_node;
+ adap->algo = &hibvt_i2c_algo;
+
+ /* Add the i2c adapter */
+ status = i2c_add_adapter(adap);
+ if (status) {
+ dev_err(i2c->dev, "failed to add bus to i2c core\n");
+ goto err_add_adapter;
+ }
+
+ dev_info(i2c->dev, "%s%d@%dhz registered\n",
+ adap->name, adap->nr, i2c->freq);
+
+ return 0;
+
+err_add_adapter:
+ clk_disable_unprepare(i2c->clk);
+ return status;
+}
+
+static int hibvt_i2c_remove(struct platform_device *pdev)
+{
+ struct hibvt_i2c_dev *i2c = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(i2c->clk);
+ i2c_del_adapter(&i2c->adap);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int hibvt_i2c_suspend(struct device *dev)
+{
+ struct hibvt_i2c_dev *i2c = dev_get_drvdata(dev);
+
+ i2c_lock_adapter(&i2c->adap);
+ clk_disable_unprepare(i2c->clk);
+ i2c_unlock_adapter(&i2c->adap);
+
+ return 0;
+}
+
+static int hibvt_i2c_resume(struct device *dev)
+{
+ struct hibvt_i2c_dev *i2c = dev_get_drvdata(dev);
+
+ i2c_lock_adapter(&i2c->adap);
+ clk_prepare_enable(i2c->clk);
+ hibvt_i2c_hw_init(i2c);
+ i2c_unlock_adapter(&i2c->adap);
+
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(hibvt_i2c_dev_pm, hibvt_i2c_suspend,
+ hibvt_i2c_resume);
+
+static const struct of_device_id hibvt_i2c_match[] = {
+ { .compatible = "hisilicon,hibvt-i2c"},
+ { .compatible = "hisilicon,hi3516cv300-i2c"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, hibvt_i2c_match);
+
+static struct platform_driver hibvt_i2c_driver = {
+ .driver = {
+ .name = "hibvt-i2c",
+ .of_match_table = hibvt_i2c_match,
+ .pm = &hibvt_i2c_dev_pm,
+ },
+ .probe = hibvt_i2c_probe,
+ .remove = hibvt_i2c_remove,
+};
+
+module_platform_driver(hibvt_i2c_driver);
+
+MODULE_AUTHOR("Pan Wen, <wenpan@hisilicon.com>");
+MODULE_DESCRIPTION("HISILICON BVT I2C Bus driver");
+MODULE_LICENSE("GPL v2");
--
2.9.3
^ permalink raw reply related
* Re: [PATCH 05/10] gpio: Introduce SAM gpio driver
From: Linus Walleij @ 2016-10-20 23:06 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Lee Jones, Alexandre Courbot, Rob Herring, Mark Rutland,
Frank Rowand, Wolfram Sang, David Woodhouse, Brian Norris,
Florian Fainelli, Wim Van Sebroeck, Peter Rosin, Debjit Ghosh,
Georgi Vlaev, Guenter Roeck, Maryam Seraj,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-i2c
In-Reply-To: <1475853518-22264-6-git-send-email-pantelis.antoniou@konsulko.com>
n Fri, Oct 7, 2016 at 5:18 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> From: Guenter Roeck <groeck@juniper.net>
>
> The SAM GPIO IP block is present in the Juniper PTX series
> of routers as part of the SAM FPGA.
>
> Signed-off-by: Georgi Vlaev <gvlaev@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
> [Ported from Juniper kernel]
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
First copy/paste my other review comments on the previous driver
I reviewed, this seems to have pretty much all the same issues.
> +config GPIO_SAM
> + tristate "SAM FPGA GPIO"
> + depends on MFD_JUNIPER_SAM
> + default y if MFD_JUNIPER_SAM
I suspect this should use
select GPIOLIB_IRQCHIP
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/gpio.h>
<linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
Not needed with GPIOLIB_IRQCHIP
> +#include <linux/errno.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
Why?
> +#include <linux/mfd/sam.h>
> +
> +/* gpio status/configuration */
> +#define SAM_GPIO_NEG_EDGE (1 << 8)
> +#define SAM_GPIO_NEG_EDGE_EN (1 << 7)
> +#define SAM_GPIO_POS_EDGE (1 << 6)
> +#define SAM_GPIO_POS_EDGE_EN (1 << 5)
Interrupt triggers I suppose.
> +#define SAM_GPIO_BLINK (1 << 4)
Cool, we don't support that in gpiolib as of now.
Maybe we should.
> +#define SAM_GPIO_OUT (1 << 3)
> +#define SAM_GPIO_OUT_TS (1 << 2)
OUT_TS ... what does TS mean here?
> +#define SAM_GPIO_DEBOUNCE_EN (1 << 1)
> +#define SAM_GPIO_IN (1 << 0)
> +
> +#define SAM_GPIO_BASE 0x1000
Base into what, and why is this not coming from PCI
or the device tree?
> +#define SAM_MAX_NGPIO 512
Why do we need to know this and what does it really mean?
That is the max number the GPIO subsystem can handle by
the way.
> +#define SAM_GPIO_ADDR(addr, nr) ((addr) + SAM_GPIO_BASE + (nr) * sizeof(u32))
Why can't we just offset the address earlier, ah well it's OK.
> +struct sam_gpio_irq_group {
> + int start; /* 1st gpio pin */
> + int count; /* # of pins in group */
> + int num_enabled; /* # of enabled interrupts */
> +};
> +
> +/**
> + * struct sam_gpio - GPIO private data structure.
> + * @base: PCI base address of Memory mapped I/O register.
> + * @dev: Pointer to device structure.
> + * @gpio: Data for GPIO infrastructure.
> + * @gpio_base: 1st gpio pin
> + * @gpio_count: # of gpio pins
> + * @irq_lock: Lock used by interrupt subsystem
> + * @domain: Pointer to interrupt domain
> + * @irq: Interrupt # from parent
> + * @irq_high: Second interrupt # from parent
> + * (currently unused)
> + * @irq_group: Interrupt group descriptions
> + * (one group per interrupt bit)
> + * @irq_type: The interrupt type for each gpio pin
> + */
Why do you need to keep all of this around? Is is all really
used? gpio_base makes me nervous we generally use dynamic
allocation of GPIO numbers these days.
> +struct sam_gpio {
> + void __iomem *base;
> + struct device *dev;
> + struct gpio_chip gpio;
> + int gpio_base;
> + int gpio_count;
> + struct mutex irq_lock;
> + struct irq_domain *domain;
> + int irq;
> + int irq_high;
> + struct sam_gpio_irq_group irq_group[18];
> + u8 irq_type[SAM_MAX_NGPIO];
> + struct sam_platform_data *pdata;
> + const char **names;
> + u32 *export_flags;
> +};
> +#define to_sam(chip) container_of((chip), struct sam_gpio, gpio)
Instead of this use gpiochip_get_data(). Applies everywhere.
> +static void sam_gpio_bitop(struct sam_gpio *sam, unsigned int nr,
> + u32 bit, bool set)
> +{
> + u32 reg;
> +
> + reg = ioread32(SAM_GPIO_ADDR(sam->base, nr));
> + if (set)
> + reg |= bit;
> + else
> + reg &= ~bit;
> + iowrite32(reg, SAM_GPIO_ADDR(sam->base, nr));
> + ioread32(SAM_GPIO_ADDR(sam->base, nr));
> +}
Does that rally need a helper function?
Use BIT() and inline like I explained in the previous patch.
> +static void sam_gpio_setup(struct sam_gpio *sam)
> +{
> + struct gpio_chip *chip = &sam->gpio;
> +
> + chip->parent = sam->dev;
> + chip->label = dev_name(sam->dev);
> + chip->owner = THIS_MODULE;
> + chip->direction_input = sam_gpio_direction_input;
> + chip->get = sam_gpio_get;
> + chip->direction_output = sam_gpio_direction_output;
Implement also chip->get_direction
> + chip->set = sam_gpio_set;
> + chip->set_debounce = sam_gpio_debounce;
> + chip->dbg_show = NULL;
> + chip->base = sam->gpio_base;
Oh no, why. Use -1 please and let gpiolib decide...
> + chip->ngpio = sam->gpio_count;
> +#ifdef CONFIG_OF_GPIO
> + chip->of_node = sam->dev->of_node;
> +#endif
I doubt this #ifdef actually. If the driver needs CONFIG_OF_GPIO to
work it should just depend on it in Kconfig.
> +static int sam_of_get_exports(struct device *dev, struct sam_gpio *sam)
> +{
> + struct device_node *child, *exports;
> + int err = 0;
> +
> + if (dev->of_node == NULL)
> + return 0; /* No FDT node, we are done */
> +
> + exports = of_get_child_by_name(dev->of_node, "gpio-exports");
> + if (exports == NULL)
> + return 0; /* No exports, we are done */
> +
> + if (of_get_child_count(exports) == 0)
> + return 0; /* No children, we are done */
> +
> + sam->names = devm_kzalloc(dev, sizeof(char *) * sam->gpio_count,
> + GFP_KERNEL);
> + if (sam->names == NULL) {
> + err = -ENOMEM;
> + goto error;
> + }
> + sam->export_flags =
> + devm_kzalloc(dev, sizeof(u32) * sam->gpio_count, GFP_KERNEL);
> + if (sam->export_flags == NULL) {
> + err = -ENOMEM;
> + goto error;
> + }
> + for_each_child_of_node(exports, child) {
> + const char *label;
> + u32 pin, flags;
> +
> + label = of_get_property(child, "label", NULL) ? : child->name;
> + err = of_property_read_u32_index(child, "pin", 0, &pin);
> + if (err)
> + break;
> + if (pin >= sam->gpio_count) {
> + err = -EINVAL;
> + break;
> + }
> + err = of_property_read_u32_index(child, "pin", 1, &flags);
> + if (err)
> + break;
> + /*
> + * flags:
> + * GPIOF_DIR_IN bit 0=1
> + * GPIOF_DIR_OUT bit 0=0
> + * GPIOF_INIT_HIGH bit 1=1
> + * GPIOF_ACTIVE_LOW bit 2=1
> + * GPIOF_OPEN_DRAIN bit 3=1
> + * GPIOF_OPEN_SOURCE bit 4=1
> + * GPIOF_EXPORT bit 5=1
> + * GPIOF_EXPORT_CHANGEABLE bit 6=1
> + */
> + sam->names[pin] = label;
> + sam->export_flags[pin] = flags;
> + }
> +error:
> + of_node_put(exports);
> + return err;
> +}
What? NAK never in my life. This looks like an old hack to
export stuff to userspace. We don't do that. The kernel supports
gpio-line-names to name lines in the device tree, and you can use
the new chardev ABI to access it from userspace, sysfs is dead.
Delete this function entirely.
> +static int sam_gpio_of_init(struct device *dev, struct sam_gpio *sam)
> +{
> + int err;
> + u32 val;
> + const u32 *igroup;
> + u32 group, start, count;
> + int i, iglen, ngpio;
> +
> + if (of_have_populated_dt() && !dev->of_node) {
> + dev_err(dev, "No device node\n");
> + return -ENODEV;
> + }
So obviously this driver Kconfig should depend on OF_GPIO.
> +
> + err = of_property_read_u32(dev->of_node, "gpio-base", &val);
> + if (err)
> + val = -1;
> + sam->gpio_base = val;
NAK, No Linux bases in the device tree. Only use -1.
> + err = of_property_read_u32(dev->of_node, "gpio-count", &val);
> + if (!err) {
> + if (val > SAM_MAX_NGPIO)
> + val = SAM_MAX_NGPIO;
> + sam->gpio_count = val;
> + }
As described in the generic bindings, use "ngpios" for this if you need it.
> + igroup = of_get_property(dev->of_node, "gpio-interrupts", &iglen);
NAK on that binding.
> + if (igroup) {
> + iglen /= sizeof(u32);
> + if (iglen < 3 || iglen % 3)
> + return -EINVAL;
> + iglen /= 3;
> + for (i = 0; i < iglen; i++) {
> + group = be32_to_cpu(igroup[i * 3]);
> + if (group >= ARRAY_SIZE(sam->irq_group))
> + return -EINVAL;
> + start = be32_to_cpu(igroup[i * 3 + 1]);
> + count = be32_to_cpu(igroup[i * 3 + 2]);
> + if (start >= sam->gpio_count || count == 0 ||
> + start + count > sam->gpio_count)
> + return -EINVAL;
> + sam->irq_group[group].start = start;
> + sam->irq_group[group].count = count;
> + }
> + }
Do not invent custom interrupt bindings like this. Use the
standard device tree mechanism to resolve IRQs from the parent
controller. Maybe you also need to use hierarchical irqdomain
in Linux.
> +static int sam_gpio_pin_to_irq_bit(struct sam_gpio *sam, int pin)
> +{
> + int bit;
> +
> + for (bit = 0; bit < ARRAY_SIZE(sam->irq_group); bit++) {
> + struct sam_gpio_irq_group *irq_group = &sam->irq_group[bit];
> +
> + if (irq_group->count &&
> + pin >= irq_group->start &&
> + pin <= irq_group->start + irq_group->count)
> + return bit;
> + }
> + return -EINVAL;
> +}
> +
> +static bool sam_gpio_irq_handle_group(struct sam_gpio *sam,
> + struct sam_gpio_irq_group *irq_group)
> +{
> + unsigned int virq = 0;
> + bool handled = false;
> + bool repeat;
> + int i;
> +
> + /* no irq_group for the interrupt bit */
> + if (!irq_group->count)
> + return false;
> +
> + WARN_ON(irq_group->num_enabled == 0);
> + do {
> + repeat = false;
> + for (i = 0; i < irq_group->count; i++) {
> + int pin = irq_group->start + i;
> + bool low, high;
> + u32 regval;
> + u8 type;
> +
> + regval = ioread32(SAM_GPIO_ADDR(sam->base, pin));
> + /*
> + * write back status to clear POS_EDGE and NEG_EDGE
> + * status for this GPIO pin (status bits are
> + * clear-on-one). This is necessary to clear the
> + * high level interrupt status.
> + * Also consider the interrupt to be handled in that
> + * case, even if there is no taker.
> + */
> + if (regval & (SAM_GPIO_POS_EDGE | SAM_GPIO_NEG_EDGE)) {
> + iowrite32(regval,
> + SAM_GPIO_ADDR(sam->base, pin));
> + ioread32(SAM_GPIO_ADDR(sam->base, pin));
> + handled = true;
> + }
> +
> + /*
> + * Check if the pin changed its state.
> + * If it did, and if the expected condition applies,
> + * generate a virtual interrupt.
> + * A pin can only generate an interrupt if
> + * - interrupts are enabled for it
> + * - it is configured as input
> + */
> +
> + if (!sam->irq_type[pin])
> + continue;
> + if (!(regval & SAM_GPIO_OUT_TS))
> + continue;
> +
> + high = regval & (SAM_GPIO_IN | SAM_GPIO_POS_EDGE);
> + low = !(regval & SAM_GPIO_IN) ||
> + (regval & SAM_GPIO_NEG_EDGE);
> + type = sam->irq_type[pin];
> + if (((type & IRQ_TYPE_EDGE_RISING) &&
> + (regval & SAM_GPIO_POS_EDGE)) ||
> + ((type & IRQ_TYPE_EDGE_FALLING) &&
> + (regval & SAM_GPIO_NEG_EDGE)) ||
> + ((type & IRQ_TYPE_LEVEL_LOW) && low) ||
> + ((type & IRQ_TYPE_LEVEL_HIGH) && high)) {
This if() clause does not look sane, you have to see that.
If you think this is proper then explain with detailed comments
what is going on.
> + virq = irq_find_mapping(sam->domain, pin);
> + handle_nested_irq(virq);
> + if (type & (IRQ_TYPE_LEVEL_LOW
> + | IRQ_TYPE_LEVEL_HIGH))
> + repeat = true;
> + }
> + }
> + schedule();
Why? Voluntary preemption? Just use a threaded interrupt handler
instead.
> + } while (repeat);
> +
> + return handled;
> +}
> +
> +static irqreturn_t sam_gpio_irq_handler(int irq, void *data)
> +{
> + struct sam_gpio *sam = data;
> + struct sam_platform_data *pdata = sam->pdata;
> + irqreturn_t ret = IRQ_NONE;
> + bool handled;
> + u32 status;
> +
> + do {
> + handled = false;
> + status = pdata->irq_status(sam->dev->parent, SAM_IRQ_GPIO,
> + sam->irq);
> + pdata->irq_status_clear(sam->dev->parent, SAM_IRQ_GPIO,
> + sam->irq, status);
> + while (status) {
> + unsigned int bit;
> +
> + bit = __ffs(status);
> + status &= ~(1 << bit);
> + handled =
> + sam_gpio_irq_handle_group(sam, &sam->irq_group[bit]);
> + if (handled)
> + ret = IRQ_HANDLED;
> + }
> + } while (handled);
This handled business looks fragile. But OK.
It is a simple IRQ handler, this driver should definately use
GPIOLIB_IRQCHIP. Please look at other drivers doing that
for inspiration. It is also well described in
Documenation/gpio/driver.txt
> +static int sam_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct sam_gpio *sam = to_sam(chip);
> +
> + return irq_create_mapping(sam->domain, offset);
> +}
With GPIOLIB_IRQCHIP you do not need to implement .to_irq()
> +static void sam_irq_mask(struct irq_data *data)
> +{
> + struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> + struct sam_platform_data *pdata = sam->pdata;
> + int bit = sam_gpio_pin_to_irq_bit(sam, data->hwirq);
> +
> + if (bit < 0)
> + return;
> +
> + if (--sam->irq_group[bit].num_enabled <= 0) {
> + pdata->disable_irq(sam->dev->parent, SAM_IRQ_GPIO, sam->irq,
> + 1 << bit);
Just BIT(bit)
> +static void sam_irq_unmask(struct irq_data *data)
> +{
> + struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> + struct sam_platform_data *pdata = sam->pdata;
> + int bit = sam_gpio_pin_to_irq_bit(sam, data->hwirq);
> +
> + if (bit < 0)
> + return;
Do you expect this to happen a lot? Else just delete the check or print
an error message.
> +
> + sam->irq_group[bit].num_enabled++;
> + pdata->enable_irq(sam->dev->parent, SAM_IRQ_GPIO, sam->irq, 1 << bit);
Dito.
> +static int sam_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> + struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> + int bit = sam_gpio_pin_to_irq_bit(sam, data->hwirq);
> +
> + if (bit < 0)
> + return bit;
> +
> + sam->irq_type[data->hwirq] = type & 0x0f;
Why storing this and going to all that trouble?
> + sam_gpio_bitop(sam, data->hwirq, SAM_GPIO_OUT_TS, true);
> + sam_gpio_bitop(sam, data->hwirq, SAM_GPIO_DEBOUNCE_EN, type & 0x10);
> + sam_gpio_bitop(sam, data->hwirq,
> + SAM_GPIO_POS_EDGE_EN | SAM_GPIO_POS_EDGE,
> + type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH));
> + sam_gpio_bitop(sam, data->hwirq,
> + SAM_GPIO_NEG_EDGE_EN | SAM_GPIO_NEG_EDGE,
> + type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW));
> +
> + return 0;
> +}
Just set up different handlers depending on edge. This likely simplifies your
IRQ handler code as well, as a special function (.irq_ack()) gets called for
edge IRQs.
Look how drivers/gpio/gpio-pl061.c does it.
> +static void sam_irq_bus_lock(struct irq_data *data)
> +{
> + struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> +
> + mutex_lock(&sam->irq_lock);
> +}
> +
> +static void sam_irq_bus_unlock(struct irq_data *data)
> +{
> + struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> +
> + /* Synchronize interrupts to chip */
> +
> + mutex_unlock(&sam->irq_lock);
> +}
Aha OK if it's necessary then we need to do this.
> +static struct irq_chip sam_irq_chip = {
> + .name = "gpio-sam",
Maybe this should have an instance number or something.
Just an idea no requirement.
> + .irq_mask = sam_irq_mask,
> + .irq_unmask = sam_irq_unmask,
So I think this needs .irq_ack() to solve the mess above in
the interrupt handler.
> + .irq_set_type = sam_irq_set_type,
> + .irq_bus_lock = sam_irq_bus_lock,
> + .irq_bus_sync_unlock = sam_irq_bus_unlock,
> +};
> +
> +static int sam_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_data(irq, domain->host_data);
> + irq_set_chip(irq, &sam_irq_chip);
> + irq_set_nested_thread(irq, true);
> +
> + irq_set_noprobe(irq);
> +
> + return 0;
> +}
This will not be needed if you use GPIOLIB_IRQCHIP
> +static const struct irq_domain_ops sam_gpio_irq_domain_ops = {
> + .map = sam_gpio_irq_map,
> + .xlate = irq_domain_xlate_twocell,
> +};
Nor this.
> +static int sam_gpio_irq_setup(struct device *dev, struct sam_gpio *sam)
> +{
> + int ret;
> +
> + sam->domain = irq_domain_add_linear(dev->of_node,
> + sam->gpio_count,
> + &sam_gpio_irq_domain_ops,
> + sam);
> + if (sam->domain == NULL)
> + return -ENOMEM;
Nor this.
> + ret = devm_request_threaded_irq(dev, sam->irq, NULL,
> + sam_gpio_irq_handler,
> + IRQF_ONESHOT,
> + dev_name(dev), sam);
> + if (ret)
> + goto out_remove_domain;
Are you not setting .can_sleep on the gpiochip?
> +
> + sam->gpio.to_irq = sam_gpio_to_irq;
> +
> + if (!try_module_get(dev->parent->driver->owner)) {
> + ret = -EINVAL;
> + goto out_remove_domain;
> + }
Why? Is that the MFD device? Isn't the device core
handling this?
> +static int sam_gpio_unexport(struct sam_gpio *sam)
> +{
> + int i;
> +
> + if (!sam->export_flags)
> + return 0;
> +
> + /* un-export all auto-exported pins */
> + for (i = 0; i < sam->gpio_count; i++) {
> + struct gpio_desc *desc = gpio_to_desc(sam->gpio.base + i);
> +
> + if (desc == NULL)
> + continue;
> +
> + if (sam->export_flags[i] & GPIOF_EXPORT)
> + gpiochip_free_own_desc(desc);
> + }
> + return 0;
> +}
> +
> +static int sam_gpio_export(struct sam_gpio *sam)
> +{
> + int i, ret;
> +
> + if (!sam->export_flags)
> + return 0;
> +
> + /* auto-export pins as requested */
> +
> + for (i = 0; i < sam->gpio_count; i++) {
> + u32 flags = sam->export_flags[i];
> + struct gpio_desc *desc;
> +
> + /* request and initialize exported pins */
> + if (!(flags & GPIOF_EXPORT))
> + continue;
> +
> + desc = gpiochip_request_own_desc(&sam->gpio, i, "sam-export");
> + if (IS_ERR(desc)) {
> + ret = PTR_ERR(desc);
> + goto error;
> + }
> + if (flags & GPIOF_DIR_IN) {
> + ret = gpiod_direction_input(desc);
> + if (ret)
> + goto error;
> + } else {
> + ret = gpiod_direction_output(desc, flags &
> + (GPIOF_OUT_INIT_HIGH |
> + GPIOF_ACTIVE_LOW));
> + if (ret)
> + goto error;
> + }
> + ret = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
> +
> + if (ret)
> + goto error;
> + }
> + return 0;
> +
> +error:
> + sam_gpio_unexport(sam);
> + return ret;
> +}
Just delete this. Use the new chardev ABI to access GPIOs from
userspace as explained earlier.
> +static int sam_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sam_gpio *sam;
> + struct resource *res;
> + int ret;
> + struct sam_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> + sam = devm_kzalloc(dev, sizeof(*sam), GFP_KERNEL);
> + if (sam == NULL)
> + return -ENOMEM;
if (!sam)
return -ENOMEM;
> + sam->dev = dev;
> + sam->pdata = pdata;
> + platform_set_drvdata(pdev, sam);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + sam->irq = platform_get_irq(pdev, 0);
> + sam->irq_high = platform_get_irq(pdev, 1);
> +
> + sam->base = devm_ioremap_nocache(dev, res->start, resource_size(res));
> + if (!sam->base)
> + return -ENOMEM;
> +
> + mutex_init(&sam->irq_lock);
> +
> + ret = sam_gpio_of_init(dev, sam);
> + if (ret)
> + return ret;
> +
> + sam_gpio_setup(sam);
> +
> + if (pdata && sam->irq >= 0 && of_find_property(dev->of_node,
> + "interrupt-controller", NULL)) {
> + ret = sam_gpio_irq_setup(dev, sam);
> + if (ret < 0)
> + return ret;
> + }
This is fair, but do it after adding the gpiochip and use GPIOLIB_IRQCHIP
accessors gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 07/10] gpio: ptxpmb-cpld: Add support for PTXPMB CPLD's GPIO
From: Linus Walleij @ 2016-10-20 22:24 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Lee Jones, Rob Herring, Alexandre Courbot, Mark Rutland,
Frank Rowand, Wolfram Sang, David Woodhouse, Brian Norris,
Wim Van Sebroeck, Guenter Roeck, Peter Rosin, Debjit Ghosh,
Georgi Vlaev, Guenter Roeck, JawaharBalaji Thirumalaisamy,
Rajat Jain, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1475853451-22121-8-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
On Fri, Oct 7, 2016 at 5:17 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> From: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>
> Support the GPIO block which is located in PTXPMB CPLDs
> on relevant Juniper platforms.
>
> Signed-off-by: Georgi Vlaev <gvlaev-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> Signed-off-by: Rajat Jain <rajatjain-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> [Ported from Juniper kernel]
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Nice with preserved credits.
> +config GPIO_PTXPMB_CPLD
> + tristate "PTXPMB CPLD GPIO"
> + depends on MFD_JUNIPER_CPLD
> + default y if MFD_JUNIPER_CPLD
Can't you do something like
default MFD_JUNIPER_CPLD
so it will become a module if the MFD driver is a module
and compiled-in if the MFD driver is compiled in?
Please check if that works.
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/gpio.h>
Just:
#include <linux/gpio/driver.h>
Should work.
> +#include <linux/errno.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
Strange that a PCI driver need so much OF stuff.
Really? But OK, do you really use all these includes?
> +#include <linux/io.h>
> +#include <linux/module.h>
> +
> +#include <linux/mfd/ptxpmb_cpld.h>
Is this include deserving a separate whitespace in
front of it?
> +static u8 *ptxpmb_cpld_gpio_get_addr(struct pmb_boot_cpld *cpld,
> + unsigned int nr)
> +{
> + if (nr < 8) /* 0..7: reset */
> + return &cpld->reset;
> + else if (nr < 16) /* 8..15: control */
> + return &cpld->control;
> + else if (nr < 24) /* 16..23: gpio1 */
> + return &cpld->gpio_1;
> + else if (nr < 32) /* 24..31: gpio2 */
> + return &cpld->gpio_2;
> + else if (nr < 40) /* 32..39: gp_reset1 */
> + return &cpld->gp_reset1;
> + return &cpld->thermal_status; /* 40..41: thermal status */
> +}
Reset, control, gp_reset and *especially* thermal status
does not seem to be GPIO at all. Rather these seem to be
special purpose lines rather than general purpose.
Can you explain why the GPIO driver should even poke at them?
It seems other subdrivers should use these registers...
> +static void ptxpmb_cpld_gpio_set(struct gpio_chip *gpio, unsigned int nr,
> + int val)
> +{
> + u32 reg;
> + u8 bit = 1 << (nr & 7);
Use this:
#include <linux/bitops.h>
Then inline BIT() instead of making a local variable like this.
See below...
> + struct ptxpmb_cpld_gpio *chip =
> + container_of(gpio, struct ptxpmb_cpld_gpio, gpio);
Use:
struct ptxpmb_cpld_gpio *cpldg = gpiochip_get_data(gpio);
Please don't name it "chip" it is confusing with the gpio chip
proper.
> + u8 *addr = ptxpmb_cpld_gpio_get_addr(chip->base, nr);
> +
> + mutex_lock(&chip->lock);
> + reg = ioread8(addr);
> + if (val)
> + reg |= bit;
> + else
> + reg &= ~bit;
So instead:
if (val)
reg |= BIT(nr);
else
reg &= ~BIT(nr);
> +static int ptxpmb_cpld_gpio_get(struct gpio_chip *gpio, unsigned int nr)
> +{
> + struct ptxpmb_cpld_gpio *chip = container_of(gpio,
> + struct ptxpmb_cpld_gpio,
> + gpio);
Same comment as before.
> + u8 *addr = ptxpmb_cpld_gpio_get_addr(chip->base, nr);
> + u8 bit = 1 << (nr & 7);
> +
> + return !!(ioread8(addr) & bit);
Same comment on using BIT()
> +static int ptxpmb_cpld_gpio_direction_output(struct gpio_chip *gpio,
> + unsigned int nr, int val)
> +{
> + return 0;
> +}
> +
> +static int ptxpmb_cpld_gpio_direction_input(struct gpio_chip *gpio,
> + unsigned int nr)
> +{
> + return 0;
> +}
Oh yeah? Can you explain how this works electronically?
If these lines are open drain you should also implement
.set_single_ended().
> +static void ptxpmb_cpld_gpio_setup(struct ptxpmb_cpld_gpio *chip)
Again rename from chip.
> +static int ptxpmb_cpld_gpio_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct ptxpmb_cpld_gpio *chip;
Rename from chip.
> + struct resource *res;
> + struct device *dev = &pdev->dev;
> +
> + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> + if (chip == NULL)
> + return -ENOMEM;
> +
> + chip->dev = dev;
> + platform_set_drvdata(pdev, chip);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + chip->base = devm_ioremap_nocache(dev, res->start, resource_size(res));
> + if (!chip->base)
> + return -ENOMEM;
> +
> + mutex_init(&chip->lock);
> + ptxpmb_cpld_gpio_setup(chip);
> + ret = gpiochip_add(&chip->gpio);
Use devm_gpiochip_add_data() so you can use gpiochip_get_data() in the
callbacks and get rid of container_of().
> + if (ret) {
> + dev_err(dev, "CPLD gpio: Failed to register GPIO\n");
> + return ret;
> + }
> + return 0;
> +}
> +
> +static int ptxpmb_cpld_gpio_remove(struct platform_device *pdev)
> +{
> + struct ptxpmb_cpld_gpio *chip = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&chip->gpio);
> +
> + return 0;
> +}
If you use devm_gpiochip_add_data() I don't think this function is
even needed.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 00/10] Introduce Juniper PTXPMB CPLD driver
From: Pantelis Antoniou @ 2016-10-20 13:43 UTC (permalink / raw)
To: Linus Walleij
Cc: Lee Jones, Rob Herring, Alexandre Courbot, Mark Rutland,
Frank Rowand, Wolfram Sang, David Woodhouse, Brian Norris,
Wim Van Sebroeck, Guenter Roeck, Peter Rosin, Debjit Ghosh,
Georgi Vlaev, Guenter Roeck, JawaharBalaji Thirumalaisamy,
Rajat Jain, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
In-Reply-To: <CACRpkdYkgondu3NuReFg=FZ29Xzi0zncpku0+++_OSsPnUs+Ew@mail.gmail.com>
Hi Linus,
> On Oct 20, 2016, at 16:42 , Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Oct 7, 2016 at 5:17 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>
>> Add Juniper's PTXPMB FPGA CPLD driver. Those FPGAs
>> are present in Juniper's PTX series of routers.
>>
>> The MFD driver provices watchdog/i2c/gpio/mtd devices.
>
> Since it's quite a bunch of patches to quite a bunch of controllers
> I think it's best if you get the MFD parts ready and merged through
> Lee first for v4.10, then we can add the subdrivers on top, maybe
> in v4.11, or on top of an immutable branch from Lee once he is
> happy with the core if the MFD core is finished quickly.
>
Will do.
> I will review all the GPIO stuff though so you can revise these
> patches too :)
>
Goodie :)
> Yours,
> Linus Walleij
Regards
— Pantelis
^ permalink raw reply
* Re: [PATCH 00/10] Introduce Juniper PTXPMB CPLD driver
From: Linus Walleij @ 2016-10-20 13:42 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Lee Jones, Rob Herring, Alexandre Courbot, Mark Rutland,
Frank Rowand, Wolfram Sang, David Woodhouse, Brian Norris,
Wim Van Sebroeck, Guenter Roeck, Peter Rosin, Debjit Ghosh,
Georgi Vlaev, Guenter Roeck, JawaharBalaji Thirumalaisamy,
Rajat Jain, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
In-Reply-To: <1475853451-22121-1-git-send-email-pantelis.antoniou@konsulko.com>
On Fri, Oct 7, 2016 at 5:17 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Add Juniper's PTXPMB FPGA CPLD driver. Those FPGAs
> are present in Juniper's PTX series of routers.
>
> The MFD driver provices watchdog/i2c/gpio/mtd devices.
Since it's quite a bunch of patches to quite a bunch of controllers
I think it's best if you get the MFD parts ready and merged through
Lee first for v4.10, then we can add the subdrivers on top, maybe
in v4.11, or on top of an immutable branch from Lee once he is
happy with the core if the MFD core is finished quickly.
I will review all the GPIO stuff though so you can revise these
patches too :)
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] m68knommu: AMCORE board, add iMX i2c support
From: Greg Ungerer @ 2016-10-20 12:37 UTC (permalink / raw)
To: Angelo Dureghello; +Cc: linux-i2c, Linux/m68k
In-Reply-To: <1476911979-4926-1-git-send-email-angelo@sysam.it>
Hi Angelo,
On 20/10/16 07:19, Angelo Dureghello wrote:
> Add iMX i2c support for the Sysam AMCORE board.
>
> Signed-off-by: Angelo Dureghello <angelo@sysam.it>
CCing linux-m68k email list as well.
I will pick this up and push it in to the m68knommu
git tree, for-next branch.
Thanks
Greg
> ---
> arch/m68k/configs/amcore_defconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/m68k/configs/amcore_defconfig b/arch/m68k/configs/amcore_defconfig
> index 2bf02e9..dc5adae 100644
> --- a/arch/m68k/configs/amcore_defconfig
> +++ b/arch/m68k/configs/amcore_defconfig
> @@ -87,7 +87,7 @@ CONFIG_I2C=y
> # CONFIG_I2C_COMPAT is not set
> CONFIG_I2C_CHARDEV=y
> # CONFIG_I2C_HELPER_AUTO is not set
> -CONFIG_I2C_COLDFIRE=y
> +CONFIG_I2C_IMX=y
> CONFIG_PPS=y
> # CONFIG_HWMON is not set
> # CONFIG_USB_SUPPORT is not set
>
^ permalink raw reply
* [PATCH] m68knommu: AMCORE board, add iMX i2c support
From: Angelo Dureghello @ 2016-10-19 21:19 UTC (permalink / raw)
To: linux-i2c; +Cc: gerg, Angelo Dureghello
Add iMX i2c support for the Sysam AMCORE board.
Signed-off-by: Angelo Dureghello <angelo@sysam.it>
---
arch/m68k/configs/amcore_defconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/m68k/configs/amcore_defconfig b/arch/m68k/configs/amcore_defconfig
index 2bf02e9..dc5adae 100644
--- a/arch/m68k/configs/amcore_defconfig
+++ b/arch/m68k/configs/amcore_defconfig
@@ -87,7 +87,7 @@ CONFIG_I2C=y
# CONFIG_I2C_COMPAT is not set
CONFIG_I2C_CHARDEV=y
# CONFIG_I2C_HELPER_AUTO is not set
-CONFIG_I2C_COLDFIRE=y
+CONFIG_I2C_IMX=y
CONFIG_PPS=y
# CONFIG_HWMON is not set
# CONFIG_USB_SUPPORT is not set
--
2.8.1
^ permalink raw reply related
* Re: [v12, 0/8] Fix eSDHC host version register bug
From: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r @ 2016-10-19 8:27 UTC (permalink / raw)
To: Y.B. Lu
Cc: Mark Rutland, Ulf Hansson, X.B. Xie, M.H. Lian,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-clk,
Qiang Zhao, Russell King, Bhupesh Sharma, Jochen Friedrich,
Claudiu Manoil,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Arnd Bergmann,
Scott Wood, Rob Herring, Santosh Shilimkar,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mmc,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <AM5PR0401MB2529BA70C6E7AF7C71631A47F8D20-oQ3wXcTHOqqGPub0A97BiI3W/0Ik+aLCnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
On Wed, Oct 19, 2016 at 02:47:07AM +0000, Y.B. Lu wrote:
> + Greg
>
> Hi Greg,
>
> I submitted this patchset for a MMC bug fix, and introduce the below patch which needs your ACK.
> > > Arnd Bergmann (1):
> > > base: soc: introduce soc_device_match() interface
> https://patchwork.kernel.org/patch/9342913/
>
> Could you help to review it and give some comments or ACK.
> Thank you very much.
Now acked.
^ permalink raw reply
* Re: [PATCH 4/4] i2c: digicolor: Fix module autoload
From: Baruch Siach @ 2016-10-19 5:05 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, Wolfram Sang, linux-arm-kernel, linux-i2c
In-Reply-To: <1476824508-4679-5-git-send-email-javier@osg.samsung.com>
Hi Javier Martinez Canillas,
On Tue, Oct 18, 2016 at 06:01:48PM -0300, Javier Martinez Canillas wrote:
> If the driver is built as a module, autoload won't work because the module
> alias information is not filled. So user-space can't match the registered
> device with the corresponding module.
>
> Export the module alias information using the MODULE_DEVICE_TABLE() macro.
>
> Before this patch:
>
> $ modinfo drivers/i2c/busses/i2c-digicolor.ko | grep alias
> $
>
> After this patch:
>
> $ modinfo drivers/i2c/busses/i2c-digicolor.ko | grep alias
> alias: of:N*T*Ccnxt,cx92755-i2cC*
> alias: of:N*T*Ccnxt,cx92755-i2c
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Acked-by: Baruch Siach <baruch@tkos.co.il>
Thanks,
baruch
> drivers/i2c/busses/i2c-digicolor.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/i2c/busses/i2c-digicolor.c b/drivers/i2c/busses/i2c-digicolor.c
> index 9604024e0eb0..49f2084f7bb5 100644
> --- a/drivers/i2c/busses/i2c-digicolor.c
> +++ b/drivers/i2c/busses/i2c-digicolor.c
> @@ -368,6 +368,7 @@ static const struct of_device_id dc_i2c_match[] = {
> { .compatible = "cnxt,cx92755-i2c" },
> { },
> };
> +MODULE_DEVICE_TABLE(of, dc_i2c_match);
>
> static struct platform_driver dc_i2c_driver = {
> .probe = dc_i2c_probe,
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
^ permalink raw reply
* [PATCH] i2c: uniphier[-f]: fix bool logic calculation
From: Masahiro Yamada @ 2016-10-19 4:38 UTC (permalink / raw)
To: linux-i2c; +Cc: Masahiro Yamada, linux-arm-kernel, linux-kernel, Wolfram Sang
This code is working, but it should not depend on how "bool" is
typedef'ed, or the bit position of I2C_M_RD.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/i2c/busses/i2c-uniphier-f.c | 2 +-
drivers/i2c/busses/i2c-uniphier.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-uniphier-f.c b/drivers/i2c/busses/i2c-uniphier-f.c
index db9105e..b54448e 100644
--- a/drivers/i2c/busses/i2c-uniphier-f.c
+++ b/drivers/i2c/busses/i2c-uniphier-f.c
@@ -309,7 +309,7 @@ static int uniphier_fi2c_master_xfer_one(struct i2c_adapter *adap,
struct i2c_msg *msg, bool stop)
{
struct uniphier_fi2c_priv *priv = i2c_get_adapdata(adap);
- bool is_read = msg->flags & I2C_M_RD;
+ bool is_read = !!(msg->flags & I2C_M_RD);
unsigned long time_left;
dev_dbg(&adap->dev, "%s: addr=0x%02x, len=%d, stop=%d\n",
diff --git a/drivers/i2c/busses/i2c-uniphier.c b/drivers/i2c/busses/i2c-uniphier.c
index 56e92af..cc80bb2 100644
--- a/drivers/i2c/busses/i2c-uniphier.c
+++ b/drivers/i2c/busses/i2c-uniphier.c
@@ -177,7 +177,7 @@ static int uniphier_i2c_stop(struct i2c_adapter *adap)
static int uniphier_i2c_master_xfer_one(struct i2c_adapter *adap,
struct i2c_msg *msg, bool stop)
{
- bool is_read = msg->flags & I2C_M_RD;
+ bool is_read = !!(msg->flags & I2C_M_RD);
bool recovery = false;
int ret;
--
1.9.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox