Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750926AbbFXFLf (ORCPT ); Wed, 24 Jun 2015 01:11:35 -0400 Received: from fish.king.net.pl ([79.190.246.46]:37264 "EHLO king.net.pl" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750764AbbFXFL0 (ORCPT ); Wed, 24 Jun 2015 01:11:26 -0400 Date: Wed, 24 Jun 2015 07:09:06 +0200 (CEST) From: Paul Osmialowski X-X-Sender: newchief@localhost.localdomain To: Stephen Boyd cc: 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, Yuri Tikhonov , Sergei Poselenov , Dmitry Cherkassov , Alexander Potashev Subject: Re: [PATCH 6/9] arm: twr-k70f120m: clock source drivers for Kinetis SoC In-Reply-To: <5589DCB9.2000908@codeaurora.org> Message-ID: References: <1435094387-20146-1-git-send-email-pawelo@king.net.pl> <1435094387-20146-7-git-send-email-pawelo@king.net.pl> <5589DCB9.2000908@codeaurora.org> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6300 Lines: 221 Hi Stephen, Thanks for the valuable input - all of those points are now on my checklist for the work on the second iteration of this patchset. On Tue, 23 Jun 2015, Stephen Boyd wrote: > 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/