Message ID | Y2iFGA3A1w+XMlYU@qemulion |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1825308wru; Sun, 6 Nov 2022 20:22:11 -0800 (PST) X-Google-Smtp-Source: AMsMyM6dcjLR+yHZNeM4Pw6oCgwcP7GezD51ljnACA5JNpIOUPs3VdJeypc8k8LaN7x5kOWjn/ZC X-Received: by 2002:a17:907:31c4:b0:78d:9b2f:4e1a with SMTP id xf4-20020a17090731c400b0078d9b2f4e1amr45514951ejb.306.1667794931166; Sun, 06 Nov 2022 20:22:11 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1667794931; cv=none; d=google.com; s=arc-20160816; b=adTXoKozuq277HzWM6XKjnBPJJu5y3V7Fnlzn6ucQc3CJdFrW4q7FyNP0Ke/if4Hk3 NpSBrHE4Y5bLIU0yIJ/TC8AYG4tZEPYhqG+wCYj/Pc05CXPpyw6tQY8X62NftSwAvo16 L6vgNCT3hx6JrR27zKfEAgnJQR7HwRls5MVgsQ2kJNhh15E8BixqM1GSP5OUgglIf0BZ rfgC6AE7IQVviF+zKlqpuXcgFO5OjorAKDjkX6cSW42XAVW3tOp+pGs4Db2Y90vqYXpn R1UYsB5RScjWT0Jj3AgexBcutQJlTpvffv+PlH6USk1ziZSYQBGaweWSE+2g9SQoNrvM Oihw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:to:from:date:dkim-signature; bh=mzdRwK+FDf9QKdejktZw8mC4DlPBcS6LU1aafD6I2jU=; b=RKWnQNKD6ukwxlYwgSdpU2QutvHX9Fyn65ElaZ2w4fv6fvucHjLugmgvLnDQzoVRGc GYhXtM4lcnx47KT+zHoQJRVDXV20ALD5WQ53mSBdKwPobyTTkij5EvI4mfz6QGAvvIir s/0NKxMl3103NGuBgIslvpbLEFb00bnL4/N+rz4e7ntm872nhgkPY4YstwD7P0I4kE3S 8S8gp1fxQPhUOeo7bkomwERl9st1lJ/WPjPins5SgQuqlizgsqCKAW/9cPLsmc6Y2ZTa oB+ilLcTnCKK99zWH2KXB7K90zDvfDqmuIsMHeh9y2PVRrxS8YV5wQ1+OzeYCXeqvfwX +mSA== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=C9iFnW7R; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id n10-20020a50cc4a000000b00463ba265d95si7169208edi.392.2022.11.06.20.21.42; Sun, 06 Nov 2022 20:22:11 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=C9iFnW7R; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230434AbiKGEKf (ORCPT <rfc822;hjfbswb@gmail.com> + 99 others); Sun, 6 Nov 2022 23:10:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37332 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230239AbiKGEKc (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 6 Nov 2022 23:10:32 -0500 Received: from msg-4.mailo.com (msg-4.mailo.com [213.182.54.15]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BE3FEC742; Sun, 6 Nov 2022 20:10:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1667794206; bh=4vpubAxBJPLICHSVHvPCZgQDNFOK73r6037oLcqxRNw=; h=X-EA-Auth:Date:From:To:Subject:Message-ID:MIME-Version: Content-Type; b=C9iFnW7RbBKdhCtxtXzOLa3JlnRrnJ7VcaqFqQVVR90dK9DJVByEiltrZ1pYEHdXW HaZ8FnufHYDVsG2dFR2FWw551lYOjE8LaT6/W7ah0A/HFTvl89M+WhaAXckJee0i1y ThqaZvQWeOJGzQDG1BwXvTM7OKTLYLbeKpByI7oE= Received: by b-4.in.mailobj.net [192.168.90.14] with ESMTP via ip-206.mailobj.net [213.182.55.206] Mon, 7 Nov 2022 05:10:06 +0100 (CET) X-EA-Auth: xmv0Uzdl1lJntsxK/BrE7s8F1LLtHp58aHBwFEduoB5F/pQe89jIg2LygmBdI/VKxLWHU6Cnupv4LfA4oPG2e08H69d/mTW0 Date: Mon, 7 Nov 2022 09:40:00 +0530 From: Deepak R Varma <drv@mailo.com> To: Lars-Peter Clausen <lars@metafoo.de>, Michael Hennerich <Michael.Hennerich@analog.com>, Jonathan Cameron <jic23@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-iio@vger.kernel.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org Subject: [PATCH] staging: iio: meter: use min() for comparison and assignment Message-ID: <Y2iFGA3A1w+XMlYU@qemulion> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748809737254803747?= X-GMAIL-MSGID: =?utf-8?q?1748809737254803747?= |
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
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
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 >
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;
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
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 >
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
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 >
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 --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,