| Message ID | 1485260203-14216-6-git-send-email-al.kochet@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
> 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.
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
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 >
> 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.
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
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; }
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(-)