Handling Signal1 in microchip-tcb-capture

Handling Signal1 in microchip-tcb-capture

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 640 bytes --]

Hello Kamel,

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?

Sincerely,

William Breathitt Gray

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Handling Signal1 in microchip-tcb-capture

[Kamel Bouhara]

On Sat, Oct 15, 2022 at 09:52:27AM -0400, William Breathitt Gray wrote:
> Hello Kamel,
>

Hello William,

> 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

>
> Sincerely,
>
> William Breathitt Gray



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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Handling Signal1 in microchip-tcb-capture

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 3714 bytes --]

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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Handling Signal1 in microchip-tcb-capture

[Kamel Bouhara]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Handling Signal1 in microchip-tcb-capture

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 5961 bytes --]

On Mon, Oct 17, 2022 at 07:06:26PM +0200, Kamel Bouhara wrote:
> 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.

Ah, I think I understand now: Signal0 and Signal1 are TIOA and TIOB
respectively; channel 0 and channel 1 are data registers; channel 0
holds the Count0 count value; channel 1 holds the revolution value (but
the microchip-tcb-capture driver does not expose it).

It might be nice to expose the channel 1 revolution value as Count1 at
some point in the future. However, channel 1 seems unrelated to the
current issue we're dicussing so we can avoid it for now.

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

Sure, that seems like a reasonable option to expose, but it does not
appear that trig_inverted is being set or otherwise configured in the
current code, unless I'm missing something. It might be best to remove
trig_inverted if the functionality is not supported yet by this driver.

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

Okay this should be simple to resolve then: return -EINVAL if Synapse is
for TIOB in mchp_tc_count_action_write(), and pass back
COUNTER_SYNAPSE_ACTION_NONE for TIOB during non-quadrature mode in
mchp_tc_counter_action_read().

I'll submit a patch fixing these changes and the signal_read() callback
mentioned previously.

By the way, I suspect there are race conditions present in
mcho_tc_count_function_write() that could be resolved by adding a lock
to the mchp_tc_data structure and acquiring it before accessing the
device state and registers. It's unrelated to the Signal1 issues so I
haven't looked any further into it, but it's something you might want to
investigate to make sure you don't get weird behavior from the driver.

Thanks,

William Breathitt Gray

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Handling Signal1 in microchip-tcb-capture

[Kamel Bouhara]

On Mon, Oct 17, 2022 at 05:57:59PM -0400, William Breathitt Gray wrote:
> On Mon, Oct 17, 2022 at 07:06:26PM +0200, Kamel Bouhara wrote:
> > 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.
>
> Ah, I think I understand now: Signal0 and Signal1 are TIOA and TIOB
> respectively; channel 0 and channel 1 are data registers; channel 0
> holds the Count0 count value; channel 1 holds the revolution value (but
> the microchip-tcb-capture driver does not expose it).

Exact, I assumed that in qdec mode only the position value is relevant.

>
> It might be nice to expose the channel 1 revolution value as Count1 at
> some point in the future. However, channel 1 seems unrelated to the
> current issue we're dicussing so we can avoid it for now.

OK do you suggest to read both count values in the same mchp_tc_count_read() ?

>
> > > 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.
>
> Sure, that seems like a reasonable option to expose, but it does not
> appear that trig_inverted is being set or otherwise configured in the
> current code, unless I'm missing something. It might be best to remove
> trig_inverted if the functionality is not supported yet by this driver.
>

OK.

> > >
> > >     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.
>
> Okay this should be simple to resolve then: return -EINVAL if Synapse is
> for TIOB in mchp_tc_count_action_write(), and pass back
> COUNTER_SYNAPSE_ACTION_NONE for TIOB during non-quadrature mode in
> mchp_tc_counter_action_read().
>
> I'll submit a patch fixing these changes and the signal_read() callback
> mentioned previously.
>

That's clear, thanks.

> By the way, I suspect there are race conditions present in
> mcho_tc_count_function_write() that could be resolved by adding a lock
> to the mchp_tc_data structure and acquiring it before accessing the
> device state and registers. It's unrelated to the Signal1 issues so I
> haven't looked any further into it, but it's something you might want to
> investigate to make sure you don't get weird behavior from the driver.
>

You mean between function_write() and action_write() ?

> Thanks,
>
> William Breathitt Gray
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Handling Signal1 in microchip-tcb-capture

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 1517 bytes --]

On Tue, Oct 18, 2022 at 10:01:49AM +0200, Kamel Bouhara wrote:
> On Mon, Oct 17, 2022 at 05:57:59PM -0400, William Breathitt Gray wrote:
> > It might be nice to expose the channel 1 revolution value as Count1 at
> > some point in the future. However, channel 1 seems unrelated to the
> > current issue we're dicussing so we can avoid it for now.
> 
> OK do you suggest to read both count values in the same mchp_tc_count_read() ?

Yes, you can differentiate between the requested Count by checking
count->id. So for id 0 return the channel 0 value, and for id 1 return
the channel 1 value.

> > By the way, I suspect there are race conditions present in
> > mcho_tc_count_function_write() that could be resolved by adding a lock
> > to the mchp_tc_data structure and acquiring it before accessing the
> > device state and registers. It's unrelated to the Signal1 issues so I
> > haven't looked any further into it, but it's something you might want to
> > investigate to make sure you don't get weird behavior from the driver.
> >
> 
> You mean between function_write() and action_write() ?

Those are the callbacks that stood out immediately. For example, it's
possible that these functions are called concurrently, resulting in a
race where qdec_mode is initially evaluated as true but changes to false
before action_write() completes; or similarly, the regmap operations in
function_write() could be clobbered by if another call is happening at
the same time.

William Breathitt Gray

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel