[PATCH v9 0/3] Improve usage of 'ret' variable and make pcf_doAdress() void

[PATCH v9 0/3] Improve usage of 'ret' variable and make pcf_doAdress() void

[Cezar Chiru]

Hello maintainers,
This patch series is a response to Change Requests made by Andi Shyti on
[PATCH v7 0/3] i2c: pcf8584: Fix errors and warnings reported by checkpatch
and more specific on PATCH: i2c: pcf8584: Move 'ret' variable inside for
loop, break if ret < 0.
Also a comment from Andy Shevchenko on "[PATCH v8 1/1] i2c: pcf8584: Move
'ret' variable inside for loop, break if ret < 0." about using 'goto out;'
instead of break since the goto out; was used in other error path branches
so it makes sense to be consistent.

2 new patches have been introduced into this patch series and they are
dependent on each other that's why the need for a new patch series v9.

Change Requests:
 -remove initialization of 'ret' variable inside for loop of pcf_xfer() as
 it is not needed
 -change pcf_doAddress() function type from int to void as it always
 returns 0.
 -change if (ret < 0) break; with if (ret < 0) goto out; to be consistent
 -change pcf_doAddress() function name to pcf_send_address()

Testing:
 *built kernel and modules with I2C_ALGOPCF=m and my 3 patches applied on
 top of 6.18.0-rc1.
 *installed kernel and external modules generated by build on my laptop
 *rebooted and loaded i2c-algo-pcf.ko without i2c_debug parameter.
 *when loading the .ko with i2c_debug parameter an error is seen in dmesg
 and this is expected as the parameter was removed.
 *No success message related to i2c_algo_pcf was seen in dmesg but also
 no failures.
 *Module loading and unloading successful.
 *No PCF8584 Hardware was available

Cezar Chiru (3):
  i2c: pcf8584: Move 'ret' variable inside for loop, goto out if ret <
    0.
  i2c: pcf8584: Make pcf_doAddress() function void
  i2c: pcf8584: Change pcf_doAdress() to pcf_send_address()

 drivers/i2c/algos/i2c-algo-pcf.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

--
2.43.0

[PATCH v9 1/3] i2c: pcf8584: Move 'ret' variable inside for loop, goto out if ret < 0.

[Cezar Chiru]

