Re: [PATCH v7 3/5] Documentation: ABI: sysfs-bus-counter: add cascade_enable and external_input_phase_clock_select

[William Breathitt Gray <william.gray@linaro.org>]

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

On Thu, Nov 24, 2022 at 05:00:16PM +0000, Biju Das wrote:
> This commit adds cascade_enable and external_input_phase_clock_
> select items to counter ABI file.
> (e.g. for Renesas MTU3 hardware used for phase counting).
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

I have a few comments below left for this patch. Assuming these are
resolved, then I expect to ack this patch in the next submission.

> ---
> v6->v7:
>  * Replaced long_word_access_ctrl_mode->cascade_enable
>  * Updated Kernel version
> v5->v6:
>  * No change
> v5:
>  * New patch
> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index ff83320b4255..abc691b13b0f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -215,6 +215,22 @@ Contact:	linux-iio@vger.kernel.org
>  Description:
>  		This attribute indicates the number of overflows of count Y.
>  
> +What:		/sys/bus/counter/devices/counterX/cascade_enable

It's possible that in the future we might cascading other things as
well, so let's make this name more specific: "cascade_counts_enable".

> +KernelVersion:	6.3
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute indicates the cascading of counts on
> +		counter X.

Add a line stating this is a boolean attribute: "Valid attribute values
are boolean."

> +
> +What:		/sys/bus/counter/devices/counterX/external_input_phase_clock_select
> +KernelVersion:	6.3
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute selects the external clock pin for phase
> +		counting mode of counter X.

This is a driver-specific enum attribute so it needs a corresponding
*_available entry. Take a look at the count_mode_available entry in this
file and use that as a template to create a new entry block for
external_input_phase_clock_select_available.

> +
> +What:		/sys/bus/counter/devices/counterX/cascade_enable
> +What:		/sys/bus/counter/devices/counterX/external_input_phase_clock_select

These two lines are missing the '_id' suffix: "cascade_enable_id" and
"external_input_phase_clock_select_id".

William Breathitt Gray

>  What:		/sys/bus/counter/devices/counterX/countY/capture_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/ceiling_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/floor_component_id
> -- 
> 2.25.1
> 

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