On Mon, Mar 22, 2021 at 05:44:01PM +0100, Fabrice Gasnier wrote: > On 3/19/21 12:00 PM, William Breathitt Gray wrote: > > static const struct atmel_tcb_config tcb_rm9200_config = { > > diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c > > index 13656957c45f..aef78a4217b5 100644 > > --- a/drivers/counter/stm32-lptimer-cnt.c > > +++ b/drivers/counter/stm32-lptimer-cnt.c > > @@ -17,6 +17,7 @@ > > #include > > #include > > #include > > +#include > > > > struct stm32_lptim_cnt { > > struct counter_device counter; > > @@ -130,32 +131,46 @@ static int stm32_lptim_setup(struct stm32_lptim_cnt *priv, int enable) > > * +---------+----------+----------+---------+----------+---------+ > > */ > > enum stm32_lptim_cnt_function { > > - STM32_LPTIM_COUNTER_INCREASE, > > - STM32_LPTIM_ENCODER_BOTH_EDGE, > > + STM32_LPTIM_COUNTER_INCREASE = COUNTER_FUNCTION_INCREASE, > > + STM32_LPTIM_ENCODER_BOTH_EDGE = COUNTER_FUNCTION_QUADRATURE_X4, > > Hi William, > > I'm wondering if this enum is still necessary. I noticed the enum is > removed from the 104-quad-8 driver. Hi Fabrice, This enum is no longer necessary. I wanted to leave it up to the maintainers to decide whether to remove any particular enum (or to do any other sort of code cleanup), so that is why I didn't remove it here. > > }; > > > > static const enum counter_function stm32_lptim_cnt_functions[] = { > > - [STM32_LPTIM_COUNTER_INCREASE] = COUNTER_FUNCTION_INCREASE, > > - [STM32_LPTIM_ENCODER_BOTH_EDGE] = COUNTER_FUNCTION_QUADRATURE_X4, > > + STM32_LPTIM_COUNTER_INCREASE, > > + STM32_LPTIM_ENCODER_BOTH_EDGE, > > }; > > > > enum stm32_lptim_synapse_action { > > + /* Index must match with stm32_lptim_cnt_polarity[] (priv->polarity) */ > > Arf... I forgot to update this comment in earlier commit: > d8ac6b4 counter: stm32-lptimer-cnt: remove iio counter abi > > "stm32_lptim_cnt_polarity" doesn't exist anymore. So, this comment can > be updated. This should match the priv->polarity, as it's used to write > the "CKPOL" bits (e.g. 0x0 for rising, 0x1 falling, 0x2 both). > > Or do you wish I send a separate patch ? This is just a simple comment fix so I think it's best to send it as its own patch to the mailing list. > > STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE, > > STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE, > > STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES, > > STM32_LPTIM_SYNAPSE_ACTION_NONE, > > }; > > > > -static const enum counter_synapse_action stm32_lptim_cnt_synapse_actions[] = { > > - /* Index must match with stm32_lptim_cnt_polarity[] (priv->polarity) */ > > +static const enum stm32_lptim_synapse_action stm32_lptim_c2l_actions_map[] = { > > + [COUNTER_SYNAPSE_ACTION_RISING_EDGE] = STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE, > > + [COUNTER_SYNAPSE_ACTION_FALLING_EDGE] = STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE, > > + [COUNTER_SYNAPSE_ACTION_BOTH_EDGES] = STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES, > > + [COUNTER_SYNAPSE_ACTION_NONE] = STM32_LPTIM_SYNAPSE_ACTION_NONE, > > +}; > > + > > +static const enum counter_synapse_action stm32_lptim_l2c_actions_map[] = { > > [STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE, > > [STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE] = COUNTER_SYNAPSE_ACTION_FALLING_EDGE, > > [STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES] = COUNTER_SYNAPSE_ACTION_BOTH_EDGES, > > [STM32_LPTIM_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE, > > }; > > > > +static const enum counter_synapse_action stm32_lptim_cnt_synapse_actions[] = { > > + COUNTER_SYNAPSE_ACTION_RISING_EDGE, > > + COUNTER_SYNAPSE_ACTION_FALLING_EDGE, > > + COUNTER_SYNAPSE_ACTION_BOTH_EDGES, > > + COUNTER_SYNAPSE_ACTION_NONE, > > +}; > > + > > I'm getting confused with the two table entries, and the two enums that > are very similar. Could it be simplified ? > > I'm thinking of something more straight-forward... in fact only one > array should be enough, to convert from synapse action to CKPOL value > before it's written to register in stm32_lptim_setup() routine. > This should be easier now that the iio part has been removed. Yes, this might just be a hold over from when we had to handle the legacy IIO Counter code. But now that it is gone, this could probably be simplified down to a single array then. If you have any idea of how to achieve that, please do. > > @@ -333,43 +333,39 @@ static int stm32_lptim_cnt_action_get(struct counter_device *counter, > > } > > } > > > > -static int stm32_lptim_cnt_action_set(struct counter_device *counter, > > - struct counter_count *count, > > - struct counter_synapse *synapse, > > - size_t action) > > +static int stm32_lptim_cnt_action_write(struct counter_device *counter, > > + struct counter_count *count, > > + struct counter_synapse *synapse, > > + enum counter_synapse_action action) > > { > > struct stm32_lptim_cnt *const priv = counter->priv; > > - size_t function; > > + enum counter_function function; > > int err; > > > > if (stm32_lptim_is_enabled(priv)) > > return -EBUSY; > > > > - err = stm32_lptim_cnt_function_get(counter, count, &function); > > + err = stm32_lptim_cnt_function_read(counter, count, &function); > > if (err) > > return err; > > > > /* only set polarity when in counter mode (on input 1) */ > > - if (function == STM32_LPTIM_COUNTER_INCREASE > > - && synapse->signal->id == count->synapses[0].signal->id) { > > - switch (action) { > > - case STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE: > > - case STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE: > > - case STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES: > > - priv->polarity = action; > > - return 0; > > - } > > - } > > + if ((enum stm32_lptim_cnt_function)function != STM32_LPTIM_COUNTER_INCREASE > > Could COUNTER_FUNCTION_INCREASE be used directly here, to avoid casting ? Sure, you can remove the stm32_lptim_cnt_function enum from this driver and just use COUNTER_FUNCTION_INCREASE here. > > + || synapse->signal->id != count->synapses[0].signal->id > > + || action == COUNTER_SYNAPSE_ACTION_NONE) > > + return -EINVAL; > > > > - return -EINVAL; > > + priv->polarity = stm32_lptim_c2l_actions_map[action]; > > + > > + return 0; > > } > > > > static const struct counter_ops stm32_lptim_cnt_ops = { > > .count_read = stm32_lptim_cnt_read, > > - .function_get = stm32_lptim_cnt_function_get, > > - .function_set = stm32_lptim_cnt_function_set, > > - .action_get = stm32_lptim_cnt_action_get, > > - .action_set = stm32_lptim_cnt_action_set, > > + .function_read = stm32_lptim_cnt_function_read, > > + .function_write = stm32_lptim_cnt_function_write, > > + .action_read = stm32_lptim_cnt_action_read, > > + .action_write = stm32_lptim_cnt_action_write, > > }; > > > > static struct counter_signal stm32_lptim_cnt_signals[] = { > > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c > > index 3fb0debd7425..c690b76e5dab 100644 > > --- a/drivers/counter/stm32-timer-cnt.c > > +++ b/drivers/counter/stm32-timer-cnt.c > > @@ -13,6 +13,7 @@ > > #include > > #include > > #include > > +#include > > > > #define TIM_CCMR_CCXS (BIT(8) | BIT(0)) > > #define TIM_CCMR_MASK (TIM_CCMR_CC1S | TIM_CCMR_CC2S | \ > > @@ -44,21 +45,21 @@ struct stm32_timer_cnt { > > * @STM32_COUNT_ENCODER_MODE_3: counts on both TI1FP1 and TI2FP2 edges > > */ > > enum stm32_count_function { > > + STM32_COUNT_SLAVE_MODE_DISABLED = COUNTER_FUNCTION_INCREASE, > > + STM32_COUNT_ENCODER_MODE_1 = COUNTER_FUNCTION_QUADRATURE_X2_A, > > + STM32_COUNT_ENCODER_MODE_2 = COUNTER_FUNCTION_QUADRATURE_X2_B, > > + STM32_COUNT_ENCODER_MODE_3 = COUNTER_FUNCTION_QUADRATURE_X4, > > +}; > > + > > Same as for the LPTIM driver above, I noticed the enum is removed from > the 104-quad-8 driver. > > I'm fine both ways... I just feel like this could be more consistent > later in the function read/write routines to report enum from the > counter_function definition (e.g. like COUNTER_FUNCTION_QUADRATURE_X4 > instead of STM32_COUNT_ENCODER_MODE_3). > > Thanks, > Fabrice Yes, this enum is just used to alias those constants. If you think the code will be clearer by using the COUNTER_FUNCTION_* constants directly, then please do so. I don't know whether this v10 revision of the patchset will be merged or if we'll need a v11. So send your updates for stm32-lptimer-cnt.c and stm32-timer-cnt.c to me directly and I'll squash them with this patch if we do have a v11; otherwise you can submit them separately to the mailing list if this v10 is merged afterall. Thanks, William Breathitt Gray