From: Zhou Yanjie <zhouyanjie@wanyeetech.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
robh+dt@kernel.org, paul.burton@mips.com, paulburton@kernel.org,
mark.rutland@arm.com, sernia.zhou@foxmail.com,
zhenwenjin@gmail.com, 2374286503@qq.com
Subject: Re: [PATCH v2 2/2] I2C: JZ4780: Add support for the X1000.
Date: Tue, 17 Dec 2019 15:50:20 +0800 [thread overview]
Message-ID: <5DF888BC.9040601@wanyeetech.com> (raw)
In-Reply-To: <1576504625.3.4@crapouillou.net>
Hi Paul,
On 2019年12月16日 21:57, Paul Cercueil wrote:
> Hi Zhou,
>
>
> Le lun., déc. 16, 2019 at 18:06, 周琰杰 (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>
>> ---
>>
>> Notes:
>> v1->v2:
>> Add code to check device_get_match_data(), if it return a NULL ptr,
>> then print an error message and return -ENODEV.
>>
>> drivers/i2c/busses/i2c-jz4780.c | 155
>> +++++++++++++++++++++++++++++-----------
>> 1 file changed, 115 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-jz4780.c
>> b/drivers/i2c/busses/i2c-jz4780.c
>> index 25dcd73..f07a07c 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,33 @@ 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 {
>> + enum ingenic_i2c_version version;
>> +
>> + int fifosize;
>> + int tx_level;
>> + int rx_level;
>> +};
>> +
>> struct jz4780_i2c {
>> void __iomem *iomem;
>> int irq;
>> struct clk *clk;
>> struct i2c_adapter adap;
>> + const struct ingenic_i2c_config *cdata;
>>
>> /* lock to protect rbuf and wbuf between xfer_rd/wr and irq
>> handler */
>> spinlock_t lock;
>> @@ -340,11 +360,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->cdata->version >= ID_X1000)
>> + jz4780_i2c_writew(i2c, X1000_I2C_SDAHD, hold_time);
>> + else {
>
> If only one branch of a conditional statement is a single statement,
> then you should use braces in both branches.
>
Sure, I will fix this in v3.
> See:
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
>
>
>
>> + 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->cdata->version >= ID_X1000)
>> + jz4780_i2c_writew(i2c, X1000_I2C_SDAHD, 0);
>> + else
>> + jz4780_i2c_writew(i2c, JZ4780_I2C_SDAHD, 0);
>> }
>>
>> return 0;
>> @@ -359,9 +386,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->cdata->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 +428,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)
>
> Sorry to be pedantic ;) but this line is not properly indented. You
> should indent with tab charaters (configure your IDE for one tab == 4
> spaces) as much as possible, then use spaces to align the first word.
>
Sure, I will fix this in v3.
Thanks and best regards!
> With these two things fixed:
> Acked-by: Paul Cercueil <paul@crapouillou.net>
>
> Cheers,
> -Paul
>
>
>> {
>> 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->cdata->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 +494,44 @@ 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->cdata->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;
>> @@ -497,23 +540,26 @@ 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)) {
>> + (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->cdata->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) {
>> + if ((!i2c->stop_hold) && (i2c->cdata->version <
>> + ID_X1000)) {
>> tmp = jz4780_i2c_readw(i2c,
>> - JZ4780_I2C_CTRL);
>> + 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 +613,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->cdata->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 +674,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->cdata->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 +766,25 @@ static const struct i2c_algorithm
>> jz4780_i2c_algorithm = {
>> .functionality = jz4780_i2c_functionality,
>> };
>>
>> +static const struct ingenic_i2c_config jz4780_i2c_config = {
>> + .version = ID_JZ4780,
>> +
>> + .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 = {
>> + .version = ID_X1000,
>> +
>> + .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 = &jz4780_i2c_config },
>> + { .compatible = "ingenic,x1000-i2c", .data = &x1000_i2c_config },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, jz4780_i2c_of_matches);
>> @@ -734,6 +801,12 @@ static int jz4780_i2c_probe(struct
>> platform_device *pdev)
>> if (!i2c)
>> return -ENOMEM;
>>
>> + i2c->cdata = device_get_match_data(&pdev->dev);
>> + if (!i2c->cdata) {
>> + dev_err(&pdev->dev, "Error: No device match found\n");
>> + return -ENODEV;
>> + }
>> +
>> i2c->adap.owner = THIS_MODULE;
>> i2c->adap.algo = &jz4780_i2c_algorithm;
>> i2c->adap.algo_data = i2c;
>> @@ -777,9 +850,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->cdata->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);
>>
>> --
>> 2.7.4
>>
>
prev parent reply other threads:[~2019-12-17 7:50 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-16 10:06 Add I2C support for the Ingenic X1000 SoC v2 周琰杰 (Zhou Yanjie)
2019-12-16 10:06 ` [PATCH v2 0/2] Add I2C support for the Ingenic X1000 SoC 周琰杰 (Zhou Yanjie)
2019-12-16 10:06 ` [PATCH v2 1/2] dt-bindings: I2C: Add X1000 bindings 周琰杰 (Zhou Yanjie)
2019-12-16 10:06 ` [PATCH v2 2/2] I2C: JZ4780: Add support for the X1000 周琰杰 (Zhou Yanjie)
2019-12-16 13:57 ` Paul Cercueil
2019-12-17 7:50 ` Zhou Yanjie [this message]
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=5DF888BC.9040601@wanyeetech.com \
--to=zhouyanjie@wanyeetech.com \
--cc=2374286503@qq.com \
--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=paul@crapouillou.net \
--cc=paulburton@kernel.org \
--cc=robh+dt@kernel.org \
--cc=sernia.zhou@foxmail.com \
--cc=zhenwenjin@gmail.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.