[PATCH] counter: add COUNTER_FUNCTION_DISABLE for energy saving

[PATCH] counter: add COUNTER_FUNCTION_DISABLE for energy saving

[Rafael V. Volkmer]

Add `COUNTER_FUNCTION_DISABLE` to the `counter_function` enum in the
counter API. This allows file operations to signal other drivers to
disable hardware resources, reducing energy consumption in
power-sensitive scenarios.

Previously, tests with Texas Instruments' eQEP modules revealed that
hardware resources remained active unless the driver was removed,
offering no user command to stop the count. This approach exposed the
fragility of these resources.

To address this, introduce a new enum option in the counter API to
receive commands for disabling the hardware. This ensures the hardware
enters an idle, power-saving state when not in use.

Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
 include/uapi/linux/counter.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
index 008a691c254b..6f9a6ef878cf 100644
--- a/include/uapi/linux/counter.h
+++ b/include/uapi/linux/counter.h
@@ -145,6 +145,7 @@ enum counter_function {
 	COUNTER_FUNCTION_QUADRATURE_X2_A,
 	COUNTER_FUNCTION_QUADRATURE_X2_B,
 	COUNTER_FUNCTION_QUADRATURE_X4,
+	COUNTER_FUNCTION_DISABLE,
 };
 
 /* Signal values */
-- 
2.25.1

Re: [PATCH] counter: add COUNTER_FUNCTION_DISABLE for energy saving

[David Lechner]

On 11/25/24 5:02 PM, Rafael V. Volkmer wrote:
> Add `COUNTER_FUNCTION_DISABLE` to the `counter_function` enum in the
> counter API. This allows file operations to signal other drivers to
> disable hardware resources, reducing energy consumption in
> power-sensitive scenarios.
> 
> Previously, tests with Texas Instruments' eQEP modules revealed that
> hardware resources remained active unless the driver was removed,
> offering no user command to stop the count. This approach exposed the
> fragility of these resources.
> 
> To address this, introduce a new enum option in the counter API to
> receive commands for disabling the hardware. This ensures the hardware
> enters an idle, power-saving state when not in use.

How does this work without an additional patch to modify the TI eQEP
counter driver to handle this new enum value? For example, I would
expect that this enum value would be added to ti_eqep_position_functions
and case statements added in ti_eqep_function_read(),
ti_eqep_function_write() and ti_eqep_action_read() to handle the new
option.
> 
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
> ---
>  include/uapi/linux/counter.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index 008a691c254b..6f9a6ef878cf 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -145,6 +145,7 @@ enum counter_function {
>  	COUNTER_FUNCTION_QUADRATURE_X2_A,
>  	COUNTER_FUNCTION_QUADRATURE_X2_B,
>  	COUNTER_FUNCTION_QUADRATURE_X4,
> +	COUNTER_FUNCTION_DISABLE,
>  };
>  
>  /* Signal values */

Re: [PATCH] Add COUNTER_FUNCTION_DISABLE to the counter API

[Rafael V. Volkmer]

On 11/25/24 11:36 AM, David Lechner wrote:
> How does this work without an additional patch to modify the TI eQEP
> counter driver to handle this new enum value? For example, I would
> expect that this enum value would be added to ti_eqep_position_functions
> and case statements added in ti_eqep_function_read(),
> ti_eqep_function_write() and ti_eqep_action_read() to handle the new
> option.

Hi, David!

Yes, the intention is to have a second path where the eQEP driver handles 
this within these file operations functions.

Best regards

Re: [PATCH] Add COUNTER_FUNCTION_DISABLE to the counter API

[David Lechner]

On 11/27/24 3:54 PM, Rafael V. Volkmer wrote:
> On 11/25/24 11:36 AM, David Lechner wrote:
>> How does this work without an additional patch to modify the TI eQEP
>> counter driver to handle this new enum value? For example, I would
>> expect that this enum value would be added to ti_eqep_position_functions
>> and case statements added in ti_eqep_function_read(),
>> ti_eqep_function_write() and ti_eqep_action_read() to handle the new
>> option.
> 
> Hi, David!
> 
> Yes, the intention is to have a second path where the eQEP driver handles 
> this within these file operations functions.
> 
> Best regards

OK, so please send those patches too so that we can see the whole picture.

Based on your discussion with William, it sounds like there are 2 things
that need to be resolved.

1. Should a power saving mode actually be a counter function or should it
   be controlled by the counter enable attribute or something else, like
   more general Linux power management stuff?
