[PATCH 0/2] Input: drv266x: Fix misuse of regmap_update_bits

[PATCH 0/2] Input: drv266x: Fix misuse of regmap_update_bits

[Florian Vaussard]

Hello,

This series fixes similar misues of the regmap_update_bits() API found
inside the drv2665.c and drv2667.c drivers. More details can be found
in the message of each patch.

Theses changes are not tested as I do not have the required hardware,
but the calls to regmap_update_bits() are clearly wrong in the current
code and the fix seems obvious. Any tests are warmly welcome.

Best regards,
Florian

Florian Vaussard (2):
  Input: drv2665: Fix misuse of regmap_update_bits
  Input: drv2667: Fix misuse of regmap_update_bits

 drivers/input/misc/drv2665.c | 5 +++--
 drivers/input/misc/drv2667.c | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.5.5

[PATCH 1/2] Input: drv2665: Fix misuse of regmap_update_bits

[Florian Vaussard]

Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
(mask & 1), which results in 0 if LSB of mask is 0. Thus the call
regmap_update_bits(..., mask, 1) is in reality equivalent to
regmap_update_bits(..., mask, 0).

In such a case, the correct use is regmap_update_bits(..., mask, mask).

This driver is performing such a mistake with the DRV2665_STANDBY mask,
which equals BIT(6). Fix the driver to make it consistent with the API,
and fix the alignment problem at the same time. Please note that this
change is untested, as I do not have this piece of hardware. Testers
are welcome!

Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
---
 drivers/input/misc/drv2665.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/drv2665.c b/drivers/input/misc/drv2665.c
index ef9bc12..b0f46e7 100644
--- a/drivers/input/misc/drv2665.c
+++ b/drivers/input/misc/drv2665.c
@@ -126,7 +126,8 @@ static void drv2665_close(struct input_dev *input)
 	cancel_work_sync(&haptics->work);
 
 	error = regmap_update_bits(haptics->regmap,
-				   DRV2665_CTRL_2, DRV2665_STANDBY, 1);
+				   DRV2665_CTRL_2, DRV2665_STANDBY,
+				   DRV2665_STANDBY);
 	if (error)
 		dev_err(&haptics->client->dev,
 			"Failed to enter standby mode: %d\n", error);
@@ -240,7 +241,7 @@ static int __maybe_unused drv2665_suspend(struct device *dev)
 
 	if (haptics->input_dev->users) {
 		ret = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
-				DRV2665_STANDBY, 1);
+					 DRV2665_STANDBY, DRV2665_STANDBY);
 		if (ret) {
 			dev_err(dev, "Failed to set standby mode\n");
 			regulator_disable(haptics->regulator);
-- 
2.5.5

[PATCH 2/2] Input: drv2667: Fix misuse of regmap_update_bits

[Florian Vaussard]

Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
(mask & 1), which results in 0 if LSB of mask is 0. Thus the call
regmap_update_bits(..., mask, 1) is in reality equivalent to
regmap_update_bits(..., mask, 0).

In such a case, the correct use is regmap_update_bits(..., mask, mask).

This driver is performing such a mistake with the DRV2667_STANDBY mask,
which equals (1 << 6). Fix the driver to make it consistent with the
API, and fix the alignment problem at the same time. Please note that
this change is untested, as I do not have this piece of hardware.
Testers are welcome!

Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
---
 drivers/input/misc/drv2667.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/drv2667.c b/drivers/input/misc/drv2667.c
index d5ba748..2849bb6 100644
--- a/drivers/input/misc/drv2667.c
+++ b/drivers/input/misc/drv2667.c
@@ -256,7 +256,7 @@ static void drv2667_close(struct input_dev *input)
 	cancel_work_sync(&haptics->work);
 
 	error = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
-				DRV2667_STANDBY, 1);
+				   DRV2667_STANDBY, DRV2667_STANDBY);
 	if (error)
 		dev_err(&haptics->client->dev,
 			"Failed to enter standby mode: %d\n", error);
