Re: [PATCH 64/64] i2c: reword i2c_algorithm in drivers according to newest specification

Re: [PATCH 64/64] i2c: reword i2c_algorithm in drivers according to newest specification

[Andy Shevchenko]

On Fri, Mar 22, 2024 at 02:25:57PM +0100, Wolfram Sang wrote:
> Match the wording in i2c_algorithm in I2C drivers wrt. the newest I2C
> v7, SMBus 3.2, I3C specifications and replace "master/slave" with more
> appropriate terms. For some drivers, this means no more conversions are
> needed. For the others more work needs to be done but this will be
> performed incrementally along with API changes/improvements. All these
> changes here are simple search/replace results.

...

>  static const struct i2c_algorithm at91_twi_algorithm = {
> -	.master_xfer	= at91_twi_xfer,
> +	.xfer	= at91_twi_xfer,

Seems you made this by a script, can you check the indentations afterwards?

>  	.functionality	= at91_twi_func,
>  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 64/64] i2c: reword i2c_algorithm in drivers according to newest specification

[Andy Shevchenko]

On Fri, Mar 22, 2024 at 05:48:54PM +0100, Wolfram Sang wrote:
> 
> > >  static const struct i2c_algorithm at91_twi_algorithm = {
> > > -	.master_xfer	= at91_twi_xfer,
> > > +	.xfer	= at91_twi_xfer,
> > 
> > Seems you made this by a script, can you check the indentations afterwards?
> 
> Yes, I noticed as well. But other (not converted) drivers have issues
> there as well, so this will be a seperate patch.

The problem is that you add to a technical debt. We don't want that.
If you have not introduced a new indentation issue, it obviously is
not needed to be fixed in a separate patch. So, please consider this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/64] i2c: reword i2c_algorithm according to newest specification

[Andi Shyti]

Hi Wolfram,

On Fri, Mar 22, 2024 at 02:24:53PM +0100, Wolfram Sang wrote:
> Okay, we need to begin somewhere...
> 
> Start changing the wording of the I2C main header wrt. the newest I2C
> v7, SMBus 3.2, I3C specifications and replace "master/slave" with more
> appropriate terms. This first step renames the members of struct
> i2c_algorithm. Once all in-tree users are converted, the anonymous union
> will go away again. All this work will also pave the way for finally
> seperating the monolithic header into more fine-grained headers like
> "i2c/clients.h" etc. So, this is not a simple renaming-excercise but
> also a chance to update the I2C core to recent Linux standards.

yes, very good! It's clearly stated in all three documentations
that Target replaces Slave and Controller replaces Master (i3c is
at the 1.1.1 version).

> My motivation is to improve the I2C core API, in general. My motivation
> is not to clean each and every driver. I think this is impossible
> because register names based on official documentation will need to stay
> as they are. But the Linux-internal names should be updated IMO.

Also because some drivers have been written based on previous
specifications where master/slave was used.

> That being said, I worked on 62 drivers in this series beyond plain
> renames inside 'struct i2c_algorithm' because the fruits were so
> low-hanging. Before this series, 112 files in the 'busses/' directory
> contained 'master' and/or 'slave'. After the series, only 57. Why not?
> 
> Next step is updating the drivers outside the 'i2c'-folder regarding
> 'struct i2c_algorithm' so we can remove the anonymous union ASAP. To be
> able to work on this with minimal dependencies, I'd like to apply this
> series between -rc1 and -rc2.
> 
> I hope this will work for you guys. The changes are really minimal. If
> you are not comfortable with changes to your driver or need more time to
> review, please NACK the patch and I will drop the patch and/or address
> the issues separeately.
> 
> @Andi: are you okay with this approach? It means you'd need to merge
> -rc2 into your for-next branch. Or rebase if all fails.

I think it's a good plan, I'll try to support you with it.

> Speaking of Andi, thanks a lot to him taking care of the controller
> drivers these days. His work really gives me the freedom to work on I2C
> core issues again.

Thank you, Wolfram!

