Re: [PATCH v6 0/5] Introduce the Counter character device interface

[David Lechner]

On 12/20/20 3:44 PM, William Breathitt Gray wrote:
> On Sun, Dec 13, 2020 at 05:15:14PM -0600, David Lechner wrote:
> > On 11/22/20 2:29 PM, William Breathitt Gray wrote:
> > > 1. Should standard Counter component data types be defined as u8 or u32?
> > >
> > >      Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
> > >      have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
> > >      COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
> > >      Counter subsystem code as u8 data types.
> > >
> > >      If u32 is used for these values instead, C enum structures could be
> > >      used by driver authors to implicitly cast these values via the driver
> > >      callback parameters.
> > >
> > >      This question is primarily addressed to David Lechner. I'm somewhat
> > >      confused about how this setup would look in device drivers. I've gone
> > >      ahead and refactored the code to support u32 enums, and pushed it to
> > >      a separate branch on my repository called counter_chrdev_v6_u32_enum:
> > >      https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum
> > >
> > >      Please check it out and let me know what you think. Is this the
> > >      support you had in mind? I'm curious to see an example of how would
> > >      your driver callback functions would look in this case. If everything
> > >      works out fine, then I'll submit this branch as v7 of this patchset.
> >
> > I haven't had time to look at this in depth, but just superficially looking
> > at it, it is mostly there. The driver callback would just use the enum type
> > in place of u32. For example:
> >
> > static int ti_eqep_function_write(struct counter_device *counter,
> > 				  struct counter_count *count,
> > 				  enum counter_function function)
> >
> > and the COUNTER_FUNCTION_* constants would be defined as:
> >
> > enum counter_function {
> > 	COUNTER_FUNCTION_INCREASE,
> > 	...
> > };
> >
> > instead of using #define macros.
> >
> > One advantage I see to using u8, at least in the user API data structures,
> > is that it increases the number of events that fit in the kfifo buffer by
> > a significant factor.
> >
> > And that is not to say that we couldn't do both: have the user API structs
> > use u8 for enum values and still use u32/strong enum types internally in
> > the callback functions.
>
> I'm including David Laight because he initially opposed enums in favor
> of fixed size types when we discussed this in an earlier revision:
> https://lkml.org/lkml/2020/5/3/159
>
> However, there have been significant changes to this patchset so the
> context now is different than those earlier discussions (i.e. we're no
> longer discussing ioctl calls).
>
> I think reimplementing these constants as enums as described could work.
> If we do so, should the enum constants be given specific values? For
> example:
>
> enum counter_function {
> 	COUNTER_FUNCTION_INCREASE = 0,
> 	COUNTER_FUNCTION_DECREASE = 1,
> 	...
> };

I would say no on the explicit values since they don't have
any significant meaning.