On 12/20/20 3:44 PM, William Breathitt Gray wrote:

On Sun, Dec 13, 2020 at 05:15:14PM -0600, David Lechner wrote:

quoted

On 11/22/20 2:29 PM, William Breathitt Gray wrote:

quoted

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.

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