On Tue, Aug 31, 2021 at 03:44:04PM +0200, Fabrice Gasnier wrote: > On 8/27/21 5:47 AM, William Breathitt Gray wrote: > > This is a reimplementation of the Generic Counter driver interface. > > There are no modifications to the Counter subsystem userspace interface, > > so existing userspace applications should continue to run seamlessly. > > > > The purpose of this patch is to internalize the sysfs interface code > > among the various counter drivers into a shared module. Counter drivers > > pass and take data natively (i.e. u8, u64, etc.) and the shared counter > > module handles the translation between the sysfs interface and the > > device drivers. This guarantees a standard userspace interface for all > > counter drivers, and helps generalize the Generic Counter driver ABI in > > order to support the Generic Counter chrdev interface (introduced in a > > subsequent patch) without significant changes to the existing counter > > drivers. > > > > Note, Counter device registration is the same as before: drivers > > populate a struct counter_device with components and callbacks, then > > pass the structure to the devm_counter_register function. However, > > what's different now is how the Counter subsystem code handles this > > registration internally. > > > > Whereas before callbacks would interact directly with sysfs data, this > > interaction is now abstracted and instead callbacks interact with native > > C data types. The counter_comp structure forms the basis for Counter > > extensions. > > > > The counter-sysfs.c file contains the code to parse through the > > counter_device structure and register the requested components and > > extensions. Attributes are created and populated based on type, with > > respective translation functions to handle the mapping between sysfs and > > the counter driver callbacks. > > > > The translation performed for each attribute is straightforward: the > > attribute type and data is parsed from the counter_attribute structure, > > the respective counter driver read/write callback is called, and sysfs > > I/O is handled before or after the driver read/write function is called. > > > > Cc: Jarkko Nikula > > Cc: Patrick Havelange > > Cc: Kamel Bouhara > > Cc: Fabrice Gasnier > > Cc: Maxime Coquelin > > Cc: Alexandre Torgue > > Cc: Dan Carpenter > > Acked-by: Syed Nayyar Waris > > Reviewed-by: David Lechner > > Tested-by: David Lechner > > Signed-off-by: William Breathitt Gray > > --- > > MAINTAINERS | 1 - > > drivers/counter/104-quad-8.c | 449 +++---- > > drivers/counter/Makefile | 1 + > > drivers/counter/counter-core.c | 142 +++ > > drivers/counter/counter-sysfs.c | 849 +++++++++++++ > > drivers/counter/counter-sysfs.h | 13 + > > drivers/counter/counter.c | 1496 ----------------------- > > drivers/counter/ftm-quaddec.c | 60 +- > > drivers/counter/intel-qep.c | 144 +-- > > drivers/counter/interrupt-cnt.c | 62 +- > > drivers/counter/microchip-tcb-capture.c | 91 +- > > drivers/counter/stm32-lptimer-cnt.c | 212 ++-- > > drivers/counter/stm32-timer-cnt.c | 179 ++- > > drivers/counter/ti-eqep.c | 180 +-- > > include/linux/counter.h | 658 +++++----- > > include/linux/counter_enum.h | 45 - > > 16 files changed, 1949 insertions(+), 2633 deletions(-) > > create mode 100644 drivers/counter/counter-core.c > > create mode 100644 drivers/counter/counter-sysfs.c > > create mode 100644 drivers/counter/counter-sysfs.h > > delete mode 100644 drivers/counter/counter.c > > delete mode 100644 include/linux/counter_enum.h > > > > Hi William, > > For the STM32 drivers, you can add my: > Reviewed-by: Fabrice Gasnier > > Thanks, > Fabrice Hello Fabrice, I noticed the stm32-lptimer-cnt.c function_write() and action_write() callbacks do not appear to write the desired function/action configuration to the device. Would you check whether the device actually supports this functionality or if these callbacks should be removed from this driver. Thanks, William Breathitt Gray > > diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c > > index 7367f46c6f91..5168833b1fdf 100644 > > --- a/drivers/counter/stm32-lptimer-cnt.c > > +++ b/drivers/counter/stm32-lptimer-cnt.c > > @@ -170,28 +154,28 @@ static int stm32_lptim_cnt_read(struct counter_device *counter, > > return 0; > > } > > > > -static int stm32_lptim_cnt_function_get(struct counter_device *counter, > > - struct counter_count *count, > > - size_t *function) > > +static int stm32_lptim_cnt_function_read(struct counter_device *counter, > > + struct counter_count *count, > > + enum counter_function *function) > > { > > struct stm32_lptim_cnt *const priv = counter->priv; > > > > if (!priv->quadrature_mode) { > > - *function = STM32_LPTIM_COUNTER_INCREASE; > > + *function = COUNTER_FUNCTION_INCREASE; > > return 0; > > } > > > > - if (priv->polarity == STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES) { > > - *function = STM32_LPTIM_ENCODER_BOTH_EDGE; > > + if (priv->polarity == STM32_LPTIM_CKPOL_BOTH_EDGES) { > > + *function = COUNTER_FUNCTION_QUADRATURE_X4; > > return 0; > > } > > > > return -EINVAL; > > } > > > > -static int stm32_lptim_cnt_function_set(struct counter_device *counter, > > - struct counter_count *count, > > - size_t function) > > +static int stm32_lptim_cnt_function_write(struct counter_device *counter, > > + struct counter_count *count, > > + enum counter_function function) > > { > > struct stm32_lptim_cnt *const priv = counter->priv; > > > > @@ -199,12 +183,12 @@ static int stm32_lptim_cnt_function_set(struct counter_device *counter, > > return -EBUSY; > > > > switch (function) { > > - case STM32_LPTIM_COUNTER_INCREASE: > > + case COUNTER_FUNCTION_INCREASE: > > priv->quadrature_mode = 0; > > return 0; > > - case STM32_LPTIM_ENCODER_BOTH_EDGE: > > + case COUNTER_FUNCTION_QUADRATURE_X4: > > priv->quadrature_mode = 1; > > - priv->polarity = STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES; > > + priv->polarity = STM32_LPTIM_CKPOL_BOTH_EDGES; > > return 0; > > default: > > /* should never reach this path */ > > @@ -333,43 +316,48 @@ 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 (function != COUNTER_FUNCTION_INCREASE > > + || synapse->signal->id != count->synapses[0].signal->id) > > + return -EINVAL; > > > > - return -EINVAL; > > + switch (action) { > > + case COUNTER_SYNAPSE_ACTION_RISING_EDGE: > > + priv->polarity = STM32_LPTIM_CKPOL_RISING_EDGE; > > + return 0; > > + case COUNTER_SYNAPSE_ACTION_FALLING_EDGE: > > + priv->polarity = STM32_LPTIM_CKPOL_FALLING_EDGE; > > + return 0; > > + case COUNTER_SYNAPSE_ACTION_BOTH_EDGES: > > + priv->polarity = STM32_LPTIM_CKPOL_BOTH_EDGES; > > + return 0; > > + default: > > + return -EINVAL; > > + } > > }