[rtc-linux] [PATCH 0/8] rtc-ab-b5ze-s3: Fine-tuning for some function implementations

[rtc-linux] [PATCH 0/8] rtc-ab-b5ze-s3: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 09:37:34 +0100

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (8):
  Better exception handling in abb5zes3_probe()
  Delete an unnecessary variable in abb5zes3_rtc_set_alarm()
  Delete an unnecessary variable initialisation in _abb5zes3_rtc_set_timer()
  Replace a variable initialisation by an assignment in _abb5zes3_rtc_set_alarm()
  Replace a variable initialisation by an assignment in _abb5zes3_rtc_read_alarm()
  Delete an unnecessary variable in _abb5zes3_rtc_read_timer()
  Delete an unnecessary variable in _abb5zes3_rtc_interrupt()
  Delete an unnecessary variable in _abb5zes3_rtc_set_timer()

 drivers/rtc/rtc-ab-b5ze-s3.c | 59 ++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 1/8] rtc-ab-b5ze-s3: Better exception handling in abb5zes3_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 07:07:49 +0100

This issue was detected by using the Coccinelle software.

* Return directly before the data structure element "irq" was assigned.

* Drop the explicit initialisation for the variable "data"
  at the beginning then.

* Adjust jump targets according to the Linux coding style convention.

* Simplify a condition check at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/rtc/rtc-ab-b5ze-s3.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
index a319bf1..1291206 100644
--- a/drivers/rtc/rtc-ab-b5ze-s3.c
+++ b/drivers/rtc/rtc-ab-b5ze-s3.c
@@ -889,35 +889,31 @@ static const struct regmap_config abb5zes3_rtc_regmap_config = {
 static int abb5zes3_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
-	struct abb5zes3_rtc_data *data = NULL;
+	struct abb5zes3_rtc_data *data;
 	struct device *dev = &client->dev;
 	struct regmap *regmap;
 	int ret;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
 				     I2C_FUNC_SMBUS_BYTE_DATA |
-				     I2C_FUNC_SMBUS_I2C_BLOCK)) {
-		ret = -ENODEV;
-		goto err;
-	}
+				     I2C_FUNC_SMBUS_I2C_BLOCK))
+		return -ENODEV;
 
 	regmap = devm_regmap_init_i2c(client, &abb5zes3_rtc_regmap_config);
 	if (IS_ERR(regmap)) {
 		ret = PTR_ERR(regmap);
 		dev_err(dev, "%s: regmap allocation failed: %d\n",
 			__func__, ret);
-		goto err;
+		return ret;
 	}
 
 	ret = abb5zes3_i2c_validate_chip(regmap);
 	if (ret)
-		goto err;
+		return ret;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
-	if (!data) {
-		ret = -ENOMEM;
-		goto err;
-	}
+	if (!data)
+		return -ENOMEM;
 
 	mutex_init(&data->lock);
 	data->regmap = regmap;