@@ -415,7 +415,7 @@ static int __maybe_unused drv2667_suspend(struct device *dev)
 
 	if (haptics->input_dev->users) {
 		ret = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
-				DRV2667_STANDBY, 1);
+					 DRV2667_STANDBY, DRV2667_STANDBY);
 		if (ret) {
 			dev_err(dev, "Failed to set standby mode\n");
 			regulator_disable(haptics->regulator);
-- 
2.5.5

Re: [PATCH 0/2] Input: drv266x: Fix misuse of regmap_update_bits

[Dan Murphy]

Florian

On 11/29/2016 10:59 AM, Florian Vaussard wrote:
> Hello,
>
> This series fixes similar misues of the regmap_update_bits() API found
> inside the drv2665.c and drv2667.c drivers. More details can be found
> in the message of each patch.
>
> Theses changes are not tested as I do not have the required hardware,
> but the calls to regmap_update_bits() are clearly wrong in the current
> code and the fix seems obvious. Any tests are warmly welcome.
>
> Best regards,
> Florian
>
> Florian Vaussard (2):
>   Input: drv2665: Fix misuse of regmap_update_bits
>   Input: drv2667: Fix misuse of regmap_update_bits

Thanks for the patches what about the drv260x.c?

Dan

>  drivers/input/misc/drv2665.c | 5 +++--
>  drivers/input/misc/drv2667.c | 4 ++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
>


-- 
------------------
Dan Murphy

Re: [PATCH 0/2] Input: drv266x: Fix misuse of regmap_update_bits

[Florian Vaussard]

Hello Dan,

Le 29. 11. 16 à 18:10, Dan Murphy a écrit :
> Florian
> 
> On 11/29/2016 10:59 AM, Florian Vaussard wrote:
>> Hello,
>>
>> This series fixes similar misues of the regmap_update_bits() API found
>> inside the drv2665.c and drv2667.c drivers. More details can be found
>> in the message of each patch.
>>
>> Theses changes are not tested as I do not have the required hardware,
>> but the calls to regmap_update_bits() are clearly wrong in the current
>> code and the fix seems obvious. Any tests are warmly welcome.
>>
>> Best regards,
>> Florian
>>
>> Florian Vaussard (2):
>>   Input: drv2665: Fix misuse of regmap_update_bits
>>   Input: drv2667: Fix misuse of regmap_update_bits
> 
> Thanks for the patches what about the drv260x.c?
> 

>From what I can see, drv260x.c does not have such a bug. The calls to
regmap_update_bits(.., mask, value) in drv260x.c where value > 0 are using
DRV260X_LIB_SEL_MASK and DRV260X_STANDBY_MASK as masks. In both cases, the range
of 'value' is such that (mask & value) is not null (if 'value' is not null of
course). Thus no obvious problems here.

Best regards,
Florian

Re: [PATCH 1/2] Input: drv2665: Fix misuse of regmap_update_bits

[Dmitry Torokhov]

On Tue, Nov 29, 2016 at 05:59:13PM +0100, Florian Vaussard wrote:
> Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
> and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
> (mask & 1), which results in 0 if LSB of mask is 0. Thus the call
> regmap_update_bits(..., mask, 1) is in reality equivalent to
> regmap_update_bits(..., mask, 0).
> 
> In such a case, the correct use is regmap_update_bits(..., mask, mask).
> 
> This driver is performing such a mistake with the DRV2665_STANDBY mask,
> which equals BIT(6). Fix the driver to make it consistent with the API,
> and fix the alignment problem at the same time. Please note that this
> change is untested, as I do not have this piece of hardware. Testers
> are welcome!
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>

Applied, thank you.

> ---
>  drivers/input/misc/drv2665.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/drv2665.c b/drivers/input/misc/drv2665.c
> index ef9bc12..b0f46e7 100644
> --- a/drivers/input/misc/drv2665.c
> +++ b/drivers/input/misc/drv2665.c
> @@ -126,7 +126,8 @@ static void drv2665_close(struct input_dev *input)
>  	cancel_work_sync(&haptics->work);
>  
>  	error = regmap_update_bits(haptics->regmap,
> -				   DRV2665_CTRL_2, DRV2665_STANDBY, 1);
> +				   DRV2665_CTRL_2, DRV2665_STANDBY,
> +				   DRV2665_STANDBY);
>  	if (error)
>  		dev_err(&haptics->client->dev,
>  			"Failed to enter standby mode: %d\n", error);
> @@ -240,7 +241,7 @@ static int __maybe_unused drv2665_suspend(struct device *dev)
>  
>  	if (haptics->input_dev->users) {
>  		ret = regmap_update_bits(haptics->regmap, DRV2665_CTRL_2,
> -				DRV2665_STANDBY, 1);
> +					 DRV2665_STANDBY, DRV2665_STANDBY);
>  		if (ret) {
>  			dev_err(dev, "Failed to set standby mode\n");
>  			regulator_disable(haptics->regulator);
> -- 
> 2.5.5
> 

-- 
Dmitry

Re: [PATCH 2/2] Input: drv2667: Fix misuse of regmap_update_bits

[Dmitry Torokhov]

On Tue, Nov 29, 2016 at 05:59:14PM +0100, Florian Vaussard wrote:
> Using regmap_update_bits(..., mask, 1) with 'mask' following (1 << k)
> and k greater than 0 is wrong. Indeed, _regmap_update_bits will perform
> (mask & 1), which results in 0 if LSB of mask is 0. Thus the call
> regmap_update_bits(..., mask, 1) is in reality equivalent to
> regmap_update_bits(..., mask, 0).
> 
> In such a case, the correct use is regmap_update_bits(..., mask, mask).
> 
> This driver is performing such a mistake with the DRV2667_STANDBY mask,
> which equals (1 << 6). Fix the driver to make it consistent with the
> API, and fix the alignment problem at the same time. Please note that
> this change is untested, as I do not have this piece of hardware.
> Testers are welcome!
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>

Applied, thank you.

> ---
>  drivers/input/misc/drv2667.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/drv2667.c b/drivers/input/misc/drv2667.c
> index d5ba748..2849bb6 100644
> --- a/drivers/input/misc/drv2667.c
> +++ b/drivers/input/misc/drv2667.c
> @@ -256,7 +256,7 @@ static void drv2667_close(struct input_dev *input)
>  	cancel_work_sync(&haptics->work);
>  
>  	error = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
> -				DRV2667_STANDBY, 1);
> +				   DRV2667_STANDBY, DRV2667_STANDBY);
>  	if (error)
>  		dev_err(&haptics->client->dev,
>  			"Failed to enter standby mode: %d\n", error);
> @@ -415,7 +415,7 @@ static int __maybe_unused drv2667_suspend(struct device *dev)
>  
>  	if (haptics->input_dev->users) {
>  		ret = regmap_update_bits(haptics->regmap, DRV2667_CTRL_2,
> -				DRV2667_STANDBY, 1);
> +					 DRV2667_STANDBY, DRV2667_STANDBY);
>  		if (ret) {
>  			dev_err(dev, "Failed to set standby mode\n");
>  			regulator_disable(haptics->regulator);
> -- 
> 2.5.5
> 

-- 
Dmitry