linux-kernel - Re: [PATCH 6/8] docs: counter: add latch_mode and latched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YXkFzK0TA5zswSrQ@shinobu>
Date:   Wed, 27 Oct 2021 16:54:52 +0900
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     David Lechner <david@...hnology.com>
Cc:     linux-iio@...r.kernel.org, Robert Nelson <robertcnelson@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/8] docs: counter: add latch_mode and latched_count
 sysfs attributes

On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
> This documents new counterX/latch_mode* and
> counterX/countY/latched_count* attributes.
> 
> The counterX/signalY/*_available are moved to the
> counterX/countY/*_available section similar to the way we already have
> The counterX/*_component_id and The counterX/signalY/*_component_id
> grouped together so that we don't have to start a 3rd redundant section
> for device-level *_available section.
> 
> Signed-off-by: David Lechner <david@...hnology.com>

Please separate these two distinct changes into two distinct patches.

> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 39 ++++++++++++++++-----
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 37d960a8cb1b..78bb1a501007 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -59,10 +59,13 @@ What:		/sys/bus/counter/devices/counterX/countY/error_noise_available
>  What:		/sys/bus/counter/devices/counterX/countY/function_available
>  What:		/sys/bus/counter/devices/counterX/countY/prescaler_available
>  What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_available
> +What:		/sys/bus/counter/devices/counterX/latch_mode_available
> +What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
> +What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
>  KernelVersion:	5.2
>  Contact:	linux-iio@...r.kernel.org
>  Description:
> -		Discrete set of available values for the respective Count Y
> +		Discrete set of available values for the respective component
>  		configuration are listed in this file. Values are delimited by
>  		newline characters.
>  
> @@ -147,6 +150,14 @@ Description:
>  			updates	the respective count. Quadrature encoding
>  			determines the direction.
>  
> +What:		/sys/bus/counter/devices/counterX/countY/latched_count
> +KernelVersion:	5.16
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +		Latched count data of Count Y represented as a string. The value
> +		is latched in based on the trigger selected by the
> +		counterX/latch_mode attribute.
> +

Latches are pretty common components of devices, and not simply limited
to latching the count data. I wonder if it would be better to omit the
"_count" suffix in order to make this more general. Well, the name
"latched_count" is suitable for counters so you probably don't need to
change it, but it's something to think about in the future.

>  What:		/sys/bus/counter/devices/counterX/countY/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@...r.kernel.org
> @@ -209,6 +220,7 @@ What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/direction_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/enable_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/error_noise_component_id
> +What:		/sys/bus/counter/devices/counterX/countY/latched_count_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/preset_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> @@ -218,6 +230,7 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> +What:		/sys/bus/counter/devices/latch_mode_component_id
>  What:		/sys/bus/counter/devices/unit_timer_enable_component_id
>  What:		/sys/bus/counter/devices/unit_timer_period_component_id
>  What:		/sys/bus/counter/devices/unit_timer_time_component_id
> @@ -244,6 +257,22 @@ Description:
>  		counter_event data structures. The number of elements will be
>  		rounded-up to a power of 2.
>  
> +What:		/sys/bus/counter/devices/counterX/latch_mode
> +KernelVersion:	5.16
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +		Read/write attribute that selects the trigger for latching
> +		values. Valid values are device-specific (given by
> +		latch_mode_available attribute) and may include:
> +
> +		"Read count":
> +			Reading the countY/count attribute latches values.
> +
> +		"Unit timeout":
> +			Unit timer timeout event latches values.
> +
> +		The latched values can be read from latched_* attributes.
> +

To make these modes more generic for use in future drivers, I suggest
removing the "Unit " prefix and just leaving that mode as "Timeout". In
a similar vein, rewording "Read count" to "Count read" would make this
mode easier to understand in case a future driver introduces a mode
called "Signal read" or similar.

William Breathitt Gray

>  What:		/sys/bus/counter/devices/counterX/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@...r.kernel.org
> @@ -298,14 +327,6 @@ Description:
>  		Active level of index input Signal Y; irrelevant in
>  		non-synchronous load mode.
>  
> -What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
> -What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
> -KernelVersion:	5.2
> -Contact:	linux-iio@...r.kernel.org
> -Description:
> -		Discrete set of available values for the respective Signal Y
> -		configuration are listed in this file.
> -
>  What:		/sys/bus/counter/devices/counterX/signalY/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@...r.kernel.org
> -- 
> 2.25.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)