diff mbox series

staging: iio: meter: use min() for comparison and assignment

Message ID Y2iFGA3A1w+XMlYU@qemulion
State New
Headers
Series staging: iio: meter: use min() for comparison and assignment |

Commit Message

Deepak R Varma Nov. 7, 2022, 4:10 a.m. UTC 
  Simplify code by using recommended min helper macro for logical
evaluation and value assignment. This issue is identified by
coccicheck using the minmax.cocci file.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
 drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.34.1

Comments

Dan Carpenter Nov. 7, 2022, 1:08 p.m. UTC | #1 
On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote:
> Simplify code by using recommended min helper macro for logical
> evaluation and value assignment. This issue is identified by
> coccicheck using the minmax.cocci file.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index a9a06e8dda51..a6ce7b24cc8f 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev,
>  unlock:
>  	mutex_unlock(&st->buf_lock);
> 
> -	return ret < 0 ? ret : 0;
> +	return min(ret, 0);

The original code is better.

If it's a failure return the error code.  If it's not return zero.

You can only compare apples to apples.  min() makes sense if you're
talking about two lengths.  But here if ret is negative that's an error
code.  If it's positive that's the number of bytes.  If the error
code is less than the number of bytes then return that?  What???  It
makes no sense.

In terms of run time, this patch is fine but in terms of reading the
code using min() makes it less readable.

regards,
dan carpenter
Deepak R Varma Nov. 7, 2022, 3:15 p.m. UTC | #2 
On Mon, Nov 07, 2022 at 04:08:31PM +0300, Dan Carpenter wrote:
> On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote:
> > Simplify code by using recommended min helper macro for logical
> > evaluation and value assignment. This issue is identified by
> > coccicheck using the minmax.cocci file.
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> > index a9a06e8dda51..a6ce7b24cc8f 100644
> > --- a/drivers/staging/iio/meter/ade7854-i2c.c
> > +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev,
> >  unlock:
> >  	mutex_unlock(&st->buf_lock);
> >
> > -	return ret < 0 ? ret : 0;
> > +	return min(ret, 0);
>
> The original code is better.
>
> If it's a failure return the error code.  If it's not return zero.
>
> You can only compare apples to apples.  min() makes sense if you're
> talking about two lengths.  But here if ret is negative that's an error
> code.  If it's positive that's the number of bytes.  If the error
> code is less than the number of bytes then return that?  What???  It
> makes no sense.

Thanks for your view point. I agree.

>
> In terms of run time, this patch is fine but in terms of reading the
> code using min() makes it less readable.

Okay, The proposal does not make much difference, so will leave the original
line as is.

Thank you,
./drv

>
> regards,
> dan carpenter
>
Joe Perches Nov. 7, 2022, 3:22 p.m. UTC | #3 
On Mon, 2022-11-07 at 16:08 +0300, Dan Carpenter wrote:
> On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote:
> > Simplify code by using recommended min helper macro for logical
> > evaluation and value assignment. This issue is identified by
> > coccicheck using the minmax.cocci file.
> > 
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> > index a9a06e8dda51..a6ce7b24cc8f 100644
> > --- a/drivers/staging/iio/meter/ade7854-i2c.c
> > +++ b/drivers/staging/iio/meter/ade7854-i2c.ck
> > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev,
> >  unlock:
> >  	mutex_unlock(&st->buf_lock);
> > 
> > -	return ret < 0 ? ret : 0;
> > +	return min(ret, 0);
> 
> The original code is better.
> 
> If it's a failure return the error code.  If it's not return zero.
> 
> You can only compare apples to apples.  min() makes sense if you're
> talking about two lengths.  But here if ret is negative that's an error
> code.  If it's positive that's the number of bytes.  If the error
> code is less than the number of bytes then return that?  What???  It
> makes no sense.
> 
> In terms of run time, this patch is fine but in terms of reading the
> code using min() makes it less readable.

It's not a runtime question, either should compile to the same object
code.  It's definitely a readabiity and standardization issue.

In this case, IMO it'd be better to use the much more common

	if (ret < 0)
		return ret;

	return 0;
Dan Carpenter Nov. 7, 2022, 3:38 p.m. UTC | #4 
On Mon, Nov 07, 2022 at 07:22:24AM -0800, Joe Perches wrote:
> > In terms of run time, this patch is fine but in terms of reading the
> > code using min() makes it less readable.
> 
> It's not a runtime question, either should compile to the same object
> code.  It's definitely a readabiity and standardization issue.
> 
> In this case, IMO it'd be better to use the much more common
> 
> 	if (ret < 0)
> 		return ret;
> 
> 	return 0;

