Re: [PATCH net-next v3 1/3] dpll: zl3073x: add channel ToD, phase step and TIE operations

[Ivan Vecera <ivecera@redhat.com>]

Replies for Sashiko findings:

 > ZL_REG_OUTPUT_STEP_TIME_MASK looks device-global rather than per-channel
 > ...
 > Would struct zl3073x_dev (next to clock_id, phase_avg_factor, 
freq_monitor)
 > or struct zl3073x_out be a better home for it?
 >
 > Related to that, zl3073x_chan_is_out_stepped(chan, out) takes a channel
 > argument but returns the same answer for every channel.

Good catch. The register is indeed device-global. Will move it to
struct zl3073x_dev, read it once during device init and change the
helper to zl3073x_dev_is_out_stepped(zldev, out).

 > Can the diff.tv_sec < 0 path here end up writing the ToD with less than
 > the documented 20 ms of margin?
 > ...
 > For diff.tv_sec <= -2 ... the single ts_next.tv_sec++ leaves the target
 > seconds value in the past

The two tod_read() calls are back-to-back regmap transactions,
microseconds to a few milliseconds apart on I2C/SPI. A tick crossing
between them is rare and leaves nearly 1 second of margin after the
increment. The diff.tv_sec <= -2 case requires >1 second of preemption
between two regmap reads which is not realistic.

That said, the code can be tightened to handle both cases correctly:

	if (!diff.tv_sec && diff.tv_nsec < threshold_ns) {
		fsleep((unsigned long)diff.tv_nsec / NSEC_PER_USEC + 1);
		ts_next.tv_sec++;
	} else if (diff.tv_sec < 0) {
		ts_next.tv_sec -= diff.tv_sec;
	}

The first branch handles the normal close-to-rollover case with a
sleep. The second branch normalizes by the exact number of elapsed
ticks, which covers both single and multi-tick crossings.

 > Is reporting a failure for an already committed write intended?
 > ...
 > If a servo retries the same ADJ_SETOFFSET delta after that error,
 > wouldn't the delta be applied twice?

Good point. Once WR_NEXT_1HZ is latched the write is committed and
reporting an error risks double-application. Will drop the trailing
tod_ready_wait() from tod_adjust() - every subsequent tod_* call
starts with its own ready_wait anyway.

 > Why is the item count here 8 rather than ZL3073X_MAX_CHANNELS?

Should be ZL3073X_MAX_CHANNELS for consistency with the other
per-channel page 5 registers. Will fix.

 > regs.h is otherwise one section per hardware page in ascending page
 > order ... Could the phase step defines go into the existing
 > "Register Page 9, Synth and Output" section instead?

Agreed, will merge into the existing page 9 section.

 > The value column of the new ToD command defines does not line up with
 > its neighbours

Will align.

 > the kernel-doc of zl3073x_chan_state_fetch() ... no longer mentions the
 > added ZL_REG_OUTPUT_STEP_TIME_MASK read.

Will update the kernel-doc.

Ivan