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
signature.asc
Description: PGP signature
