| Message ID | 9d3e4997519fd9ecebae6fd241148cc22d3fe04f.1671688961.git.zhoubinbin@loongson.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller | expand |
On Fri, Dec 23, 2022 at 05:00:51PM +0800, Binbin Zhou wrote: > This I2C module is integrated into the Loongson-2K SoCs and Loongson > LS7A bridge chip. ... > +static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, > + struct i2c_msg *msg, bool stop) > +{ > + int ret; > + bool is_read = msg->flags & I2C_M_RD; > + > + /* Contains steps to send start condition and address */ > + ret = ls2x_i2c_start(priv, msg); > + if (!ret) { > + if (is_read) > + ret = ls2x_i2c_rx(priv, msg->buf, msg->len); > + else > + ret = ls2x_i2c_tx(priv, msg->buf, msg->len); > + > + if (!ret && stop) > + ret = ls2x_i2c_stop(priv); > + } > + > + if (ret == -ENXIO) > + ls2x_i2c_stop(priv); > + else if (ret < 0) > + ls2x_i2c_init(priv); > + > + return ret; > +} Still this code is odd from reader's perspective. It's in particular not clear if the stop can be called twice in a row. I recommend to split it to two functions and then do something like _read_one() { ret = start(); if (ret) goto _stop; // Do we really need this? ret = rx(); if (ret) goto _stop; // Do we need this? /* By setting this call the stop */ if (stop) ret = -ENXIO; out_send_stop: if (ret == ...) return _stop(); // I don't like above, so this error checking/setting parts // also can be rethought and refactored accordingly return ret; } if (is_read) ret = _read_one(); else ret = _write_one(); if (ret) _init(); return ret;
Hi Andy: Sorry for my late reply. On Wed, Dec 28, 2022 at 5:57 AM Andy Shevchenko <andy@kernel.org> wrote: > > On Fri, Dec 23, 2022 at 05:00:51PM +0800, Binbin Zhou wrote: > > This I2C module is integrated into the Loongson-2K SoCs and Loongson > > LS7A bridge chip. > > ... > > > +static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, > > + struct i2c_msg *msg, bool stop) > > +{ > > + int ret; > > + bool is_read = msg->flags & I2C_M_RD; > > + > > + /* Contains steps to send start condition and address */ > > + ret = ls2x_i2c_start(priv, msg); > > + if (!ret) { > > + if (is_read) > > + ret = ls2x_i2c_rx(priv, msg->buf, msg->len); > > + else > > + ret = ls2x_i2c_tx(priv, msg->buf, msg->len); > > + > > + if (!ret && stop) > > + ret = ls2x_i2c_stop(priv); > > + } > > + > > + if (ret == -ENXIO) > > + ls2x_i2c_stop(priv); > > + else if (ret < 0) > > + ls2x_i2c_init(priv); > > + > > + return ret; > > +} > > Still this code is odd from reader's perspective. It's in particular not clear > if the stop can be called twice in a row. I recommend to split it to two Sorry, Actually, I don't quite understand why you keep thinking that the stop can be called twice in a row. As I said in my last email, the logic here should be: In the first case, stop is called when the last msg is transmitted successfully; In the second case, stop is called when there is a NOACK during the transmission; In the third case, init is called when other errors occur during the transmission, such as TIMEOUT. The key pointer is the stop function will only return a TIMEOUT error or 0 for success, so if the stop function above is failed, the stop function below will never be called twice. Anyway, I also admit that this part of the code may not be concise and clear enough, and I have tried the following changes: 1. put the start function into the rx/tx function respectively. As followers: @@ -177,10 +177,16 @@ static int ls2x_i2c_start(struct ls2x_i2c_priv *priv, struct i2c_msg *msgs) return ls2x_i2c_send_byte(priv, LS2X_CR_START | LS2X_CR_WRITE); } -static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) +static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, struct i2c_msg *msg) { int ret; - u8 rxdata; + u8 rxdata, *buf = msg->buf; + u16 len = msg->len; + + /* Contains steps to send start condition and address */ + ret = ls2x_i2c_start(priv, msg); + if (ret) + return ret; while (len--) { ret = ls2x_i2c_xfer_byte(priv, @@ -195,9 +201,16 @@ static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) return 0; } -static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) +static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, struct i2c_msg *msg) { int ret; + u8 *buf = msg->buf; + u16 len = msg->len; + + /* Contains steps to send start condition and address */ + ret = ls2x_i2c_start(priv, msg); + if (ret) + return ret; while (len--) { writeb(*buf++, priv->base + I2C_LS2X_TXR); 2. define the variable 'reinit' in the xfer_one function to mark the cases where reinit is needed. As follows: static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, struct i2c_msg *msg, bool stop) { int ret, ret2; bool reinit = false; bool is_read = msg->flags & I2C_M_RD; if (is_read) ret = ls2x_i2c_rx(priv, msg); else ret = ls2x_i2c_tx(priv, msg); if (ret == -EAGAIN) /* could not acquire bus. bail out without STOP */ return ret; if (ret == -ETIMEDOUT) { /* Fatal error. Needs reinit. */ stop = false; reinit = true; } if (stop) { ret2 = ls2x_i2c_stop(priv); if (ret2) { /* Failed to issue STOP. Needs reinit. */ reinit = true; ret = ret ?: ret2; } } if (reinit) ls2x_i2c_init(priv); return ret; } Do you think this is better? Thanks. Binbin > functions and then do something like > > _read_one() > { > ret = start(); > if (ret) > goto _stop; // Do we really need this? > > ret = rx(); > if (ret) > goto _stop; // Do we need this? > > /* By setting this call the stop */ > if (stop) > ret = -ENXIO; > > out_send_stop: > if (ret == ...) > return _stop(); > // I don't like above, so this error checking/setting parts > // also can be rethought and refactored accordingly > > return ret; > } > > > if (is_read) > ret = _read_one(); > else > ret = _write_one(); > > if (ret) > _init(); > > return ret; > > > -- > With Best Regards, > Andy Shevchenko > > >
On Thu, Dec 29, 2022 at 11:20:01AM +0200, Andy Shevchenko wrote: > On Thu, Dec 29, 2022 at 03:31:47PM +0800, Binbin Zhou wrote: > > On Wed, Dec 28, 2022 at 5:57 AM Andy Shevchenko <andy@kernel.org> wrote: > > > On Fri, Dec 23, 2022 at 05:00:51PM +0800, Binbin Zhou wrote: ... > > > > +static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, > > > > + struct i2c_msg *msg, bool stop) > > > > +{ > > > > + int ret; > > > > + bool is_read = msg->flags & I2C_M_RD; > > > > + > > > > + /* Contains steps to send start condition and address */ > > > > + ret = ls2x_i2c_start(priv, msg); > > > > + if (!ret) { > > > > + if (is_read) > > > > + ret = ls2x_i2c_rx(priv, msg->buf, msg->len); > > > > + else > > > > + ret = ls2x_i2c_tx(priv, msg->buf, msg->len); > > > > + > > > > + if (!ret && stop) > > > > + ret = ls2x_i2c_stop(priv); > > > > + } > > > > + > > > > + if (ret == -ENXIO) > > > > + ls2x_i2c_stop(priv); > > > > + else if (ret < 0) > > > > + ls2x_i2c_init(priv); > > > > + > > > > + return ret; > > > > +} > > > > > > Still this code is odd from reader's perspective. It's in particular not clear > > > if the stop can be called twice in a row. I recommend to split it to two > > > > Sorry, > > Actually, I don't quite understand why you keep thinking that the stop > > can be called twice in a row. > > Because nothing in the code suggests otherwise. You need deeply understand > the flow to ensure that it won't. This means that the code is fragile and > needs refactoring (even comment, which you can do a least won't help, because > changing code in the other parts may break all this and you won't notice it). > > > As I said in my last email, the logic here should be: > > In the first case, stop is called when the last msg is transmitted successfully; > > In the second case, stop is called when there is a NOACK during the > > transmission; > > In the third case, init is called when other errors occur during the > > transmission, such as TIMEOUT. > > > > The key pointer is the stop function will only return a TIMEOUT error > > or 0 for success, so if the stop function above is failed, the stop > > function below will never be called twice. > > > > Anyway, I also admit that this part of the code may not be concise and > > clear enough, and I have tried the following changes: > > > > 1. put the start function into the rx/tx function respectively. As followers: > > > > @@ -177,10 +177,16 @@ static int ls2x_i2c_start(struct ls2x_i2c_priv > > *priv, struct i2c_msg *msgs) > > return ls2x_i2c_send_byte(priv, LS2X_CR_START | LS2X_CR_WRITE); > > } > > > > -static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) > > +static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, struct i2c_msg *msg) > > { > > int ret; > > - u8 rxdata; > > + u8 rxdata, *buf = msg->buf; > > + u16 len = msg->len; > > + > > + /* Contains steps to send start condition and address */ > > + ret = ls2x_i2c_start(priv, msg); > > + if (ret) > > + return ret; > > > > while (len--) { > > ret = ls2x_i2c_xfer_byte(priv, > > @@ -195,9 +201,16 @@ static int ls2x_i2c_rx(struct ls2x_i2c_priv > > *priv, u8 *buf, u16 len) > > return 0; > > } > > > > -static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) > > +static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, struct i2c_msg *msg) > > { > > int ret; > > + u8 *buf = msg->buf; > > + u16 len = msg->len; > > + > > + /* Contains steps to send start condition and address */ > > + ret = ls2x_i2c_start(priv, msg); > > + if (ret) > > + return ret; > > > > while (len--) { > > writeb(*buf++, priv->base + I2C_LS2X_TXR); > > > > 2. define the variable 'reinit' in the xfer_one function to mark the > > cases where reinit is needed. As follows: > > > > static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, > > struct i2c_msg *msg, bool stop) > > { > > int ret, ret2; > > bool reinit = false; > > bool is_read = msg->flags & I2C_M_RD; > > > > if (is_read) > > ret = ls2x_i2c_rx(priv, msg); > > else > > ret = ls2x_i2c_tx(priv, msg); > > > > if (ret == -EAGAIN) /* could not acquire bus. bail out without STOP */ > > return ret; > > > > if (ret == -ETIMEDOUT) { > > /* Fatal error. Needs reinit. */ > > stop = false; > > reinit = true; Why do you need to initialize stop here? Why not to call reinit here and bailout? > > } > > > > if (stop) { > > ret2 = ls2x_i2c_stop(priv); > > > > if (ret2) { > > /* Failed to issue STOP. Needs reinit. */ > > reinit = true; > > ret = ret ?: ret2; All the same, try to be less verbose with unneeded variables. > > } > > } > > > > if (reinit) > > ls2x_i2c_init(priv); > > > > return ret; > > } > > > > Do you think this is better? > > Slightly, but still twisted at the end with the play of error codes. Try to > make it even more clear. > > > > functions and then do something like > > > > > > _read_one() > > > { > > > ret = start(); > > > if (ret) > > > goto _stop; // Do we really need this? > > > > > > ret = rx(); > > > if (ret) > > > goto _stop; // Do we need this? > > > > > > /* By setting this call the stop */ > > > if (stop) > > > ret = -ENXIO; > > > > > > out_send_stop: > > > if (ret == ...) > > > return _stop(); > > > // I don't like above, so this error checking/setting parts > > > // also can be rethought and refactored accordingly > > > > > > return ret; > > > } > > > > > > > > > if (is_read) > > > ret = _read_one(); > > > else > > > ret = _write_one(); > > > > > > if (ret) > > > _init(); > > > > > > return ret;
On Fri, Dec 30, 2022 at 09:46:25AM +0800, Binbin Zhou wrote: > On Thu, Dec 29, 2022 at 5:23 PM Andy Shevchenko <andy@kernel.org> wrote: > > > > On Thu, Dec 29, 2022 at 11:20:01AM +0200, Andy Shevchenko wrote: > > > On Thu, Dec 29, 2022 at 03:31:47PM +0800, Binbin Zhou wrote: > > > > On Wed, Dec 28, 2022 at 5:57 AM Andy Shevchenko <andy@kernel.org> wrote: > > > > > On Fri, Dec 23, 2022 at 05:00:51PM +0800, Binbin Zhou wrote: ... > > > > > > +static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, > > > > > > + struct i2c_msg *msg, bool stop) > > > > > > +{ > > > > > > + int ret; > > > > > > + bool is_read = msg->flags & I2C_M_RD; > > > > > > + > > > > > > + /* Contains steps to send start condition and address */ > > > > > > + ret = ls2x_i2c_start(priv, msg); > > > > > > + if (!ret) { > > > > > > + if (is_read) > > > > > > + ret = ls2x_i2c_rx(priv, msg->buf, msg->len); > > > > > > + else > > > > > > + ret = ls2x_i2c_tx(priv, msg->buf, msg->len); > > > > > > + > > > > > > + if (!ret && stop) > > > > > > + ret = ls2x_i2c_stop(priv); > > > > > > + } > > > > > > + > > > > > > + if (ret == -ENXIO) > > > > > > + ls2x_i2c_stop(priv); > > > > > > + else if (ret < 0) > > > > > > + ls2x_i2c_init(priv); > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > > > > > Still this code is odd from reader's perspective. It's in particular not clear > > > > > if the stop can be called twice in a row. I recommend to split it to two > > > > > > > > Sorry, > > > > Actually, I don't quite understand why you keep thinking that the stop > > > > can be called twice in a row. > > > > > > Because nothing in the code suggests otherwise. You need deeply understand > > > the flow to ensure that it won't. This means that the code is fragile and > > > needs refactoring (even comment, which you can do a least won't help, because > > > changing code in the other parts may break all this and you won't notice it). > > > > > > > As I said in my last email, the logic here should be: > > > > In the first case, stop is called when the last msg is transmitted successfully; > > > > In the second case, stop is called when there is a NOACK during the > > > > transmission; > > > > In the third case, init is called when other errors occur during the > > > > transmission, such as TIMEOUT. > > > > > > > > The key pointer is the stop function will only return a TIMEOUT error > > > > or 0 for success, so if the stop function above is failed, the stop > > > > function below will never be called twice. > > > > > > > > Anyway, I also admit that this part of the code may not be concise and > > > > clear enough, and I have tried the following changes: > > > > > > > > 1. put the start function into the rx/tx function respectively. As followers: > > > > > > > > @@ -177,10 +177,16 @@ static int ls2x_i2c_start(struct ls2x_i2c_priv > > > > *priv, struct i2c_msg *msgs) > > > > return ls2x_i2c_send_byte(priv, LS2X_CR_START | LS2X_CR_WRITE); > > > > } > > > > > > > > -static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) > > > > +static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, struct i2c_msg *msg) > > > > { > > > > int ret; > > > > - u8 rxdata; > > > > + u8 rxdata, *buf = msg->buf; > > > > + u16 len = msg->len; > > > > + > > > > + /* Contains steps to send start condition and address */ > > > > + ret = ls2x_i2c_start(priv, msg); > > > > + if (ret) > > > > + return ret; > > > > > > > > while (len--) { > > > > ret = ls2x_i2c_xfer_byte(priv, > > > > @@ -195,9 +201,16 @@ static int ls2x_i2c_rx(struct ls2x_i2c_priv > > > > *priv, u8 *buf, u16 len) > > > > return 0; > > > > } > > > > > > > > -static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) > > > > +static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, struct i2c_msg *msg) > > > > { > > > > int ret; > > > > + u8 *buf = msg->buf; > > > > + u16 len = msg->len; > > > > + > > > > + /* Contains steps to send start condition and address */ > > > > + ret = ls2x_i2c_start(priv, msg); > > > > + if (ret) > > > > + return ret; > > > > > > > > while (len--) { > > > > writeb(*buf++, priv->base + I2C_LS2X_TXR); > > > > > > > > 2. define the variable 'reinit' in the xfer_one function to mark the > > > > cases where reinit is needed. As follows: > > > > > > > > static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, > > > > struct i2c_msg *msg, bool stop) > > > > { > > > > int ret, ret2; > > > > bool reinit = false; > > > > bool is_read = msg->flags & I2C_M_RD; > > > > > > > > if (is_read) > > > > ret = ls2x_i2c_rx(priv, msg); > > > > else > > > > ret = ls2x_i2c_tx(priv, msg); > > > > > > > > if (ret == -EAGAIN) /* could not acquire bus. bail out without STOP */ > > > > return ret; > > > > > > > > if (ret == -ETIMEDOUT) { > > > > /* Fatal error. Needs reinit. */ > > > > stop = false; > > > > reinit = true; > > > > Why do you need to initialize stop here? > > Why not to call reinit here and bailout? > > > > > > } > > > > > > > > if (stop) { > > > > ret2 = ls2x_i2c_stop(priv); > > > > > > > > if (ret2) { > > > > /* Failed to issue STOP. Needs reinit. */ > > > > reinit = true; > > > > ret = ret ?: ret2; > > > > All the same, try to be less verbose with unneeded variables. > > Ok, the reinit and ret2 variables seem to be a bit redundant, I will > remove them. > > I will divide the whole thing into two parts: > The first part is to handle errors: if ret < 0, return ret directly. > One of the special handling is the fatal error timeout, which requires > reinit. > > if (ret < 0) { > if (ret == -ETIMEDOUT) /* Fatel error. Needs reinit. */ > ls2x_i2c_init(priv); > return ret; > } > > The second part is to handle the final stop command: it should be > noted that if the stop command fails, reinit is also required. > > if (stop) { > /* Failed to issue STOP. Needs reinit. */ > ret = ls2x_i2c_stop(priv); > if (ret) > ls2x_i2c_init(priv); > } > > The complete code is as follows: This looks much better! See a couple of nit-picks below. > static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, > struct i2c_msg *msg, bool stop) > { > int ret; > bool is_read = msg->flags & I2C_M_RD; > > if (is_read) With this you don't need to have is_read variable. > ret = ls2x_i2c_rx(priv, msg); > else > ret = ls2x_i2c_tx(priv, msg); > > if (ret < 0) { > if (ret == -ETIMEDOUT) /* Fatel error. Needs reinit. */ Split comment and code, so the comment is followed by the code. > ls2x_i2c_init(priv); > return ret; > } > > if (stop) { > /* Failed to issue STOP. Needs reinit. */ > ret = ls2x_i2c_stop(priv); > if (ret) > ls2x_i2c_init(priv); > } > > return ret; > } > > Do you think this is better? > > > > } > > > > } > > > > > > > > if (reinit) > > > > ls2x_i2c_init(priv); > > > > > > > > return ret; > > > > } > > > > > > > > Do you think this is better? > > > > > > Slightly, but still twisted at the end with the play of error codes. Try to > > > make it even more clear. > > > > > > > > functions and then do something like > > > > > > > > > > _read_one() > > > > > { > > > > > ret = start(); > > > > > if (ret) > > > > > goto _stop; // Do we really need this? > > > > > > > > > > ret = rx(); > > > > > if (ret) > > > > > goto _stop; // Do we need this? > > > > > > > > > > /* By setting this call the stop */ > > > > > if (stop) > > > > > ret = -ENXIO; > > > > > > > > > > out_send_stop: > > > > > if (ret == ...) > > > > > return _stop(); > > > > > // I don't like above, so this error checking/setting parts > > > > > // also can be rethought and refactored accordingly > > > > > > > > > > return ret; > > > > > } > > > > > > > > > > > > > > > if (is_read) > > > > > ret = _read_one(); > > > > > else > > > > > ret = _write_one(); > > > > > > > > > > if (ret) > > > > > _init(); > > > > > > > > > > return ret;
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index e50f9603d189..d80c938142f5 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -761,6 +761,17 @@ config I2C_LPC2K This driver can also be built as a module. If so, the module will be called i2c-lpc2k. +config I2C_LS2X + tristate "Loongson LS2X I2C adapter" + depends on MACH_LOONGSON64 || COMPILE_TEST + help + If you say yes to this option, support will be included for the + I2C interface on the Loongson-2K SoCs and Loongson LS7A bridge + chip. + + This driver can also be built as a module. If so, the module + will be called i2c-ls2x. + config I2C_MLXBF tristate "Mellanox BlueField I2C controller" depends on MELLANOX_PLATFORM && ARM64 diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index e73cdb1d2b5a..0584fe160824 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -77,6 +77,7 @@ obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o obj-$(CONFIG_I2C_JZ4780) += i2c-jz4780.o obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o obj-$(CONFIG_I2C_LPC2K) += i2c-lpc2k.o +obj-$(CONFIG_I2C_LS2X) += i2c-ls2x.o obj-$(CONFIG_I2C_MESON) += i2c-meson.o obj-$(CONFIG_I2C_MICROCHIP_CORE) += i2c-microchip-corei2c.o obj-$(CONFIG_I2C_MPC) += i2c-mpc.o diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c new file mode 100644 index 000000000000..665faa009c7c --- /dev/null +++ b/drivers/i2c/busses/i2c-ls2x.c @@ -0,0 +1,366 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Loongson-2K/Loongson LS7A I2C master mode driver + * + * Copyright (C) 2013 Loongson Technology Corporation Limited. + * Copyright (C) 2014-2017 Lemote, Inc. + * Copyright (C) 2018-2022 Loongson Technology Corporation Limited. + * + * Originally written by liushaozong + * Rewritten for mainline by Binbin Zhou <zhoubinbin@loongson.cn> + */ + +#include <linux/bits.h> +#include <linux/completion.h> +#include <linux/device.h> +#include <linux/iopoll.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/pm_runtime.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/units.h> + +/* I2C Registers */ +#define I2C_LS2X_PRER 0x0 /* Freq Division Register(16 bits) */ +#define I2C_LS2X_CTR 0x2 /* Control Register */ +#define I2C_LS2X_TXR 0x3 /* Transport Data Register */ +#define I2C_LS2X_RXR 0x3 /* Receive Data Register */ +#define I2C_LS2X_CR 0x4 /* Command Control Register */ +#define I2C_LS2X_SR 0x4 /* State Register */ + +/* Command Control Register Bit */ +#define LS2X_CR_START BIT(7) /* Start signal */ +#define LS2X_CR_STOP BIT(6) /* Stop signal */ +#define LS2X_CR_READ BIT(5) /* Read signal */ +#define LS2X_CR_WRITE BIT(4) /* Write signal */ +#define LS2X_CR_ACK BIT(3) /* Response signal */ +#define LS2X_CR_IACK BIT(0) /* Interrupt response signal */ + +/* State Register Bit */ +#define LS2X_SR_NOACK BIT(7) /* Receive NACK */ +#define LS2X_SR_BUSY BIT(6) /* Bus busy state */ +#define LS2X_SR_AL BIT(5) /* Arbitration lost */ +#define LS2X_SR_TIP BIT(1) /* Transmission state */ +#define LS2X_SR_IF BIT(0) /* Interrupt flag */ + +/* Control Register Bit */ +#define LS2X_CTR_EN BIT(7) /* 0: I2c frequency setting 1: Normal */ +#define LS2X_CTR_IEN BIT(6) /* Enable i2c interrupt */ +#define LS2X_CTR_MST BIT(5) /* 0: Slave mode 1: Master mode */ +#define CTR_FREQ_MASK GENMASK(7, 6) +#define CTR_READY_MASK GENMASK(7, 5) + +/* The PCLK frequency from LPB */ +#define LS2X_I2C_PCLK_FREQ (50 * HZ_PER_MHZ) + +/* The default bus frequency, which is an empirical value */ +#define LS2X_I2C_FREQ_STD (33 * HZ_PER_KHZ) + +struct ls2x_i2c_priv { + struct i2c_adapter adapter; + void __iomem *base; + struct i2c_timings i2c_t; + struct completion cmd_complete; +}; + +/* + * Interrupt service routine. + * This gets called whenever an I2C interrupt occurs. + */ +static irqreturn_t ls2x_i2c_isr(int this_irq, void *dev_id) +{ + struct ls2x_i2c_priv *priv = dev_id; + + if (!(readb(priv->base + I2C_LS2X_SR) & LS2X_SR_IF)) + return IRQ_NONE; + + writeb(LS2X_CR_IACK, priv->base + I2C_LS2X_CR); + complete(&priv->cmd_complete); + return IRQ_HANDLED; +} + +/* + * The ls2x i2c controller supports standard mode and fast mode, so the + * maximum bus frequency is '400kHz'. + * The bus frequency is set to the empirical value of '33KHz' by default, + * but it can also be taken from ACPI or FDT for compatibility with more + * devices. + */ +static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv) +{ + struct i2c_timings *t = &priv->i2c_t; + struct device *dev = priv->adapter.dev.parent; + u32 acpi_speed = i2c_acpi_find_bus_speed(dev); + + i2c_parse_fw_timings(dev, t, false); + + if (acpi_speed || t->bus_freq_hz) + t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed); + else + t->bus_freq_hz = LS2X_I2C_FREQ_STD; + + /* Calculate and set i2c frequency */ + writew(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1, + priv->base + I2C_LS2X_PRER); +} + +static void ls2x_i2c_init(struct ls2x_i2c_priv *priv) +{ + /* Set i2c frequency setting mode and disable interrupts */ + writeb(readb(priv->base + I2C_LS2X_CTR) & ~CTR_FREQ_MASK, + priv->base + I2C_LS2X_CTR); + + ls2x_i2c_adjust_bus_speed(priv); + + /* Set i2c normal operating mode and enable interrupts */ + writeb(readb(priv->base + I2C_LS2X_CTR) | CTR_READY_MASK, + priv->base + I2C_LS2X_CTR); +} + +static int ls2x_i2c_xfer_byte(struct ls2x_i2c_priv *priv, u8 txdata, u8 *rxdatap) +{ + u8 rxdata; + unsigned long time_left; + + writeb(txdata, priv->base + I2C_LS2X_CR); + + time_left = wait_for_completion_timeout(&priv->cmd_complete, + priv->adapter.timeout); + if (!time_left) + return -ETIMEDOUT; + + rxdata = readb(priv->base + I2C_LS2X_SR); + if (rxdatap) + *rxdatap = rxdata; + + return 0; +} + +static int ls2x_i2c_send_byte(struct ls2x_i2c_priv *priv, u8 txdata) +{ + int ret; + u8 rxdata; + + ret = ls2x_i2c_xfer_byte(priv, txdata, &rxdata); + if (ret) + return ret; + + if (rxdata & LS2X_SR_AL) + return -EAGAIN; + + if (rxdata & LS2X_SR_NOACK) + return -ENXIO; + + return 0; +} + +static int ls2x_i2c_stop(struct ls2x_i2c_priv *priv) +{ + u8 value; + + writeb(LS2X_CR_STOP, priv->base + I2C_LS2X_CR); + return readb_poll_timeout(priv->base + I2C_LS2X_SR, value, + !(value & LS2X_SR_BUSY), 100, + jiffies_to_usecs(priv->adapter.timeout)); +} + +static int ls2x_i2c_start(struct ls2x_i2c_priv *priv, struct i2c_msg *msgs) +{ + reinit_completion(&priv->cmd_complete); + + writeb(i2c_8bit_addr_from_msg(msgs), priv->base + I2C_LS2X_TXR); + return ls2x_i2c_send_byte(priv, LS2X_CR_START | LS2X_CR_WRITE); +} + +static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) +{ + int ret; + u8 rxdata; + + while (len--) { + ret = ls2x_i2c_xfer_byte(priv, + LS2X_CR_READ | (len ? 0 : LS2X_CR_ACK), + &rxdata); + if (ret) + return ret; + + *buf++ = readb(priv->base + I2C_LS2X_RXR); + } + + return 0; +} + +static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len) +{ + int ret; + + while (len--) { + writeb(*buf++, priv->base + I2C_LS2X_TXR); + + ret = ls2x_i2c_send_byte(priv, LS2X_CR_WRITE); + if (ret) + return ret; + } + + return 0; +} + +static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv, + struct i2c_msg *msg, bool stop) +{ + int ret; + bool is_read = msg->flags & I2C_M_RD; + + /* Contains steps to send start condition and address */ + ret = ls2x_i2c_start(priv, msg); + if (!ret) { + if (is_read) + ret = ls2x_i2c_rx(priv, msg->buf, msg->len); + else + ret = ls2x_i2c_tx(priv, msg->buf, msg->len); + + if (!ret && stop) + ret = ls2x_i2c_stop(priv); + } + + if (ret == -ENXIO) + ls2x_i2c_stop(priv); + else if (ret < 0) + ls2x_i2c_init(priv); + + return ret; +} + +static int ls2x_i2c_master_xfer(struct i2c_adapter *adap, + struct i2c_msg *msgs, int num) +{ + int ret; + struct i2c_msg *msg, *emsg = msgs + num; + struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap); + + for (msg = msgs; msg < emsg; msg++) { + ret = ls2x_i2c_xfer_one(priv, msg, msg == emsg - 1); + if (ret) + return ret; + } + + return num; +} + +static unsigned int ls2x_i2c_func(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; +} + +static const struct i2c_algorithm ls2x_i2c_algo = { + .master_xfer = ls2x_i2c_master_xfer, + .functionality = ls2x_i2c_func, +}; + +static int ls2x_i2c_probe(struct platform_device *pdev) +{ + int ret, irq; + struct i2c_adapter *adap; + struct ls2x_i2c_priv *priv; + struct device *dev = &pdev->dev; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + /* Map hardware registers */ + priv->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + /* Add the i2c adapter */ + adap = &priv->adapter; + adap->nr = pdev->id; + adap->owner = THIS_MODULE; + adap->retries = 5; + adap->algo = &ls2x_i2c_algo; + adap->dev.parent = dev; + device_set_node(&adap->dev, dev_fwnode(dev)); + i2c_set_adapdata(adap, priv); + strscpy(adap->name, pdev->name, sizeof(adap->name)); + init_completion(&priv->cmd_complete); + platform_set_drvdata(pdev, priv); + + ls2x_i2c_init(priv); + + ret = devm_request_irq(dev, irq, ls2x_i2c_isr, IRQF_SHARED, "ls2x-i2c", + priv); + if (ret < 0) + return dev_err_probe(dev, ret, "Unable to request irq %d\n", irq); + + return devm_i2c_add_adapter(dev, adap); +} + +static int ls2x_i2c_suspend(struct device *dev) +{ + struct ls2x_i2c_priv *priv = dev_get_drvdata(dev); + + /* Disable interrupts */ + writeb(readb(priv->base + I2C_LS2X_CTR) & ~LS2X_CTR_IEN, + priv->base + I2C_LS2X_CTR); + + return 0; +} + +static int ls2x_i2c_resume(struct device *dev) +{ + ls2x_i2c_init(dev_get_drvdata(dev)); + return 0; +} + +static DEFINE_RUNTIME_DEV_PM_OPS(ls2x_i2c_pm_ops, + ls2x_i2c_suspend, ls2x_i2c_resume, NULL); + +static const struct of_device_id ls2x_i2c_id_table[] = { + { .compatible = "loongson,ls2k-i2c" }, + { .compatible = "loongson,ls7a-i2c" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, ls2x_i2c_id_table); + +static const struct acpi_device_id ls2x_i2c_acpi_match[] = { + { "LOON0004" }, /* Loongson LS7A */ + { } +}; +MODULE_DEVICE_TABLE(acpi, ls2x_i2c_acpi_match); + +static struct platform_driver ls2x_i2c_driver = { + .probe = ls2x_i2c_probe, + .driver = { + .name = "ls2x-i2c", + .pm = pm_sleep_ptr(&ls2x_i2c_pm_ops), + .of_match_table = ls2x_i2c_id_table, + .acpi_match_table = ls2x_i2c_acpi_match, + }, +}; + +/* The DC subsystem depends on it, we should initialize it earlier */ +static int __init ls2x_i2c_init_driver(void) +{ + return platform_driver_register(&ls2x_i2c_driver); +} +subsys_initcall(ls2x_i2c_init_driver); + +static void __exit ls2x_i2c_exit_driver(void) +{ + platform_driver_unregister(&ls2x_i2c_driver); +} +module_exit(ls2x_i2c_exit_driver); + +MODULE_DESCRIPTION("Loongson LS2X I2C Bus driver"); +MODULE_AUTHOR("Loongson Technology Corporation Limited"); +MODULE_LICENSE("GPL");
This I2C module is integrated into the Loongson-2K SoCs and Loongson LS7A bridge chip. Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> --- drivers/i2c/busses/Kconfig | 11 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-ls2x.c | 366 ++++++++++++++++++++++++++++++++++ 3 files changed, 378 insertions(+) create mode 100644 drivers/i2c/busses/i2c-ls2x.c