Re: [PATCH v3 2/3] counter: add GPIO-based quadrature encoder driver

[Wadim Mueller]

Hi William,

thanks a lot for the very thorough review -- this was really
helpful. I went through all your points and addressed them in v4
which i will send shortly as a follow-up to this thread (in-reply-to
the v3 cover).

A short rundown so you can already see what to expect:

  - gpio_qenc_function enum dropped, priv stores enum counter_function
    directly, function_read/write are now trivial accessors.
  - priv->count is now u64 and ceiling defaults to U64_MAX in probe,
    so ceiling == 0 is a real "ceiling of 0" again, not a sentinel.
  - update_count rewritten direction-first, with proper saturation
    at 0 / ceiling, +/-1 per transition.
  - CREATE_QE_STATE(prev_a, prev_b, curr_a, curr_b) macro plus a
    single 16-entry X4 transition table; the delta == 2 path is gone,
    the table just has 0 in those slots.
  - ISR switches use default: instead of listing every ignored case.
  - The "enabled" flag in the ISRs is dropped. Gating happens via
    enable_irq/disable_irq anyway and enable_read derives the state
    from irqd_irq_disabled(irq_get_irq_data(priv->irq_a)) so there
    is only one source of truth.
  - enable_write simplified: no !!, assign directly, two branches
    split by return.
  - events_configure() removed (was no-op).
  - Synapses: A and B keep the full action list including
    FALLING_EDGE; Index gets its own list with only NONE and
    RISING_EDGE.
  - action_read restructured: default to NONE, Index handled as
    early return, per-function switch. X1_A and X1_B now report
    RISING_EDGE when going forward and FALLING_EDGE when going
    backward, as the COUNTER_FUNCTION_QUADRATURE_X1_{A,B} semantics
    say.
  - All generic Count functions supported now: INCREASE, DECREASE,
    PULSE_DIRECTION, QUADRATURE_X1_{A,B}, QUADRATURE_X2_{A,B},
    QUADRATURE_X4.
  - Index migrated to the COUNTER_COMP_PRESET + PRESET_ENABLE pair,
    the custom "index_enabled" COUNTER_COMP_COUNT_BOOL is gone. The
    Index ISR loads preset into count when preset_enable is set,
    like intel-qep does.
  - signal_read: the !gpio guard is gone; the Index synapse is only
    registered in probe when an index GPIO is actually wired, so no
    zombie entries.
  - cnts is now a single-element array (priv->cnts[1]) and
    counter->num_counts uses ARRAY_SIZE(priv->cnts). To not trigger
    checkpatch's "deprecated flexible array" warning the array is
    not the last field of the priv struct anymore.
  - The Count is renamed from "Position" to "Count".

About the COMPARE/FLOOR components and the OVERFLOW/UNDERFLOW/
THRESHOLD/DIRECTION_CHANGE events you suggested: i would prefer to
send those as a separate series on top of this one, once it lands,
so the diff size for the initial driver stays in a reviewable shape.
I will announce that as a follow-up in the v4 cover letter.

While at it i also picked up Krzysztofs note from v2 on the const
scalar parameters -- v3 only had the Acked-by from Conor on the
binding and no code changes, so the const scalars were still in v3.
They are gone in v4.

If anything from above is not what you had in mind please tell me,
i will fix it in v5.

Thanks again for the time,

Wadim