Re: [PATCH net-next v8 05/15] dpll: sit9531x: register DPLL devices and pins

[Ali Rouhi]

Replies inline.

> The register carries two independent bits, SE_P_EN and SE_N_EN, but this
> folds them into one pair-wide value and writes it to both lanes. [...] Does
> this publish a pin for a lane whose receiver the loaded configuration never
> enabled [...]?

The fold is intentional for classification: sig_mode only answers
"is this pair wired differential or single-ended", which is a pair-level board
fact -- a DE pair must hide its N lane, an SE pair exposes both lanes as
independently selectable hardware.  A lane whose receiver the profile left
disabled is still real, connectable hardware; in v9 its reported state derives
from priority-table membership, so it shows DISCONNECTED until userspace
connects it, and connecting it powers the receiver -- which is exactly the
operation the pin exists for.  Whether the profile pre-enabled the lane does
not change what the board supports.

> Can this branch ever run at this commit?  Nothing in the driver reads
> "sitime,output-pll-map" or "sitime,pll-fvco" yet [...] would it be better to
> keep the binding and these fields in the same patch as the parser?

The bindings land first by convention, so the compatible string never
precedes its schema; the parser lands with the feature patch at the end.  Both
properties are optional, so a board using them just gets the pre-override
behaviour at the intermediate commits -- the same behaviour it would get from
a kernel one release older.  We kept the struct fields with the state-caching
patch so the caching code compiles as one piece.

> What stops this tick when the parent I2C adapter suspends? [...] The driver
> still has no .pm member at the end of the series.

Fixed in v9: the driver now has DEFINE_SIMPLE_DEV_PM_OPS wired through
pm_sleep_ptr(); suspend parks the poll worker and disables the IRQ, resume
re-enables them, so no paged I2C sequence can tear across suspend.

> When the acknowledge write itself fails, this still returns IRQ_HANDLED.

Fixed in v9: the handler returns IRQ_NONE when the notification
latches cannot be acknowledged, so a persistently failing ack engages the
genirq spurious detector instead of re-entering back to back.

> Should the zero-CRC case be treated as "no EEPROM" instead of a warning?

Yes -- fixed in v9: a 0/0 CRC pair is reported as "no EEPROM profile"
at dev_dbg level, and the v9 changelog mentions the probe-time reporting.

> Could the changelog mention these two [INTSYNC] pins, since they are visible
> over netlink?

Yes -- the v9 changelog enumerates the INTSYNC source and destination
pins along with the inputs, the crystal and the routed outputs.

> Do two same-variant parts at the same address on different I2C busses (or
> behind a mux) end up sharing a clock_id?

They did -- fixed in v9: the I2C adapter number is mixed into the
clock_id, so same-variant chips at one address on different busses keep
distinct DPLL objects.

> [prio_add] When the table already carries eleven distinct sources, the
> source sitting in slot 10 is silently ejected [...] What should the twelfth
> request do?  Returning -ENOSPC and leaving the table alone looks like the
> honest answer [...]

Fixed in v9: sit9531x_input_prio_add() returns -ENOSPC when
the table is full and leaves it untouched, so pll_mask and the table stay in
step by construction.

> roughly fifteen functions declared in this hunk have no definition anywhere
> in the module at this commit [...] could the declarations move to the
> patches that define them?

The module builds and links at every commit -- nothing calls a
declared-but-undefined helper, so there is no undefined reference at any
bisect point.  Moving each prototype to its defining patch would churn core.h
in nearly every patch of the series for no functional gain; we prefer to keep
the core API surface readable in one hunk.

> If the small-update write fails here, the OUTER_DIS bit cleared by the
> preceding sit9531x_update_pll_u8() is neither committed nor restored.
> Should this path put the bit back?

Yes -- v9 restores the bit on that error path
so the cached mode cannot drift from the hardware.

> Does probe succeed with this table?  It has no .state_on_dpll_get, and it
> sets .supported_ffo without an .ffo_get, both of which the core rejects [...]
> Could the callbacks land in the same patch that registers the pins?

Correct: at this commit dpll_pin_register() fails for the first non-XO
pin, so the
intermediate commits do not probe.  v9 restructures the registration so that
each pin class is registered in the patch that implements its state op (the
crystal here, inputs with the input-state patch, outputs with the output-state
patch, the INTSYNC pair with the inter-PLL sync patch), and .supported_ffo
moves to the patch that adds .ffo_get.  Every commit of the series then both
builds and probes.

> Same missing callback here: [...] would this call dereference NULL if a pin
> ever reached this loop?

Resolved by the same v9 restructure: no pin is
registered before its ops table carries .state_on_dpll_get, so the poller's
dispatch cannot reach a NULL member.