From: Paul Cercueil <paul@crapouillou.net>
To: "周琰杰 (Zhou Yanjie)" <zhouyanjie@wanyeetech.com>
Cc: linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
robh+dt@kernel.org, mark.rutland@arm.com, paul.burton@mips.com,
paulburton@kernel.org, sernia.zhou@foxmail.com,
zhenwenjin@gmail.com
Subject: Re: [PATCH 2/2] I2C: JZ4780: Add support for the X1000.
Date: Fri, 13 Dec 2019 22:21:05 +0100 [thread overview]
Message-ID: <1576272065.3.2@crapouillou.net> (raw)
In-Reply-To: <1576165850-20727-4-git-send-email-zhouyanjie@wanyeetech.com>
Hi Zhou,
Le jeu., déc. 12, 2019 at 23:50, 周琰杰 (Zhou Yanjie)
<zhouyanjie@wanyeetech.com> a écrit :
> Add support for probing i2c driver on the X1000 Soc from Ingenic.
> call the corresponding fifo parameter according to the device
> model obtained from the devicetree.
>
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
> drivers/i2c/busses/i2c-jz4780.c | 159
> +++++++++++++++++++++++++++++-----------
> 1 file changed, 117 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-jz4780.c
> b/drivers/i2c/busses/i2c-jz4780.c
> index 25dcd73..3b21896 100644
> --- a/drivers/i2c/busses/i2c-jz4780.c
> +++ b/drivers/i2c/busses/i2c-jz4780.c
> @@ -4,6 +4,7 @@
> *
> * Copyright (C) 2006 - 2009 Ingenic Semiconductor Inc.
> * Copyright (C) 2015 Imagination Technologies
> + * Copyright (C) 2019 周琰杰 (Zhou Yanjie)
> <zhouyanjie@wanyeetech.com>
> */
>
> #include <linux/bitops.h>
> @@ -17,6 +18,7 @@
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> @@ -55,6 +57,7 @@
> #define JZ4780_I2C_ACKGC 0x98
> #define JZ4780_I2C_ENSTA 0x9C
> #define JZ4780_I2C_SDAHD 0xD0
> +#define X1000_I2C_SDAHD 0x7C
>
> #define JZ4780_I2C_CTRL_STPHLD BIT(7)
> #define JZ4780_I2C_CTRL_SLVDIS BIT(6)
> @@ -73,6 +76,8 @@
> #define JZ4780_I2C_STA_TFNF BIT(1)
> #define JZ4780_I2C_STA_ACT BIT(0)
>
> +#define X1000_I2C_DC_STOP BIT(9)
> +
> static const char * const jz4780_i2c_abrt_src[] = {
> "ABRT_7B_ADDR_NOACK",
> "ABRT_10ADDR1_NOACK",
> @@ -130,18 +135,32 @@ static const char * const jz4780_i2c_abrt_src[]
> = {
> #define JZ4780_I2CFLCNT_ADJUST(n) (((n) - 1) < 8 ? 8 : ((n) - 1))
>
> #define JZ4780_I2C_FIFO_LEN 16
> -#define TX_LEVEL 3
> -#define RX_LEVEL (JZ4780_I2C_FIFO_LEN - TX_LEVEL - 1)
> +
> +#define X1000_I2C_FIFO_LEN 64
>
> #define JZ4780_I2C_TIMEOUT 300
>
> #define BUFSIZE 200
>
> +enum ingenic_i2c_version {
> + ID_JZ4780,
> + ID_X1000,
> +};
> +
> +/** ingenic_i2c_config: SOC specific config data. */
> +struct ingenic_i2c_config {
> + int fifosize;
> + int tx_level;
> + int rx_level;
> +};
> +
> struct jz4780_i2c {
> void __iomem *iomem;
> int irq;
> struct clk *clk;
> struct i2c_adapter adap;
> + enum ingenic_i2c_version version;
> + const struct ingenic_i2c_config *cdata;
>
> /* lock to protect rbuf and wbuf between xfer_rd/wr and irq handler
> */
> spinlock_t lock;
> @@ -340,11 +359,18 @@ static int jz4780_i2c_set_speed(struct
> jz4780_i2c *i2c)
>
> if (hold_time >= 0) {
> /*i2c hold time enable */
> - hold_time |= JZ4780_I2C_SDAHD_HDENB;
> - jz4780_i2c_writew(i2c, JZ4780_I2C_SDAHD, hold_time);
> + if (i2c->version >= ID_X1000)
> + jz4780_i2c_writew(i2c, X1000_I2C_SDAHD, hold_time);
> + else {
> + hold_time |= JZ4780_I2C_SDAHD_HDENB;
> + jz4780_i2c_writew(i2c, JZ4780_I2C_SDAHD, hold_time);
> + }
> } else {
> /* disable hold time */
> - jz4780_i2c_writew(i2c, JZ4780_I2C_SDAHD, 0);
> + if (i2c->version >= ID_X1000)
> + jz4780_i2c_writew(i2c, X1000_I2C_SDAHD, 0);
> + else
> + jz4780_i2c_writew(i2c, JZ4780_I2C_SDAHD, 0);
> }
>
> return 0;
> @@ -359,9 +385,11 @@ static int jz4780_i2c_cleanup(struct jz4780_i2c
> *i2c)
> spin_lock_irqsave(&i2c->lock, flags);
>
> /* can send stop now if need */
> - tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> - tmp &= ~JZ4780_I2C_CTRL_STPHLD;
> - jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + if (i2c->version < ID_X1000) {
> + tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> + tmp &= ~JZ4780_I2C_CTRL_STPHLD;
> + jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + }
>
> /* disable all interrupts first */
> jz4780_i2c_writew(i2c, JZ4780_I2C_INTM, 0);
> @@ -399,11 +427,18 @@ static int jz4780_i2c_prepare(struct jz4780_i2c
> *i2c)
> return jz4780_i2c_enable(i2c);
> }
>
> -static void jz4780_i2c_send_rcmd(struct jz4780_i2c *i2c, int
> cmd_count)
> +static void jz4780_i2c_send_rcmd(struct jz4780_i2c *i2c,
> + int cmd_count, int cmd_left)
> {
> int i;
>
> - for (i = 0; i < cmd_count; i++)
> + for (i = 0; i < cmd_count - 1; i++)
> + jz4780_i2c_writew(i2c, JZ4780_I2C_DC, JZ4780_I2C_DC_READ);
> +
> + if ((cmd_left == 0) && (i2c->version >= ID_X1000))
> + jz4780_i2c_writew(i2c, JZ4780_I2C_DC,
> + JZ4780_I2C_DC_READ | X1000_I2C_DC_STOP);
> + else
> jz4780_i2c_writew(i2c, JZ4780_I2C_DC, JZ4780_I2C_DC_READ);
> }
>
> @@ -458,37 +493,40 @@ static irqreturn_t jz4780_i2c_irq(int irqno,
> void *dev_id)
>
> rd_left = i2c->rd_total_len - i2c->rd_data_xfered;
>
> - if (rd_left <= JZ4780_I2C_FIFO_LEN)
> + if (rd_left <= i2c->cdata->fifosize)
> jz4780_i2c_writew(i2c, JZ4780_I2C_RXTL, rd_left - 1);
> }
>
> if (intst & JZ4780_I2C_INTST_TXEMP) {
> if (i2c->is_write == 0) {
> int cmd_left = i2c->rd_total_len - i2c->rd_cmd_xfered;
> - int max_send = (JZ4780_I2C_FIFO_LEN - 1)
> + int max_send = (i2c->cdata->fifosize - 1)
> - (i2c->rd_cmd_xfered
> - i2c->rd_data_xfered);
> int cmd_to_send = min(cmd_left, max_send);
>
> if (i2c->rd_cmd_xfered != 0)
> cmd_to_send = min(cmd_to_send,
> - JZ4780_I2C_FIFO_LEN
> - - TX_LEVEL - 1);
> + i2c->cdata->fifosize
> + - i2c->cdata->tx_level - 1);
>
> if (cmd_to_send) {
> - jz4780_i2c_send_rcmd(i2c, cmd_to_send);
> i2c->rd_cmd_xfered += cmd_to_send;
> + cmd_left = i2c->rd_total_len - i2c->rd_cmd_xfered;
> + jz4780_i2c_send_rcmd(i2c, cmd_to_send, cmd_left);
> +
> }
>
> - cmd_left = i2c->rd_total_len - i2c->rd_cmd_xfered;
> if (cmd_left == 0) {
> intmsk = jz4780_i2c_readw(i2c, JZ4780_I2C_INTM);
> intmsk &= ~JZ4780_I2C_INTM_MTXEMP;
> jz4780_i2c_writew(i2c, JZ4780_I2C_INTM, intmsk);
>
> - tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> - tmp &= ~JZ4780_I2C_CTRL_STPHLD;
> - jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + if (i2c->version < ID_X1000) {
> + tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> + tmp &= ~JZ4780_I2C_CTRL_STPHLD;
> + jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + }
> }
> } else {
> unsigned short data;
> @@ -496,24 +534,22 @@ static irqreturn_t jz4780_i2c_irq(int irqno,
> void *dev_id)
>
> i2c_sta = jz4780_i2c_readw(i2c, JZ4780_I2C_STA);
>
> - while ((i2c_sta & JZ4780_I2C_STA_TFNF) &&
> - (i2c->wt_len > 0)) {
> + while ((i2c_sta & JZ4780_I2C_STA_TFNF) && (i2c->wt_len > 0)) {
> i2c_sta = jz4780_i2c_readw(i2c, JZ4780_I2C_STA);
> data = *i2c->wbuf;
> data &= ~JZ4780_I2C_DC_READ;
> - jz4780_i2c_writew(i2c, JZ4780_I2C_DC,
> - data);
> + if ((!i2c->stop_hold) && (i2c->version >= ID_X1000))
> + data |= X1000_I2C_DC_STOP;
> + jz4780_i2c_writew(i2c, JZ4780_I2C_DC, data);
> i2c->wbuf++;
> i2c->wt_len--;
> }
>
> if (i2c->wt_len == 0) {
> - if (!i2c->stop_hold) {
> - tmp = jz4780_i2c_readw(i2c,
> - JZ4780_I2C_CTRL);
> + if ((!i2c->stop_hold) && (i2c->version < ID_X1000)) {
> + tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> tmp &= ~JZ4780_I2C_CTRL_STPHLD;
> - jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL,
> - tmp);
> + jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> }
>
> jz4780_i2c_trans_done(i2c);
> @@ -567,20 +603,22 @@ static inline int jz4780_i2c_xfer_read(struct
> jz4780_i2c *i2c,
> i2c->rd_data_xfered = 0;
> i2c->rd_cmd_xfered = 0;
>
> - if (len <= JZ4780_I2C_FIFO_LEN)
> + if (len <= i2c->cdata->fifosize)
> jz4780_i2c_writew(i2c, JZ4780_I2C_RXTL, len - 1);
> else
> - jz4780_i2c_writew(i2c, JZ4780_I2C_RXTL, RX_LEVEL);
> + jz4780_i2c_writew(i2c, JZ4780_I2C_RXTL, i2c->cdata->rx_level);
>
> - jz4780_i2c_writew(i2c, JZ4780_I2C_TXTL, TX_LEVEL);
> + jz4780_i2c_writew(i2c, JZ4780_I2C_TXTL, i2c->cdata->tx_level);
>
> jz4780_i2c_writew(i2c, JZ4780_I2C_INTM,
> JZ4780_I2C_INTM_MRXFL | JZ4780_I2C_INTM_MTXEMP
> | JZ4780_I2C_INTM_MTXABT | JZ4780_I2C_INTM_MRXOF);
>
> - tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> - tmp |= JZ4780_I2C_CTRL_STPHLD;
> - jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + if (i2c->version < ID_X1000) {
> + tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> + tmp |= JZ4780_I2C_CTRL_STPHLD;
> + jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + }
>
> spin_unlock_irqrestore(&i2c->lock, flags);
>
> @@ -626,14 +664,16 @@ static inline int jz4780_i2c_xfer_write(struct
> jz4780_i2c *i2c,
> i2c->wbuf = buf;
> i2c->wt_len = len;
>
> - jz4780_i2c_writew(i2c, JZ4780_I2C_TXTL, TX_LEVEL);
> + jz4780_i2c_writew(i2c, JZ4780_I2C_TXTL, i2c->cdata->tx_level);
>
> jz4780_i2c_writew(i2c, JZ4780_I2C_INTM, JZ4780_I2C_INTM_MTXEMP
> | JZ4780_I2C_INTM_MTXABT);
>
> - tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> - tmp |= JZ4780_I2C_CTRL_STPHLD;
> - jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + if (i2c->version < ID_X1000) {
> + tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> + tmp |= JZ4780_I2C_CTRL_STPHLD;
> + jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + }
>
> spin_unlock_irqrestore(&i2c->lock, flags);
>
> @@ -716,8 +756,21 @@ static const struct i2c_algorithm
> jz4780_i2c_algorithm = {
> .functionality = jz4780_i2c_functionality,
> };
>
> +static const struct ingenic_i2c_config jz4780_i2c_config = {
> + .fifosize = JZ4780_I2C_FIFO_LEN,
> + .tx_level = JZ4780_I2C_FIFO_LEN / 2,
> + .rx_level = JZ4780_I2C_FIFO_LEN / 2 - 1,
> +};
> +
> +static const struct ingenic_i2c_config x1000_i2c_config = {
> + .fifosize = X1000_I2C_FIFO_LEN,
> + .tx_level = X1000_I2C_FIFO_LEN / 2,
> + .rx_level = X1000_I2C_FIFO_LEN / 2 - 1,
> +};
> +
> static const struct of_device_id jz4780_i2c_of_matches[] = {
> - { .compatible = "ingenic,jz4780-i2c", },
> + { .compatible = "ingenic,jz4780-i2c", .data = (void *) ID_JZ4780 },
> + { .compatible = "ingenic,x1000-i2c", .data = (void *) ID_X1000 },
I think in general you should pass pointers to your ingenic_i2c_config
structures directly in .data here, and have a "version" field in your
ingenic_i2c_config struct.
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, jz4780_i2c_of_matches);
> @@ -729,11 +782,24 @@ static int jz4780_i2c_probe(struct
> platform_device *pdev)
> unsigned short tmp;
> struct resource *r;
> struct jz4780_i2c *i2c;
> + const struct platform_device_id *id = platform_get_device_id(pdev);
> + const struct of_device_id *of_id = of_match_device(
> + jz4780_i2c_of_matches, &pdev->dev);
>
> i2c = devm_kzalloc(&pdev->dev, sizeof(struct jz4780_i2c),
> GFP_KERNEL);
> if (!i2c)
> return -ENOMEM;
>
> + if (of_id)
> + i2c->version = (enum ingenic_i2c_version)of_id->data;
> + else
> + i2c->version = (enum ingenic_i2c_version)id->driver_data;
> +
> + if (i2c->version >= ID_X1000)
> + i2c->cdata = &x1000_i2c_config;
> + else
> + i2c->cdata = &jz4780_i2c_config;
> +
Then you can replace all these lines with:
i2c->cdata = device_get_match_data(dev);
And your checks on i2c->version become checks on i2c->cdata->version.
> i2c->adap.owner = THIS_MODULE;
> i2c->adap.algo = &jz4780_i2c_algorithm;
> i2c->adap.algo_data = i2c;
> @@ -777,9 +843,11 @@ static int jz4780_i2c_probe(struct
> platform_device *pdev)
>
> dev_info(&pdev->dev, "Bus frequency is %d KHz\n", i2c->speed);
>
> - tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> - tmp &= ~JZ4780_I2C_CTRL_STPHLD;
> - jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + if (i2c->version < ID_X1000) {
> + tmp = jz4780_i2c_readw(i2c, JZ4780_I2C_CTRL);
> + tmp &= ~JZ4780_I2C_CTRL_STPHLD;
> + jz4780_i2c_writew(i2c, JZ4780_I2C_CTRL, tmp);
> + }
>
> jz4780_i2c_writew(i2c, JZ4780_I2C_INTM, 0x0);
>
> @@ -809,6 +877,12 @@ static int jz4780_i2c_remove(struct
> platform_device *pdev)
> return 0;
> }
>
> +static const struct platform_device_id ingenic_i2c_ids[] = {
> + { "jz4780-i2c", ID_JZ4780 },
> + { "x1000-i2c", ID_X1000 },
> + {},
I honestly think you can drop the platform ID table. It will never be
used.
Cheers,
-Paul
> +};
> +
> static struct platform_driver jz4780_i2c_driver = {
> .probe = jz4780_i2c_probe,
> .remove = jz4780_i2c_remove,
> @@ -816,6 +890,7 @@ static struct platform_driver jz4780_i2c_driver =
> {
> .name = "jz4780-i2c",
> .of_match_table = of_match_ptr(jz4780_i2c_of_matches),
> },
> + .id_table = ingenic_i2c_ids,
> };
>
> module_platform_driver(jz4780_i2c_driver);
> --
> 2.7.4
>
next prev parent reply other threads:[~2019-12-13 21:21 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1576165850-20727-1-git-send-email-zhouyanjie@wanyeetech.com>
2019-12-12 15:50 ` [PATCH 0/2] Add I2C support for the Ingenic X1000 SoC 周琰杰 (Zhou Yanjie)
2019-12-12 15:50 ` [PATCH 1/2] dt-bindings: I2C: Add X1000 bindings 周琰杰 (Zhou Yanjie)
2019-12-12 15:50 ` [PATCH 2/2] I2C: JZ4780: Add support for the X1000 周琰杰 (Zhou Yanjie)
2019-12-13 21:21 ` Paul Cercueil [this message]
2019-12-14 12:32 ` zhouyanjie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1576272065.3.2@crapouillou.net \
--to=paul@crapouillou.net \
--cc=devicetree@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=paul.burton@mips.com \
--cc=paulburton@kernel.org \
--cc=robh+dt@kernel.org \
--cc=sernia.zhou@foxmail.com \
--cc=zhenwenjin@gmail.com \
--cc=zhouyanjie@wanyeetech.com \
--subject='Re: [PATCH 2/2] I2C: JZ4780: Add support for the X1000.' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).