Andi

Re: [PATCH 05/64] i2c: aspeed: reword according to newest specification

[Andrew Jeffery]

On Fri, 2024-03-22 at 14:24 +0100, Wolfram Sang wrote:
> Match the wording of this driver wrt. the newest I2C v7, SMBus 3.2, I3C
> specifications and replace "master/slave" with more appropriate terms.
> They are also more specific because we distinguish now between a remote
> entity ("client") and a local one ("target").
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Re: [PATCH 05/64] i2c: aspeed: reword according to newest specification

[Andi Shyti]

Hi Wolfram,

On Fri, Mar 22, 2024 at 02:24:58PM +0100, Wolfram Sang wrote:
> Match the wording of this driver wrt. the newest I2C v7, SMBus 3.2, I3C
> specifications and replace "master/slave" with more appropriate terms.
> They are also more specific because we distinguish now between a remote
> entity ("client") and a local one ("target").
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index ce8c4846b7fa..4e6ea4a5cab9 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -159,7 +159,7 @@ struct aspeed_i2c_bus {
>  	bool				send_stop;
>  	int				cmd_err;
>  	/* Protected only by i2c_lock_bus */
> -	int				master_xfer_result;
> +	int				xfer_result;
>  	/* Multi-master */
>  	bool				multi_master;
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -608,9 +608,9 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  out_complete:
>  	bus->msgs = NULL;
>  	if (bus->cmd_err)
> -		bus->master_xfer_result = bus->cmd_err;
> +		bus->xfer_result = bus->cmd_err;
>  	else
> -		bus->master_xfer_result = bus->msgs_index + 1;
> +		bus->xfer_result = bus->msgs_index + 1;
>  	complete(&bus->cmd_complete);
>  out_no_complete:
>  	return irq_handled;
> @@ -679,7 +679,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>  }
>  
> -static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> +static int aspeed_i2c_xfer(struct i2c_adapter *adap,
>  				  struct i2c_msg *msgs, int num)

here the alignment goes a bi off.

>  {
>  	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> @@ -738,7 +738,7 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>  		return -ETIMEDOUT;
>  	}
>  
> -	return bus->master_xfer_result;
> +	return bus->xfer_result;
>  }
>  
>  static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
> @@ -748,7 +748,7 @@ static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
>  
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  /* precondition: bus.lock has been acquired. */
> -static void __aspeed_i2c_reg_slave(struct aspeed_i2c_bus *bus, u16 slave_addr)
> +static void __aspeed_i2c_reg_target(struct aspeed_i2c_bus *bus, u16 slave_addr)

We  have the word master/slave forgotten here and there, but as
we are here, /slave_addr/target_addr/