@@ -925,7 +921,7 @@ static int abb5zes3_probe(struct i2c_client *client,
 
 	ret = abb5zes3_rtc_check_setup(dev);
 	if (ret)
-		goto err;
+		return ret;
 
 	if (client->irq > 0) {
 		ret = devm_request_threaded_irq(dev, client->irq, NULL,
@@ -940,7 +936,7 @@ static int abb5zes3_probe(struct i2c_client *client,
 		} else {
 			dev_err(dev, "%s: irq %d unavailable (%d)\n",
 				__func__, client->irq, ret);
-			goto err;
+			return ret;
 		}
 	}
 
@@ -950,7 +946,7 @@ static int abb5zes3_probe(struct i2c_client *client,
 	if (ret) {
 		dev_err(dev, "%s: unable to register RTC device (%d)\n",
 			__func__, ret);
-		goto err;
+		goto check_irq;
 	}
 
 	/* Enable battery low detection interrupt if battery not already low */
@@ -959,12 +955,12 @@ static int abb5zes3_probe(struct i2c_client *client,
 		if (ret) {
 			dev_err(dev, "%s: enabling battery low interrupt "
 				"generation failed (%d)\n", __func__, ret);
-			goto err;
+			goto check_irq;
 		}
 	}
-
-err:
-	if (ret && data && data->irq)
+	return 0;
+check_irq:
+	if (data->irq)
 		device_init_wakeup(dev, false);
 	return ret;
 }
-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 2/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in abb5zes3_rtc_set_alarm()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 07:35:09 +0100

Pass the address of the data structure element "time" directly in a call
of the function "rtc_tm_to_time" instead of an extra initialisation
for one local variable at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/rtc/rtc-ab-b5ze-s3.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
index 1291206..ed9b873 100644
--- a/drivers/rtc/rtc-ab-b5ze-s3.c
+++ b/drivers/rtc/rtc-ab-b5ze-s3.c
@@ -596,7 +596,6 @@ err:
 static int abb5zes3_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 {
 	struct abb5zes3_rtc_data *data = dev_get_drvdata(dev);
-	struct rtc_time *alarm_tm = &alarm->time;
 	unsigned long rtc_secs, alarm_secs;
 	struct rtc_time rtc_tm;
 	int ret;
@@ -610,7 +609,7 @@ static int abb5zes3_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	if (ret)
 		goto err;
 
-	ret = rtc_tm_to_time(alarm_tm, &alarm_secs);
+	ret = rtc_tm_to_time(&alarm->time, &alarm_secs);
 	if (ret)
 		goto err;
 
-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 3/8] rtc-ab-b5ze-s3: Delete an unnecessary variable initialisation in _abb5zes3_rtc_set_timer()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 07:42:18 +0100

Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/rtc/rtc-ab-b5ze-s3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
index ed9b873..33a7cf1 100644
--- a/drivers/rtc/rtc-ab-b5ze-s3.c
+++ b/drivers/rtc/rtc-ab-b5ze-s3.c
@@ -560,7 +560,7 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm,
 	struct abb5zes3_rtc_data *data = dev_get_drvdata(dev);
 	u8 regs[ABB5ZES3_TIMA_SEC_LEN];
 	u8 mask = ABB5ZES3_REG_TIM_CLK_TAC0 | ABB5ZES3_REG_TIM_CLK_TAC1;
-	int ret = 0;
+	int ret;
 
 	/* Program given number of seconds to Timer A registers */
 	sec_to_timer_a(secs, &regs[0], &regs[1]);
-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 4/8] rtc-ab-b5ze-s3: Replace a variable initialisation by an assignment in _abb5zes3_rtc_set_alarm()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 07:51:49 +0100

Replace an explicit initialisation for one local variable at the beginning
by an assignment.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/rtc/rtc-ab-b5ze-s3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
index 33a7cf1..0986715 100644
--- a/drivers/rtc/rtc-ab-b5ze-s3.c
+++ b/drivers/rtc/rtc-ab-b5ze-s3.c
@@ -476,7 +476,7 @@ static int abb5zes3_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 static int _abb5zes3_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 {
 	struct abb5zes3_rtc_data *data = dev_get_drvdata(dev);
-	struct rtc_time *alarm_tm = &alarm->time;
+	struct rtc_time *alarm_tm;
 	unsigned long rtc_secs, alarm_secs;
 	u8 regs[ABB5ZES3_ALRM_SEC_LEN];
 	struct rtc_time rtc_tm;
@@ -490,6 +490,7 @@ static int _abb5zes3_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	if (ret)
 		goto err;
 
+	alarm_tm = &alarm->time;
 	ret = rtc_tm_to_time(alarm_tm, &alarm_secs);
 	if (ret)
 		goto err;
-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 5/8] rtc-ab-b5ze-s3: Replace a variable initialisation by an assignment in _abb5zes3_rtc_read_alarm()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 08:00:29 +0100

Replace an explicit initialisation for one local variable at the beginning
by an assignment.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/rtc/rtc-ab-b5ze-s3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
index 0986715..02e3443 100644
--- a/drivers/rtc/rtc-ab-b5ze-s3.c
+++ b/drivers/rtc/rtc-ab-b5ze-s3.c
@@ -382,7 +382,7 @@ static int _abb5zes3_rtc_read_alarm(struct device *dev,
 				    struct rtc_wkalrm *alarm)
 {
 	struct abb5zes3_rtc_data *data = dev_get_drvdata(dev);
-	struct rtc_time rtc_tm, *alarm_tm = &alarm->time;
+	struct rtc_time rtc_tm, *alarm_tm;
 	unsigned long rtc_secs, alarm_secs;
 	u8 regs[ABB5ZES3_ALRM_SEC_LEN];
 	unsigned int reg;
@@ -396,6 +396,7 @@ static int _abb5zes3_rtc_read_alarm(struct device *dev,
 		goto err;
 	}
 
+	alarm_tm = &alarm->time;
 	alarm_tm->tm_sec  = 0;
 	alarm_tm->tm_min  = bcd2bin(regs[0] & 0x7f);
 	alarm_tm->tm_hour = bcd2bin(regs[1] & 0x3f);
-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 6/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_read_timer()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 08:46:50 +0100

Pass the address of the data structure element "time" directly in a call
of the function "rtc_time_to_tm" instead of an extra initialisation
for one local variable at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/rtc/rtc-ab-b5ze-s3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
index 02e3443..e3a015a 100644
--- a/drivers/rtc/rtc-ab-b5ze-s3.c
+++ b/drivers/rtc/rtc-ab-b5ze-s3.c
@@ -326,7 +326,7 @@ static int _abb5zes3_rtc_read_timer(struct device *dev,
 				    struct rtc_wkalrm *alarm)
 {
 	struct abb5zes3_rtc_data *data = dev_get_drvdata(dev);
-	struct rtc_time rtc_tm, *alarm_tm = &alarm->time;
+	struct rtc_time rtc_tm;
 	u8 regs[ABB5ZES3_TIMA_SEC_LEN + 1];
 	unsigned long rtc_secs;
 	unsigned int reg;
@@ -362,7 +362,7 @@ static int _abb5zes3_rtc_read_timer(struct device *dev,
 		goto err;
 
 	/* ... and convert back. */
-	rtc_time_to_tm(rtc_secs + timer_secs, alarm_tm);
+	rtc_time_to_tm(rtc_secs + timer_secs, &alarm->time);
 
 	ret = regmap_read(data->regmap, ABB5ZES3_REG_CTRL2, &reg);
 	if (ret) {
-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 7/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_interrupt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 09:00:30 +0100

Pass the address of the data structure element "time" directly in calls
of the function "rtc_update_irq" instead of an extra initialisation
for one local variable at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/rtc/rtc-ab-b5ze-s3.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
index e3a015a..88f1d0b 100644
--- a/drivers/rtc/rtc-ab-b5ze-s3.c
+++ b/drivers/rtc/rtc-ab-b5ze-s3.c
@@ -816,7 +816,6 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
 	struct i2c_client *client = data;
 	struct device *dev = &client->dev;
 	struct abb5zes3_rtc_data *rtc_data = dev_get_drvdata(dev);
-	struct rtc_device *rtc = rtc_data->rtc;
 	u8 regs[ABB5ZES3_CTRL_SEC_LEN];
 	int ret, handled = IRQ_NONE;
 
@@ -844,8 +843,7 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
 	/* Check alarm flag */
 	if (regs[ABB5ZES3_REG_CTRL2] & ABB5ZES3_REG_CTRL2_AF) {
 		dev_dbg(dev, "RTC alarm!\n");
-
-		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
+		rtc_update_irq(rtc_data->rtc, 1, RTC_IRQF | RTC_AF);
 
 		/* Acknowledge and disable the alarm */
 		_abb5zes3_rtc_clear_alarm(dev);
@@ -857,8 +855,7 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
 	/* Check watchdog Timer A flag */
 	if (regs[ABB5ZES3_REG_CTRL2] & ABB5ZES3_REG_CTRL2_WTAF) {
 		dev_dbg(dev, "RTC timer!\n");
-
-		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
+		rtc_update_irq(rtc_data->rtc, 1, RTC_IRQF | RTC_AF);
 
 		/*
 		 * Acknowledge and disable the alarm. Note: WTAF
-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 7/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_interrupt()

[Julia Lawall]

On Sun, 3 Jan 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 3 Jan 2016 09:00:30 +0100
> 
> Pass the address of the data structure element "time" directly in calls
> of the function "rtc_update_irq" instead of an extra initialisation
> for one local variable at the beginning.

Why is it better?

julia
 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/rtc/rtc-ab-b5ze-s3.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
> index e3a015a..88f1d0b 100644
> --- a/drivers/rtc/rtc-ab-b5ze-s3.c
> +++ b/drivers/rtc/rtc-ab-b5ze-s3.c
> @@ -816,7 +816,6 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
>  	struct i2c_client *client = data;
>  	struct device *dev = &client->dev;
>  	struct abb5zes3_rtc_data *rtc_data = dev_get_drvdata(dev);
> -	struct rtc_device *rtc = rtc_data->rtc;
>  	u8 regs[ABB5ZES3_CTRL_SEC_LEN];
>  	int ret, handled = IRQ_NONE;
>  
> @@ -844,8 +843,7 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
>  	/* Check alarm flag */
>  	if (regs[ABB5ZES3_REG_CTRL2] & ABB5ZES3_REG_CTRL2_AF) {
>  		dev_dbg(dev, "RTC alarm!\n");
> -
> -		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> +		rtc_update_irq(rtc_data->rtc, 1, RTC_IRQF | RTC_AF);
>  
>  		/* Acknowledge and disable the alarm */
>  		_abb5zes3_rtc_clear_alarm(dev);
> @@ -857,8 +855,7 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
>  	/* Check watchdog Timer A flag */
>  	if (regs[ABB5ZES3_REG_CTRL2] & ABB5ZES3_REG_CTRL2_WTAF) {
>  		dev_dbg(dev, "RTC timer!\n");
> -
> -		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> +		rtc_update_irq(rtc_data->rtc, 1, RTC_IRQF | RTC_AF);
>  
>  		/*
>  		 * Acknowledge and disable the alarm. Note: WTAF
> -- 
> 2.6.3
> 
> 

[rtc-linux] Re: [PATCH 7/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_interrupt()

[SF Markus Elfring]

>> Pass the address of the data structure element "time" directly in calls
>> of the function "rtc_update_irq" instead of an extra initialisation
>> for one local variable at the beginning.
> 
> Why is it better?

I suggest to refer to the data item "rtc_data->rtc" directly because
the variable "rtc" was read only in two if branches.
Does it make sense then to reduce the variable allocation?

Regards,
Markus

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 7/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_interrupt()

[Julia Lawall]

On Sun, 3 Jan 2016, SF Markus Elfring wrote:

> >> Pass the address of the data structure element "time" directly in calls
> >> of the function "rtc_update_irq" instead of an extra initialisation
> >> for one local variable at the beginning.
> > 
> > Why is it better?
> 
> I suggest to refer to the data item "rtc_data->rtc" directly because
> the variable "rtc" was read only in two if branches.
> Does it make sense then to reduce the variable allocation?

No.  That is the job of the compiler.  For a local variable whose address 
is never taken, the compiler can easily detect its live region, and place 
the initialization in an optimal way.

julia

[rtc-linux] Re: [PATCH 7/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_interrupt()

[Julia Lawall]

On Sun, 3 Jan 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 3 Jan 2016 09:00:30 +0100
> 
> Pass the address of the data structure element "time" directly in calls
> of the function "rtc_update_irq" instead of an extra initialisation
> for one local variable at the beginning.

Also, I don't see anything related to time in this patch.

julia

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/rtc/rtc-ab-b5ze-s3.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
> index e3a015a..88f1d0b 100644
> --- a/drivers/rtc/rtc-ab-b5ze-s3.c
> +++ b/drivers/rtc/rtc-ab-b5ze-s3.c
> @@ -816,7 +816,6 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
>  	struct i2c_client *client = data;
>  	struct device *dev = &client->dev;
>  	struct abb5zes3_rtc_data *rtc_data = dev_get_drvdata(dev);
> -	struct rtc_device *rtc = rtc_data->rtc;
>  	u8 regs[ABB5ZES3_CTRL_SEC_LEN];
>  	int ret, handled = IRQ_NONE;
>  
> @@ -844,8 +843,7 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
>  	/* Check alarm flag */
>  	if (regs[ABB5ZES3_REG_CTRL2] & ABB5ZES3_REG_CTRL2_AF) {
>  		dev_dbg(dev, "RTC alarm!\n");
> -
> -		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> +		rtc_update_irq(rtc_data->rtc, 1, RTC_IRQF | RTC_AF);
>  
>  		/* Acknowledge and disable the alarm */
>  		_abb5zes3_rtc_clear_alarm(dev);
> @@ -857,8 +855,7 @@ static irqreturn_t _abb5zes3_rtc_interrupt(int irq, void *data)
>  	/* Check watchdog Timer A flag */
>  	if (regs[ABB5ZES3_REG_CTRL2] & ABB5ZES3_REG_CTRL2_WTAF) {
>  		dev_dbg(dev, "RTC timer!\n");
> -
> -		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
> +		rtc_update_irq(rtc_data->rtc, 1, RTC_IRQF | RTC_AF);
>  
>  		/*
>  		 * Acknowledge and disable the alarm. Note: WTAF
> -- 
> 2.6.3
> 
> 

[rtc-linux] Re: [PATCH 7/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_interrupt()

[SF Markus Elfring]

>> Pass the address of the data structure element "time" directly in calls
>> of the function "rtc_update_irq" instead of an extra initialisation
>> for one local variable at the beginning.
> 
> Also, I don't see anything related to time in this patch.

I should have referred to the data structure element "rtc" here.
Should I resend this patch series with a corrected commit message now?

Regards,
Markus

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] [PATCH 8/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_set_timer()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 3 Jan 2016 09:19:32 +0100

Pass a value directly in a call of the function "regmap_update_bits"
instead of an extra initialisation for one local variable at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/rtc/rtc-ab-b5ze-s3.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
index 88f1d0b..fed52d0 100644
--- a/drivers/rtc/rtc-ab-b5ze-s3.c
+++ b/drivers/rtc/rtc-ab-b5ze-s3.c
@@ -561,7 +561,6 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm,
 {
 	struct abb5zes3_rtc_data *data = dev_get_drvdata(dev);
 	u8 regs[ABB5ZES3_TIMA_SEC_LEN];
-	u8 mask = ABB5ZES3_REG_TIM_CLK_TAC0 | ABB5ZES3_REG_TIM_CLK_TAC1;
 	int ret;
 
 	/* Program given number of seconds to Timer A registers */
@@ -575,7 +574,9 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm,
 
 	/* Configure Timer A as a watchdog timer */
 	ret = regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK,
-				 mask, ABB5ZES3_REG_TIM_CLK_TAC1);
+				 ABB5ZES3_REG_TIM_CLK_TAC0
+				 | ABB5ZES3_REG_TIM_CLK_TAC1,
+				 ABB5ZES3_REG_TIM_CLK_TAC1);
 	if (ret)
 		dev_err(dev, "%s: failed to update timer\n", __func__);
 
-- 
2.6.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 8/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_set_timer()

[Julia Lawall]

On Sun, 3 Jan 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 3 Jan 2016 09:19:32 +0100
> 
> Pass a value directly in a call of the function "regmap_update_bits"
> instead of an extra initialisation for one local variable at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/rtc/rtc-ab-b5ze-s3.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ab-b5ze-s3.c b/drivers/rtc/rtc-ab-b5ze-s3.c
> index 88f1d0b..fed52d0 100644
> --- a/drivers/rtc/rtc-ab-b5ze-s3.c
> +++ b/drivers/rtc/rtc-ab-b5ze-s3.c
> @@ -561,7 +561,6 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm,
>  {
>  	struct abb5zes3_rtc_data *data = dev_get_drvdata(dev);
>  	u8 regs[ABB5ZES3_TIMA_SEC_LEN];
> -	u8 mask = ABB5ZES3_REG_TIM_CLK_TAC0 | ABB5ZES3_REG_TIM_CLK_TAC1;
>  	int ret;
>  
>  	/* Program given number of seconds to Timer A registers */
> @@ -575,7 +574,9 @@ static int _abb5zes3_rtc_set_timer(struct device *dev, struct rtc_wkalrm *alarm,
>  
>  	/* Configure Timer A as a watchdog timer */
>  	ret = regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK,
> -				 mask, ABB5ZES3_REG_TIM_CLK_TAC1);
> +				 ABB5ZES3_REG_TIM_CLK_TAC0
> +				 | ABB5ZES3_REG_TIM_CLK_TAC1,
> +				 ABB5ZES3_REG_TIM_CLK_TAC1);

This doesn't seem like an improvement.  The concept (mask) has 
disappeared, the binary operation is strangely broken, and the function 
call has one more line of arguments, which all look sort of the same and 
thus are hard to understand.

Don't underestimate the value of naming things.

julia

>  	if (ret)
>  		dev_err(dev, "%s: failed to update timer\n", __func__);
>  
> -- 
> 2.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[rtc-linux] Re: [PATCH 8/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_set_timer()

[SF Markus Elfring]

>>  	ret =3D regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK,
>> -				 mask, ABB5ZES3_REG_TIM_CLK_TAC1);
>> +				 ABB5ZES3_REG_TIM_CLK_TAC0
>> +				 | ABB5ZES3_REG_TIM_CLK_TAC1,
>> +				 ABB5ZES3_REG_TIM_CLK_TAC1);
>=20
> This doesn't seem like an improvement.

Interesting =E2=80=A6


> The concept (mask) has disappeared,

I suggest to drop another local variable.
Can the operator "Bitwise OR" be sufficient to indicate the concept "mask"?


> the binary operation is strangely broken,

Do you prefer an other source code formatting within the usual line length =
range?


> and the function call has one more line of arguments,

How should several long preprocessor symbols be combined together with inde=
ntation
so that they will fit into the limit of 80 characters?


> which all look sort of the same and thus are hard to understand.

Is this an usual consequence from an ordinary name pattern?

Regards,
Markus

--=20
--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH 8/8] rtc-ab-b5ze-s3: Delete an unnecessary variable in _abb5zes3_rtc_set_timer()

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1155 bytes --]

On Sun, 3 Jan 2016, SF Markus Elfring wrote:

> >>  	ret = regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK,
> >> -				 mask, ABB5ZES3_REG_TIM_CLK_TAC1);
> >> +				 ABB5ZES3_REG_TIM_CLK_TAC0
> >> +				 | ABB5ZES3_REG_TIM_CLK_TAC1,
> >> +				 ABB5ZES3_REG_TIM_CLK_TAC1);
> > 
> > This doesn't seem like an improvement.
> 
> Interesting …
> 
> 
> > The concept (mask) has disappeared,
> 
> I suggest to drop another local variable.
> Can the operator "Bitwise OR" be sufficient to indicate the concept "mask"?
> 
> 
> > the binary operation is strangely broken,
> 
> Do you prefer an other source code formatting within the usual line length range?
> 
> 
> > and the function call has one more line of arguments,
> 
> How should several long preprocessor symbols be combined together with indentation
> so that they will fit into the limit of 80 characters?
> 
> 
> > which all look sort of the same and thus are hard to understand.
> 
> Is this an usual consequence from an ordinary name pattern?

The original code was better.  No 80 character problem, easy to 
distinguish one argument from another, moderately meaningful variable 
name, etc.

julia