Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v6 0/5] Introduce the Counter character device interface
To:     William Breathitt Gray <vilhelm.gray@gmail.com>,
        David.Laight@ACULAB.COM
Cc:     jic23@kernel.org, kernel@pengutronix.de,
        linux-stm32@st-md-mailman.stormreply.com, a.fatoum@pengutronix.de,
        kamel.bouhara@bootlin.com, gwendal@chromium.org,
        alexandre.belloni@bootlin.com, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        syednwaris@gmail.com, patrick.havelange@essensium.com,
        fabrice.gasnier@st.com, mcoquelin.stm32@gmail.com,
        alexandre.torgue@st.com
References: <cover.1606075915.git.vilhelm.gray@gmail.com>
 <6f0d78ae-9724-f67f-f133-a1148a5f1688@lechnology.com>
 <X9/Foc6wGl5dR1yK@shinobu>
From:   David Lechner <david@lechnology.com>
Message-ID: <b10c5ba6-e8c7-96d5-db9f-3828018330f6@lechnology.com>
Date:   Mon, 21 Dec 2020 09:19:17 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <X9/Foc6wGl5dR1yK@shinobu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.