Re: [PATCH v12 1/3] pps: generators: Add PPS Generator TIO Driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Aug 23, 2024 at 12:31:06PM +0530, subramanian.mohan@intel.com wrote:
> From: Subramanian Mohan <subramanian.mohan@intel.com>
> 
> 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