On Fri, Aug 23, 2024 at 12:31:06PM +0530, subramanian.mohan@xxxxxxxxx wrote:
> From: Subramanian Mohan <subramanian.mohan@xxxxxxxxx>
>
> The Intel Timed IO PPS generator driver outputs a PPS signal using
> dedicated hardware that is more accurate than software actuated PPS.
> The Timed IO hardware generates output events using the ART timer.
> The ART timer period varies based on platform type, but is less than 100
> nanoseconds for all current platforms. Timed IO output accuracy is
> within 1 ART period.
>
> PPS output is enabled by writing '1' the 'enable' sysfs attribute. The
> driver uses hrtimers to schedule a wake-up 10 ms before each event
> (edge) target time. At wakeup, the driver converts the target time in
> terms of CLOCK_REALTIME to ART trigger time and writes this to the Timed
> IO hardware. The Timed IO hardware generates an event precisely at the
> requested system time without software involvement.
...
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
These two should be swapped in ordering
> +#include <linux/cleanup.h>
> +#include <linux/container_of.h>
> +#include <linux/cpu.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hrtimer.h>
> +#include <linux/io-64-nonatomic-hi-lo.h>
> +#include <linux/kstrtox.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/sysfs.h>
> +#include <linux/timekeeping.h>
> +#include <linux/types.h>
...
> +#define SAFE_TIME_NS (10 * NSEC_PER_MSEC) /* Safety time to set hrtimer early */
It's better to have
/* ...comment... */
...definition...
...
> +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
> +{
> + struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
> + ktime_t expires, now;
> + u32 event_count;
> +
> + guard(spinlock)(&tio->lock);
> +
> + /* Check if any event is missed. If an event is missed, TIO will be disabled*/
/*
* Missing space at the of the comment. But since it's two-sentences comment,
* make it multi-line like in this example.
*/
> + event_count = pps_tio_read(tio, TIOEC);
> + if (tio->prev_count && tio->prev_count == event_count)
> + goto err;
> + tio->prev_count = event_count;
+ Blank line.
> + expires = hrtimer_get_expires(timer);
(1)
> + now = ktime_get_real();
> +
The location of this blank line seems incorrect and should be moved to (1) above.
> + if (now - expires >= SAFE_TIME_NS)
> + goto err;
> +
> + tio->enabled = pps_generate_next_pulse(tio, expires + SAFE_TIME_NS);
> + if (!tio->enabled)
> + return HRTIMER_NORESTART;
> +
> + hrtimer_forward(timer, now, NSEC_PER_SEC / 2);
> + return HRTIMER_RESTART;
+ Blank line.
> +err:
> + dev_err(tio->dev, "Event missed, Disabling Timed I/O");
> + pps_tio_disable(tio);
> + return HRTIMER_NORESTART;
> +}
...
Note, the above are nit-picks and may be applied if you ever need a v13 to be
sent. For now let's wait for the more serious comments against the series.
--
With Best Regards,
Andy Shevchenko
