Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1565816 > unrolled thread

[PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device Alexander Kochetkov <al.kochet@gmail.com> - 2017-01-24 13:20 +0100
    Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device Heiko Stübner <heiko@sntech.de> - 2017-01-24 16:10 +0100
      Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device Alexander Kochetkov <al.kochet@gmail.com> - 2017-01-24 16:20 +0100
        Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device Heiko Stuebner <heiko@sntech.de> - 2017-01-29 21:40 +0100
    Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split  bc_timer into rk_timer and rk_clock_event_device Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-01-30 14:20 +0100
      Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device Alexander Kochetkov <al.kochet@gmail.com> - 2017-01-30 15:00 +0100
        Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split  bc_timer into rk_timer and rk_clock_event_device Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-01-30 15:30 +0100

#1565816 — [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

The patch move ce field out of struct bc_timer into struct
rk_clock_event_device and rename struct bc_timer to struct rk_timer.

The main idea for the commit is to exctact low level timer
routines from current implementation so they could be
reused in the following clocksource implementation commit.

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko Stübner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 23e267a..6d68d4c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -29,18 +29,28 @@
 #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
 #define TIMER_INT_UNMASK			(1 << 2)
 
-struct bc_timer {
-	struct clock_event_device ce;
+struct rk_timer {
 	void __iomem *base;
 	void __iomem *ctrl;
 	u32 freq;
 };
 
-static struct bc_timer bc_timer;
+struct rk_clock_event_device {
+	struct clock_event_device ce;
+	struct rk_timer timer;
+};
+
+static struct rk_clock_event_device bc_timer;
+
+static inline struct rk_clock_event_device*
+rk_clock_event_device(struct clock_event_device *ce)
+{
+	return container_of(ce, struct rk_clock_event_device, ce);
+}
 
-static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
+static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 {
-	return container_of(ce, struct bc_timer, ce);
+	return &rk_clock_event_device(ce)->timer;
 }
 
 static inline void __iomem *rk_base(struct clock_event_device *ce)
@@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
+	struct rk_timer *timer = &bc_timer.timer;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
 
-	bc_timer.base = of_iomap(np, 0);
-	if (!bc_timer.base) {
+	timer->base = of_iomap(np, 0);
+	if (!timer->base) {
 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
 		return -ENXIO;
 	}
-	bc_timer.ctrl = bc_timer.base + ctrl_reg;
+	timer->ctrl = timer->base + ctrl_reg;
 
 	pclk = of_clk_get_by_name(np, "pclk");
 	if (IS_ERR(pclk)) {
@@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_timer_clk;
 	}
 
-	bc_timer.freq = clk_get_rate(timer_clk);
+	timer->freq = clk_get_rate(timer_clk);
 
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
@@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
+	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 
 	return 0;
 
@@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 out_timer_clk:
 	clk_disable_unprepare(pclk);
 out_unmap:
-	iounmap(bc_timer.base);
+	iounmap(timer->base);
 
 	return ret;
 }
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1565927

Am Dienstag, 24. Januar 2017, 15:16:40 CET schrieb Alexander Kochetkov:
> The patch move ce field out of struct bc_timer into struct
> rk_clock_event_device and rename struct bc_timer to struct rk_timer.
> 
> The main idea for the commit is to exctact low level timer
> routines from current implementation so they could be
> reused in the following clocksource implementation commit.
> 
> This is refactoring step without functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko Stübner <heiko@sntech.de>

Please don't add Reviewed-by tags without explicit mention of them by 
reviewers. (Also it's spelled wrong).

I haven't looked that deeply into the driver itself and the changes made to 
feel comfortable with a "Reviewed-by".

> ---
>  drivers/clocksource/rockchip_timer.c |   33
> ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11
> deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c
> b/drivers/clocksource/rockchip_timer.c index 23e267a..6d68d4c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -29,18 +29,28 @@
>  #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
>  #define TIMER_INT_UNMASK			(1 << 2)
> 
> -struct bc_timer {
> -	struct clock_event_device ce;
> +struct rk_timer {
>  	void __iomem *base;
>  	void __iomem *ctrl;
>  	u32 freq;
>  };
> 
> -static struct bc_timer bc_timer;
> +struct rk_clock_event_device {
> +	struct clock_event_device ce;
> +	struct rk_timer timer;
> +};
> +
> +static struct rk_clock_event_device bc_timer;
> +
> +static inline struct rk_clock_event_device*
> +rk_clock_event_device(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct rk_clock_event_device, ce);
> +}
> 
> -static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
>  {
> -	return container_of(ce, struct bc_timer, ce);
> +	return &rk_clock_event_device(ce)->timer;
>  }
> 
>  static inline void __iomem *rk_base(struct clock_event_device *ce)
> @@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void
> *dev_id) static int __init rk_timer_init(struct device_node *np, u32
> ctrl_reg) {
>  	struct clock_event_device *ce = &bc_timer.ce;
> +	struct rk_timer *timer = &bc_timer.timer;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
> 
> -	bc_timer.base = of_iomap(np, 0);
> -	if (!bc_timer.base) {
> +	timer->base = of_iomap(np, 0);
> +	if (!timer->base) {
>  		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
>  		return -ENXIO;
>  	}
> -	bc_timer.ctrl = bc_timer.base + ctrl_reg;
> +	timer->ctrl = timer->base + ctrl_reg;
> 
>  	pclk = of_clk_get_by_name(np, "pclk");
>  	if (IS_ERR(pclk)) {
> @@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) goto out_timer_clk;
>  	}
> 
> -	bc_timer.freq = clk_get_rate(timer_clk);
> +	timer->freq = clk_get_rate(timer_clk);
> 
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
> @@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) goto out_irq;
>  	}
> 
> -	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
> 
>  	return 0;
> 
> @@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) out_timer_clk:
>  	clk_disable_unprepare(pclk);
>  out_unmap:
> -	iounmap(bc_timer.base);
> +	iounmap(timer->base);
> 
>  	return ret;
>  }