I also prefer this format.

But at the same time, I can't advise Deepak to go around changing
existing code where the author like ternaries.

regards,
dan carpenter
Deepak R Varma Nov. 7, 2022, 4:27 p.m. UTC | #5 
On Mon, Nov 07, 2022 at 06:38:35PM +0300, Dan Carpenter wrote:
> On Mon, Nov 07, 2022 at 07:22:24AM -0800, Joe Perches wrote:
> > > In terms of run time, this patch is fine but in terms of reading the
> > > code using min() makes it less readable.
> >
> > It's not a runtime question, either should compile to the same object
> > code.  It's definitely a readabiity and standardization issue.
> >
> > In this case, IMO it'd be better to use the much more common
> >
> > 	if (ret < 0)
> > 		return ret;
> >
> > 	return 0;
>
> I also prefer this format.
>
> But at the same time, I can't advise Deepak to go around changing
> existing code where the author like ternaries.

Thank you Joe, Dan. Just to conclude, I will leave the line untouched as it is
no big advantage and the current format is more readable.

./drv

>
> regards,
> dan carpenter
>
Greg KH Nov. 8, 2022, 3:12 p.m. UTC | #6 
On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote:
> Simplify code by using recommended min helper macro for logical
> evaluation and value assignment. This issue is identified by
> coccicheck using the minmax.cocci file.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index a9a06e8dda51..a6ce7b24cc8f 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev,
>  unlock:
>  	mutex_unlock(&st->buf_lock);
> 
> -	return ret < 0 ? ret : 0;
> +	return min(ret, 0);

As others have said, this isn't ok, and I hate ? : usage, so if you
want, spell that out please.

thanks,

greg k-h
Deepak R Varma Nov. 8, 2022, 3:36 p.m. UTC | #7 
On Tue, Nov 08, 2022 at 04:12:17PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote:
> > Simplify code by using recommended min helper macro for logical
> > evaluation and value assignment. This issue is identified by
> > coccicheck using the minmax.cocci file.
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> > index a9a06e8dda51..a6ce7b24cc8f 100644
> > --- a/drivers/staging/iio/meter/ade7854-i2c.c
> > +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev,
> >  unlock:
> >  	mutex_unlock(&st->buf_lock);
> >
> > -	return ret < 0 ? ret : 0;
> > +	return min(ret, 0);
>
> As others have said, this isn't ok, and I hate ? : usage, so if you
> want, spell that out please.

Hello Greg,
Just want to make sure I am getting it right:
Are you suggesting me to resubmit the patch with revised patch description?

Should I consider using the "if" based evaluation rather than using min() macro?

Thank you,
./drv

>
> thanks,
>
> greg k-h
>
Jonathan Cameron Nov. 12, 2022, 4:35 p.m. UTC | #8 
On Tue, 8 Nov 2022 21:06:24 +0530
Deepak R Varma <drv@mailo.com> wrote:

> On Tue, Nov 08, 2022 at 04:12:17PM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote:  
> > > Simplify code by using recommended min helper macro for logical
> > > evaluation and value assignment. This issue is identified by
> > > coccicheck using the minmax.cocci file.
> > >
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > >  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> > > index a9a06e8dda51..a6ce7b24cc8f 100644
> > > --- a/drivers/staging/iio/meter/ade7854-i2c.c
> > > +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> > > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev,
> > >  unlock:
> > >  	mutex_unlock(&st->buf_lock);
> > >
> > > -	return ret < 0 ? ret : 0;
> > > +	return min(ret, 0);  
> >
> > As others have said, this isn't ok, and I hate ? : usage, so if you
> > want, spell that out please.  
> 
> Hello Greg,
> Just want to make sure I am getting it right:
> Are you suggesting me to resubmit the patch with revised patch description?
> 
> Should I consider using the "if" based evaluation rather than using min() macro?

For IIO staging drivers, I'd take a cleanup that moved to

	if (ret < 0)
		return ret;

	return 0;

As others have suggested though, not a good idea to do this broadly as it
would be a lot of noise.  We don't mind noise so much for staging drivers :)

Jonathan

> 
> Thank you,
> ./drv
> 
> >
> > thanks,
> >
> > greg k-h
> >  
> 
>
diff mbox series

Patch

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index a9a06e8dda51..a6ce7b24cc8f 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -61,7 +61,7 @@  static int ade7854_i2c_write_reg(struct device *dev,
 unlock:
 	mutex_unlock(&st->buf_lock);

-	return ret < 0 ? ret : 0;
+	return min(ret, 0);
 }

 static int ade7854_i2c_read_reg(struct device *dev,