mbox series

[v2,00/10] i2c: busses: Introduce and use i2c_10bit_addr_*_from_msg()

Message ID 20250213141045.2716943-1-andriy.shevchenko@linux.intel.com
Headers show
Series i2c: busses: Introduce and use i2c_10bit_addr_*_from_msg() | expand

Message

Andy Shevchenko Feb. 13, 2025, 2:07 p.m. UTC 
For 8-bit addresses we have a helper function, define similar ones
for 10-bit addresses and use it in the drivers. It allows to remove
some boilerplate code.

Fabrizio, I haven't collected your tags as code changed more than 50%, however
semantically the parts you reviewed stay the same.

In v2:
- introduced hi/lo helpers (Geert)
- rewrote series based on the above
- added a couple of new converted drivers

v1: https://lore.kernel.org/r/20250212163359.2407327-1-andriy.shevchenko@linux.intel.com

Andy Shevchenko (10):
  i2c: Introduce i2c_10bit_addr_*_from_msg() helpers
  i2c: axxia: Use i2c_10bit_addr_*_from_msg() helpers
  i2c: bcm-kona: Use i2c_10bit_addr_*_from_msg() helpers
  i2c: brcmstb: Use i2c_10bit_addr_*_from_msg() helpers
  i2c: eg20t: Use i2c_10bit_addr_*_from_msg() helpers
  i2c: kempld: Use i2c_10bit_addr_*_from_msg() helpers
  i2c: mt7621: Use i2c_10bit_addr_*_from_msg() helpers
  i2c: rzv2m: Use i2c_10bit_addr_*_from_msg() helpers
  i2c: ibm_iic: Use i2c_*bit_addr*_from_msg() helpers
  i2c: mv64xxx: Use i2c_*bit_addr*_from_msg() helpers

 drivers/i2c/busses/i2c-axxia.c    | 21 +++------------------
 drivers/i2c/busses/i2c-bcm-kona.c |  6 +++---
 drivers/i2c/busses/i2c-brcmstb.c  | 11 +++++------
 drivers/i2c/busses/i2c-eg20t.c    | 28 +++++-----------------------
 drivers/i2c/busses/i2c-ibm_iic.c  | 14 ++++++--------
 drivers/i2c/busses/i2c-kempld.c   | 10 +++++-----
 drivers/i2c/busses/i2c-mt7621.c   | 20 ++++++++------------
 drivers/i2c/busses/i2c-mv64xxx.c  | 12 +++---------
 drivers/i2c/busses/i2c-rzv2m.c    | 15 +++++----------
 include/linux/i2c.h               | 14 ++++++++++++++
 10 files changed, 57 insertions(+), 94 deletions(-)

Comments

AngeloGioacchino Del Regno Feb. 13, 2025, 4:01 p.m. UTC | #1 
Il 13/02/25 15:07, Andy Shevchenko ha scritto:
> Use i2c_10bit_addr_*_from_msg() helpers instead of local copy.
> No functional change intended.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Can we please do the helper conversion as one commit and the (much needed)
cleanup of assigning len and de-duplicating the call to mtk_i2c_cmd() as
two commits?

One with just the conversion, one with the cleanup (or in inverse order,
as you wish).

Thanks,
Angelo

> ---
>   drivers/i2c/busses/i2c-mt7621.c | 20 ++++++++------------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mt7621.c b/drivers/i2c/busses/i2c-mt7621.c
> index 2103f21f9ddd..0a288c998419 100644
> --- a/drivers/i2c/busses/i2c-mt7621.c
> +++ b/drivers/i2c/busses/i2c-mt7621.c
> @@ -164,22 +164,18 @@ static int mtk_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>   		/* write address */
>   		if (pmsg->flags & I2C_M_TEN) {
>   			/* 10 bits address */
> -			addr = 0xf0 | ((pmsg->addr >> 7) & 0x06);
> -			addr |= (pmsg->addr & 0xff) << 8;
> -			if (pmsg->flags & I2C_M_RD)
> -				addr |= 1;
> -			iowrite32(addr, i2c->base + REG_SM0D0_REG);
> -			ret = mtk_i2c_cmd(i2c, SM0CTL1_WRITE, 2);
> -			if (ret)
> -				goto err_timeout;
> +			addr = i2c_10bit_addr_hi_from_msg(pmsg);
> +			addr |= i2c_10bit_addr_lo_from_msg(pmsg) << 8;
> +			len = 2;
>   		} else {
>   			/* 7 bits address */
>   			addr = i2c_8bit_addr_from_msg(pmsg);
> -			iowrite32(addr, i2c->base + REG_SM0D0_REG);
> -			ret = mtk_i2c_cmd(i2c, SM0CTL1_WRITE, 1);
> -			if (ret)
> -				goto err_timeout;
> +			len = 1;
>   		}
> +		iowrite32(addr, i2c->base + REG_SM0D0_REG);
> +		ret = mtk_i2c_cmd(i2c, SM0CTL1_WRITE, len);
> +		if (ret)
> +			goto err_timeout;
>   
>   		/* check address ACK */
>   		if (!(pmsg->flags & I2C_M_IGNORE_NAK)) {
Wolfram Sang Feb. 13, 2025, 4:15 p.m. UTC | #2 
> @@ -132,10 +130,12 @@ static int kempld_i2c_process(struct kempld_i2c_data *i2c)
>  
>  	/* Second part of 10 bit addressing */
>  	if (i2c->state == STATE_ADDR10) {
> -		kempld_write8(pld, KEMPLD_I2C_DATA, i2c->msg->addr & 0xff);
> +		addr = i2c_10bit_addr_lo_from_msg(msg);
> +		i2c->state = STATE_START;

Any reason you moved this?

> +
> +		kempld_write8(pld, KEMPLD_I2C_DATA, addr);

Maybe we could skip using 'addr' here?

>  		kempld_write8(pld, KEMPLD_I2C_CMD, I2C_CMD_WRITE);
>  
> -		i2c->state = STATE_START;
>  		return 0;
>  	}
Andy Shevchenko Feb. 13, 2025, 4:37 p.m. UTC | #3 
On Thu, Feb 13, 2025 at 05:15:09PM +0100, Wolfram Sang wrote:

...

> >  	/* Second part of 10 bit addressing */
> >  	if (i2c->state == STATE_ADDR10) {
> > -		kempld_write8(pld, KEMPLD_I2C_DATA, i2c->msg->addr & 0xff);
> > +		addr = i2c_10bit_addr_lo_from_msg(msg);
> > +		i2c->state = STATE_START;
> 
> Any reason you moved this?

Yes, I would like to be in sync in the above state machine case, just upper in
the code which is not visible in this patch.

> > +		kempld_write8(pld, KEMPLD_I2C_DATA, addr);
> 
> Maybe we could skip using 'addr' here?

Same reason as above.

> >  		kempld_write8(pld, KEMPLD_I2C_CMD, I2C_CMD_WRITE);
> >  
> > -		i2c->state = STATE_START;
> >  		return 0;
> >  	}