lkml.org 
[lkml]   [2023]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v1 09/43] clocksource: ep93xx: Add driver for Cirrus Logic EP93xx
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


\
 
 \ /
  Last update: 2023-06-03 22:09    [W:2.129 / U:1.660 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site