Require spaces around '=' and '<'. Add spaces around binary operators.
Enforce error fixing based on checkpatch.pl output on file.
Move 'ret' variable inside for loop. Then check if (ret < 0) goto out. This
improves usage of ret variable.

Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
Suggested-by: Andi Shyti <andi.shyti@kernel.org>
Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
 drivers/i2c/algos/i2c-algo-pcf.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 41a81d37e880..06b9fd355bff 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -183,7 +183,7 @@ static int pcf_sendbytes(struct i2c_adapter *i2c_adap, const char *buf,
 	struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
 	int wrcount, status, timeout;

-	for (wrcount=0; wrcount<count; ++wrcount) {
+	for (wrcount = 0; wrcount < count; ++wrcount) {
 		i2c_outb(adap, buf[wrcount]);
 		timeout = wait_for_pin(adap, &status);
 		if (timeout) {
@@ -272,7 +272,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 	struct i2c_algo_pcf_data *adap = i2c_adap->algo_data;
 	struct i2c_msg *pmsg;
 	int i;
-	int ret=0, timeout, status;
+	int timeout, status;

 	if (adap->xfer_begin)
 		adap->xfer_begin(adap->data);
@@ -284,9 +284,10 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 		goto out;
 	}

-	for (i = 0;ret >= 0 && i < num; i++) {
-		pmsg = &msgs[i];
+	for (i = 0; i < num; i++) {
+		int ret;

+		pmsg = &msgs[i];
 		ret = pcf_doAddress(adap, pmsg);

 		/* Send START */
@@ -321,6 +322,9 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 			ret = pcf_sendbytes(i2c_adap, pmsg->buf, pmsg->len,
 					    (i + 1 == num));
 		}
+
+		if (ret < 0)
+			goto out;
 	}

 out:
--
2.43.0

[PATCH v9 2/3] i2c: pcf8584: Make pcf_doAddress() function void

[Cezar Chiru]

Change pcf_doAddress() function's type from int to void as it always
returns 0. This way there is no need for extra assignment and extra checks
when the function is called.
Remove assignment of pcf_doAddress() and replace it with a simple function
call.

Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
Suggested-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/algos/i2c-algo-pcf.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 06b9fd355bff..6352314e48ed 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -253,7 +253,7 @@ static int pcf_readbytes(struct i2c_adapter *i2c_adap, char *buf,
 }
 
 
-static int pcf_doAddress(struct i2c_algo_pcf_data *adap,
+static void pcf_doAddress(struct i2c_algo_pcf_data *adap,
 			 struct i2c_msg *msg)
 {
 	unsigned char addr = i2c_8bit_addr_from_msg(msg);
@@ -261,8 +261,6 @@ static int pcf_doAddress(struct i2c_algo_pcf_data *adap,
 	if (msg->flags & I2C_M_REV_DIR_ADDR)
 		addr ^= 1;
 	i2c_outb(adap, addr);
-
-	return 0;
 }
 
 static int pcf_xfer(struct i2c_adapter *i2c_adap,
@@ -288,7 +286,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 		int ret;
 
 		pmsg = &msgs[i];
-		ret = pcf_doAddress(adap, pmsg);
+		pcf_doAddress(adap, pmsg);
 
 		/* Send START */
 		if (i == 0)
-- 
2.43.0

[PATCH v9 3/3] i2c: pcf8584: Change pcf_doAdress() to pcf_send_address()

[Cezar Chiru]

Change name of pcf_doAddress() function to pcf_send_address() to be
more in line with the kernel functions naming.

Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
Suggested-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/algos/i2c-algo-pcf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-pcf.c b/drivers/i2c/algos/i2c-algo-pcf.c
index 6352314e48ed..a87ecea7f510 100644
--- a/drivers/i2c/algos/i2c-algo-pcf.c
+++ b/drivers/i2c/algos/i2c-algo-pcf.c
@@ -253,7 +253,7 @@ static int pcf_readbytes(struct i2c_adapter *i2c_adap, char *buf,
 }


-static void pcf_doAddress(struct i2c_algo_pcf_data *adap,
+static void pcf_send_address(struct i2c_algo_pcf_data *adap,
 			 struct i2c_msg *msg)
 {
 	unsigned char addr = i2c_8bit_addr_from_msg(msg);
@@ -286,7 +286,7 @@ static int pcf_xfer(struct i2c_adapter *i2c_adap,
 		int ret;

 		pmsg = &msgs[i];
-		pcf_doAddress(adap, pmsg);
+		pcf_send_address(adap, pmsg);

 		/* Send START */
 		if (i == 0)
--
2.43.0

Re: [PATCH v9 1/3] i2c: pcf8584: Move 'ret' variable inside for loop, goto out if ret < 0.

[Andy Shevchenko]

On Thu, Oct 23, 2025 at 03:00:41PM +0300, Cezar Chiru wrote:
> Require spaces around '=' and '<'. Add spaces around binary operators.
> Enforce error fixing based on checkpatch.pl output on file.
> Move 'ret' variable inside for loop. Then check if (ret < 0) goto out. This
> improves usage of ret variable.

...

> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>

I haven't suggested much, but if you insist in the tag I won't object.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 1/3] i2c: pcf8584: Move 'ret' variable inside for loop, goto out if ret < 0.

[Andi Shyti]

Hi Cezar,

On Thu, Oct 23, 2025 at 03:00:41PM +0300, Cezar Chiru wrote:
> Require spaces around '=' and '<'. Add spaces around binary operators.
> Enforce error fixing based on checkpatch.pl output on file.
> Move 'ret' variable inside for loop. Then check if (ret < 0) goto out. This
> improves usage of ret variable.
> 
> Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
> Suggested-by: Andi Shyti <andi.shyti@kernel.org>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>

you don't really need to resend patches for updating the tag
list. Anyway, that's OK, better to send than not to send, when in
doubt, ask.

For this patch I think neither me or Andy have been suggesting
the change. The change came from you, we made observation which
you applied, this is the normal review process.

If you don't mind, I'm going to remove them when applying (let me
know if you don't agree). No need to resend.

Andi

Re: [PATCH v9 1/3] i2c: pcf8584: Move 'ret' variable inside for loop, goto out if ret < 0.

[Cezar Chiru]

Hi Andi,

> > Require spaces around '=' and '<'. Add spaces around binary operators.
> > Enforce error fixing based on checkpatch.pl output on file.
> > Move 'ret' variable inside for loop. Then check if (ret < 0) goto out. This
> > improves usage of ret variable.
> >
> > Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
> > Suggested-by: Andi Shyti <andi.shyti@kernel.org>
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>
> you don't really need to resend patches for updating the tag
> list. Anyway, that's OK, better to send than not to send, when in
> doubt, ask.

Anyways I had to resend as I grouped 3 patches into 1 patch series

> For this patch I think neither me or Andy have been suggesting
> the change. The change came from you, we made observation which
> you applied, this is the normal review process.

Can you please let me know the process of tagging with Suggested-by
without resending the patch. I don't know how people add reviewed-by
or ACK-ed-by or Suggested-by other than resend the patch?
I've seen it but have yet to figure how to do it.

> If you don't mind, I'm going to remove them when applying (let me
> know if you don't agree). No need to resend.

OK. Do as you want and think it's right.

Regards,
Cezar Chiru

Re: [PATCH v9 1/3] i2c: pcf8584: Move 'ret' variable inside for loop, goto out if ret < 0.

[Andi Shyti]

Hi Cezar,

On Fri, Oct 24, 2025 at 04:55:28PM +0300, Cezar Chiru wrote:
> > > Require spaces around '=' and '<'. Add spaces around binary operators.
> > > Enforce error fixing based on checkpatch.pl output on file.
> > > Move 'ret' variable inside for loop. Then check if (ret < 0) goto out. This
> > > improves usage of ret variable.
> > >
> > > Signed-off-by: Cezar Chiru <chiru.cezar.89@gmail.com>
> > > Suggested-by: Andi Shyti <andi.shyti@kernel.org>
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> >
> > you don't really need to resend patches for updating the tag
> > list. Anyway, that's OK, better to send than not to send, when in
> > doubt, ask.
> 
> Anyways I had to resend as I grouped 3 patches into 1 patch series
> 
> > For this patch I think neither me or Andy have been suggesting
> > the change. The change came from you, we made observation which
> > you applied, this is the normal review process.
> 
> Can you please let me know the process of tagging with Suggested-by
> without resending the patch. I don't know how people add reviewed-by
> or ACK-ed-by or Suggested-by other than resend the patch?
> I've seen it but have yet to figure how to do it.

with Reviewed-by I understand that you have carefully reviewed
the code line by line, agree with everything written, and do not
see any issues or major improvements to be made.

With Acked-by I understand that you have read the change and
agree with it, but you may not have gone into the fine details.
You are simply OK with the patch being applied.

With Suggested-by I understand that someone has proposed the
entire idea behind the patch, not just smaller improvements or
changes made during review. For example, if I tell you that 'ret'
does not need to be initialised and can be moved inside the for
loop, that comes from review. However, if I suggest sending a new
patch changing the function type, that would justify a
Suggested-by tag.

Then there are other tags, such as Reported-by when someone
reports an issue, and many more that you will learn if you stay
in the community (and I hope you will).

Everything is more or less documented in
Documentation/process/submitting-patches.rst, and when in doubt,
asking is always the right thing to do.

Again, thank you for your patches,
Andi

Re: [PATCH v9 0/3] Improve usage of 'ret' variable and make pcf_doAdress() void

[Andi Shyti]

Hi Cezar,

> Cezar Chiru (3):
>   i2c: pcf8584: Move 'ret' variable inside for loop, goto out if ret <
>     0.
>   i2c: pcf8584: Make pcf_doAddress() function void
>   i2c: pcf8584: Change pcf_doAdress() to pcf_send_address()

merged to i2c/i2c-host.

Thanks,
Andi