Hi,
On 2025. 02. 24. 4:07, William Breathitt Gray wrote:
On Fri, Feb 21, 2025 at 03:14:44PM +0100, Csókás Bence wrote:
On 2025. 02. 21. 13:39, William Breathitt Gray wrote:
First, register RC seems to serve only as a threshold value for a
compare operation. So it shouldn't be exposed as "capture2", but rather
as its own dedicated threshold component. I think the 104-quad-8 module
is the only other driver supporting THRESHOLD events; it exposes the
threshold value configuration via the "preset" component, but perhaps we
should introduce a proper "threshold" component instead so counter
drivers have a standard way to expose this functionality. What do you
think?
Possibly. What's the semantics of the `preset` component BTW? If we can
re-use that here as well, that could work too.
You can find the semantics of each attribute under the sysfs ABI doc
file located at Documentation/ABI/testing/sysfs-bus-counter. For the
`preset` component, its essential purpose is to configure a value to
preset register to reload the Count when some condition is met (e.g.
when an external INDEX/SYNC trigger line goes high).
Hmm, that doesn't really match this use case. All right, then, for now,
I'll skip the RC part, and then we can add it in a later patch when the
"threshold" component is in place and used by the 104-quad-8 module.
In the same vein, move the uapi header introduction to its own patch.
That will separate the userspace-exposed changes and make things easier
for future users when bisecting the linux kernel history when tracking
down possible bugs.
Isn't it better to keep API header changes in the same commit as the
implementation using them? That way if someone bisects/blames the API
header, they get the respective implementation as well.
Regarding future additions, I took a look at the Microchip SAMA5D2
datasheet[^1] (is the right document?)
There are many versions, but yes, it is one of them, and is usable.
and it looks like this chip has
three timer counter channels described in section 54. Currently, the
microchip-tcb-capture module is exposing only one timer counter channel
(as Count0), correct? Should this driver expose all three channels (as
Count0, Count1, and Count2)?
No, as this device is actually instantiated per-channel, i.e. in the DT,
there are two TCB nodes (as the SoC has two peripherals, each with 3
channels), and then the counter is a sub-node with `reg = <0/1/2>`,
specifying which timer channel to use. Or, in quadrature decode mode,
you'd have two elements in `reg`, i.e. `reg = <0>, <1>`.
The `mchp_tc_count_function_write()` function already disables PWM mode by
clearing the `ATMEL_TC_WAVE` bit from the Channel Mode Register (CMR).
So capture mode is unconditionally set by mchp_tc_count_function_write()
which means the first time the user sets the Count function then PWM
mode will be disabled. However, what happens if the user does not set
the Count function? Should PWM mode be disabled by default in
mchp_tc_probe(), or does that already happen?
You're right, and it is a problem I encounter regularly: almost all HW
initialization happens in `mchp_tc_count_function_write()`, the probe()
function mostly just allocates stuff. Meaning, if you want to do
anything with the counter, you have to set the "increase" function first
(even though, if you `cat function`, it will seem like it's already in
"increase" mode). I don't know if it was deliberate, or what, but again,
that would be a separate bugfix patch.
Bence
