Messages in this thread Patch in this message | | | Subject | Re: [PATCH] h8300: Initialize cleanup and remove module code | From | Daniel Lezcano <> | Date | Wed, 4 Nov 2015 14:49:53 +0100 |
| |
On 11/04/2015 08:35 AM, Yoshinori Sato wrote: > h8300's clocksource driver cleanup. > - Use CLOCKSOURCE_OF_DECLARE > - Remove module exit
Hi Yoshinori,
the patches does not apply on tip/timers/core.
Also it seems to contain changes not related to initialization cleanups and module code.
I have a few comment above regarding the initialization.
If you can separate the patches, it will be easier to review.
Thanks.
-- Daniel
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> > --- > drivers/clocksource/h8300_timer16.c | 133 ++++++++++++----------------- > drivers/clocksource/h8300_timer8.c | 164 ++++++++++++++---------------------- > drivers/clocksource/h8300_tpu.c | 105 ++++++++--------------- > 3 files changed, 151 insertions(+), 251 deletions(-) > > diff --git a/drivers/clocksource/h8300_timer16.c b/drivers/clocksource/h8300_timer16.c > index 82941c1..e2e10cf 100644 > --- a/drivers/clocksource/h8300_timer16.c > +++ b/drivers/clocksource/h8300_timer16.c > @@ -17,6 +17,8 @@ > #include <linux/clk.h> > #include <linux/io.h> > #include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > > #include <asm/segment.h> > #include <asm/irq.h> > @@ -47,7 +49,6 @@ > #define ABSOLUTE 1 > > struct timer16_priv { > - struct platform_device *pdev; > struct clocksource cs; > struct irqaction irqaction; > unsigned long total_cycles; > @@ -147,43 +148,58 @@ static void timer16_disable(struct clocksource *cs) > #define REG_CH 0 > #define REG_COMM 1 > > -static int timer16_setup(struct timer16_priv *p, struct platform_device *pdev) > +static void __init h8300_16timer_init(struct device_node *node) > { > - struct resource *res[2]; > + void __iomem *base[2]; > int ret, irq; > unsigned int ch; > + struct timer16_priv *p; > + struct clk *clk; > + > + clk = of_clk_get(node, 0); > + if (IS_ERR(clk)) { > + pr_err("failed to get clock for clocksource\n"); > + return; > + } > > - memset(p, 0, sizeof(*p)); > - p->pdev = pdev; > + base[REG_CH] = of_iomap(node, 0); > + if (!base[0]) {
if (!base[REG_CH]) {
> + pr_err("failed to map registers for clocksource\n"); > + goto free_clk; > + } > > - res[REG_CH] = platform_get_resource(p->pdev, > - IORESOURCE_MEM, REG_CH); > - res[REG_COMM] = platform_get_resource(p->pdev, > - IORESOURCE_MEM, REG_COMM); > - if (!res[REG_CH] || !res[REG_COMM]) { > - dev_err(&p->pdev->dev, "failed to get I/O memory\n"); > - return -ENXIO; > + base[REG_COMM] = of_iomap(node, 1); > + if (!base[1]) {
if (!base[REG_COMM]) {
> + pr_err("failed to map registers for clocksource\n"); > + goto unmap_ch; > } > - irq = platform_get_irq(p->pdev, 0); > + > + irq = irq_of_parse_and_map(node, 0); > if (irq < 0) { > - dev_err(&p->pdev->dev, "failed to get irq\n"); > - return irq; > + pr_err("failed to get irq for clockevent\n"); > + goto unmap_comm; > } > > - p->clk = clk_get(&p->pdev->dev, "fck"); > - if (IS_ERR(p->clk)) { > - dev_err(&p->pdev->dev, "can't get clk\n"); > - return PTR_ERR(p->clk); > + p = kzalloc(sizeof(*p), GFP_KERNEL); > + if (!p) { > + pr_err("failed to allocate memory for clocksource\n");
No need to add an error for an allocation error and as the module static, this variable could be static also and initialized directly when declared.
> + goto unmap_comm; > } > - of_property_read_u32(p->pdev->dev.of_node, "renesas,channel", &ch); > > - p->pdev = pdev; > - p->mapbase = res[REG_CH]->start; > - p->mapcommon = res[REG_COMM]->start; > + of_property_read_u32(node, "renesas,channel", &ch); > + > + p->mapbase = (unsigned long)base[REG_CH]; > + p->mapcommon = (unsigned long)base[REG_COMM]; > p->enb = 1 << ch; > p->imfa = 1 << ch; > p->imiea = 1 << (4 + ch); > - p->cs.name = pdev->name; > + > + p->irqaction.name = node->name; > + p->irqaction.handler = timer16_interrupt; > + p->irqaction.dev_id = p; > + p->irqaction.flags = IRQF_TIMER; > + > + p->cs.name = node->name; > p->cs.rating = 200; > p->cs.read = timer16_clocksource_read; > p->cs.enable = timer16_enable; > @@ -191,64 +207,23 @@ static int timer16_setup(struct timer16_priv *p, struct platform_device *pdev) > p->cs.mask = CLOCKSOURCE_MASK(sizeof(unsigned long) * 8); > p->cs.flags = CLOCK_SOURCE_IS_CONTINUOUS; > > - ret = request_irq(irq, timer16_interrupt, > - IRQF_TIMER, pdev->name, p); > + ret = setup_irq(irq, &p->irqaction);
It is better to keep request_irq.
> if (ret < 0) { > - dev_err(&p->pdev->dev, "failed to request irq %d\n", irq); > - return ret; > + pr_err("failed to request irq %d of clocksource\n", irq); > + goto free_priv; > } > > clocksource_register_hz(&p->cs, clk_get_rate(p->clk) / 8); > - > - return 0; > -}
-- <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
| |