Hi Biju,
On Thu, Nov 24, 2022 at 6:00 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> 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_enable" to enable/disable
> cascading of channels.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v6->v7:
> * Updated commit description
> * Added Register descriptions
> * Opimized size of cache variable by using union
> * Used test_bit() in rz_mtu3_is_counter_invalid()
> * Replaced val->timer_mode in rz_mtu3_count_function_{read,write}
> * Added TODO comment phase3 and phase5 modes.
> * replaced if-else with ternary expression in rz_mtu3_count_direction_read()
> * Used switch statement in rz_mtu3_count_ceiling_read to consistent with write
> * Provided default case for all switch statement.
> * Add mutex lock for avoiding races with other devices
> * Updated comments in rz_mtu3_action_read
> * Replaced COUNTER_COMP_DEVICE_BOOL->COUNTER_COMP_DEVICE_BOOL for
> cascade_enable
> * Replaced RZ_MTU3_GET_HW_CH->rz_mtu3_get_hw_ch
> * Added rz_mtu3_get_ch() to get channels
> * used rz_mtu3_shared_reg_update_bit for cascade_enable and
> selecting phase input clock.
> * Added rz_mtu3_is_counter_invalid() check in rz_mtu3_count_ceiling_read()
Thanks for the update!
> --- /dev/null
> +++ b/drivers/counter/rz-mtu3-cnt.c
> +static int rz_mtu3_initialize_counter(struct counter_device *counter, int id)
> +{
> + struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, id);
> + struct rz_mtu3_channel *const ch1 = rz_mtu3_get_ch(counter, 0);
> + struct rz_mtu3_channel *const ch2 = rz_mtu3_get_ch(counter, 1);
> +
> + switch (id) {
> + case RZ_MTU3_16_BIT_MTU1_CH:
> + case RZ_MTU3_16_BIT_MTU2_CH:
> + mutex_lock(&ch->lock);
> + if (ch->function == RZ_MTU3_NORMAL)
> + ch->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> + mutex_unlock(&ch->lock);
> +
> + if (ch->function != RZ_MTU3_16BIT_PHASE_COUNTING)
> + return -EBUSY;
I think having a request_channel() API would make this more readable,
and avoid duplication (here and in the PWM driver).
> +
> + 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.
> + */
> + mutex_lock(&ch1->lock);
> + if (ch1->function == RZ_MTU3_NORMAL)
> + ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> + mutex_unlock(&ch1->lock);
> +
> + mutex_lock(&ch2->lock);
> + if (ch2->function == RZ_MTU3_NORMAL)
> + ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> + mutex_unlock(&ch2->lock);
> +
> + if (ch1->function != RZ_MTU3_32BIT_PHASE_COUNTING ||
> + ch2->function != RZ_MTU3_32BIT_PHASE_COUNTING)
> + return -EBUSY;
Surely you need to release the channel you did obtain (if any)?
> +
> + rz_mtu3_32bit_cnt_setting(counter, id);
> + break;
> + default:
> + /* should never reach this path */
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static void rz_mtu3_terminate_counter(struct counter_device *counter, int id)
> +{
> + struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, id);
> + struct rz_mtu3_channel *const ch1 = rz_mtu3_get_ch(counter, 0);
> + struct rz_mtu3_channel *const ch2 = rz_mtu3_get_ch(counter, 1);
> +
> + if (id == RZ_MTU3_32_BIT_CH) {
> + mutex_lock(&ch1->lock);
> + ch1->function = RZ_MTU3_NORMAL;
> + mutex_unlock(&ch1->lock);
Locking around a simple integer assignment doesn't do much.
You might as well just use WRITE_ONCE(), to avoid reordering by the
compiler.
> +
> + mutex_lock(&ch2->lock);
> + ch2->function = RZ_MTU3_NORMAL;
> + mutex_unlock(&ch2->lock);
> +
> + rz_mtu3_disable(ch2);
> + rz_mtu3_disable(ch1);
> + } else {
> + mutex_lock(&ch->lock);
> + ch->function = RZ_MTU3_NORMAL;
> + mutex_unlock(&ch->lock);
> +
> + rz_mtu3_disable(ch);
> + }
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
