On Mon, Oct 17, 2022 at 11:59:37AM +0200, Kamel Bouhara wrote:
> On Sat, Oct 15, 2022 at 09:52:27AM -0400, William Breathitt Gray wrote:
> > I was looking over the microchip-tcb-capture driver recently and noticed
> > that the code doesn't seem to account for Signal1. In particular, it
> > appears that mchp_tc_count_signal_read() and mchp_tc_count_action_read()
> > don't check the Signal id at all and just assume they are handling
> > Signal0. This creates a situation where the information returned for the
> > Signal1 sysfs attributes are just duplicated reports of Signal0.
> >
> > What exactly is the relationship of Signal0 ("Channel A") and Signal1
> > ("Channel B"); is SignalB only relevant when the counter device is
> > configured for quadrature mode?
>
> Indeed both signals are required when in quadrature mode, where the
> signal0 is representing the speed and signal1 the revolution or number
> of rotation.
>
> We have described all availables modes in details in the following blog post: https://bootlin.com/blog/timer-counters-linux-microchip/
>
> Regards,
> Kamel
Thank you for the link, the block diagram helps illustrate how the
signals correlate to the TCB channels.
Let me check if I understand correctly. In microchip-tcb-capture.c,
mchp_tc_count_signals[0] is TIOA0 while mchp_tc_count_signals[1] is
TIOB0? In quadrature mode, are TIOA and TIOB the two phases of a
quadrature encoder? You mentioned one signal is speed while the other is
the number of rotations; does this mean one signal serves as the
position incrementation from a rotary wheel while the other signal is
the index (z-phase) indicate for each full rotation?
In particular, I'm having trouble understanding
mchp_tc_count_signal_read(). I suspect it is unintentionally always
returning the signal status for TIOA::
regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr);
if (priv->trig_inverted)
sigstatus = (sr & ATMEL_TC_MTIOB);
else
sigstatus = (sr & ATMEL_TC_MTIOA);
*lvl = sigstatus ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW;
Here we read the status register for channel 0, select between TIOA and
TIOB based on priv->trig_inverted, and then return the signal level.
I don't see priv->trig_inverted referenced anywhere else so it appears
that priv->trig_inverted will always be 0, thus resulting in
mchp_tc_count_signal_read() always returning the TIOA status. I think
the intended behavior is to return the status of the selected signal::
if (signal->id == 1)
sigstatus = (sr & ATMEL_TC_MTIOB);
else
sigstatus = (sr & ATMEL_TC_MTIOA);
As for mchp_tc_count_action_read(), we have a similar problem: no
distinction is made for the Synapse requested. The channel mode register
for channel 0 is read and then masked against ATMEL_TC_ETRGEDG to
determine the action mode. It appears that this code is always assuming
the Synapse for TIOA is requested, but the Synapse for TIOB could be
passed. You can determine which corresponding Signal you have by
checking synapse->signal->id before deciding what action mode to return.
To clarify, in COUNTER_FUNCTION_INCREASE mode, does the Count value
increment based on the edge of TIOA and not TIOB? In
COUNTER_FUNCTION_QUADRATURE_X4 mode, does the Count value increment
based on both edges of TIOA and TIOB serving as quadrature encoding
phase A and B signals?
The fixes for this issue are trivial enough that I can submit a patch
for them later, but I want to make sure I'm understanding the nature of
these signals correctly before I do so.
Thanks,
William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
