diff mbox

[4/6] rtc-rv3029: Add i2c register update-bits helper

Message ID 20160304195611.48d5177b@wiggum
State Superseded
Headers show

Commit Message

Michael Büsch March 4, 2016, 6:56 p.m. UTC 
This simplifies mask/set operations on device I2C registers.

Signed-off-by: Michael Buesch <m@bues.ch>
---
 drivers/rtc/rtc-rv3029c2.c | 54 ++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

Comments

Alexandre Belloni March 4, 2016, 7:42 p.m. UTC | #1 
On 04/03/2016 at 19:56:11 +0100, Michael Büsch wrote :
> This simplifies mask/set operations on device I2C registers.
> 
> Signed-off-by: Michael Buesch <m@bues.ch>
> ---
>  drivers/rtc/rtc-rv3029c2.c | 54 ++++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rv3029c2.c b/drivers/rtc/rtc-rv3029c2.c
> index 29cc871..769f73a 100644
> --- a/drivers/rtc/rtc-rv3029c2.c
> +++ b/drivers/rtc/rtc-rv3029c2.c
> @@ -2,6 +2,7 @@
>   * Micro Crystal RV-3029 rtc class driver
>   *
>   * Author: Gregory Hermant <gregory.hermant@calao-systems.com>
> + *         Michael Buesch <m@bues.ch>
>   *
>   * based on previously existing rtc class drivers
>   *
> @@ -143,6 +144,24 @@ rv3029_i2c_write_regs(struct i2c_client *client, u8 reg, u8 const buf[],
>  }
>  
>  static int
> +rv3029_i2c_update_bits(struct i2c_client *client, u8 reg, u8 mask, u8 set)
> +{
> +	u8 buf;
> +	int ret;
> +
> +	ret = rv3029_i2c_read_regs(client, reg, &buf, 1);
> +	if (ret < 0)
> +		return ret;
> +	buf &= mask;
> +	buf |= set;

Well, this is not exactly what is expected from an update_bits function,
it should be:

	buf &= ~mask;
	buf |= set & mask;

What you pass to the function is the mask of bits you want to set.

> +	ret = rv3029_i2c_write_regs(client, reg, &buf, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int
>  rv3029_i2c_get_sr(struct i2c_client *client, u8 *buf)
>  {
>  	int ret = rv3029_i2c_read_regs(client, RV3029_STATUS, buf, 1);
> @@ -260,22 +279,13 @@ static int rv3029_rtc_i2c_alarm_set_irq(struct i2c_client *client,
>  					int enable)
>  {
>  	int ret;
> -	u8 buf[1];
> -
> -	/* enable AIE irq */
> -	ret = rv3029_i2c_read_regs(client, RV3029_IRQ_CTRL, buf, 1);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "can't read INT reg\n");
> -		return ret;
> -	}
> -	if (enable)
> -		buf[0] |= RV3029_IRQ_CTRL_AIE;
> -	else
> -		buf[0] &= ~RV3029_IRQ_CTRL_AIE;
>  
> -	ret = rv3029_i2c_write_regs(client, RV3029_IRQ_CTRL, buf, 1);
> +	/* enable/disable AIE irq */
> +	ret = rv3029_i2c_update_bits(client, RV3029_IRQ_CTRL,
> +				     (u8)~RV3029_IRQ_CTRL_AIE,

This allows to remove the bitwise not here

> +				     (enable ? RV3029_IRQ_CTRL_AIE : 0));
>  	if (ret < 0) {
> -		dev_err(&client->dev, "can't set INT reg\n");
> +		dev_err(&client->dev, "can't update INT reg\n");
>  		return ret;
>  	}
>  
> @@ -316,20 +326,11 @@ static int rv3029_rtc_i2c_set_alarm(struct i2c_client *client,
>  		return ret;
>  
>  	if (alarm->enabled) {
> -		u8 buf[1];
> -
>  		/* clear AF flag */
> -		ret = rv3029_i2c_read_regs(client, RV3029_IRQ_FLAGS,
> -					   buf, 1);
> -		if (ret < 0) {
> -			dev_err(&client->dev, "can't read alarm flag\n");
> -			return ret;
> -		}
> -		buf[0] &= ~RV3029_IRQ_FLAGS_AF;
> -		ret = rv3029_i2c_write_regs(client, RV3029_IRQ_FLAGS,
> -					    buf, 1);
> +		ret = rv3029_i2c_update_bits(client, RV3029_IRQ_FLAGS,
> +					     (u8)~RV3029_IRQ_FLAGS_AF, 0);

and here.

>  		if (ret < 0) {
> -			dev_err(&client->dev, "can't set alarm flag\n");
> +			dev_err(&client->dev, "can't clear alarm flag\n");
>  			return ret;
>  		}
>  		/* enable AIE irq */
> @@ -454,5 +455,6 @@ static struct i2c_driver rv3029_driver = {
>  module_i2c_driver(rv3029_driver);
>  
>  MODULE_AUTHOR("Gregory Hermant <gregory.hermant@calao-systems.com>");
> +MODULE_AUTHOR("Michael Buesch <m@bues.ch>");
>  MODULE_DESCRIPTION("Micro Crystal RV3029 RTC driver");
>  MODULE_LICENSE("GPL");
> -- 
> 2.7.0
>
Michael Büsch March 4, 2016, 7:46 p.m. UTC | #2 
On Fri, 4 Mar 2016 20:42:23 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> > +	buf &= mask;
> > +	buf |= set;  
> 
> Well, this is not exactly what is expected from an update_bits function,
> it should be:
> 
> 	buf &= ~mask;
> 	buf |= set & mask;
> 
> What you pass to the function is the mask of bits you want to set.

Meh, ok. Everybody does seem to feel differently on that issue.
I did it exactly like this with ~mask in the b43 driver and received a
lot of bashing for it. :)

But well, to me either way is fine. So I'll change this to do it with
~mask too.
diff mbox

Patch

diff --git a/drivers/rtc/rtc-rv3029c2.c b/drivers/rtc/rtc-rv3029c2.c
index 29cc871..769f73a 100644
--- a/drivers/rtc/rtc-rv3029c2.c
+++ b/drivers/rtc/rtc-rv3029c2.c
@@ -2,6 +2,7 @@ 
  * Micro Crystal RV-3029 rtc class driver
  *
  * Author: Gregory Hermant <gregory.hermant@calao-systems.com>
+ *         Michael Buesch <m@bues.ch>
  *
  * based on previously existing rtc class drivers
  *
@@ -143,6 +144,24 @@  rv3029_i2c_write_regs(struct i2c_client *client, u8 reg, u8 const buf[],
 }
 
 static int
+rv3029_i2c_update_bits(struct i2c_client *client, u8 reg, u8 mask, u8 set)
+{
+	u8 buf;
+	int ret;
+
+	ret = rv3029_i2c_read_regs(client, reg, &buf, 1);
+	if (ret < 0)
+		return ret;
+	buf &= mask;
+	buf |= set;
+	ret = rv3029_i2c_write_regs(client, reg, &buf, 1);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int
 rv3029_i2c_get_sr(struct i2c_client *client, u8 *buf)
 {
 	int ret = rv3029_i2c_read_regs(client, RV3029_STATUS, buf, 1);
@@ -260,22 +279,13 @@  static int rv3029_rtc_i2c_alarm_set_irq(struct i2c_client *client,
 					int enable)
 {
 	int ret;
-	u8 buf[1];
-
-	/* enable AIE irq */
-	ret = rv3029_i2c_read_regs(client, RV3029_IRQ_CTRL, buf, 1);
-	if (ret < 0) {
-		dev_err(&client->dev, "can't read INT reg\n");
-		return ret;
-	}
-	if (enable)
-		buf[0] |= RV3029_IRQ_CTRL_AIE;
-	else
-		buf[0] &= ~RV3029_IRQ_CTRL_AIE;
 
-	ret = rv3029_i2c_write_regs(client, RV3029_IRQ_CTRL, buf, 1);
+	/* enable/disable AIE irq */
+	ret = rv3029_i2c_update_bits(client, RV3029_IRQ_CTRL,
+				     (u8)~RV3029_IRQ_CTRL_AIE,
+				     (enable ? RV3029_IRQ_CTRL_AIE : 0));
 	if (ret < 0) {
-		dev_err(&client->dev, "can't set INT reg\n");
+		dev_err(&client->dev, "can't update INT reg\n");
 		return ret;
 	}
 
@@ -316,20 +326,11 @@  static int rv3029_rtc_i2c_set_alarm(struct i2c_client *client,
 		return ret;
 
 	if (alarm->enabled) {
-		u8 buf[1];
-
 		/* clear AF flag */
-		ret = rv3029_i2c_read_regs(client, RV3029_IRQ_FLAGS,
-					   buf, 1);
-		if (ret < 0) {
-			dev_err(&client->dev, "can't read alarm flag\n");
-			return ret;
-		}
-		buf[0] &= ~RV3029_IRQ_FLAGS_AF;
-		ret = rv3029_i2c_write_regs(client, RV3029_IRQ_FLAGS,
-					    buf, 1);
+		ret = rv3029_i2c_update_bits(client, RV3029_IRQ_FLAGS,
+					     (u8)~RV3029_IRQ_FLAGS_AF, 0);
 		if (ret < 0) {
-			dev_err(&client->dev, "can't set alarm flag\n");
+			dev_err(&client->dev, "can't clear alarm flag\n");
 			return ret;
 		}
 		/* enable AIE irq */
@@ -454,5 +455,6 @@  static struct i2c_driver rv3029_driver = {
 module_i2c_driver(rv3029_driver);
 
 MODULE_AUTHOR("Gregory Hermant <gregory.hermant@calao-systems.com>");
+MODULE_AUTHOR("Michael Buesch <m@bues.ch>");
 MODULE_DESCRIPTION("Micro Crystal RV3029 RTC driver");
 MODULE_LICENSE("GPL");