...

The following are some questions I have about this patchset:

1. Should the data format of the character device be configured via a
   sysfs attribute?

   In this patchset, the first 196095 bytes of the character device are
   dedicated as a selection area to choose which Counter components or
   extensions should be exposed; the subsequent bytes are the actual
   data for the Counter components and extensions that were selected.

That sounds like the worst of all possible worlds.  Reality is you need
to do some magic library so at that point you might as well have ioctl
options to configure it.   I wonder if you can keep the data flow
to be a simple 'read' from the chardev but move the control away from
that.  Either control via some chrdevs but keep them to the 'set / get'
if this element is going to turn up in the read or not.  You rapidly
run into problems though, such as now to see how large a given element
is going to be etc.  Plus ioctls are rather messier to extend than
simply adding a new sysfs file.  Various subsystems do complex
'descriptor' type approaches to get around this, or you could do
self describing records rather than raw data - like an input
ev_dev event.

   Moving this selection to a sysfs attribute and dedicating the
   character device to just data transfer might be a better design. If
   such a design is chosen, should the selection attribute be
   human-readable or binary?

Sysfs basically requires things are more or less human readable.
So if you go that way I think it needs to be.

2. How much space should allotted for strings?

   Each Counter component and extension has a respective size allotted
   for its data (u8 data is allotted 1 byte, u64 data is allotted 8
   bytes, etc.); I have arbitrarily chosen to allot 64 bytes for
   strings. Is this an apt size, or should string data be allotted more
   or less space?

I'd go with that being big enough, but try to keep the expose interface
such that the size can change it it needs to the in the future.

3. Should the owning component of an extension be handled by the device
   driver or Counter subsystem?

   The Counter subsystem figures out the owner (enum counter_owner_type)
   for each component/extension in the counter-sysfs and counter-chrdev
   code. When a callback must be executed, there are various switch
   statements throughout the code to check whether the respective
   Device, Signal, or Count version of the callback should be executed;
   similarly, the appropriate owner type must match for the struct
   counter_data macros such as COUNTER_DATA_DEVICE_U64,
   COUNTER_DATA_SIGNAL_U64, COUNTER_DATA_COUNT_U64, etc.

   All this complexity in the Counter subsystem code can be eliminated
   if a single callback type with a `void *owner` parameter is defined
   for use with all three owner types (Device, Signal, and Count). The
   device driver would then be responsible for casting the callback
   argument to the appropriate owner type; but in theory, this should
   not be much of a problem since the device driver is responsible for
   assigning the callbacks to the owning component anyway.

Whilst its more complex for subsytem I think it's better to keep everything
typed if we possibly can.  Always a trade off though, so use your discretion.

Jonathan

William Breathitt Gray (4):
  counter: Internalize sysfs interface code
  docs: counter: Update to reflect sysfs internalization
  counter: Add character device interface
  docs: counter: Document character device interface

 Documentation/driver-api/generic-counter.rst |  275 +++-
 MAINTAINERS                                  |    3 +-
 drivers/counter/104-quad-8.c                 |  547 +++----
 drivers/counter/Makefile                     |    1 +
 drivers/counter/counter-chrdev.c             |  656 ++++++++
 drivers/counter/counter-chrdev.h             |   16 +
 drivers/counter/counter-core.c               |  187 +++
 drivers/counter/counter-sysfs.c              |  881 +++++++++++
 drivers/counter/counter-sysfs.h              |   14 +
 drivers/counter/counter.c                    | 1496 ------------------
 drivers/counter/ftm-quaddec.c                |   89 +-
 drivers/counter/stm32-lptimer-cnt.c          |  161 +-
 drivers/counter/stm32-timer-cnt.c            |  139 +-
 drivers/counter/ti-eqep.c                    |  211 +--
 include/linux/counter.h                      |  626 ++++----
 include/linux/counter_enum.h                 |   45 -
 include/uapi/linux/counter-types.h           |   45 +
 17 files changed, 2826 insertions(+), 2566 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-types.h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel