Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965499AbbKDNt6 (ORCPT ); Wed, 4 Nov 2015 08:49:58 -0500 Received: from mail-wm0-f43.google.com ([74.125.82.43]:38688 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965474AbbKDNt4 (ORCPT ); Wed, 4 Nov 2015 08:49:56 -0500 Subject: Re: [PATCH] h8300: Initialize cleanup and remove module code To: Yoshinori Sato , Thomas Gleixner References: <1446622548-18248-1-git-send-email-ysato@users.sourceforge.jp> Cc: linux-kernel@vger.kernel.org From: Daniel Lezcano Message-ID: <563A0D01.8000804@linaro.org> Date: Wed, 4 Nov 2015 14:49:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1446622548-18248-1-git-send-email-ysato@users.sourceforge.jp> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5222 Lines: 176 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(&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; > -} -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/