The signal_read(), action_read(), and action_write() callbacks have been assuming Signal0 is requested without checking. This results in requests for Signal1 returning data for Signal0. This patch fixes these oversights by properly checking for the Signal's id in the respective callbacks and handling accordingly based on the particular Signal requested. The trig_inverted member of the mchp_tc_data is removed as superfluous.
Fixes: 106b104137fd ("counter: Add microchip TCB capture counter") Cc: stable@kernel.org Cc: Kamel Bouhara kamel.bouhara@bootlin.com Signed-off-by: William Breathitt Gray william.gray@linaro.org --- drivers/counter/microchip-tcb-capture.c | 57 ++++++++++++++++--------- 1 file changed, 38 insertions(+), 19 deletions(-)
diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c index f9dee15d9777..438f82b07a03 100644 --- a/drivers/counter/microchip-tcb-capture.c +++ b/drivers/counter/microchip-tcb-capture.c @@ -28,7 +28,6 @@ struct mchp_tc_data { int qdec_mode; int num_channels; int channel[2]; - bool trig_inverted; };
static const enum counter_function mchp_tc_count_functions[] = { @@ -153,7 +152,7 @@ static int mchp_tc_count_signal_read(struct counter_device *counter,
regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr);
- if (priv->trig_inverted) + if (signal->id == 1) sigstatus = (sr & ATMEL_TC_MTIOB); else sigstatus = (sr & ATMEL_TC_MTIOA); @@ -169,26 +168,46 @@ static int mchp_tc_count_action_read(struct counter_device *counter, enum counter_synapse_action *action) { struct mchp_tc_data *const priv = counter_priv(counter); + const unsigned int cmr_reg = ATMEL_TC_REG(priv->channel[0], CMR); + enum counter_function function; + int err; u32 cmr;
- regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); + err = mchp_tc_count_function_read(counter, count, &function); + if (err) + return err;
- switch (cmr & ATMEL_TC_ETRGEDG) { - default: - *action = COUNTER_SYNAPSE_ACTION_NONE; - break; - case ATMEL_TC_ETRGEDG_RISING: - *action = COUNTER_SYNAPSE_ACTION_RISING_EDGE; - break; - case ATMEL_TC_ETRGEDG_FALLING: - *action = COUNTER_SYNAPSE_ACTION_FALLING_EDGE; - break; - case ATMEL_TC_ETRGEDG_BOTH: + /* Default action mode */ + *action = COUNTER_SYNAPSE_ACTION_NONE; + + switch (function) { + case COUNTER_FUNCTION_INCREASE: + /* TIOB signal is ignored */ + if (synapse->signal->id == 1) + return 0; + + regmap_read(priv->regmap, cmr_reg, &cmr); + + switch (cmr & ATMEL_TC_ETRGEDG) { + case ATMEL_TC_ETRGEDG_RISING: + *action = COUNTER_SYNAPSE_ACTION_RISING_EDGE; + return 0; + case ATMEL_TC_ETRGEDG_FALLING: + *action = COUNTER_SYNAPSE_ACTION_FALLING_EDGE; + return 0; + case ATMEL_TC_ETRGEDG_BOTH: + *action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES; + return 0; + default: + return 0; + } + case COUNTER_FUNCTION_QUADRATURE_X4: *action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES; - break; + return 0; + default: + /* should never reach this path */ + return -EINVAL; } - - return 0; }
static int mchp_tc_count_action_write(struct counter_device *counter, @@ -199,8 +218,8 @@ static int mchp_tc_count_action_write(struct counter_device *counter, struct mchp_tc_data *const priv = counter_priv(counter); u32 edge = ATMEL_TC_ETRGEDG_NONE;
- /* QDEC mode is rising edge only */ - if (priv->qdec_mode) + /* QDEC mode is rising edge only; only TIOA handled in non-QDEC mode */ + if (priv->qdec_mode || synapse->signal->id != 0) return -EINVAL;
switch (action) {
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.' Subject: [PATCH] counter: microchip-tcb-capture: Handle Signal1 read and Synapse Link: https://lore.kernel.org/stable/20221017225404.67127-1-william.gray%40linaro....
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
On Mon, Oct 17, 2022 at 06:54:04PM -0400, William Breathitt Gray wrote:
The signal_read(), action_read(), and action_write() callbacks have been assuming Signal0 is requested without checking. This results in requests for Signal1 returning data for Signal0. This patch fixes these oversights by properly checking for the Signal's id in the respective callbacks and handling accordingly based on the particular Signal requested. The trig_inverted member of the mchp_tc_data is removed as superfluous.
Fixes: 106b104137fd ("counter: Add microchip TCB capture counter") Cc: stable@kernel.org Cc: Kamel Bouhara kamel.bouhara@bootlin.com Signed-off-by: William Breathitt Gray william.gray@linaro.org
Looks ok to me, thanks.
Reviewed-by: Kamel Bouhara kamel.bouhara@bootlin.com
drivers/counter/microchip-tcb-capture.c | 57 ++++++++++++++++--------- 1 file changed, 38 insertions(+), 19 deletions(-)
diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c index f9dee15d9777..438f82b07a03 100644 --- a/drivers/counter/microchip-tcb-capture.c +++ b/drivers/counter/microchip-tcb-capture.c @@ -28,7 +28,6 @@ struct mchp_tc_data { int qdec_mode; int num_channels; int channel[2];
- bool trig_inverted;
};
static const enum counter_function mchp_tc_count_functions[] = { @@ -153,7 +152,7 @@ static int mchp_tc_count_signal_read(struct counter_device *counter,
regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr);
- if (priv->trig_inverted)
- if (signal->id == 1) sigstatus = (sr & ATMEL_TC_MTIOB); else sigstatus = (sr & ATMEL_TC_MTIOA);
@@ -169,26 +168,46 @@ static int mchp_tc_count_action_read(struct counter_device *counter, enum counter_synapse_action *action) { struct mchp_tc_data *const priv = counter_priv(counter);
- const unsigned int cmr_reg = ATMEL_TC_REG(priv->channel[0], CMR);
- enum counter_function function;
- int err; u32 cmr;
- regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr);
- err = mchp_tc_count_function_read(counter, count, &function);
- if (err)
return err;
- switch (cmr & ATMEL_TC_ETRGEDG) {
- default:
*action = COUNTER_SYNAPSE_ACTION_NONE;break;- case ATMEL_TC_ETRGEDG_RISING:
*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;break;- case ATMEL_TC_ETRGEDG_FALLING:
*action = COUNTER_SYNAPSE_ACTION_FALLING_EDGE;break;- case ATMEL_TC_ETRGEDG_BOTH:
- /* Default action mode */
- *action = COUNTER_SYNAPSE_ACTION_NONE;
- switch (function) {
- case COUNTER_FUNCTION_INCREASE:
/* TIOB signal is ignored */if (synapse->signal->id == 1)return 0;regmap_read(priv->regmap, cmr_reg, &cmr);switch (cmr & ATMEL_TC_ETRGEDG) {case ATMEL_TC_ETRGEDG_RISING:*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;return 0;case ATMEL_TC_ETRGEDG_FALLING:*action = COUNTER_SYNAPSE_ACTION_FALLING_EDGE;return 0;case ATMEL_TC_ETRGEDG_BOTH:*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;return 0;default:return 0;}- case COUNTER_FUNCTION_QUADRATURE_X4: *action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
break;
return 0;- default:
/* should never reach this path */return -EINVAL;
- return 0;
}
static int mchp_tc_count_action_write(struct counter_device *counter, @@ -199,8 +218,8 @@ static int mchp_tc_count_action_write(struct counter_device *counter, struct mchp_tc_data *const priv = counter_priv(counter); u32 edge = ATMEL_TC_ETRGEDG_NONE;
- /* QDEC mode is rising edge only */
- if (priv->qdec_mode)
/* QDEC mode is rising edge only; only TIOA handled in non-QDEC mode */
if (priv->qdec_mode || synapse->signal->id != 0) return -EINVAL;
switch (action) {
-- 2.37.3
-- Kamel Bouhara, Bootlin Embedded Linux and kernel engineering https://bootlin.com
On Tue, Oct 18, 2022 at 10:13:51AM +0200, Kamel Bouhara wrote:
On Mon, Oct 17, 2022 at 06:54:04PM -0400, William Breathitt Gray wrote:
The signal_read(), action_read(), and action_write() callbacks have been assuming Signal0 is requested without checking. This results in requests for Signal1 returning data for Signal0. This patch fixes these oversights by properly checking for the Signal's id in the respective callbacks and handling accordingly based on the particular Signal requested. The trig_inverted member of the mchp_tc_data is removed as superfluous.
Fixes: 106b104137fd ("counter: Add microchip TCB capture counter") Cc: stable@kernel.org Cc: Kamel Bouhara kamel.bouhara@bootlin.com Signed-off-by: William Breathitt Gray william.gray@linaro.org
Looks ok to me, thanks.
Reviewed-by: Kamel Bouhara kamel.bouhara@bootlin.com
I'm going to submit a v2 of this patch for the sake of making it a tad easier to backport and also simpler to handle if a lock is introduced at a later point for this driver.
William Breathitt Gray
linux-stable-mirror@lists.linaro.org
-
Kamel Bouhara -
kernel test robot -
William Breathitt Gray