| Message ID | 0fce410bbf7c3d5e01cf679fd435fa0065e55e53.1698889581.git.hanshu-oc@zhaoxin.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: add zhaoxin i2c controller driver | expand |
On Thu, Nov 02, 2023 at 10:53:54AM +0800, Hans Hu wrote: > These Settings for the same register, REG_CR, > can be put together to reduce code redundancy. > > Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com> Some registers require that you first write a certain value, and only then can write another value. Have you double checked with datasheets that these register writes can safely be aggregated? > --- > drivers/i2c/busses/i2c-wmt.c | 35 +++++++++-------------------------- > 1 file changed, 9 insertions(+), 26 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-wmt.c b/drivers/i2c/busses/i2c-wmt.c > index a44ce5fdde8a..d503e85752ea 100644 > --- a/drivers/i2c/busses/i2c-wmt.c > +++ b/drivers/i2c/busses/i2c-wmt.c > @@ -144,9 +144,6 @@ static int wmt_i2c_write(struct i2c_adapter *adap, struct i2c_msg *pmsg, > if (!(pmsg->flags & I2C_M_NOSTART)) { > val = readw(i2c_dev->base + REG_CR); > val &= ~CR_TX_END; > - writew(val, i2c_dev->base + REG_CR); > - > - val = readw(i2c_dev->base + REG_CR); > val |= CR_CPU_RDY; > writew(val, i2c_dev->base + REG_CR); > } > @@ -204,24 +201,15 @@ static int wmt_i2c_read(struct i2c_adapter *adap, struct i2c_msg *pmsg, > u32 xfer_len = 0; > > val = readw(i2c_dev->base + REG_CR); > - val &= ~CR_TX_END; > - writew(val, i2c_dev->base + REG_CR); > + val &= ~(CR_TX_END | CR_TX_NEXT_NO_ACK); > > - val = readw(i2c_dev->base + REG_CR); > - val &= ~CR_TX_NEXT_NO_ACK; > - writew(val, i2c_dev->base + REG_CR); > - > - if (!(pmsg->flags & I2C_M_NOSTART)) { > - val = readw(i2c_dev->base + REG_CR); > + if (!(pmsg->flags & I2C_M_NOSTART)) > val |= CR_CPU_RDY; > - writew(val, i2c_dev->base + REG_CR); > - } > > - if (pmsg->len == 1) { > - val = readw(i2c_dev->base + REG_CR); > + if (pmsg->len == 1) > val |= CR_TX_NEXT_NO_ACK; > - writew(val, i2c_dev->base + REG_CR); > - } > + > + writew(val, i2c_dev->base + REG_CR); > > reinit_completion(&i2c_dev->complete); > > @@ -243,15 +231,10 @@ static int wmt_i2c_read(struct i2c_adapter *adap, struct i2c_msg *pmsg, > pmsg->buf[xfer_len] = readw(i2c_dev->base + REG_CDR) >> 8; > xfer_len++; > > - if (xfer_len == pmsg->len - 1) { > - val = readw(i2c_dev->base + REG_CR); > - val |= (CR_TX_NEXT_NO_ACK | CR_CPU_RDY); > - writew(val, i2c_dev->base + REG_CR); > - } else { > - val = readw(i2c_dev->base + REG_CR); > - val |= CR_CPU_RDY; > - writew(val, i2c_dev->base + REG_CR); > - } > + val = readw(i2c_dev->base + REG_CR) | CR_CPU_RDY; > + if (xfer_len == pmsg->len - 1) > + val |= CR_TX_NEXT_NO_ACK; > + writew(val, i2c_dev->base + REG_CR); > } > > return 0; > -- > 2.34.1 >
On Fri, Dec 22, 2023 at 10:38:29AM +0100, Wolfram Sang wrote: > On Thu, Nov 02, 2023 at 10:53:54AM +0800, Hans Hu wrote: > > These Settings for the same register, REG_CR, > > can be put together to reduce code redundancy. > > > > Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com> > > Some registers require that you first write a certain value, and only > then can write another value. Have you double checked with datasheets > that these register writes can safely be aggregated? I found a WM8505 datasheet and it doesn't say anything about consecutive writes. And it works on your hardware. Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-wmt.c b/drivers/i2c/busses/i2c-wmt.c index a44ce5fdde8a..d503e85752ea 100644 --- a/drivers/i2c/busses/i2c-wmt.c +++ b/drivers/i2c/busses/i2c-wmt.c @@ -144,9 +144,6 @@ static int wmt_i2c_write(struct i2c_adapter *adap, struct i2c_msg *pmsg, if (!(pmsg->flags & I2C_M_NOSTART)) { val = readw(i2c_dev->base + REG_CR); val &= ~CR_TX_END; - writew(val, i2c_dev->base + REG_CR); - - val = readw(i2c_dev->base + REG_CR); val |= CR_CPU_RDY; writew(val, i2c_dev->base + REG_CR); } @@ -204,24 +201,15 @@ static int wmt_i2c_read(struct i2c_adapter *adap, struct i2c_msg *pmsg, u32 xfer_len = 0; val = readw(i2c_dev->base + REG_CR); - val &= ~CR_TX_END; - writew(val, i2c_dev->base + REG_CR); + val &= ~(CR_TX_END | CR_TX_NEXT_NO_ACK); - val = readw(i2c_dev->base + REG_CR); - val &= ~CR_TX_NEXT_NO_ACK; - writew(val, i2c_dev->base + REG_CR); - - if (!(pmsg->flags & I2C_M_NOSTART)) { - val = readw(i2c_dev->base + REG_CR); + if (!(pmsg->flags & I2C_M_NOSTART)) val |= CR_CPU_RDY; - writew(val, i2c_dev->base + REG_CR); - } - if (pmsg->len == 1) { - val = readw(i2c_dev->base + REG_CR); + if (pmsg->len == 1) val |= CR_TX_NEXT_NO_ACK; - writew(val, i2c_dev->base + REG_CR); - } + + writew(val, i2c_dev->base + REG_CR); reinit_completion(&i2c_dev->complete); @@ -243,15 +231,10 @@ static int wmt_i2c_read(struct i2c_adapter *adap, struct i2c_msg *pmsg, pmsg->buf[xfer_len] = readw(i2c_dev->base + REG_CDR) >> 8; xfer_len++; - if (xfer_len == pmsg->len - 1) { - val = readw(i2c_dev->base + REG_CR); - val |= (CR_TX_NEXT_NO_ACK | CR_CPU_RDY); - writew(val, i2c_dev->base + REG_CR); - } else { - val = readw(i2c_dev->base + REG_CR); - val |= CR_CPU_RDY; - writew(val, i2c_dev->base + REG_CR); - } + val = readw(i2c_dev->base + REG_CR) | CR_CPU_RDY; + if (xfer_len == pmsg->len - 1) + val |= CR_TX_NEXT_NO_ACK; + writew(val, i2c_dev->base + REG_CR); } return 0;
These Settings for the same register, REG_CR, can be put together to reduce code redundancy. Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com> --- drivers/i2c/busses/i2c-wmt.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-)