Hi,
On 2025. 03. 04. 8:47, William Breathitt Gray wrote:
It's cleaner to exit early on an error than to carry it to the end.
Instead of if (!ret), perform an if (ret) return ret to exit early on an
error, then simply return 0 at the end of the funtion.
Ok.
The capture2 extension doesn't exist in this patch so remove this
comment line.
@@ -30,6 +33,12 @@ enum counter_mchp_signals {
COUNTER_MCHP_SIG_TIOB,
};
+enum counter_mchp_capture_extensions {
+ COUNTER_MCHP_EXCAP_RA,
+ COUNTER_MCHP_EXCAP_RB,
+ COUNTER_MCHP_EXCAP_RC,
+};
Remove COUNTER_MCHP_EXCAP_RC for the same reason as above.
Also, I would argue for these to be preprocessor defines rather than
enum for the same reasons as in my other review[^1].
Ok.
One final comment: is RA/RB the best way to differentiate these? One of
the benefits of abstraction layers is that users won't need to be
concerned about the hardware details, and naming the capture values
after their respective general register hardware names feels somewhat
antithetic to that end.
I imagine there are better ways to refer to these that would communicate
their relationship better, such as "primary capture" and "secondary
capture". However at that point capture0 and capture1 would seem
obvious enough, in which case you might not even need to expose these to
userspace at all.
Hmm. Well, RA and RB is what it says in the datasheet, and since we
don't do much processing on their value, I'd say we're still closely
coupled to the hardware. So, if one wants to understand what they do,
they will have to read the datasheet anyways in which case I think it's
best to be consistent with it naming-wise.
William Breathitt Gray
[^1] https://lore.kernel.org/all/Z8alaOTjZeRuXnUI@ishi/
Bence
