[PATCH] staging/adt7316 Fix some 'interesting' string operations

[PATCH] staging/adt7316 Fix some 'interesting' string operations

[Luck, Tony]

Calling memcmp() to check the value of the first byte in a string is overkill.
Just use buf[0] == '1' or buf[0] != '1' as appropriate.

Signed-off-by: Tony Luck <tony.luck@intel.com>

---

[Inspired by a rant on IRC about a different driver doing something similar]

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 0b431bc..506b5a7 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -256,7 +256,7 @@ static ssize_t adt7316_store_enabled(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 	int enable;
 
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		enable = 1;
 	else
 		enable = 0;
@@ -299,7 +299,7 @@ static ssize_t adt7316_store_select_ex_temp(struct device *dev,
 		return -EPERM;
 
 	config1 = chip->config1 & (~ADT7516_SEL_EX_TEMP);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		config1 |= ADT7516_SEL_EX_TEMP;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
@@ -495,7 +495,7 @@ static ssize_t adt7316_store_disable_averaging(struct device *dev,
 	int ret;
 
 	config2 = chip->config2 & (~ADT7316_DISABLE_AVERAGING);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		config2 |= ADT7316_DISABLE_AVERAGING;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG2, config2);
@@ -534,7 +534,7 @@ static ssize_t adt7316_store_enable_smbus_timeout(struct device *dev,
 	int ret;
 
 	config2 = chip->config2 & (~ADT7316_EN_SMBUS_TIMEOUT);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		config2 |= ADT7316_EN_SMBUS_TIMEOUT;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG2, config2);
@@ -597,7 +597,7 @@ static ssize_t adt7316_store_powerdown(struct device *dev,
 	int ret;
 
 	config1 = chip->config1 & (~ADT7316_PD);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		config1 |= ADT7316_PD;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
@@ -635,7 +635,7 @@ static ssize_t adt7316_store_fast_ad_clock(struct device *dev,
 	int ret;
 
 	config3 = chip->config3 & (~ADT7316_ADCLK_22_5);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		config3 |= ADT7316_ADCLK_22_5;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
@@ -681,7 +681,7 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev,
 
 	chip->dac_bits = 8;
 
-	if (!memcmp(buf, "1", 1)) {
+	if (buf[0] == '1') {
 		config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION;
 		if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516)
 			chip->dac_bits = 12;
@@ -731,7 +731,7 @@ static ssize_t adt7316_store_AIN_internal_Vref(struct device *dev,
 	if ((chip->id & ID_FAMILY_MASK) != ID_ADT75XX)
 		return -EPERM;
 
-	if (memcmp(buf, "1", 1))
+	if (buf[0] != '1')
 		config3 = chip->config3 & (~ADT7516_AIN_IN_VREF);
 	else
 		config3 = chip->config3 | ADT7516_AIN_IN_VREF;
@@ -773,7 +773,7 @@ static ssize_t adt7316_store_enable_prop_DACA(struct device *dev,
 	int ret;
 
 	config3 = chip->config3 & (~ADT7316_EN_IN_TEMP_PROP_DACA);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		config3 |= ADT7316_EN_IN_TEMP_PROP_DACA;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
@@ -812,7 +812,7 @@ static ssize_t adt7316_store_enable_prop_DACB(struct device *dev,
 	int ret;
 
 	config3 = chip->config3 & (~ADT7316_EN_EX_TEMP_PROP_DACB);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		config3 |= ADT7316_EN_EX_TEMP_PROP_DACB;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
@@ -1018,7 +1018,7 @@ static ssize_t adt7316_store_DA_AB_Vref_bypass(struct device *dev,
 		return -EPERM;
 
 	dac_config = chip->dac_config & (~ADT7316_VREF_BYPASS_DAC_AB);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		dac_config |= ADT7316_VREF_BYPASS_DAC_AB;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
@@ -1063,7 +1063,7 @@ static ssize_t adt7316_store_DA_CD_Vref_bypass(struct device *dev,
 		return -EPERM;
 
 	dac_config = chip->dac_config & (~ADT7316_VREF_BYPASS_DAC_CD);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		dac_config |= ADT7316_VREF_BYPASS_DAC_CD;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
@@ -1982,7 +1982,7 @@ static ssize_t adt7316_set_int_enabled(struct device *dev,
 	int ret;
 
 	config1 = chip->config1 & (~ADT7316_INT_EN);
-	if (!memcmp(buf, "1", 1))
+	if (buf[0] == '1')
 		config1 |= ADT7316_INT_EN;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);

Re: [PATCH] staging/adt7316 Fix some 'interesting' string operations

[Greg Kroah-Hartman]

On Thu, Apr 04, 2013 at 02:37:24PM -0700, Luck, Tony wrote:
> Calling memcmp() to check the value of the first byte in a string is overkill.
> Just use buf[0] == '1' or buf[0] != '1' as appropriate.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>

I'll let Jonathan take this through his tree which eventually makes it
to mine.

thanks,

greg k-h

Re: [PATCH] staging/adt7316 Fix some 'interesting' string operations

[Lars-Peter Clausen]

On 04/04/2013 11:37 PM, Luck, Tony wrote:
> Calling memcmp() to check the value of the first byte in a string is overkill.
> Just use buf[0] == '1' or buf[0] != '1' as appropriate.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> 

I think it is a good idea to switch directly to strtobool. But anyway, if you
don't want to respin the patch it is fine as it is.

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
> 
> [Inspired by a rant on IRC about a different driver doing something similar]
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 0b431bc..506b5a7 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -256,7 +256,7 @@ static ssize_t adt7316_store_enabled(struct device *dev,
>  	struct adt7316_chip_info *chip = iio_priv(dev_info);
>  	int enable;
>  
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		enable = 1;
>  	else
>  		enable = 0;
> @@ -299,7 +299,7 @@ static ssize_t adt7316_store_select_ex_temp(struct device *dev,
>  		return -EPERM;
>  
>  	config1 = chip->config1 & (~ADT7516_SEL_EX_TEMP);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		config1 |= ADT7516_SEL_EX_TEMP;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
> @@ -495,7 +495,7 @@ static ssize_t adt7316_store_disable_averaging(struct device *dev,
>  	int ret;
>  
>  	config2 = chip->config2 & (~ADT7316_DISABLE_AVERAGING);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		config2 |= ADT7316_DISABLE_AVERAGING;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG2, config2);
> @@ -534,7 +534,7 @@ static ssize_t adt7316_store_enable_smbus_timeout(struct device *dev,
>  	int ret;
>  
>  	config2 = chip->config2 & (~ADT7316_EN_SMBUS_TIMEOUT);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		config2 |= ADT7316_EN_SMBUS_TIMEOUT;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG2, config2);
> @@ -597,7 +597,7 @@ static ssize_t adt7316_store_powerdown(struct device *dev,
>  	int ret;
>  
>  	config1 = chip->config1 & (~ADT7316_PD);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		config1 |= ADT7316_PD;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
> @@ -635,7 +635,7 @@ static ssize_t adt7316_store_fast_ad_clock(struct device *dev,
>  	int ret;
>  
>  	config3 = chip->config3 & (~ADT7316_ADCLK_22_5);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		config3 |= ADT7316_ADCLK_22_5;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
> @@ -681,7 +681,7 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev,
>  
>  	chip->dac_bits = 8;
>  
> -	if (!memcmp(buf, "1", 1)) {
> +	if (buf[0] == '1') {
>  		config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION;
>  		if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516)
>  			chip->dac_bits = 12;
> @@ -731,7 +731,7 @@ static ssize_t adt7316_store_AIN_internal_Vref(struct device *dev,
>  	if ((chip->id & ID_FAMILY_MASK) != ID_ADT75XX)
>  		return -EPERM;
>  
> -	if (memcmp(buf, "1", 1))
> +	if (buf[0] != '1')
>  		config3 = chip->config3 & (~ADT7516_AIN_IN_VREF);
>  	else
>  		config3 = chip->config3 | ADT7516_AIN_IN_VREF;
> @@ -773,7 +773,7 @@ static ssize_t adt7316_store_enable_prop_DACA(struct device *dev,
>  	int ret;
>  
>  	config3 = chip->config3 & (~ADT7316_EN_IN_TEMP_PROP_DACA);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		config3 |= ADT7316_EN_IN_TEMP_PROP_DACA;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
> @@ -812,7 +812,7 @@ static ssize_t adt7316_store_enable_prop_DACB(struct device *dev,
>  	int ret;
>  
>  	config3 = chip->config3 & (~ADT7316_EN_EX_TEMP_PROP_DACB);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		config3 |= ADT7316_EN_EX_TEMP_PROP_DACB;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
> @@ -1018,7 +1018,7 @@ static ssize_t adt7316_store_DA_AB_Vref_bypass(struct device *dev,
>  		return -EPERM;
>  
>  	dac_config = chip->dac_config & (~ADT7316_VREF_BYPASS_DAC_AB);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		dac_config |= ADT7316_VREF_BYPASS_DAC_AB;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
> @@ -1063,7 +1063,7 @@ static ssize_t adt7316_store_DA_CD_Vref_bypass(struct device *dev,
>  		return -EPERM;
>  
>  	dac_config = chip->dac_config & (~ADT7316_VREF_BYPASS_DAC_CD);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		dac_config |= ADT7316_VREF_BYPASS_DAC_CD;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
> @@ -1982,7 +1982,7 @@ static ssize_t adt7316_set_int_enabled(struct device *dev,
>  	int ret;
>  
>  	config1 = chip->config1 & (~ADT7316_INT_EN);
> -	if (!memcmp(buf, "1", 1))
> +	if (buf[0] == '1')
>  		config1 |= ADT7316_INT_EN;
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
> --
> 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] staging/adt7316 Fix some 'interesting' string operations

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 719 bytes --]

> I think it is a good idea to switch directly to strtobool. But anyway, if you
> don't want to respin the patch it is fine as it is.

I didn't know that strtobool() existed ... but now that I do I agree that it would
be better to use it here.  But ... I'm less comfortable updating the patch to use
it. User visible behavior would change (currently just "1" is considered "true",
but strtobool would also accept "y" and "Y").  I'd also have to think hard about
what to do if strtobool() said the input was not a valid boolean.

Thanks for the review.

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] staging/adt7316 Fix some 'interesting' string operations

[Jonathan Cameron]

On 04/08/2013 06:54 PM, Luck, Tony wrote:
>> I think it is a good idea to switch directly to strtobool. But anyway, if you
>> don't want to respin the patch it is fine as it is.
> I didn't know that strtobool() existed ... but now that I do I agree that it would
> be better to use it here.  But ... I'm less comfortable updating the patch to use
> it. User visible behavior would change (currently just "1" is considered "true",
> but strtobool would also accept "y" and "Y").  I'd also have to think hard about
> what to do if strtobool() said the input was not a valid boolean.
>
> Thanks for the review.
>
> -Tony
I'll take the patch as is.  If anyone wants to do the strtobool conversion
as a follow up that would be great as well.  As a staging driver which doesn't
terribly closely correspond to any standard abi's I'd be inclined to not worry
to much about breaking userspace code that relies on the exact comparison
currently used as long as the obvious case still works.

Patch applied to togreg branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git

Thanks,

Jonathan