>  {
>  	u32 addr_reg_val, func_ctrl_reg_val;
>  
> @@ -770,7 +770,7 @@ static void __aspeed_i2c_reg_slave(struct aspeed_i2c_bus *bus, u16 slave_addr)
>  	bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
>  }
>  
> -static int aspeed_i2c_reg_slave(struct i2c_client *client)
> +static int aspeed_i2c_reg_target(struct i2c_client *client)
>  {
>  	struct aspeed_i2c_bus *bus = i2c_get_adapdata(client->adapter);
>  	unsigned long flags;
> @@ -781,7 +781,7 @@ static int aspeed_i2c_reg_slave(struct i2c_client *client)
>  		return -EINVAL;
>  	}
>  
> -	__aspeed_i2c_reg_slave(bus, client->addr);
> +	__aspeed_i2c_reg_target(bus, client->addr);
>  
>  	bus->slave = client;
>  	spin_unlock_irqrestore(&bus->lock, flags);
> @@ -789,7 +789,7 @@ static int aspeed_i2c_reg_slave(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static int aspeed_i2c_unreg_slave(struct i2c_client *client)
> +static int aspeed_i2c_unreg_target(struct i2c_client *client)
>  {
>  	struct aspeed_i2c_bus *bus = i2c_get_adapdata(client->adapter);
>  	u32 func_ctrl_reg_val;
> @@ -814,11 +814,11 @@ static int aspeed_i2c_unreg_slave(struct i2c_client *client)
>  #endif /* CONFIG_I2C_SLAVE */
>  
>  static const struct i2c_algorithm aspeed_i2c_algo = {
> -	.master_xfer	= aspeed_i2c_master_xfer,
> +	.xfer	= aspeed_i2c_xfer,

here the alignment goes a bit off.

Andi

Re: [PATCH 00/64] i2c: reword i2c_algorithm according to newest specification

[Andi Shyti]

Hi Wolfram,

> > @Andi: are you okay with this approach? It means you'd need to merge
> > -rc2 into your for-next branch. Or rebase if all fails.
> 
> I think it's a good plan, I'll try to support you with it.

Do you feel more comfortable if I take the patches as soon as
they are reviewd?

So far I have tagged patch 1-4 and I can already merge 2,3,4 as
long as you merge patch 1.

Andi

Re: [PATCH 20/64] i2c: fsi: reword according to newest specification

[Andi Shyti]

Hi Wolfram,

On Fri, Mar 22, 2024 at 02:25:13PM +0100, Wolfram Sang wrote:
> Match the wording of this driver wrt. the newest I2C v7, SMBus 3.2, I3C
> specifications and replace "master/slave" with more appropriate terms.
> They are also more specific because we distinguish now between a remote
> entity ("client") and a local one ("target").
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-fsi.c | 56 ++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
> index 10332693edf0..5eaf2c85a72c 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * FSI-attached I2C master algorithm
> + * FSI-attached I2C controller algorithm

As this change is all about renaming, I think we need to be a bit
more consistent in using either host or controller, at least
within the same driver.

In this driver you are using sometimes host and sometimes
controller.

Thanks,
Andi

Re: [PATCH 64/64] i2c: reword i2c_algorithm in drivers according to newest specification

[Linus Walleij]

On Fri, Mar 22, 2024 at 2:27 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:

> Match the wording in i2c_algorithm in I2C drivers wrt. the newest I2C
> v7, SMBus 3.2, I3C specifications and replace "master/slave" with more
> appropriate terms. For some drivers, this means no more conversions are
> needed. For the others more work needs to be done but this will be
> performed incrementally along with API changes/improvements. All these
> changes here are simple search/replace results.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 00/64] i2c: reword i2c_algorithm according to newest specification

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

Hi Andi, hi everyone,

thank you for reviewing and waiting. I had a small personal hiatus over
Easter but now I am back. This series needs another cycle, so no need to
hurry. I will address some of the review comments but not all. The
conversion (and API improvements) are some bigger tasks, so
inconsistencies inbetween can't be avoided AFAICS.

I'll keep you updated.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 05/64] i2c: aspeed: reword according to newest specification

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]


> > -static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> > +static int aspeed_i2c_xfer(struct i2c_adapter *adap,
> >  				  struct i2c_msg *msgs, int num)
> 
> here the alignment goes a bi off.

Thanks, I missed this.

> >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >  /* precondition: bus.lock has been acquired. */
> > -static void __aspeed_i2c_reg_slave(struct aspeed_i2c_bus *bus, u16 slave_addr)
> > +static void __aspeed_i2c_reg_target(struct aspeed_i2c_bus *bus, u16 slave_addr)
> 
> We  have the word master/slave forgotten here and there, but as
> we are here, /slave_addr/target_addr/

I can do this now. My plan was to convert it when I convert the whole
CONFIG_I2C_SLAVE interface. But "since we are here" can be argued.

> >  static const struct i2c_algorithm aspeed_i2c_algo = {
> > -	.master_xfer	= aspeed_i2c_master_xfer,
> > +	.xfer	= aspeed_i2c_xfer,
> 
> here the alignment goes a bit off.

I also wanted to fix this afterwards together with all the tab-indented
struct declarations in busses/. But maybe I better do the tab-removal
series beforehand? Would you accept such a thing?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]