Re: [PATCH v10 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver

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

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

On Fri, Dec 16, 2022 at 08:50:27PM +0000, Biju Das wrote:
> Add RZ/G2L MTU3a counter driver. This IP supports the following
> phase counting modes on MTU1 and MTU2 channels
> 
> 1) 16-bit phase counting modes on MTU1 and MTU2 channels.
> 2) 32-bit phase counting mode by cascading MTU1 and MTU2 channels.
> 
> This patch adds 3 counter value channels.
> 	count0: 16-bit phase counter value channel on MTU1
> 	count1: 16-bit phase counter value channel on MTU2
> 	count2: 32-bit phase counter value channel by cascading
>                 MTU1 and MTU2 channels.
> 
> The external input phase clock pin for the counter value channels
> are as follows:
> 	count0: "MTCLKA-MTCLKB"
> 	count1: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> 	count2: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> 
> Use the sysfs variable "external_input_phase_clock_select" to select the
> external input phase clock pin and "cascade_counts_enable" to enable/
> disable cascading of channels.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Hi Biju,

You're missing an entry for this driver in the MAINTAINERS file so
please add one. The code for this version looks good so you're welcome
to add my Rb line.

Reviewed-by: William Breathitt Gray <william.gray@linaro.org>

I do have a minor suggestion below.

> +static struct counter_count rz_mtu3_counts[] = {
> +	{
> +		.id = RZ_MTU3_16_BIT_MTU1_CH,
> +		.name = "Channel 1 Count",
> +		.functions_list = rz_mtu3_count_functions,
> +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> +		.synapses = rz_mtu3_mtu1_count_synapses,
> +		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu1_count_synapses),
> +		.ext = rz_mtu3_count_ext,
> +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> +	},
> +	{
> +		.id = RZ_MTU3_16_BIT_MTU2_CH,
> +		.name = "Channel 2 Count",
> +		.functions_list = rz_mtu3_count_functions,
> +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> +		.synapses = rz_mtu3_mtu2_count_synapses,
> +		.num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
> +		.ext = rz_mtu3_count_ext,
> +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> +	},
> +	{
> +		.id = RZ_MTU3_32_BIT_CH,
> +		.name = "Channel 1 and 2 (combined) Count",

These channels are actually cascaded, so replacing "combined" with
"cascaded" here would be better.

William Breathitt Gray

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