Re: [PATCH] h8300: Initialize cleanup and remove module code
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 --- 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 #include #include +#include +#include #include #include @@ -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(>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(>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(>pdev->dev, "fck"); - if (IS_ERR(p->clk)) { - dev_err(>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", ); - p->pdev = pdev; - p->mapbase = res[REG_CH]->start; - p->mapcommon = res[REG_COMM]->start; + of_property_read_u32(node, "renesas,channel", ); + + 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, >irqaction); It is better to keep request_irq. if (ret < 0) { - dev_err(>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(>cs,
Re: [PATCH] h8300: Initialize cleanup and remove module code
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--- 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 #include #include +#include +#include #include #include @@ -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(>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(>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(>pdev->dev, "fck"); - if (IS_ERR(p->clk)) { - dev_err(>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", ); - p->pdev = pdev; - p->mapbase = res[REG_CH]->start; - p->mapcommon = res[REG_COMM]->start; + of_property_read_u32(node, "renesas,channel", ); + + 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, >irqaction); It is better to keep request_irq. if (ret < 0) { - dev_err(>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; }
[PATCH] h8300: Initialize cleanup and remove module code
h8300's clocksource driver cleanup. - Use CLOCKSOURCE_OF_DECLARE - Remove module exit Signed-off-by: Yoshinori Sato --- 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 #include #include +#include +#include #include #include @@ -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]) { + 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(>pdev->dev, "failed to get I/O memory\n"); - return -ENXIO; + base[REG_COMM] = of_iomap(node, 1); + if (!base[1]) { + 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(>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(>pdev->dev, "fck"); - if (IS_ERR(p->clk)) { - dev_err(>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"); + goto unmap_comm; } - of_property_read_u32(p->pdev->dev.of_node, "renesas,channel", ); - p->pdev = pdev; - p->mapbase = res[REG_CH]->start; - p->mapcommon = res[REG_COMM]->start; + of_property_read_u32(node, "renesas,channel", ); + + 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, >irqaction); if (ret < 0) { - dev_err(>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(>cs, clk_get_rate(p->clk) / 8); - - return 0; -} - -static int timer16_probe(struct platform_device *pdev) -{ - struct timer16_priv *p = platform_get_drvdata(pdev); - - if (p) { - dev_info(>dev, "kept as earlytimer\n"); - return 0; - } - - p = devm_kzalloc(>dev, sizeof(*p), GFP_KERNEL); - if (!p) - return -ENOMEM; - - return timer16_setup(p, pdev); -} - -static int timer16_remove(struct platform_device *pdev) -{ - return -EBUSY; -} - -static const struct of_device_id timer16_of_table[] = { - { .compatible =
[PATCH] h8300: Initialize cleanup and remove module code
h8300's clocksource driver cleanup. - Use CLOCKSOURCE_OF_DECLARE - Remove module exit Signed-off-by: Yoshinori Sato--- 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 #include #include +#include +#include #include #include @@ -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]) { + 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(>pdev->dev, "failed to get I/O memory\n"); - return -ENXIO; + base[REG_COMM] = of_iomap(node, 1); + if (!base[1]) { + 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(>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(>pdev->dev, "fck"); - if (IS_ERR(p->clk)) { - dev_err(>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"); + goto unmap_comm; } - of_property_read_u32(p->pdev->dev.of_node, "renesas,channel", ); - p->pdev = pdev; - p->mapbase = res[REG_CH]->start; - p->mapcommon = res[REG_COMM]->start; + of_property_read_u32(node, "renesas,channel", ); + + 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, >irqaction); if (ret < 0) { - dev_err(>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(>cs, clk_get_rate(p->clk) / 8); - - return 0; -} - -static int timer16_probe(struct platform_device *pdev) -{ - struct timer16_priv *p = platform_get_drvdata(pdev); - - if (p) { - dev_info(>dev, "kept as earlytimer\n"); - return 0; - } - - p = devm_kzalloc(>dev, sizeof(*p), GFP_KERNEL); - if (!p) - return -ENOMEM; - - return timer16_setup(p, pdev); -} - -static int timer16_remove(struct platform_device *pdev) -{ - return -EBUSY; -} - -static const struct of_device_id timer16_of_table[] = { -