Messages in this thread | | | Subject | Re: [PATCH v6 0/5] Introduce the Counter character device interface | From | David Lechner <> | Date | Mon, 21 Dec 2020 09:19:17 -0600 |
| |
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.
| |