[toc] | [prev] | [next] | [standalone]

#1565931

> 24 янв. 2017 г., в 18:02, Heiko Stübner <heiko@sntech.de> написал(а):
> 
> Please don't add Reviewed-by tags without explicit mention of them by 
> reviewers. (Also it's spelled wrong).
> 
> I haven't looked that deeply into the driver itself and the changes made to 
> feel comfortable with a "Reviewed-by".

Than I posted another linux code I was asked to add such tags (and others)
to add credits to people.

Sorry for that.

v6? or feel free to drop any tags during merge.

[toc] | [prev] | [next] | [standalone]

#1569279

Hi Alexander,

Am Dienstag, 24. Januar 2017, 18:16:20 CET schrieb Alexander Kochetkov:
> > 24 янв. 2017 г., в 18:02, Heiko Stübner <heiko@sntech.de> написал(а):
> > 
> > Please don't add Reviewed-by tags without explicit mention of them by
> > reviewers. (Also it's spelled wrong).
> > 
> > I haven't looked that deeply into the driver itself and the changes made
> > to
> > feel comfortable with a "Reviewed-by".
> 
> Than I posted another linux code I was asked to add such tags (and others)
> to add credits to people.
> 
> Sorry for that.

no problem. In general people reviewing patches will respond to the patch with 
tags they are comfortable with being added. And a Reviewed-by is a pretty 
strong tag, indicating that some thought went into reviewing a specific patch, 
which I hadn't done with my cursory glance at the time :-)

Heiko

[toc] | [prev] | [next] | [standalone]

#1569730 — Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

On Tue, Jan 24, 2017 at 03:16:40PM +0300, Alexander Kochetkov wrote:
> The patch move ce field out of struct bc_timer into struct
> rk_clock_event_device and rename struct bc_timer to struct rk_timer.
> 
> The main idea for the commit is to exctact low level timer
> routines from current implementation so they could be
> reused in the following clocksource implementation commit.
> 
> This is refactoring step without functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko Stübner <heiko@sntech.de>

I don't get the point of these changes. The patch does not explain why they are
needed.

> ---
>  drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index 23e267a..6d68d4c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -29,18 +29,28 @@
>  #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
>  #define TIMER_INT_UNMASK			(1 << 2)
>  
> -struct bc_timer {
> -	struct clock_event_device ce;
> +struct rk_timer {
>  	void __iomem *base;
>  	void __iomem *ctrl;
>  	u32 freq;
>  };
>  
> -static struct bc_timer bc_timer;
> +struct rk_clock_event_device {
> +	struct clock_event_device ce;
> +	struct rk_timer timer;
> +};
> +
> +static struct rk_clock_event_device bc_timer;
> +
> +static inline struct rk_clock_event_device*
> +rk_clock_event_device(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct rk_clock_event_device, ce);
> +}
>  
> -static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
>  {
> -	return container_of(ce, struct bc_timer, ce);
> +	return &rk_clock_event_device(ce)->timer;
>  }
>  
>  static inline void __iomem *rk_base(struct clock_event_device *ce)
> @@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
>  static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  {
>  	struct clock_event_device *ce = &bc_timer.ce;
> +	struct rk_timer *timer = &bc_timer.timer;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
>  
> -	bc_timer.base = of_iomap(np, 0);
> -	if (!bc_timer.base) {
> +	timer->base = of_iomap(np, 0);
> +	if (!timer->base) {
>  		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
>  		return -ENXIO;
>  	}
> -	bc_timer.ctrl = bc_timer.base + ctrl_reg;
> +	timer->ctrl = timer->base + ctrl_reg;
>  
>  	pclk = of_clk_get_by_name(np, "pclk");
>  	if (IS_ERR(pclk)) {
> @@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_timer_clk;
>  	}
>  
> -	bc_timer.freq = clk_get_rate(timer_clk);
> +	timer->freq = clk_get_rate(timer_clk);
>  
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
> @@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_irq;
>  	}
>  
> -	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
>  
>  	return 0;
>  
> @@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  out_timer_clk:
>  	clk_disable_unprepare(pclk);
>  out_unmap:
> -	iounmap(bc_timer.base);
> +	iounmap(timer->base);
>  
>  	return ret;
>  }
> -- 
> 1.7.9.5
> 

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1569752

> 30 янв. 2017 г., в 16:12, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):
> 
> I don't get the point of these changes. The patch does not explain why they are
> needed.

I’d like to extract timer API from current implementation.
And to make code more readable I’d like to introduce 'struct rk_timer’ what can be
reused with current implementation and with my patch (8/8). And in order keep patches
simple and readable I split that into three patches: 5/8, 6/8, 7/8.

Current implementation named rockchip timer as ‘struct bc_timer’ (broadcast timer).
I renamed it to more suitable to it role (may be bad choice).

Yes, the patch itself looks strange. You are right.

What do you think about that solution:
- in the patch 6/8 i will Introduce 'struct rk_timer’ and 'struct rk_time_clkevt’ (renamed ‘struct bc_timer’).
- rk_timer_init() changes from 5/8 I will merge with 8/8
- 8/8 introduce 'struct rk_time_clksrc' 
- 5/8 drop

Alexander.

[toc] | [prev] | [next] | [standalone]

#1569777 — Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

On Mon, Jan 30, 2017 at 04:55:33PM +0300, Alexander Kochetkov wrote:
> 
> > 30 янв. 2017 г., в 16:12, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):
> > 
> > I don't get the point of these changes. The patch does not explain why they are
> > needed.
> 
> I’d like to extract timer API from current implementation.
> And to make code more readable I’d like to introduce 'struct rk_timer’ what can be
> reused with current implementation and with my patch (8/8). And in order keep patches
> simple and readable I split that into three patches: 5/8, 6/8, 7/8.
> 
> Current implementation named rockchip timer as ‘struct bc_timer’ (broadcast timer).
> I renamed it to more suitable to it role (may be bad choice).
> 
> Yes, the patch itself looks strange. You are right.
> 
> What do you think about that solution:
> - in the patch 6/8 i will Introduce 'struct rk_timer’ and 'struct rk_time_clkevt’ (renamed ‘struct bc_timer’).

I prefer rk_clksrc and rk_clkevt.

> - rk_timer_init() changes from 5/8 I will merge with 8/8
> - 8/8 introduce 'struct rk_time_clksrc' 
> - 5/8 drop

Ok, let's see what that gives.

  -- Daniel

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web