From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <54F9704B.3040501@metafoo.de> References: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> <54F9704B.3040501@metafoo.de> Date: Fri, 6 Mar 2015 15:25:25 +0530 Message-ID: Subject: Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16 From: Somya Anand To: Lars-Peter Clausen Cc: Daniel Baluta , outreachy-kernel@googlegroups.com, Jonathan Cameron , "linux-iio@vger.kernel.org" Content-Type: multipart/alternative; boundary=001a113655b600cc1105109baf7e List-ID: --001a113655b600cc1105109baf7e Content-Type: text/plain; charset=UTF-8 On Fri, Mar 6, 2015 at 2:45 PM, Lars-Peter Clausen wrote: > On 03/06/2015 09:58 AM, Daniel Baluta wrote: > >> Hi Somya, >> >> On Fri, Mar 6, 2015 at 10:36 AM, Somya Anand >> wrote: >> >>> In the adis16220_read16bit() function we earlier used a s16 value 'val' >>> which is used by the adis_read_reg_16 function to read data and takes a >>> u16 value as a parameter. >>> >>> So, this patch changes the data type of 'val' from s16 to u16 for further >>> simplification of code and thereby avoiding unnecessary typecast. >>> >>> Signed-off-by: Somya Anand >>> >> > Patch looks ok. But it changes the semantics of the function, the reason > why this is a s16 instead of a u16 is because at some point the function > was used to read register that contained signed 16 bit values. The code is > essentially the short version of: > > u16 uval; > adis_read_reg_16(&st->adis, this_attr->address, &uval); > sprintf("%d\n", sign_extend32(uval, 15)); > I have asked Daniel regarding this earlier because while reading the code, I could not get the exact purpose of using s16. Thanks for clearing my doubt. > > This patch removes the extra sign extension. Which is ok, since the only > user of the function uses it to read a 10 unsigned value which will lead to > the same result in both cases. But the commit message should mention this, > since it is not just the removal of a unnecessary type cast. > > Sure, I will write a better commit message so that it would be more descriptive. > - Lars > Somya --001a113655b600cc1105109baf7e Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Fri, Mar 6, 2015 at 2:45 PM, Lars-Peter Clausen &l= t;lars@metafoo.de&= gt; wrote:
On 03/= 06/2015 09:58 AM, Daniel Baluta wrote:
Hi Somya,

On Fri, Mar 6, 2015 at 10:36 AM, Somya Anand <somyaanand214@gmail.com> wrote:
In the adis16220_read16bit() function we earlier used a s16 value 'val&= #39;
which is used by the adis_read_reg_16 function to read data and takes a
u16 value as a parameter.

So, this patch changes the data type of 'val' from s16 to u16 for f= urther
simplification of code and thereby avoiding unnecessary typecast.

Signed-off-by: Somya Anand <somyaanand214@gmail.com>

 Patch looks ok. But it changes the semantics of the function, the reason wh= y this is a s16 instead of a u16 is because at some point the function was = used to read register that contained signed 16 bit values. The code is esse= ntially the short version of:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 u16 uval;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 adis_read_reg_16(&st->adis, this_attr-&g= t;address, &uval);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 sprintf("%d\n", sign_extend32(uval, 1= 5));

=C2=A0 =C2=A0 I have asked Daniel = regarding this earlier because while reading the code, I could not get the = exact purpose of using s16. Thanks for clearing my doubt.=C2=A0

This patch removes the extra sign extension. Which is ok, since the only us= er of the function uses it to read a 10 unsigned value which will lead to t= he same result in both cases. But the commit message should mention this, s= ince it is not just the removal of a unnecessary type cast.

=C2=A0 =C2=A0Sure, I will write a bette= r commit message so that it would be more descriptive. =C2=A0
- Lars

=C2=A0 Somya=C2=A0=

