From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2D683C7EE24 for ; Sat, 3 Jun 2023 20:08:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231269AbjFCUIq (ORCPT ); Sat, 3 Jun 2023 16:08:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51530 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230521AbjFCUID (ORCPT ); Sat, 3 Jun 2023 16:08:03 -0400 Received: from fgw22-7.mail.saunalahti.fi (fgw22-7.mail.saunalahti.fi [62.142.5.83]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E2582E4C for ; Sat, 3 Jun 2023 13:07:34 -0700 (PDT) Received: from localhost (88-113-26-95.elisa-laajakaista.fi [88.113.26.95]) by fgw22.mail.saunalahti.fi (Halon) with ESMTP id 2144f3fc-024a-11ee-a9de-005056bdf889; Sat, 03 Jun 2023 23:06:30 +0300 (EEST) From: andy.shevchenko@gmail.com Date: Sat, 3 Jun 2023 23:06:30 +0300 To: Nikita Shubin Cc: Alexander Sverdlin , Arnd Bergmann , Linus Walleij , Daniel Lezcano , Thomas Gleixner , Michael Peters , Kris Bahnsen , linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 09/43] clocksource: ep93xx: Add driver for Cirrus Logic EP93xx Message-ID: References: <20230424123522.18302-1-nikita.shubin@maquefel.me> <20230601053546.9574-10-nikita.shubin@maquefel.me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230601053546.9574-10-nikita.shubin@maquefel.me> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thu, Jun 01, 2023 at 08:34:00AM +0300, Nikita Shubin kirjoitti: > This us a rewrite of EP93xx timer driver in > arch/arm/mach-ep93xx/timer-ep93xx.c trying to do everything > the device tree way: > > - Make every IO-access relative to a base address and dynamic > so we can do a dynamic ioremap and get going. > - Find register range and interrupt from the device tree. ... > +config EP93XX_TIMER > + bool "Cirrus Logic ep93xx timer driver" if COMPILE_TEST This is strange. What do you gain with this "if COMPILE_TEST"? > + depends on ARCH_EP93XX > + depends on GENERIC_CLOCKEVENTS > + depends on HAS_IOMEM > + select CLKSRC_MMIO > + select TIMER_OF ... > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Can you keep that ordered? Missing bits.h. + Blank line. > +#include ... > +/* This read-only register contains the low word of the time stamp debug timer > + * ( Timer4). When this register is read, the high byte of the Timer4 counter is > + * saved in the Timer4ValueHigh register. > + */ /* * Wrong multi-line comment style. * Use this example, for example. */ ... > +static struct ep93xx_tcu *ep93xx_tcu; Global?! Can it be derived from struct clocksource? ... > +static u64 ep93xx_clocksource_read(struct clocksource *c) > +{ > + struct ep93xx_tcu *tcu = ep93xx_tcu; > + u64 ret; > + > + ret = readl(tcu->base + EP93XX_TIMER4_VALUE_LOW); > + ret |= ((u64) (readl(tcu->base + EP93XX_TIMER4_VALUE_HIGH) & 0xff) << 32); GENMASK() Why you are not using non-atomic 64-bit io accessors? Becomes as simple as return lo_hi_readq() & GENMASK(); > + return (u64) ret; Redundant casting. > +} ... > + irq = irq_of_parse_and_map(np, 0); > + if (irq <= 0) { > + pr_err("ERROR: invalid interrupt number\n"); > + ret = -EINVAL; Shadowed error in case of negative returned code. Why? > + goto out_free; > + } ... > + clockevents_config_and_register(&ep93xx_clockevent, > + EP93XX_TIMER123_RATE, > + 1, > + 0xffffffffU); UINT_MAX? GENMASK() ? ... > + Redundant blank line. > +TIMER_OF_DECLARE(ep93xx_timer, "cirrus,ep9301-timer", ep93xx_timer_of_init); -- With Best Regards, Andy Shevchenko