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

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

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

On Sat, Dec 10, 2022 at 10:21:09AM +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,

I see you use rz_mtu3_request_channel()/rz_mtu3_release_channel() to
share access to the channels between the drivers. The Counter sysfs
attributes can still be accessed when a channel is released, so should
ch->is_busy be checked before every Counter callback to ensure we do not
try to access a busy channel?

> +static inline struct rz_mtu3_channel *
> +rz_mtu3_get_ch(struct counter_device *counter, int id)

I'm not sure why this is split between two lines but you can put it all
on one.

> +static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter, int id)

It doesn't look like you're using the 'id' parameter in this function so
you might as well remove it.

> +	switch (id) {
> +	case RZ_MTU3_16_BIT_MTU1_CH:
> +	case RZ_MTU3_16_BIT_MTU2_CH:
> +		if (!rz_mtu3_request_channel(ch))
> +			return -EBUSY;
> +
> +		rz_mtu3_16bit_cnt_setting(counter, id);
> +
> +		break;
> +	case RZ_MTU3_32_BIT_CH:
> +		/*
> +		 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit
> +		 * cascade counter.
> +		 */
> +		if (!rz_mtu3_request_channel(ch1))
> +			return -EBUSY;
> +
> +		if (!rz_mtu3_request_channel(ch2)) {
> +			rz_mtu3_release_channel(ch1);
> +			return -EBUSY;
> +		}
> +
> +		rz_mtu3_32bit_cnt_setting(counter, id);
> +		break;
> +	default:
> +		/* should never reach this path */
> +		return -EINVAL;
> +	}
> +
> +	return 0;

Instead of the two 'break' statements in the switch block above, replace
them both with 'return 0' and then you can get rid of this 'return 0'
at the end.

> +		if ((mtclkc_mtclkd && (synapse->signal->id == SIGNAL_A_ID ||
> +				       synapse->signal->id == SIGNAL_B_ID)) ||
> +		    (!mtclkc_mtclkd && (synapse->signal->id == SIGNAL_C_ID ||
> +					synapse->signal->id == SIGNAL_D_ID))) {

That's a lot of expressions to evaluate, so it's easy for someone to get
lost in what's happening here. It'll be good to refactor by spinning off
the signal check to a bool variable. For example:

    const bool is_signal_ab = (synapse->signal->id == SIGNAL_A_ID) ||
                              (synapse->signal->id == SIGNAL_B_ID);
    ...
    if ((mtclkc_mtclkd && is_signal_ab) ||
        (!mtclkc_mtclkd && !is_signal_ab)) {
            mutex_unlock(&priv->lock
	    return 0;
    }

William Breathitt Gray

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