2. If there are going to be in-kernel users calling these functions, then
   we will likely want to introduce some new APIs in the kernel for this.
   Using platform_get_drvdata() from one driver to another is a bit
   fragile. Likely we will want some devicetree bindings for counter
   consumers and providers and some kernel APIs like a counter_get() that
   takes an index or string ID to get the counter provider assigned to a
   counter consumer. Then we will probably want some wrappers around the
   counter ops pointers so that consumer drivers don't have to depend on
   the internal implementation of the counter subsystem.

Re: [PATCH] counter: add COUNTER_FUNCTION_DISABLE for energy saving

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On Mon, Nov 25, 2024 at 08:02:20PM -0300, Rafael V. Volkmer wrote:
> Add `COUNTER_FUNCTION_DISABLE` to the `counter_function` enum in the
> counter API. This allows file operations to signal other drivers to
> disable hardware resources, reducing energy consumption in
> power-sensitive scenarios.
> 
> Previously, tests with Texas Instruments' eQEP modules revealed that
> hardware resources remained active unless the driver was removed,
> offering no user command to stop the count. This approach exposed the
> fragility of these resources.
> 
> To address this, introduce a new enum option in the counter API to
> receive commands for disabling the hardware. This ensures the hardware
> enters an idle, power-saving state when not in use.
> 
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>

Hello Rafael,

I wonder whether a new enum option is actually needed in this case.
Wouldn't the Count "enable" extension already provide a way for users to
stop the counting? I imagine the driver would determine implicitly that
the device can enter a power-saving state in that scenario when counting
has been disabled by the user.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] Add COUNTER_FUNCTION_DISABLE to the counter API

[Rafael V. Volkmer]

On Mon, Nov 25, 2024 at 05:26 PM, William Breathitt Gray wrote:
> Hello Rafael,
>
> I wonder whether a new enum option is actually needed in this case.
> Wouldn't the Count "enable" extension already provide a way for users to
> stop the counting? I imagine the driver would determine implicitly that
> the device can enter a power-saving state in that scenario when counting
> has been disabled by the user.
>
> William Breathitt Gray

Hello, William!

Thanks a lot for the feedback! You raised an important point.

I would like to explain my reasoning behind the implementation 
so you can see if it makes sense or if I misunderstood. Basically, 
Texas drivers are described as user-operated through the sysfs file 
system, echoing some directories in /dev/.

The requirement of my application was to communicate at the kernel 
space level with another driver, and to do so, I retrieved the pdev 
with platform_get_drvdata(), which was allocated with platform_set_drvdata 
in the texas driver probe. To apply the required settings, I used the pdev 
I retrieved pointing to the fops, such as ti_eqep_function_write(). I made 
this small change in the module for this enum value and a patch in the ti 
eqep driver to handle this in order to establish a safe shutdown.

In my application it worked perfectly, and so I had the idea of applying 
a patch to kernel.org to see if my changes could contribute something to 
the Texas driver infrastructure.

Best regards

Re: [PATCH] Add COUNTER_FUNCTION_DISABLE to the counter API

[Rafael V. Volkmer]

On Mon, Nov 25, 2024 at 05:26 PM, William Breathitt Gray wrote:
> Hello Rafael,
>
> I wonder whether a new enum option is actually needed in this case.
> Wouldn't the Count "enable" extension already provide a way for users to
> stop the counting? I imagine the driver would determine implicitly that
> the device can enter a power-saving state in that scenario when counting
> has been disabled by the user.
>
> William Breathitt Gray

Hello, William!

Thanks a lot for the feedback! You raised an important point.

I would like to explain my reasoning behind the implementation 
so you can see if it makes sense or if I misunderstood. Basically, 
Texas drivers are described as user-operated through the sysfs file 
system, echoing some directories in /dev/.

The requirement of my application was to communicate at the kernel 
space level with another driver, and to do so, I retrieved the pdev 
with platform_get_drvdata(), which was allocated with platform_set_drvdata 
in the texas driver probe. To apply the required settings, I used the pdev 
I retrieved pointing to the fops, such as ti_eqep_function_write(). I made 
this small change in the module for this enum value and a patch in the ti 
eqep driver to handle this in order to establish a safe shutdown.

In my application it worked perfectly, and so I had the idea of applying 
a patch to kernel.org to see if my changes could contribute something to 
the Texas driver infrastructure.

Best regards