| Message ID | 20250318021632.2710792-3-aryan.srivastava@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi again, On Tue, Mar 18, 2025 at 03:16:30PM +1300, Aryan Srivastava wrote: > The driver gives the illusion of 10-bit address support, but the upper > 3 bits of the given address are always thrown away. Remove unnecessary > considerations for 10 bit addressing and always complete 7 bit ops when > using the hlc methods. > > Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz> sorry, I commented on the previous version. Pasting it here to keep the discussion on here. " Can someone from Marvell comment here? The datasheet I have isn't very clear on this part. Also, I'd like to know if there's any product line that could be negatively impacted by this patch. " Thanks Andi
> The datasheet I have isn't very clear on this part. Also, I'd > like to know if there's any product line that could be negatively > impacted by this patch. In my whole I2C life, I have neither seen a target supporting 10-bit addressing nor a a system that uses 10-bit addressing. I am even tempted to remove support from the kernel omce in a while. If the support is broken in this driver, it can be removed. A working version (if possible) can be added again by someone who needs it. I am taking bets it will be "never". Besides, the driver never set I2C_FUNC_10BIT_ADDR, so it really shouldn't have been used anywhere. Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On Wed, Mar 19, 2025 at 06:41:24AM +0100, Wolfram Sang wrote: > > The datasheet I have isn't very clear on this part. Also, I'd > > like to know if there's any product line that could be negatively > > impacted by this patch. > > In my whole I2C life, I have neither seen a target supporting 10-bit > addressing nor a a system that uses 10-bit addressing. mmhhh... I have to work with my memory and dig into my documentations, but I think I've seen some STM sensors supporting also 10 bit addresses. > I am even tempted > to remove support from the kernel omce in a while. If the support is > broken in this driver, it can be removed. The documentation I have is in line with the patch (I had to read it twice, though), but I didn't want to exclude corner cases. > A working version (if > possible) can be added again by someone who needs it. I am taking bets > it will be "never". Besides, the driver never set I2C_FUNC_10BIT_ADDR, > so it really shouldn't have been used anywhere. > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> OK, I had a little hesitation here, but if you are sure about it (and Andy as well) I'll take it in. Thanks, Wolfram! Andi
Hi Andi, > mmhhh... I have to work with my memory and dig into my > documentations, but I think I've seen some STM sensors supporting > also 10 bit addresses. I'd be super-super-interested if you can remember which devices had that! All the best, Wolfram
diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c index 0094fe5f7460..baf6b27f3752 100644 --- a/drivers/i2c/busses/i2c-octeon-core.c +++ b/drivers/i2c/busses/i2c-octeon-core.c @@ -421,17 +421,12 @@ static int octeon_i2c_hlc_read(struct octeon_i2c *i2c, struct i2c_msg *msgs) octeon_i2c_hlc_enable(i2c); octeon_i2c_hlc_int_clear(i2c); - cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR; + cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR | SW_TWSI_OP_7; /* SIZE */ cmd |= (u64)(msgs[0].len - 1) << SW_TWSI_SIZE_SHIFT; /* A */ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT; - if (msgs[0].flags & I2C_M_TEN) - cmd |= SW_TWSI_OP_10; - else - cmd |= SW_TWSI_OP_7; - octeon_i2c_writeq_flush(cmd, i2c->twsi_base + OCTEON_REG_SW_TWSI(i2c)); ret = octeon_i2c_hlc_wait(i2c); if (ret) @@ -463,17 +458,12 @@ static int octeon_i2c_hlc_write(struct octeon_i2c *i2c, struct i2c_msg *msgs) octeon_i2c_hlc_enable(i2c); octeon_i2c_hlc_int_clear(i2c); - cmd = SW_TWSI_V | SW_TWSI_SOVR; + cmd = SW_TWSI_V | SW_TWSI_SOVR | SW_TWSI_OP_7; /* SIZE */ cmd |= (u64)(msgs[0].len - 1) << SW_TWSI_SIZE_SHIFT; /* A */ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT; - if (msgs[0].flags & I2C_M_TEN) - cmd |= SW_TWSI_OP_10; - else - cmd |= SW_TWSI_OP_7; - for (i = 0, j = msgs[0].len - 1; i < msgs[0].len && i < 4; i++, j--) cmd |= (u64)msgs[0].buf[j] << (8 * i); @@ -513,11 +503,6 @@ static bool octeon_i2c_hlc_ext(struct octeon_i2c *i2c, struct i2c_msg msg, u64 * bool set_ext = false; u64 cmd = 0; - if (msg.flags & I2C_M_TEN) - cmd |= SW_TWSI_OP_10_IA; - else - cmd |= SW_TWSI_OP_7_IA; - if (msg.len == 2) { cmd |= SW_TWSI_EIA; *ext = (u64)msg.buf[0] << SW_TWSI_IA_SHIFT; @@ -550,7 +535,7 @@ static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs octeon_i2c_hlc_enable(i2c); - cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR; + cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR | SW_TWSI_OP_7_IA; /* SIZE */ cmd |= (u64)(msgs[1].len - 1) << SW_TWSI_SIZE_SHIFT; /* A */ @@ -587,7 +572,7 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg octeon_i2c_hlc_enable(i2c); - cmd = SW_TWSI_V | SW_TWSI_SOVR; + cmd = SW_TWSI_V | SW_TWSI_SOVR | SW_TWSI_OP_7_IA; /* SIZE */ cmd |= (u64)(msgs[1].len - 1) << SW_TWSI_SIZE_SHIFT; /* A */
The driver gives the illusion of 10-bit address support, but the upper 3 bits of the given address are always thrown away. Remove unnecessary considerations for 10 bit addressing and always complete 7 bit ops when using the hlc methods. Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz> --- drivers/i2c/busses/i2c-octeon-core.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)