diff mbox

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

Message ID 5688E229.4010400@users.sourceforge.net
State Rejected
Headers show

Commit Message

SF Markus Elfring Jan. 3, 2016, 8:56 a.m. UTC
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(-)

Comments

Julia Lawall Jan. 3, 2016, 12:48 p.m. UTC | #1
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
> 
>
Julia Lawall Jan. 3, 2016, 12:48 p.m. UTC | #2
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
> 
>
SF Markus Elfring Jan. 3, 2016, 4:54 p.m. UTC | #3
>> 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
Julia Lawall Jan. 3, 2016, 4:59 p.m. UTC | #4
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
SF Markus Elfring Jan. 3, 2016, 5 p.m. UTC | #5
>> 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
diff mbox

Patch

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