Re: [PATCH v1 09/43] clocksource: ep93xx: Add driver for Cirrus Logic EP93xx
From: andy . shevchenko
Date: Sat Jun 03 2023 - 16:08:50 EST
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 <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/sched_clock.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
Can you keep that ordered?
Missing bits.h.
+ Blank line.
> +#include <asm/mach/time.h>
...
> +/* 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