Re: Handling Signal1 in microchip-tcb-capture

[Kamel Bouhara <kamel.bouhara@bootlin.com>]

On Mon, Oct 17, 2022 at 08:53:18AM -0400, William Breathitt Gray wrote:
> 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?
>

IIRC this is indeed both signal edges (phase A and B) are accumulated on
channel 0 and channel 1 stores the revolution or number of rotation of
the qdec encoder.

> 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::

IIRC the trig_inverted shall be used when signals are inverted which
means we read position on TIOB and revolution on TIOA.

>
>     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.
>

That is indeed a good point as both signals are eligible to trigger the
TC for both modes (capture/qdec).

> To clarify, in COUNTER_FUNCTION_INCREASE mode, does the Count value
> increment based on the edge of TIOA and not TIOB? In

Yes, currently the driver only support TIOA.

> 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?

Yes as explained above.

>
> 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



--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com