Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751055AbbFWWZQ (ORCPT ); Tue, 23 Jun 2015 18:25:16 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:46220 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932319AbbFWWZC (ORCPT ); Tue, 23 Jun 2015 18:25:02 -0400 Message-ID: <5589DCB9.2000908@codeaurora.org> Date: Tue, 23 Jun 2015 15:24:57 -0700 From: Stephen Boyd User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Paul Osmialowski , Andrew Morton , Anson Huang , Ard Biesheuvel , Arnd Bergmann , Bhupesh Sharma , Daniel Lezcano , Frank Li , Geert Uytterhoeven , Greg Kroah-Hartman , Guenter Roeck , Haojian Zhuang , Ian Campbell , Jingchang Lu , Jiri Slaby , Kees Cook , Kumar Gala , Laurent Pinchart , Linus Walleij , Magnus Damm , Michael Turquette , Nathan Lynch , Nicolas Pitre , Maxime Coquelin stm32 , Olof Johansson , Paul Bolle , Rob Herring , Rob Herring , Russell King , Sergey Senozhatsky , Shawn Guo , Simon Horman , Stefan Agner , Thomas Gleixner , Uwe Kleine-Koenig , Catalin Marinas , Dave Martin , Mark Rutland , Pawel Moll , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org, linux-serial@vger.kernel.org, devicetree@vger.kernel.org, dmaengine@vger.kernel.org CC: Yuri Tikhonov , Sergei Poselenov , Dmitry Cherkassov , Alexander Potashev Subject: Re: [PATCH 6/9] arm: twr-k70f120m: clock source drivers for Kinetis SoC References: <1435094387-20146-1-git-send-email-pawelo@king.net.pl> <1435094387-20146-7-git-send-email-pawelo@king.net.pl> In-Reply-To: <1435094387-20146-7-git-send-email-pawelo@king.net.pl> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5884 Lines: 214 On 06/23/2015 02:19 PM, Paul Osmialowski wrote: > > diff --git a/drivers/clk/clk-kinetis.c b/drivers/clk/clk-kinetis.c > new file mode 100644 > index 0000000..dea1054 > --- /dev/null > +++ b/drivers/clk/clk-kinetis.c > @@ -0,0 +1,226 @@ > +/* > + * clk-kinetis.c - Clock driver for Kinetis K70 MCG > + * > + * Based on legacy pre-OF code by Alexander Potashev > + * > + * Copyright (C) 2015 Paul Osmialowski > + * > + * This program is free software; you can redistribute it and/or modify it under > + * the terms of the GNU General Public License version 2 as published by the > + * Free Software Foundation. > + */ > + > +#include Is this using the consumer API? Please remove this include. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include It would be nice if we didn't need these mach includes so that this driver can be easily build tested. > + > +#include [..] > +} > + > +CLK_OF_DECLARE(kinetis_mcg, "fsl,kinetis-cmu", kinetis_mcg_init); A clocksource isn't the same as a clk provider. Please split this patch into two, one for the clk provider (drivers/clk) and one for the clocksource driver (drivers/clocksource). > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index 0f1c77e..1d2ecde 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -106,6 +106,11 @@ config CLKSRC_EFM32 > Support to use the timers of EFM32 SoCs as clock source and clock > event device. > > +config CLKSRC_KINETIS > + bool "Clocksource for Kinetis SoCs" > + depends on OF && ARM && ARCH_KINETIS Doesn't ARCH_KINETIS imply ARM? Seems that we can drop the ARM dependency here. > + select CLKSRC_OF > > > diff --git a/drivers/clocksource/timer-kinetis.c b/drivers/clocksource/timer-kinetis.c > new file mode 100644 > index 0000000..634f365 > --- /dev/null > +++ b/drivers/clocksource/timer-kinetis.c [..] > + > +/* > + * Clock event device set mode function > + */ > +static void kinetis_clockevent_tmr_set_mode( > + enum clock_event_mode mode, struct clock_event_device *clk) s/clk/evt/ ? > +{ > + struct kinetis_clock_event_ddata *pit = > + container_of(clk, struct kinetis_clock_event_ddata, evtdev); > + > + switch (mode) { > + case CLOCK_EVT_MODE_PERIODIC: > + kinetis_pit_enable(pit->base, 1); > + break; > + case CLOCK_EVT_MODE_ONESHOT: > + case CLOCK_EVT_MODE_UNUSED: > + case CLOCK_EVT_MODE_SHUTDOWN: > + default: > + kinetis_pit_enable(pit->base, 0); > + } > +} > + > +/* > + * Configure the timer to generate an interrupt in the specified amount of ticks > + */ > +static int kinetis_clockevent_tmr_set_next_event( > + unsigned long delta, struct clock_event_device *c) > +{ > + struct kinetis_clock_event_ddata *pit = > + container_of(c, struct kinetis_clock_event_ddata, evtdev); > + unsigned long flags; > + > + raw_local_irq_save(flags); What is this protecting against? > + kinetis_pit_init(pit->base, delta); > + kinetis_pit_enable(pit->base, 1); > + raw_local_irq_restore(flags); > + > + return 0; > +} > + > +static struct kinetis_clock_event_ddata > + kinetis_clockevent_tmrs[KINETIS_PIT_CHANNELS] = { > + { > + .evtdev = { > + .name = "fsl,kinetis-pit-timer0", > + .rating = 200, > + .features = > + CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, > + .set_mode = kinetis_clockevent_tmr_set_mode, > + .set_next_event = kinetis_clockevent_tmr_set_next_event, > + }, > + }, > + { > + .evtdev = { > + .name = "fsl,kinetis-pit-timer1", > + }, > + }, > + { > + .evtdev = { > + .name = "fsl,kinetis-pit-timer2", > + }, > + }, > + { > + .evtdev = { > + .name = "fsl,kinetis-pit-timer3", > + }, > + }, > +}; > + > +/* > + * Timer IRQ handler > + */ > +static irqreturn_t kinetis_clockevent_tmr_irq_handler(int irq, void *dev_id) > +{ > + struct kinetis_clock_event_ddata *tmr = dev_id; > + > + KINETIS_PIT_WR(tmr->base, tflg, KINETIS_PIT_TFLG_TIF_MSK); > + > + tmr->evtdev.event_handler(&(tmr->evtdev)); Unnecessary parentheses, please remove them. > + > + return IRQ_HANDLED; > +} > + > +/* > + * System timer IRQ action > + */ > +static struct irqaction kinetis_clockevent_irqaction[KINETIS_PIT_CHANNELS] = { > + { > + .name = "Kinetis Kernel Time Tick (pit0)", > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > + .dev_id = &kinetis_clockevent_tmrs[0], > + .handler = kinetis_clockevent_tmr_irq_handler, > + }, { > + .name = "Kinetis Kernel Time Tick (pit1)", > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > + .dev_id = &kinetis_clockevent_tmrs[1], > + .handler = kinetis_clockevent_tmr_irq_handler, > + }, { > + .name = "Kinetis Kernel Time Tick (pit2)", > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > + .dev_id = &kinetis_clockevent_tmrs[2], > + .handler = kinetis_clockevent_tmr_irq_handler, > + }, { > + .name = "Kinetis Kernel Time Tick (pit3)", > + .flags = IRQF_TIMER | IRQF_IRQPOLL, > + .dev_id = &kinetis_clockevent_tmrs[3], > + .handler = kinetis_clockevent_tmr_irq_handler, > + }, > +}; Any reason we can't just use request_irq() instead of having a set of static irq actions? > + > +static void __init kinetis_clockevent_init(struct device_node *np) > +{ [..] > irq; > + } > + > + chan = of_alias_get_id(np, "pit"); > + if ((chan < 0) || (chan >= KINETIS_PIT_CHANNELS)) { Unnecessary parentheses, please remove them. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/