Re: [Linux-stm32] [PATCH v10 22/33] counter: Internalize sysfs interface code

[William Breathitt Gray <vilhelm.gray@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8984 bytes --]

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