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 <linux/module.h>
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/types.h>
> >  
> >  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 <linux/module.h>
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/types.h>
> >  
> >  #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

Attachment: signature.asc
Description: PGP signature

Reply via email to