| Message ID | cover.1630031207.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
On Fri, 27 Aug 2021 12:47:44 +0900 William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > Changes in v16: > - Define magic numbers for stm32-lptimer-cnt clock polarities > - Define magic numbers for stm32-timer-cnt encoder modes > - Bump KernelVersion to 5.16 in sysfs-bus-counter ABI documentation > - Fix typos in driver API generic-counter.rst documentation file > > For convenience, this patchset is also available on my personal git > repo: https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v16 > > The patches preceding "counter: Internalize sysfs interface code" are > primarily cleanup and fixes that can be picked up and applied now to the > IIO tree if so desired. The "counter: Internalize sysfs interface code" > patch as well may be considered for pickup because it is relatively safe > and makes no changes to the userspace interface. > > To summarize the main points of this patchset: there are no changes to > the existing Counter sysfs userspace interface; a Counter character > device interface is introduced that allows Counter events and associated > data to be read() by userspace; the events_configure() and > watch_validate() driver callbacks are introduced to support Counter > events; and IRQ support is added to the 104-QUAD-8 driver, serving as an > example of how to support the new Counter events functionality. Hi William, I'll aim to pick up the first part in a week (too tired today after a lot of reviewing to even manage the basic sanity check on the changes). For the rest... What I'd really like to know is if anyone other than William and I is planning to review them in depth? (particularly 7 and 8 which are the new interface patch and docs) So if anyone reading this is in that category please let me know. We can wait, but conversely if no one is going to get time / inclination to do it then I don't want to hold these up any longer and maximum time in linux-next may be more useful than sitting unloved on the mailing list. Jonathan > > William Breathitt Gray (14): > counter: stm32-lptimer-cnt: Provide defines for clock polarities > counter: stm32-timer-cnt: Provide defines for slave mode selection > counter: Internalize sysfs interface code > counter: Update counter.h comments to reflect sysfs internalization > docs: counter: Update to reflect sysfs internalization > counter: Move counter enums to uapi header > counter: Add character device interface > docs: counter: Document character device interface > tools/counter: Create Counter tools > counter: Implement signalZ_action_component_id sysfs attribute > counter: Implement *_component_id sysfs attributes > counter: Implement events_queue_size sysfs attribute > counter: 104-quad-8: Replace mutex with spinlock > counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 > > Documentation/ABI/testing/sysfs-bus-counter | 38 +- > Documentation/driver-api/generic-counter.rst | 358 +++- > .../userspace-api/ioctl/ioctl-number.rst | 1 + > MAINTAINERS | 3 +- > drivers/counter/104-quad-8.c | 699 ++++---- > drivers/counter/Kconfig | 6 +- > drivers/counter/Makefile | 1 + > drivers/counter/counter-chrdev.c | 553 ++++++ > drivers/counter/counter-chrdev.h | 14 + > drivers/counter/counter-core.c | 191 +++ > drivers/counter/counter-sysfs.c | 960 +++++++++++ > 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 | 195 +-- > drivers/counter/ti-eqep.c | 180 +- > include/linux/counter.h | 715 ++++---- > include/linux/counter_enum.h | 45 - > include/linux/mfd/stm32-lptimer.h | 5 + > include/linux/mfd/stm32-timers.h | 4 + > include/uapi/linux/counter.h | 154 ++ > tools/Makefile | 13 +- > tools/counter/Build | 1 + > tools/counter/Makefile | 53 + > tools/counter/counter_example.c | 93 + > 29 files changed, 3569 insertions(+), 2791 deletions(-) > create mode 100644 drivers/counter/counter-chrdev.c > create mode 100644 drivers/counter/counter-chrdev.h > 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 > create mode 100644 include/uapi/linux/counter.h > create mode 100644 tools/counter/Build > create mode 100644 tools/counter/Makefile > create mode 100644 tools/counter/counter_example.c > > > base-commit: 5ffeb17c0d3dd44704b4aee83e297ec07666e4d6
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 <jarkko.nikula@linux.intel.com> > > Cc: Patrick Havelange <patrick.havelange@essensium.com> > > Cc: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Cc: Fabrice Gasnier <fabrice.gasnier@st.com> > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > Cc: Alexandre Torgue <alexandre.torgue@st.com> > > Cc: Dan Carpenter <dan.carpenter@oracle.com> > > Acked-by: Syed Nayyar Waris <syednwaris@gmail.com> > > Reviewed-by: David Lechner <david@lechnology.com> > > Tested-by: David Lechner <david@lechnology.com> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > --- > > 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 <fabrice.gasnier@foss.st.com> > > 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; > > + } > > }
On Tue, Aug 31, 2021 at 11:16:59PM +0900, William Breathitt Gray wrote: > 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 <jarkko.nikula@linux.intel.com> > > > Cc: Patrick Havelange <patrick.havelange@essensium.com> > > > Cc: Kamel Bouhara <kamel.bouhara@bootlin.com> > > > Cc: Fabrice Gasnier <fabrice.gasnier@st.com> > > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > > Cc: Alexandre Torgue <alexandre.torgue@st.com> > > > Cc: Dan Carpenter <dan.carpenter@oracle.com> > > > Acked-by: Syed Nayyar Waris <syednwaris@gmail.com> > > > Reviewed-by: David Lechner <david@lechnology.com> > > > Tested-by: David Lechner <david@lechnology.com> > > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > > --- > > > 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 <fabrice.gasnier@foss.st.com> > > > > 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 Nevermind, I see that these configurations are sent to the device via stm32_lptim_setup() when the counter is enabled. So that setup should be fine after all. Thanks, William Breathitt Gray
On Mon, 30 Aug 2021 18:17:06 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 27 Aug 2021 12:47:44 +0900 > William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > > > Changes in v16: > > - Define magic numbers for stm32-lptimer-cnt clock polarities > > - Define magic numbers for stm32-timer-cnt encoder modes > > - Bump KernelVersion to 5.16 in sysfs-bus-counter ABI documentation > > - Fix typos in driver API generic-counter.rst documentation file > > > > For convenience, this patchset is also available on my personal git > > repo: https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v16 > > > > The patches preceding "counter: Internalize sysfs interface code" are > > primarily cleanup and fixes that can be picked up and applied now to the > > IIO tree if so desired. The "counter: Internalize sysfs interface code" > > patch as well may be considered for pickup because it is relatively safe > > and makes no changes to the userspace interface. > > > > To summarize the main points of this patchset: there are no changes to > > the existing Counter sysfs userspace interface; a Counter character > > device interface is introduced that allows Counter events and associated > > data to be read() by userspace; the events_configure() and > > watch_validate() driver callbacks are introduced to support Counter > > events; and IRQ support is added to the 104-QUAD-8 driver, serving as an > > example of how to support the new Counter events functionality. > > Hi William, > > I'll aim to pick up the first part in a week (too tired today after a lot > of reviewing to even manage the basic sanity check on the changes). > > For the rest... > > What I'd really like to know is if anyone other than William and I is planning > to review them in depth? (particularly 7 and 8 which are the new interface > patch and docs) > > So if anyone reading this is in that category please let me know. We can wait, > but conversely if no one is going to get time / inclination to do it then I > don't want to hold these up any longer and maximum time in linux-next may > be more useful than sitting unloved on the mailing list. Ah well, looks like it's just the two of us for the chrdev core patches :) Anyhow, I found time for a more thorough review. I'm not 100% convinced on the model for the chrdev but you know a lot more about this sort of hardware than I do and it definitely seems reasonable - if anything it might be more flexible than it needs to be. I've highlighted a few small things in the patches. With those fixed I'm happy to apply the remainder of this series unless someone shouts in the meantime. There has been plenty of time for review, so fingers crossed that anyone who hasn't commented, but cares, is happy with how you have done it. So, lucky v17! Persistence pays off in the end. Thanks, Jonathan > > Jonathan > > > > > William Breathitt Gray (14): > > counter: stm32-lptimer-cnt: Provide defines for clock polarities > > counter: stm32-timer-cnt: Provide defines for slave mode selection > > counter: Internalize sysfs interface code > > counter: Update counter.h comments to reflect sysfs internalization > > docs: counter: Update to reflect sysfs internalization > > counter: Move counter enums to uapi header > > counter: Add character device interface > > docs: counter: Document character device interface > > tools/counter: Create Counter tools > > counter: Implement signalZ_action_component_id sysfs attribute > > counter: Implement *_component_id sysfs attributes > > counter: Implement events_queue_size sysfs attribute > > counter: 104-quad-8: Replace mutex with spinlock > > counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 > > > > Documentation/ABI/testing/sysfs-bus-counter | 38 +- > > Documentation/driver-api/generic-counter.rst | 358 +++- > > .../userspace-api/ioctl/ioctl-number.rst | 1 + > > MAINTAINERS | 3 +- > > drivers/counter/104-quad-8.c | 699 ++++---- > > drivers/counter/Kconfig | 6 +- > > drivers/counter/Makefile | 1 + > > drivers/counter/counter-chrdev.c | 553 ++++++ > > drivers/counter/counter-chrdev.h | 14 + > > drivers/counter/counter-core.c | 191 +++ > > drivers/counter/counter-sysfs.c | 960 +++++++++++ > > 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 | 195 +-- > > drivers/counter/ti-eqep.c | 180 +- > > include/linux/counter.h | 715 ++++---- > > include/linux/counter_enum.h | 45 - > > include/linux/mfd/stm32-lptimer.h | 5 + > > include/linux/mfd/stm32-timers.h | 4 + > > include/uapi/linux/counter.h | 154 ++ > > tools/Makefile | 13 +- > > tools/counter/Build | 1 + > > tools/counter/Makefile | 53 + > > tools/counter/counter_example.c | 93 + > > 29 files changed, 3569 insertions(+), 2791 deletions(-) > > create mode 100644 drivers/counter/counter-chrdev.c > > create mode 100644 drivers/counter/counter-chrdev.h > > 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 > > create mode 100644 include/uapi/linux/counter.h > > create mode 100644 tools/counter/Build > > create mode 100644 tools/counter/Makefile > > create mode 100644 tools/counter/counter_example.c > > > > > > base-commit: 5ffeb17c0d3dd44704b4aee83e297ec07666e4d6 >