| From | andy.shevchenko@gmail ... | Date | Sat, 3 Jun 2023 23:06:30 +0300 | Subject | Re: [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
|