--001a113655b600cc1105109baf7e-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <54F9704B.3040501@metafoo.de> Date: Fri, 06 Mar 2015 10:15:55 +0100 From: Lars-Peter Clausen MIME-Version: 1.0 To: Daniel Baluta , Somya Anand CC: outreachy-kernel@googlegroups.com, Jonathan Cameron , "linux-iio@vger.kernel.org" Subject: Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16 References: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed List-ID: On 03/06/2015 09:58 AM, Daniel Baluta wrote: > Hi Somya, > > On Fri, Mar 6, 2015 at 10:36 AM, Somya Anand wrote: >> In the adis16220_read16bit() function we earlier used a s16 value 'val' >> which is used by the adis_read_reg_16 function to read data and takes a >> u16 value as a parameter. >> >> So, this patch changes the data type of 'val' from s16 to u16 for further >> simplification of code and thereby avoiding unnecessary typecast. >> >> Signed-off-by: Somya Anand Patch looks ok. But it changes the semantics of the function, the reason why this is a s16 instead of a u16 is because at some point the function was used to read register that contained signed 16 bit values. The code is essentially the short version of: u16 uval; adis_read_reg_16(&st->adis, this_attr->address, &uval); sprintf("%d\n", sign_extend32(uval, 15)); This patch removes the extra sign extension. Which is ok, since the only user of the function uses it to read a 10 unsigned value which will lead to the same result in both cases. But the commit message should mention this, since it is not just the removal of a unnecessary type cast. - Lars From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> References: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> Date: Fri, 6 Mar 2015 10:58:54 +0200 Message-ID: Subject: Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16 From: Daniel Baluta To: Somya Anand Cc: outreachy-kernel@googlegroups.com, Jonathan Cameron , Lars-Peter Clausen , "linux-iio@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 List-ID: Hi Somya, On Fri, Mar 6, 2015 at 10:36 AM, Somya Anand wrote: > In the adis16220_read16bit() function we earlier used a s16 value 'val' > which is used by the adis_read_reg_16 function to read data and takes a > u16 value as a parameter. > > So, this patch changes the data type of 'val' from s16 to u16 for further > simplification of code and thereby avoiding unnecessary typecast. > > Signed-off-by: Somya Anand Ideally, subject should tell us "Why?" the patch is needed, instead of "What?" the patch does. The latter should be easy to understand reading the code. e.g: staging: iio: adis16220: Avoid unnecessary typecast > --- > drivers/staging/iio/accel/adis16220_core.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/accel/adis16220_core.c b/drivers/staging/iio/accel/adis16220_core.c > index d478f51..7a4a0fd 100644 > --- a/drivers/staging/iio/accel/adis16220_core.c > +++ b/drivers/staging/iio/accel/adis16220_core.c > @@ -28,16 +28,15 @@ static ssize_t adis16220_read_16bit(struct device *dev, > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > struct adis16220_state *st = iio_priv(indio_dev); > ssize_t ret; > - s16 val = 0; > + u16 val = 0; > > /* Take the iio_dev status lock */ > mutex_lock(&indio_dev->mlock); > - ret = adis_read_reg_16(&st->adis, this_attr->address, > - (u16 *)&val); > + ret = adis_read_reg_16(&st->adis, this_attr->address, &val); > mutex_unlock(&indio_dev->mlock); > if (ret) > return ret; > - return sprintf(buf, "%d\n", val); > + return sprintf(buf, "%u\n", val); > } > > static ssize_t adis16220_write_16bit(struct device *dev, Other than that it looks good to me. We will need an ACK from Jonathan / Lars on this. thanks, Daniel. From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 6845034921984 X-Received: by 10.152.6.41 with SMTP id x9mr2131857lax.10.1425633169020; Fri, 06 Mar 2015 01:12:49 -0800 (PST) X-BeenThere: outreachy-kernel@googlegroups.com Received: by 10.180.98.233 with SMTP id el9ls58147wib.8.gmail; Fri, 06 Mar 2015 01:12:48 -0800 (PST) X-Received: by 10.194.201.10 with SMTP id jw10mr2145774wjc.3.1425633168107; Fri, 06 Mar 2015 01:12:48 -0800 (PST) Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de. [212.227.126.130]) by gmr-mx.google.com with ESMTPS id by11si69319wib.1.2015.03.06.01.12.47 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 06 Mar 2015 01:12:48 -0800 (PST) Received-SPF: none (google.com: arnd@arndb.de does not designate permitted sender hosts) client-ip=212.227.126.130; Authentication-Results: gmr-mx.google.com; spf=none (google.com: arnd@arndb.de does not designate permitted sender hosts) smtp.mail=arnd@arndb.de Received: from oxbaltgw05.schlund.de ([172.19.246.11]) by mrelayeu.kundenserver.de (mreue004) with ESMTPSA (Nemesis) id 0M5Kwt-1XXrIu2F2t-00zT0E; Fri, 06 Mar 2015 10:12:47 +0100 Date: Fri, 6 Mar 2015 10:12:47 +0100 (CET) From: arnd@arndb.de Reply-To: arnd@arndb.de To: Somya Anand , outreachy-kernel Message-ID: <899581237.61503.1425633167521.JavaMail.open-xchange@oxbaltgw05.schlund.de> In-Reply-To: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> References: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> Subject: Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Priority: 3 Importance: Medium X-Mailer: Open-Xchange Mailer v7.6.0-Rev38 X-Originating-Client: open-xchange-appsuite X-Provags-ID: V03:K0:5hpaEfSCt+5TnGSSJMLPb0DHdswahljS5Uz8alRasN88i9zu6af QMC1QTrrNpM9uODqwD8+iiihk1oCTiSCUhTYXDvJ4eU7dDCFUqKcMkVRahtc2RK9wu+kJvY hZcdMuKMWV7zHyh0GOIie5f+5jqnlOE/+MTfdO93Un/wRukk396J2YTd456htavEai9/pVh bZjqLTTfgQEYhMTPPzzug== X-UI-Out-Filterresults: notjunk:1; > Somya Anand hat am 6. M=C3=A4rz 2015 um 09:36 > geschrieben: > diff --git a/drivers/staging/iio/accel/adis16220_core.c > b/drivers/staging/iio/accel/adis16220_core.c > index d478f51..7a4a0fd 100644 > --- a/drivers/staging/iio/accel/adis16220_core.c > +++ b/drivers/staging/iio/accel/adis16220_core.c > @@ -28,16 +28,15 @@ static ssize_t adis16220_read_16bit(struct device *de= v, > struct iio_dev *indio_dev =3D dev_to_iio_dev(dev); > struct adis16220_state *st =3D iio_priv(indio_dev); > ssize_t ret; > - s16 val =3D 0; > + u16 val =3D 0; > =20 > /* Take the iio_dev status lock */ > mutex_lock(&indio_dev->mlock); In a patch like this, it would be best to also drop the initialization to z= ero, which is not used here. Not initializing it helps the compiler provide useful warnin= gs if the code gets changed to return an otherwise uninitialized result. > - ret =3D adis_read_reg_16(&st->adis, this_attr->address, > - (u16 *)&val); > + ret =3D adis_read_reg_16(&st->adis, this_attr->address, &val); > mutex_unlock(&indio_dev->mlock); > if (ret) > return ret; > - return sprintf(buf, "%d\n", val); > + return sprintf(buf, "%u\n", val); > } > =20 This looks wrong: Unless you know better, you should assume that the sprint= f format string is correct and that the value is indeed signed for the purpose of th= is file. The reason why adis_read_reg_16() takes a u16 argument instead of s16 is th= at it accesses a register that doesn't have a concept of signedness. I would just leave the code as it is, alternatively you could have two variables and explicitl= y cast from u16 to s16 with a comment. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 6845034921984 X-Received: by 10.236.223.41 with SMTP id u39mr13748309yhp.49.1425630883221; Fri, 06 Mar 2015 00:34:43 -0800 (PST) X-BeenThere: outreachy-kernel@googlegroups.com Received: by 10.107.132.162 with SMTP id o34ls203271ioi.64.gmail; Fri, 06 Mar 2015 00:34:43 -0800 (PST) X-Received: by 10.42.76.145 with SMTP id e17mr6692613ick.34.1425630883029; Fri, 06 Mar 2015 00:34:43 -0800 (PST) Return-Path: Received: from mail-pa0-x229.google.com (mail-pa0-x229.google.com. [2607:f8b0:400e:c03::229]) by gmr-mx.google.com with ESMTPS id hs7si1250033pad.1.2015.03.06.00.34.42 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 06 Mar 2015 00:34:43 -0800 (PST) Received-SPF: pass (google.com: domain of somyaanand214@gmail.com designates 2607:f8b0:400e:c03::229 as permitted sender) client-ip=2607:f8b0:400e:c03::229; Authentication-Results: gmr-mx.google.com; spf=pass (google.com: domain of somyaanand214@gmail.com designates 2607:f8b0:400e:c03::229 as permitted sender) smtp.mail=somyaanand214@gmail.com; dkim=pass header.i=@gmail.com; dmarc=pass (p=NONE dis=NONE) header.from=gmail.com Received: by mail-pa0-x229.google.com with SMTP id rd3so25870755pab.5 for ; Fri, 06 Mar 2015 00:34:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=6wddSRE28BGLxRcpRVku9oRlV6N7IHP0b36s5YNM4/8=; b=mtJeyiUs7wzeipGoFXFCpFuAbgz1Wd8Ukel0JEb1DSSUlNGIuSLgYsG1JpaZddusvl bHk+Xj/p5KAYuUXjsXC8Tdpzeq2uhXq8p8S0BI2SG2lmm0TP3mZzwokwnneaGwADAMQZ 2lZEDwyG6+VZHqJxpBzXc+02dpIyLlQlIJuqiHDND7JkB19Za0I520th+8Dp+lUXkPGQ sG5WGbf3gKcNie1q7P/Wx/VRNm5AE+YzoGNSgRWNLaOEqy5/9MiQVq8g16FXYDsHmiQI xYT+1l9zqxicwLoGUUB4LBcYN80PQlyrGANtXSbmODiJoeiEVIJxAJjktWZh3OBACdkE hm0Q== X-Received: by 10.68.189.105 with SMTP id gh9mr23235918pbc.142.1425630882901; Fri, 06 Mar 2015 00:34:42 -0800 (PST) Return-Path: Received: from localhost.localdomain ([182.66.41.7]) by mx.google.com with ESMTPSA id gf2sm1033422pbc.60.2015.03.06.00.34.39 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 06 Mar 2015 00:34:42 -0800 (PST) From: Somya Anand To: outreachy-kernel@googlegroups.com Cc: Somya Anand Subject: [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16 Date: Fri, 6 Mar 2015 14:06:00 +0530 Message-Id: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> X-Mailer: git-send-email 1.9.1 In the adis16220_read16bit() function we earlier used a s16 value 'val' which is used by the adis_read_reg_16 function to read data and takes a u16 value as a parameter. So, this patch changes the data type of 'val' from s16 to u16 for further simplification of code and thereby avoiding unnecessary typecast. Signed-off-by: Somya Anand --- drivers/staging/iio/accel/adis16220_core.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/iio/accel/adis16220_core.c b/drivers/staging/iio/accel/adis16220_core.c index d478f51..7a4a0fd 100644 --- a/drivers/staging/iio/accel/adis16220_core.c +++ b/drivers/staging/iio/accel/adis16220_core.c @@ -28,16 +28,15 @@ static ssize_t adis16220_read_16bit(struct device *dev, struct iio_dev *indio_dev = dev_to_iio_dev(dev); struct adis16220_state *st = iio_priv(indio_dev); ssize_t ret; - s16 val = 0; + u16 val = 0; /* Take the iio_dev status lock */ mutex_lock(&indio_dev->mlock); - ret = adis_read_reg_16(&st->adis, this_attr->address, - (u16 *)&val); + ret = adis_read_reg_16(&st->adis, this_attr->address, &val); mutex_unlock(&indio_dev->mlock); if (ret) return ret; - return sprintf(buf, "%d\n", val); + return sprintf(buf, "%u\n", val); } static ssize_t adis16220_write_16bit(struct device *dev, -- 1.9.1