| Message ID | 58e7c59bb7c7bb94c8655903308842d9d9e9907a.1613131238.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
On Fri, 12 Feb 2021 21:13:33 +0900 William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > Only a select set of modes (function, action, etc.) are valid for a > given device configuration. This patch ensures that invalid modes result > in a return -EINVAL. Such a situation should never occur in reality, but > it's good to define a default switch cases for the sake of making the > intent of the code clear. In many of these cases it may make sense to also return early in the good paths rather than share a return 0 at the end of the function? > > Cc: Syed Nayyar Waris <syednwaris@gmail.com> > Cc: Kamel Bouhara <kamel.bouhara@bootlin.com> > Cc: Fabrice Gasnier <fabrice.gasnier@st.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > Cc: David Lechner <david@lechnology.com> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > --- > drivers/counter/104-quad-8.c | 10 ++++++++++ > drivers/counter/microchip-tcb-capture.c | 6 ++++++ > drivers/counter/stm32-lptimer-cnt.c | 10 ++++++---- > drivers/counter/stm32-timer-cnt.c | 3 +++ > drivers/counter/ti-eqep.c | 3 +++ > 5 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c > index 9a96296b0625..674263b4d2c4 100644 > --- a/drivers/counter/104-quad-8.c > +++ b/drivers/counter/104-quad-8.c > @@ -273,6 +273,10 @@ static int quad8_function_set(struct counter_device *counter, > *scale = 2; > mode_cfg |= QUAD8_CMR_QUADRATURE_X4; > break; > + default: > + /* should never reach this path */ > + mutex_unlock(&priv->lock); > + return -EINVAL; > } > } > > @@ -367,6 +371,9 @@ static int quad8_action_get(struct counter_device *counter, > case QUAD8_COUNT_FUNCTION_QUADRATURE_X4: > *action = QUAD8_SYNAPSE_ACTION_BOTH_EDGES; > break; > + default: > + /* should never reach this path */ > + return -EINVAL; > } > > return 0; > @@ -529,6 +536,9 @@ static int quad8_count_mode_set(struct counter_device *counter, > case COUNTER_COUNT_MODE_MODULO_N: > cnt_mode = 3; > break; > + default: > + /* should never reach this path */ > + return -EINVAL; > } > > mutex_lock(&priv->lock); > diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c > index 710acc0a3704..ee979b011012 100644 > --- a/drivers/counter/microchip-tcb-capture.c > +++ b/drivers/counter/microchip-tcb-capture.c > @@ -133,6 +133,9 @@ static int mchp_tc_count_function_set(struct counter_device *counter, > bmr |= ATMEL_TC_QDEN | ATMEL_TC_POSEN; > cmr |= ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_ABETRG | ATMEL_TC_XC0; > break; > + default: > + /* should never reach this path */ > + return -EINVAL; > } > > regmap_write(priv->regmap, ATMEL_TC_BMR, bmr); > @@ -226,6 +229,9 @@ static int mchp_tc_count_action_set(struct counter_device *counter, > case MCHP_TC_SYNAPSE_ACTION_BOTH_EDGE: > edge = ATMEL_TC_ETRGEDG_BOTH; > break; > + default: > + /* should never reach this path */ > + return -EINVAL; > } > > return regmap_write_bits(priv->regmap, > diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c > index 937439635d53..daf988e7b208 100644 > --- a/drivers/counter/stm32-lptimer-cnt.c > +++ b/drivers/counter/stm32-lptimer-cnt.c > @@ -206,9 +206,10 @@ static int stm32_lptim_cnt_function_set(struct counter_device *counter, > priv->quadrature_mode = 1; > priv->polarity = STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES; > return 0; > + default: > + /* should never reach this path */ > + return -EINVAL; > } > - > - return -EINVAL; > } > > static ssize_t stm32_lptim_cnt_enable_read(struct counter_device *counter, > @@ -326,9 +327,10 @@ static int stm32_lptim_cnt_action_get(struct counter_device *counter, > case STM32_LPTIM_ENCODER_BOTH_EDGE: > *action = priv->polarity; > return 0; > + default: > + /* should never reach this path */ > + return -EINVAL; > } > - > - return -EINVAL; > } > > static int stm32_lptim_cnt_action_set(struct counter_device *counter, > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c > index ef2a974a2f10..431a3d08ed6c 100644 > --- a/drivers/counter/stm32-timer-cnt.c > +++ b/drivers/counter/stm32-timer-cnt.c > @@ -296,6 +296,9 @@ static int stm32_action_get(struct counter_device *counter, > /* counts up/down on both TI1FP1 and TI2FP2 edges */ > *action = STM32_SYNAPSE_ACTION_BOTH_EDGES; > break; > + default: > + /* should never reach this path */ > + return -EINVAL; > } > > return 0; > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index a60aee1a1a29..7844fdf78a97 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -192,6 +192,9 @@ static int ti_eqep_action_get(struct counter_device *counter, > break; > } > break; > + default: > + /* should never reach this path */ > + return -EINVAL; > } > > return 0;
On 2/12/21 6:13 AM, William Breathitt Gray wrote: > Only a select set of modes (function, action, etc.) are valid for a > given device configuration. This patch ensures that invalid modes result > in a return -EINVAL. Such a situation should never occur in reality, but > it's good to define a default switch cases for the sake of making the > intent of the code clear. > > Cc: Syed Nayyar Waris <syednwaris@gmail.com> > Cc: Kamel Bouhara <kamel.bouhara@bootlin.com> > Cc: Fabrice Gasnier <fabrice.gasnier@st.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > Cc: David Lechner <david@lechnology.com> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > --- Reviewed-by: David Lechner <david@lechnology.com> (In response to Jonathan's comment, I think this is fine rather than adding more churn to change all of the breaks to returns - but will keep that in mind for future changes.)
On Sat, Feb 20, 2021 at 10:43:06AM -0600, David Lechner wrote: > On 2/12/21 6:13 AM, William Breathitt Gray wrote: > > Only a select set of modes (function, action, etc.) are valid for a > > given device configuration. This patch ensures that invalid modes result > > in a return -EINVAL. Such a situation should never occur in reality, but > > it's good to define a default switch cases for the sake of making the > > intent of the code clear. > > > > Cc: Syed Nayyar Waris <syednwaris@gmail.com> > > Cc: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Cc: Fabrice Gasnier <fabrice.gasnier@st.com> > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > Cc: Alexandre Torgue <alexandre.torgue@st.com> > > Cc: David Lechner <david@lechnology.com> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > --- > > Reviewed-by: David Lechner <david@lechnology.com> > > (In response to Jonathan's comment, I think this is fine rather than > adding more churn to change all of the breaks to returns - but will > keep that in mind for future changes.) Due to some other updates I'm making to this patchset, I went ahead already and updated the breaks to returns in the few places where applicable. The changes to this patch are minor, but being pedantic I'll hold off on adding your Reviewed-by line until the next revision so you have the opportunity to formally approve it. William Breathitt Gray
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c index 9a96296b0625..674263b4d2c4 100644 --- a/drivers/counter/104-quad-8.c +++ b/drivers/counter/104-quad-8.c @@ -273,6 +273,10 @@ static int quad8_function_set(struct counter_device *counter, *scale = 2; mode_cfg |= QUAD8_CMR_QUADRATURE_X4; break; + default: + /* should never reach this path */ + mutex_unlock(&priv->lock); + return -EINVAL; } } @@ -367,6 +371,9 @@ static int quad8_action_get(struct counter_device *counter, case QUAD8_COUNT_FUNCTION_QUADRATURE_X4: *action = QUAD8_SYNAPSE_ACTION_BOTH_EDGES; break; + default: + /* should never reach this path */ + return -EINVAL; } return 0; @@ -529,6 +536,9 @@ static int quad8_count_mode_set(struct counter_device *counter, case COUNTER_COUNT_MODE_MODULO_N: cnt_mode = 3; break; + default: + /* should never reach this path */ + return -EINVAL; } mutex_lock(&priv->lock); diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c index 710acc0a3704..ee979b011012 100644 --- a/drivers/counter/microchip-tcb-capture.c +++ b/drivers/counter/microchip-tcb-capture.c @@ -133,6 +133,9 @@ static int mchp_tc_count_function_set(struct counter_device *counter, bmr |= ATMEL_TC_QDEN | ATMEL_TC_POSEN; cmr |= ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_ABETRG | ATMEL_TC_XC0; break; + default: + /* should never reach this path */ + return -EINVAL; } regmap_write(priv->regmap, ATMEL_TC_BMR, bmr); @@ -226,6 +229,9 @@ static int mchp_tc_count_action_set(struct counter_device *counter, case MCHP_TC_SYNAPSE_ACTION_BOTH_EDGE: edge = ATMEL_TC_ETRGEDG_BOTH; break; + default: + /* should never reach this path */ + return -EINVAL; } return regmap_write_bits(priv->regmap, diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c index 937439635d53..daf988e7b208 100644 --- a/drivers/counter/stm32-lptimer-cnt.c +++ b/drivers/counter/stm32-lptimer-cnt.c @@ -206,9 +206,10 @@ static int stm32_lptim_cnt_function_set(struct counter_device *counter, priv->quadrature_mode = 1; priv->polarity = STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES; return 0; + default: + /* should never reach this path */ + return -EINVAL; } - - return -EINVAL; } static ssize_t stm32_lptim_cnt_enable_read(struct counter_device *counter, @@ -326,9 +327,10 @@ static int stm32_lptim_cnt_action_get(struct counter_device *counter, case STM32_LPTIM_ENCODER_BOTH_EDGE: *action = priv->polarity; return 0; + default: + /* should never reach this path */ + return -EINVAL; } - - return -EINVAL; } static int stm32_lptim_cnt_action_set(struct counter_device *counter, diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c index ef2a974a2f10..431a3d08ed6c 100644 --- a/drivers/counter/stm32-timer-cnt.c +++ b/drivers/counter/stm32-timer-cnt.c @@ -296,6 +296,9 @@ static int stm32_action_get(struct counter_device *counter, /* counts up/down on both TI1FP1 and TI2FP2 edges */ *action = STM32_SYNAPSE_ACTION_BOTH_EDGES; break; + default: + /* should never reach this path */ + return -EINVAL; } return 0; diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c index a60aee1a1a29..7844fdf78a97 100644 --- a/drivers/counter/ti-eqep.c +++ b/drivers/counter/ti-eqep.c @@ -192,6 +192,9 @@ static int ti_eqep_action_get(struct counter_device *counter, break; } break; + default: + /* should never reach this path */ + return -EINVAL; } return 0;
Only a select set of modes (function, action, etc.) are valid for a given device configuration. This patch ensures that invalid modes result in a return -EINVAL. Such a situation should never occur in reality, but it's good to define a default switch cases for the sake of making the intent of the code clear. Cc: Syed Nayyar Waris <syednwaris@gmail.com> Cc: Kamel Bouhara <kamel.bouhara@bootlin.com> Cc: Fabrice Gasnier <fabrice.gasnier@st.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Alexandre Torgue <alexandre.torgue@st.com> Cc: David Lechner <david@lechnology.com> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> --- drivers/counter/104-quad-8.c | 10 ++++++++++ drivers/counter/microchip-tcb-capture.c | 6 ++++++ drivers/counter/stm32-lptimer-cnt.c | 10 ++++++---- drivers/counter/stm32-timer-cnt.c | 3 +++ drivers/counter/ti-eqep.c | 3 +++ 5 files changed, 28 insertions(+), 4 deletions(-)