* Re: [PATCH V2 5/5] i2c: riic: add driver
[not found] ` <1387402321-21866-6-git-send-email-wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
@ 2013-12-19 0:01 ` Laurent Pinchart
2013-12-19 12:06 ` Wolfram Sang
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2013-12-19 0:01 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-sh-u79uwXL29TY76Z2rM5mHXA, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Magnus Damm, Simon Horman,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
Hi Wolfram,
Thank you for the patch.
As this patch adds DT bindings, please remember to CC the devicetree mailing
list (which I have CC'ed on this reply).
On Wednesday 18 December 2013 22:32:01 Wolfram Sang wrote:
> From: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
>
> Tested with a r7s72100 genmai board acessing an eeprom.
>
> Signed-off-by: Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> ---
>
> V2: fixed the name typo in the binding docs
>
> Documentation/devicetree/bindings/i2c/i2c-riic.txt | 29 ++
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-riic.c | 426 ++++++++++++++++++
> 4 files changed, 466 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-riic.txt
> create mode 100644 drivers/i2c/busses/i2c-riic.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-riic.txt
> b/Documentation/devicetree/bindings/i2c/i2c-riic.txt new file mode 100644
> index 0000000..0bcc471
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-riic.txt
> @@ -0,0 +1,29 @@
> +Device tree configuration for Renesas RIIC driver
> +
> +Required properties:
> +- compatible : "renesas,riic-<soctype>". "renesas,riic-rz" as fallback
> +- reg : address start and address range size of device
> +- interrupts : 8 interrupts (TEI, RI, TI, SPI, STI, NAKI, ALI, TMOI)
> +- clock-frequency : frequency of bus clock in Hz
> +- #address-cells : should be <1>
> +- #size-cells : should be <0>
> +
> +Pinctrl properties might be needed, too. See there.
> +
> +Example:
> +
> + i2c0: i2c@fcfee000 {
> + compatible = "renesas,riic-r7s72100", "renesas,riic-rz";
> + reg = <0xfcfee000 0x44>;
> + interrupts = <0 157 IRQ_TYPE_LEVEL_HIGH>,
> + <0 158 IRQ_TYPE_EDGE_RISING>,
> + <0 159 IRQ_TYPE_EDGE_RISING>,
> + <0 160 IRQ_TYPE_LEVEL_HIGH>,
> + <0 161 IRQ_TYPE_LEVEL_HIGH>,
> + <0 162 IRQ_TYPE_LEVEL_HIGH>,
> + <0 163 IRQ_TYPE_LEVEL_HIGH>,
> + <0 164 IRQ_TYPE_LEVEL_HIGH>;
> + clock-frequency = <100000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 3b26129..8e8332d 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -648,6 +648,16 @@ config I2C_PXA_SLAVE
> is necessary for systems where the PXA may be a target on the
> I2C bus.
>
> +config I2C_RIIC
> + tristate "Renesas RIIC adapter"
> + depends on ARCH_SHMOBILE || COMPILE_TEST
> + help
> + If you say yes to this option, support will be included for the
> + Renesas RIIC I2C interface.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-riic.
> +
> config HAVE_S3C2410_I2C
> bool
> help
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index c73eb0e..dca041b 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
> obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o
> obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
> obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
> +obj-$(CONFIG_I2C_RIIC) += i2c-riic.o
> obj-$(CONFIG_I2C_S3C2410) += i2c-s3c2410.o
> obj-$(CONFIG_I2C_S6000) += i2c-s6000.o
> obj-$(CONFIG_I2C_SH7760) += i2c-sh7760.o
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> new file mode 100644
> index 0000000..ae0df13
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -0,0 +1,426 @@
> +/*
> + * Renesas RIIC driver
> + *
> + * Copyright (C) 2013 Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> + * Copyright (C) 2013 Renesas Solutions Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +/*
> + * This i2c core has a lot of interrupts, namely 8. We use their chaining
> as
> + * some kind of state machine.
I have mixed feelings about this. Wouldn't it be more efficient to have an
internal state machine (which you partially have already, using RIIC_INIT_MSG
for instance) instead of relying on enabling/disabling interrupts ? The latter
has a larger overhead.
> + * 1) The main xfer routine kicks off a transmission by putting the start
> bit
> + * (or repeated start) on the bus and enabling the transmit interrupt (TIE)
> + * since we need to send the slave address + RW bit in every case.
> + *
> + * 2) TIE sends slave address + RW bit and selects how to continue.
> + *
> + * 3a) Write case: We keep utilizing TIE as long as we have data to send.
> If we
> + * are done, we switch over to the transmission done interrupt (TEIE) and
> mark
> + * the message as completed (includes sending STOP) there.
> + *
> + * 3b) Read case: We switch over to receive interrupt (RIE). One dummy read
> is
> + * needed to start clocking, then we keep receiving until we are done. Note
> + * that we use the RDRFS mode all the time, i.e. we ACK/NACK every byte by
> + * writing to the ACKBT bit. I tried using the RDRFS mode only at the end
> of a
> + * message to create the final NACK as sketched in the datasheet. This
> caused
> + * some subtle races (when byte n was processed and byte n+1 was already
> + * waiting), though, and I started with the safe approach.
> + *
> + * 4) If we got a NACK somewhere, we flag the error and stop the
> transmission
> + * via NAKIE.
> + *
> + * Also check the comments in the interrupt routines for some gory details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define RIIC_ICCR1 0x00
> +#define RIIC_ICCR2 0x04
> +#define RIIC_ICMR1 0x08
> +#define RIIC_ICMR3 0x10
> +#define RIIC_ICSER 0x18
> +#define RIIC_ICIER 0x1c
> +#define RIIC_ICSR2 0x24
> +#define RIIC_ICBRL 0x34
> +#define RIIC_ICBRH 0x38
> +#define RIIC_ICDRT 0x3c
> +#define RIIC_ICDRR 0x40
> +
> +#define ICCR1_ICE 0x80
> +#define ICCR1_IICRST 0x40
> +#define ICCR1_SOWP 0x10
> +
> +#define ICCR2_BBSY 0x80
> +#define ICCR2_SP 0x08
> +#define ICCR2_RS 0x04
> +#define ICCR2_ST 0x02
> +
> +#define ICMR1_CKS_MASK 0x70
> +#define ICMR1_BCWP 0x08
> +#define ICMR1_CKS(_x) ((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
> +
> +#define ICMR3_RDRFS 0x20
> +#define ICMR3_ACKWP 0x10
> +#define ICMR3_ACKBT 0x08
> +
> +#define ICIER_TIE 0x80
> +#define ICIER_TEIE 0x40
> +#define ICIER_RIE 0x20
> +#define ICIER_NAKIE 0x10
> +
> +#define ICSR2_NACKF 0x10
> +
> +/* ICBRx (@ PCLK 33MHz) */
> +#define ICBR_RESERVED 0xe0 /* Should be 1 on writes */
> +#define ICBRL_SP100K (19 | ICBR_RESERVED)
> +#define ICBRH_SP100K (16 | ICBR_RESERVED)
> +#define ICBRL_SP400K (21 | ICBR_RESERVED)
> +#define ICBRH_SP400K (9 | ICBR_RESERVED)
> +
> +#define RIIC_INIT_MSG -1
> +
> +struct riic_dev {
> + void __iomem *base;
> + u8 *buf;
> + struct i2c_msg *msg;
> + int bytes_left;
> + int err;
> + int is_last;
> + struct completion msg_done;
> + struct i2c_adapter adapter;
> + struct clk *clk;
> +};
> +
> +struct riic_irq_desc {
> + int res_num;
> + irq_handler_t isr;
> + char *name;
> +};
> +
> +static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8
> set, u8 reg)
> +{
> + writeb((readb(riic->base + reg) & ~clear) | set, riic->base + reg);
> +}
> +
> +static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int
> num)
> +{
> + struct riic_dev *riic = i2c_get_adapdata(adap);
> + int i, ret;
One of my favorite bikeshedding comments is to ask for unsigned int when the
variable can't be negative :-)
> + u8 start_bit;
> +
> + ret = clk_prepare_enable(riic->clk);
> + if (ret)
> + return ret;
> +
> + if (readb(riic->base + RIIC_ICCR2) & ICCR2_BBSY) {
> + riic->err = -EBUSY;
> + goto out;
> + }
> +
> + reinit_completion(&riic->msg_done);
> + riic->err = 0;
> +
> + writeb(0, riic->base + RIIC_ICSR2);
> +
> + for (i = 0, start_bit = ICCR2_ST; i < num; i++) {
> + riic->bytes_left = RIIC_INIT_MSG;
> + riic->buf = msgs[i].buf;
> + riic->msg = &msgs[i];
> + riic->is_last = (i == num - 1);
> +
> + writeb(ICIER_NAKIE | ICIER_TIE, riic->base + RIIC_ICIER);
> +
> + writeb(start_bit, riic->base + RIIC_ICCR2);
> +
> + ret = wait_for_completion_timeout(&riic->msg_done,
> riic->adapter.timeout);
> + if (ret == 0)
> + riic->err = -ETIMEDOUT;
> +
> + if (riic->err)
> + break;
> +
> + start_bit = ICCR2_RS;
> + }
> +
> + out:
> + clk_disable_unprepare(riic->clk);
> +
> + return riic->err ?: num;
> +}
> +
> +static irqreturn_t riic_tdre_isr(int irq, void *data)
> +{
> + struct riic_dev *riic = data;
> + u8 val;
> +
> + if (!riic->bytes_left)
> + return IRQ_NONE;
> +
> + if (riic->bytes_left == RIIC_INIT_MSG) {
> + val = !!(riic->msg->flags & I2C_M_RD);
> + if (val)
> + /* On read, switch over to receive interrupt */
> + riic_clear_set_bit(riic, ICIER_TIE, ICIER_RIE, RIIC_ICIER);
> + else
> + /* On write, initialize length */
> + riic->bytes_left = riic->msg->len;
> +
> + val |= (riic->msg->addr << 1);
> + } else {
> + val = *riic->buf;
> + riic->buf++;
> + riic->bytes_left--;
> + }
> +
> + /*
> + * Switch to transmission ended interrupt when done. Do check here
> + * after bytes_left was initialized to support SMBUS_QUICK (new msg has
> + * 0 length then)
> + */
> + if (riic->bytes_left == 0)
> + riic_clear_set_bit(riic, ICIER_TIE, ICIER_TEIE, RIIC_ICIER);
> +
> + /*
> + * This acks the TIE interrupt. We get another TIE immediately if our
> + * value could be moved to the shadow shift register right away. So
> + * this must be after updates to ICIER (where we want to disable TIE)!
> + */
> + writeb(val, riic->base + RIIC_ICDRT);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t riic_tend_isr(int irq, void *data)
> +{
> + struct riic_dev *riic = data;
> +
> + if (readb(riic->base + RIIC_ICSR2) & ICSR2_NACKF) {
> + /* We got a NACKIE */
> + readb(riic->base + RIIC_ICDRR); /* dummy read */
> + riic->err = -ENXIO;
> + } else if (riic->bytes_left) {
> + return IRQ_NONE;
> + }
> +
> + if (riic->is_last || riic->err)
> + writeb(ICCR2_SP, riic->base + RIIC_ICCR2);
> +
> + writeb(0, riic->base + RIIC_ICIER);
> + complete(&riic->msg_done);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t riic_rdrf_isr(int irq, void *data)
> +{
> + struct riic_dev *riic = data;
> +
> + if (!riic->bytes_left)
> + return IRQ_NONE;
> +
> + if (riic->bytes_left == RIIC_INIT_MSG) {
> + riic->bytes_left = riic->msg->len;
> + readb(riic->base + RIIC_ICDRR); /* dummy read */
> + return IRQ_HANDLED;
> + }
> +
> + if (riic->bytes_left == 1) {
> + /* STOP must come before we set ACKBT! */
> + if (riic->is_last)
> + writeb(ICCR2_SP, riic->base + RIIC_ICCR2);
> +
> + riic_clear_set_bit(riic, 0, ICMR3_ACKBT, RIIC_ICMR3);
> +
> + writeb(0, riic->base + RIIC_ICIER);
> + complete(&riic->msg_done);
> + } else {
> + riic_clear_set_bit(riic, ICMR3_ACKBT, 0, RIIC_ICMR3);
> + }
> +
> + /* Reading acks the RIE interrupt */
> + *riic->buf = readb(riic->base + RIIC_ICDRR);
> + riic->buf++;
> + riic->bytes_left--;
> +
> + return IRQ_HANDLED;
> +}
> +
> +static u32 riic_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm riic_algo = {
> + .master_xfer = riic_xfer,
> + .functionality = riic_func,
> +};
> +
> +static int riic_init_hw(struct riic_dev *riic, u32 spd)
> +{
> + int ret;
> + unsigned long rate;
> +
> + ret = clk_prepare_enable(riic->clk);
> + if (ret)
> + return ret;
> +
> + /*
> + * TODO: Implement formula to calculate the timing values depending on
> + * variable parent clock rate and arbitrary bus speed
> + */
> + rate = clk_get_rate(riic->clk);
> + if (rate != 33325000) {
> + dev_err(&riic->adapter.dev,
> + "invalid parent clk (%lu). Must be 33325000Hz\n", rate);
What about a "goto done;" here and below to avoid repeating the
clk_disable_unprepare() call ?
> + clk_disable_unprepare(riic->clk);
> + return -EINVAL;
> + }
> +
> + /* Changing the order of accessing IICRST and ICE may break things! */
> + writeb(ICCR1_IICRST | ICCR1_SOWP, riic->base + RIIC_ICCR1);
> + riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1);
> +
> + switch (spd) {
> + case 100000:
> + writeb(ICMR1_CKS(3), riic->base + RIIC_ICMR1);
> + writeb(ICBRH_SP100K, riic->base + RIIC_ICBRH);
> + writeb(ICBRL_SP100K, riic->base + RIIC_ICBRL);
> + break;
> + case 400000:
> + writeb(ICMR1_CKS(1), riic->base + RIIC_ICMR1);
> + writeb(ICBRH_SP400K, riic->base + RIIC_ICBRH);
> + writeb(ICBRL_SP400K, riic->base + RIIC_ICBRL);
Couldn't you compute the ICMR1, ICBRH and ICBRL values at runtime instead ?
> + break;
> + default:
> + dev_err(&riic->adapter.dev,
> + "unsupported bus speed (%dHz). Use 100000 or 400000\n", spd);
> + clk_disable_unprepare(riic->clk);
> + return -EINVAL;
> + }
> +
> + writeb(0, riic->base + RIIC_ICSER);
> + writeb(ICMR3_ACKWP | ICMR3_RDRFS, riic->base + RIIC_ICMR3);
> +
> + riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
> +
... with
done:
here, and a return ret.
> + clk_disable_unprepare(riic->clk);
> +
> + return 0;
> +}
> +
> +static struct riic_irq_desc riic_irqs[] = {
> + { .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
> + { .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
> + { .res_num = 2, .isr = riic_tdre_isr, .name = "riic-tdre" },
> + { .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
> +};
> +
> +static int riic_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct riic_dev *riic;
> + struct i2c_adapter *adap;
> + struct resource *res;
> + u32 bus_rate = 0;
> + int i, ret;
> +
> + riic = devm_kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL);
> + if (!riic)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + riic->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(riic->base))
> + return PTR_ERR(riic->base);
> +
> + riic->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(riic->clk)) {
> + dev_err(&pdev->dev, "missing controller clock");
> + return PTR_ERR(riic->clk);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
> + res = platform_get_resource(pdev, IORESOURCE_IRQ,
riic_irqs[i].res_num);
> + if (!res)
> + return -ENODEV;
> +
> + ret = devm_request_irq(&pdev->dev, res->start, riic_irqs[i].isr,
> + 0, riic_irqs[i].name, riic);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request irq %s\n",
riic_irqs[i].name);
> + return ret;
> + }
> + }
> +
> + adap = &riic->adapter;
> + i2c_set_adapdata(adap, riic);
> + strlcpy(adap->name, "Renesas RIIC adapter", sizeof(adap->name));
> + adap->owner = THIS_MODULE;
> + adap->algo = &riic_algo;
> + adap->dev.parent = &pdev->dev;
> + adap->dev.of_node = pdev->dev.of_node;
> +
> + init_completion(&riic->msg_done);
> +
> + of_property_read_u32(np, "clock-frequency", &bus_rate);
As the property is mandatory, shouldn't you check the return value of this
function ? Another option would be to make the clock-frequency property
optional and use a default value. What do the other I2C bus drivers usually do
?
> + ret = riic_init_hw(riic, bus_rate);
> + if (ret)
> + return ret;
> +
> +
> + ret = i2c_add_adapter(adap);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add adapter\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, riic);
> +
> + dev_info(&pdev->dev, "registered with %dHz bus speed\n", bus_rate);
> + return 0;
> +}
> +
> +static int riic_i2c_remove(struct platform_device *pdev)
> +{
> + struct riic_dev *riic = platform_get_drvdata(pdev);
> +
> + writeb(0, riic->base + RIIC_ICIER);
> + i2c_del_adapter(&riic->adapter);
> +
> + return 0;
> +}
> +
> +static struct of_device_id riic_i2c_dt_ids[] = {
> + { .compatible = "renesas,riic-rz" },
> + { /* Sentinel */ },
> +};
> +
> +static struct platform_driver riic_i2c_driver = {
> + .probe = riic_i2c_probe,
> + .remove = riic_i2c_remove,
> + .driver = {
> + .name = "i2c-riic",
> + .owner = THIS_MODULE,
> + .of_match_table = riic_i2c_dt_ids,
> + },
> +};
> +
> +module_platform_driver(riic_i2c_driver);
> +
> +MODULE_DESCRIPTION("Renesas RIIC adapter");
> +MODULE_AUTHOR("Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DEVICE_TABLE(of, riic_i2c_dt_ids);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread