add sign-extend functions for 8 and 16 bit values

add sign-extend functions for 8 and 16 bit values

[Martin Kepplinger]

In short: If you want the first 2 changes, please merge them and notify 
me.

This adds sign_extend8() and sign_extend16() for the quite many cases
where this is needed, like sign_extend32().

Sign-extending is done in a few different ways throughout the kernel
and most of them look not very beautiful, adding non-obvious constant
values. This would simplify things.

I append four example changes for existing drivers. I'm aware that you
are not resposible for these, but I'd post them to the relevant
maintainers only if you want to apply to bitops.h.

While I would definitely move on moving drivers over to this, when I find
them, I can't guarantee I'd change every sign-extension out there myself.

[PATCH 1/6] bitops.h: add sign_extend8 function

[Martin Kepplinger]

This adds a helper function that extends any signed value smaller than
8 bits to a s8.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 include/linux/bitops.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 5d858e0..3c2a539 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -161,6 +161,17 @@ static inline __u8 ror8(__u8 word, unsigned int shift)
 }
 
 /**
+ * sign_extend8 - sign extend a 8-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<8) to sign bit
+ */
+static inline __s8 sign_extend8(__u8 value, int index)
+{
+	__u8 shift = 7 - index;
+	return (__s8)(value << shift) >> shift;
+}
+
+/**
  * sign_extend32 - sign extend a 32-bit value using specified bit as sign-bit
  * @value: value to sign extend
  * @index: 0 based bit index (0<=index<32) to sign bit
-- 
2.1.3

[PATCH 2/6] bitops.h: add sign_extend16 function

[Martin Kepplinger]

This adds sign_exten16 to sign extend any signed value shorter than 16 bits
to a s16.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 include/linux/bitops.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 3c2a539..70190f4 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -172,6 +172,17 @@ static inline __s8 sign_extend8(__u8 value, int index)
 }
 
 /**
+ * sign_extend16 - sign extend a 16-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<16) to sign bit
+ */
+static inline __s16 sign_extend16(__u16 value, int index)
+{
+	__u8 shift = 15 - index;
+	return (__s16)(value << shift) >> shift;
+}
+
+/**
  * sign_extend32 - sign extend a 32-bit value using specified bit as sign-bit
  * @value: value to sign extend
  * @index: 0 based bit index (0<=index<32) to sign bit
-- 
2.1.3

[PATCH 3/6] hwmon: jc42: use bitops' sign_extend16

[Martin Kepplinger]

---
 drivers/hwmon/jc42.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 388f8bc..335a2de 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -31,6 +31,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/bitops.h>
 
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = {
@@ -215,9 +216,7 @@ static int jc42_temp_from_reg(s16 reg)
 {
 	reg &= 0x1fff;
 
-	/* sign extend register */
-	if (reg & 0x1000)
-		reg |= 0xf000;
+	reg = sign_extend16(reg, 12);
 
 	/* convert from 0.0625 to 0.001 resolution */
 	return reg * 125 / 2;
-- 
2.1.3

[PATCH 4/6] input: gtco: use bitops' sign_extend8

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/input/tablet/gtco.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c
index 8580456..25b3834 100644
--- a/drivers/input/tablet/gtco.c
+++ b/drivers/input/tablet/gtco.c
@@ -59,7 +59,7 @@ Scott Hill shill@gtcocalcomp.com
 #include <asm/uaccess.h>
 #include <asm/unaligned.h>
 #include <asm/byteorder.h>
-
+#include <linux/bitops.h>
 
 #include <linux/usb/input.h>
 
@@ -666,13 +666,8 @@ static void gtco_urb_callback(struct urb *urbinfo)
 		case 4:
 			/* Tilt */
 
-			/* Sign extend these 7 bit numbers.  */
-			if (device->buffer[6] & 0x40)
-				device->buffer[6] |= 0x80;
-
-			if (device->buffer[7] & 0x40)
-				device->buffer[7] |= 0x80;
-
+			device->buffer[6] = sign_extend8(device->buffer[6], 6);
+			device->buffer[7] = sign_extend8(device->buffer[6], 6);
 
 			valsigned = (device->buffer[6]);
 			input_report_abs(inputdev, ABS_TILT_X, (s32)valsigned);
-- 
2.1.3

[PATCH 5/6] rtc: use sign_extend8 instead of manual conversion

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/rtc/rtc-x1205.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-x1205.c b/drivers/rtc/rtc-x1205.c
index b1de58e..3ec0b95 100644
--- a/drivers/rtc/rtc-x1205.c
+++ b/drivers/rtc/rtc-x1205.c
@@ -22,6 +22,7 @@
 #include <linux/rtc.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/bitops.h>
 
 #define DRV_VERSION "1.0.8"
 
@@ -366,8 +367,7 @@ static int x1205_get_atrim(struct i2c_client *client, int *trim)
 	 * perform sign extension. The formula is
 	 * Catr = (atr * 0.25pF) + 11.00pF.
 	 */
-	if (atr & 0x20)
-		atr |= 0xC0;
+	atr = sign_extend8(atr, 5);
 
 	dev_dbg(&client->dev, "%s: raw atr=%x (%d)\n", __func__, atr, atr);
 
-- 
2.1.3

[PATCH 6/6] media: stb0899: use sign_extend8 instead of manual work

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/media/dvb-frontends/stb0899_algo.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/stb0899_algo.c b/drivers/media/dvb-frontends/stb0899_algo.c
index 93596e0..7bbcfde 100644
--- a/drivers/media/dvb-frontends/stb0899_algo.c
+++ b/drivers/media/dvb-frontends/stb0899_algo.c
@@ -19,6 +19,7 @@
 	Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */
 
+#include <linux/bitops.h>
 #include "stb0899_drv.h"
 #include "stb0899_priv.h"
 #include "stb0899_reg.h"
@@ -1490,9 +1491,7 @@ enum stb0899_status stb0899_dvbs2_algo(struct stb0899_state *state)
 		/* Store signal parameters	*/
 		offsetfreq = STB0899_READ_S2REG(STB0899_S2DEMOD, CRL_FREQ);
 
-		/* sign extend 30 bit value before using it in calculations */
-		if (offsetfreq & (1 << 29))
-			offsetfreq |= -1 << 30;
+		offsetfreq = sign_extend32(offset_freq, 29);
 
 		offsetfreq = offsetfreq / ((1 << 30) / 1000);
 		offsetfreq *= (internal->master_clk / 1000000);
-- 
2.1.3

Re: add sign-extend functions for 8 and 16 bit values

[Peter Zijlstra]

On Mon, Dec 15, 2014 at 05:18:31PM +0100, Martin Kepplinger wrote:
> This adds sign_extend8() and sign_extend16() for the quite many cases
> where this is needed, like sign_extend32().

We're still missing sign_extend64() after this.

[PATCH] bitops.h: add sign_extend64() function

[Martin Kepplinger]

This adds sign_exten64 to sign extend any signed value shorter than 64 bits
to a s64.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
Suggested-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
Without looking for it's users yet: This applies on top of it's 8 and 16 bit
friends.

This reminded me of giving credit for the initial idea. At least in one patch
I'd like to have him mentioned.


 include/linux/bitops.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 70190f4..9c31680 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -193,6 +193,17 @@ static inline __s32 sign_extend32(__u32 value, int index)
 	return (__s32)(value << shift) >> shift;
 }
 
+/**
+ * sign_extend64 - sign extend a 64-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<64) to sign bit
+ */
+static inline __s64 sign_extend64(__u64 value, int index)
+{
+	__u8 shift = 63 - index;
+	return (__s64)(value << shift) >> shift;
+}
+
 static inline unsigned fls_long(unsigned long l)
 {
 	if (sizeof(l) == 4)
-- 
2.1.3

Re: [PATCH 2/6] bitops.h: add sign_extend16 function

[Martin Kepplinger]

Am 2014-12-15 um 17:18 schrieb Martin Kepplinger:
> This adds sign_exten16 to sign extend any signed value shorter than 16 bits
> to a s16.
> 
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
if you feel motivated, you can add this to the commit message:

Suggested-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

Re: [PATCH] bitops.h: add sign_extend64() function

[Martin Kepplinger]

Am 2014-12-15 um 18:54 schrieb Martin Kepplinger:
> This adds sign_exten64 to sign extend any signed value shorter than 64 bits
> to a s64.
I'm sorry for this typo: sign_extend64()

Re: [PATCH 3/6] hwmon: jc42: use bitops' sign_extend16

[Guenter Roeck]

On Mon, Dec 15, 2014 at 05:18:34PM +0100, Martin Kepplinger wrote:
> ---

Some description would be nice. Also, please consider adding
relevant subsystem mailing lists and maintainers to your patches.

>  drivers/hwmon/jc42.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> index 388f8bc..335a2de 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -31,6 +31,7 @@
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/err.h>
>  #include <linux/mutex.h>
> +#include <linux/bitops.h>
>  
>  /* Addresses to scan */
>  static const unsigned short normal_i2c[] = {
> @@ -215,9 +216,7 @@ static int jc42_temp_from_reg(s16 reg)
>  {
>  	reg &= 0x1fff;
>  
If I understand the code in sign_extend16 correctly, the above mask
should no longer be necessary.

Thanks,
Guenter

> -	/* sign extend register */
> -	if (reg & 0x1000)
> -		reg |= 0xf000;
> +	reg = sign_extend16(reg, 12);
>  
>  	/* convert from 0.0625 to 0.001 resolution */
>  	return reg * 125 / 2;
> -- 
> 2.1.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH 3/6] hwmon: jc42: use bitops' sign_extend16

[Martin Kepplinger]

Am 2014-12-15 um 22:29 schrieb Guenter Roeck:
> On Mon, Dec 15, 2014 at 05:18:34PM +0100, Martin Kepplinger wrote:
>> ---
> 
> Some description would be nice. Also, please consider adding
> relevant subsystem mailing lists and maintainers to your patches.
> 

I shouldn't have added the Signed-off-by line to some of them. Sorry.

The driver-patches are meant to be examples of what can be changed if
the sign_extend functions are added. I don't know if they are taken and
planned to post the driver patches (probably more) thereafter, and of
course to the relevant people.

>>  drivers/hwmon/jc42.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
>> index 388f8bc..335a2de 100644
>> --- a/drivers/hwmon/jc42.c
>> +++ b/drivers/hwmon/jc42.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/hwmon-sysfs.h>
>>  #include <linux/err.h>
>>  #include <linux/mutex.h>
>> +#include <linux/bitops.h>
>>  
>>  /* Addresses to scan */
>>  static const unsigned short normal_i2c[] = {
>> @@ -215,9 +216,7 @@ static int jc42_temp_from_reg(s16 reg)
>>  {
>>  	reg &= 0x1fff;
>>  
> If I understand the code in sign_extend16 correctly, the above mask
> should no longer be necessary.

exactly. The mask would then be shifting. Thanks!

> 
> Thanks,
> Guenter
> 
>> -	/* sign extend register */
>> -	if (reg & 0x1000)
>> -		reg |= 0xf000;
>> +	reg = sign_extend16(reg, 12);
>>  
>>  	/* convert from 0.0625 to 0.001 resolution */
>>  	return reg * 125 / 2;
>> -- 
>> 2.1.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>

Re: [PATCH 3/6] hwmon: jc42: use bitops' sign_extend16

[Martin Kepplinger]

Am 2014-12-16 um 08:17 schrieb Martin Kepplinger:
> Am 2014-12-15 um 22:29 schrieb Guenter Roeck:
>> On Mon, Dec 15, 2014 at 05:18:34PM +0100, Martin Kepplinger wrote:
>>> ---
>>
>> Some description would be nice. Also, please consider adding
>> relevant subsystem mailing lists and maintainers to your patches.
>>
> 
> I shouldn't have added the Signed-off-by line to some of them. Sorry.
> 
> The driver-patches are meant to be examples of what can be changed if
> the sign_extend functions are added. I don't know if they are taken and
> planned to post the driver patches (probably more) thereafter, and of
> course to the relevant people.

Is this sign_extendXX() set of functions considered to be added to
bitops.h ?

Just checking if I can prepare some driver-patches (the ones I posted
are just examples, meant to be re-sent to relevant maintainers when it's
time), or not.

thanks

> 
>>>  drivers/hwmon/jc42.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
>>> index 388f8bc..335a2de 100644
>>> --- a/drivers/hwmon/jc42.c
>>> +++ b/drivers/hwmon/jc42.c
>>> @@ -31,6 +31,7 @@
>>>  #include <linux/hwmon-sysfs.h>
>>>  #include <linux/err.h>
>>>  #include <linux/mutex.h>
>>> +#include <linux/bitops.h>
>>>  
>>>  /* Addresses to scan */
>>>  static const unsigned short normal_i2c[] = {
>>> @@ -215,9 +216,7 @@ static int jc42_temp_from_reg(s16 reg)
>>>  {
>>>  	reg &= 0x1fff;
>>>  
>> If I understand the code in sign_extend16 correctly, the above mask
>> should no longer be necessary.
> 
> exactly. The mask would then be shifting. Thanks!
> 
>>
>> Thanks,
>> Guenter
>>
>>> -	/* sign extend register */
>>> -	if (reg & 0x1000)
>>> -		reg |= 0xf000;
>>> +	reg = sign_extend16(reg, 12);
>>>  
>>>  	/* convert from 0.0625 to 0.001 resolution */
>>>  	return reg * 125 / 2;
>>> -- 
>>> 2.1.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
>>>
> 

Re: [PATCH 3/6] hwmon: jc42: use bitops' sign_extend16

[Guenter Roeck]

On 01/11/2015 02:34 AM, Martin Kepplinger wrote:
> Am 2014-12-16 um 08:17 schrieb Martin Kepplinger:
>> Am 2014-12-15 um 22:29 schrieb Guenter Roeck:
>>> On Mon, Dec 15, 2014 at 05:18:34PM +0100, Martin Kepplinger wrote:
>>>> ---
>>>
>>> Some description would be nice. Also, please consider adding
>>> relevant subsystem mailing lists and maintainers to your patches.
>>>
>>
>> I shouldn't have added the Signed-off-by line to some of them. Sorry.
>>
>> The driver-patches are meant to be examples of what can be changed if
>> the sign_extend functions are added. I don't know if they are taken and
>> planned to post the driver patches (probably more) thereafter, and of
>> course to the relevant people.
>
> Is this sign_extendXX() set of functions considered to be added to
> bitops.h ?
>
> Just checking if I can prepare some driver-patches (the ones I posted
> are just examples, meant to be re-sent to relevant maintainers when it's
> time), or not.
>

You should probably ask the question as response to patch 1/6.

In general, it might be useful to send example patches like this one as RFC.
Sending it as real patch and then saying "it is just an example" may result
in the entire series being ignored. This is not a matter of Signed-off or not
(originally I didn't even understand what you wanted to say with your reply),
but a matter of a non-misleading headline.

Thanks,
Guenter

Re: [PATCH 3/6] hwmon: jc42: use bitops' sign_extend16

[Martin Kepplinger]

Am 2015-01-11 um 17:52 schrieb Guenter Roeck:
> On 01/11/2015 02:34 AM, Martin Kepplinger wrote:
>> Am 2014-12-16 um 08:17 schrieb Martin Kepplinger:
>>> Am 2014-12-15 um 22:29 schrieb Guenter Roeck:
>>>> On Mon, Dec 15, 2014 at 05:18:34PM +0100, Martin Kepplinger wrote:
>>>>> ---
>>>>
>>>> Some description would be nice. Also, please consider adding
>>>> relevant subsystem mailing lists and maintainers to your patches.
>>>>
>>>
>>> I shouldn't have added the Signed-off-by line to some of them. Sorry.
>>>
>>> The driver-patches are meant to be examples of what can be changed if
>>> the sign_extend functions are added. I don't know if they are taken and
>>> planned to post the driver patches (probably more) thereafter, and of
>>> course to the relevant people.
>>
>> Is this sign_extendXX() set of functions considered to be added to
>> bitops.h ?
>>
>> Just checking if I can prepare some driver-patches (the ones I posted
>> are just examples, meant to be re-sent to relevant maintainers when it's
>> time), or not.
>>
> 
> You should probably ask the question as response to patch 1/6.
> 
> In general, it might be useful to send example patches like this one as
> RFC.
> Sending it as real patch and then saying "it is just an example" may result
> in the entire series being ignored. This is not a matter of Signed-off
> or not
> (originally I didn't even understand what you wanted to say with your
> reply),
> but a matter of a non-misleading headline.
> 
> Thanks,
> Guenter
> 

Thanks for the advice. I resent, https://lkml.org/lkml/2015/1/12/590
because the thing is easier than it might now seem, and hope that
doesn't add confusion.

thanks again,
                         martin