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(-)

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 */
-- 
2.47.1

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

On Tue, Mar 18, 2025 at 4:16 AM Aryan Srivastava
<aryan.srivastava@alliedtelesis.co.nz> 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.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko