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

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

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

Hi Biju,

I have a few follow-up comments that came to my mind.

On Sun, Nov 13, 2022 at 10:47:13PM -0500, William Breathitt Gray wrote:
> On Sun, Nov 13, 2022 at 05:15:44PM +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.
> > 
> > This patch adds 3 counters by creating 3 logical channels
> > 	counter0: 16-bit phase counter on MTU1 channel
> > 	counter1: 16-bit phase counter on MTU2 channel
> > 	counter2: 32-bit phase counter by cascading MTU1 and MTU2
> > 		  channels.
> 
> Within the context of the Counter subsystem, the term "counter"
> specifically refers to the device (Counts + Synapses + Signals). Instead
> you should use "count" here to refer to the counter value channels (i.e.
> count0, count1, and count2).

Include a brief description of the Signals and their relationship to the
three Counts as well in this commit message. In particular, mention how
"MTCLKA-MTCLKB" and "MTCLKC-MTCLKD" can be toggled for MTU2, etc.

> > +static int rz_mtu3_long_word_access_ctrl_mode_set(struct counter_device *counter,
> > +						  u32 lwa_ctrl_mode)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u16 val;
> > +
> > +	pm_runtime_get_sync(priv->ch->dev);
> > +	val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> > +	if (lwa_ctrl_mode)
> > +		val |= RZ_MTU3_TMDR3_LWA;
> > +	else
> > +		val &= ~RZ_MTU3_TMDR3_LWA;
> > +
> > +	rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
> > +	pm_runtime_put(priv->ch->dev);
> 
> When you want to assign a bit to a buffer, you can use __assign_bit() to
> simplify your code:
> 
>     unsigned long tmdr;
>     ...
>     tmdr = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
>     __assign_bit(RZ_MTU3_TMDR3_LWA, &tmdr, !!lwa_ctrl_node);
>     rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, tmdr);

You should consider implementing a rz_mtu3_shared_reg_update_bits() that
will perform this read => assign bits => write sequence so that you can
reuse this pattern in the rz_mtu3_ext_input_phase_clock_select_set().

> > +static int rz_mtu3_action_read(struct counter_device *counter,
> > +			       struct counter_count *count,
> > +			       struct counter_synapse *synapse,
> > +			       enum counter_synapse_action *action)
> > +{
> > +	enum counter_function function;
> > +	int err;
> > +
> > +	err = rz_mtu3_count_function_read(counter, count, &function);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Default action mode */
> > +	*action = COUNTER_SYNAPSE_ACTION_NONE;
> 
> You can exit early here depending on which ext_input_phase_clock mode is
> currently selected: if "MTCLKA-MTCLKB" then return early if id is signal
> C or D, while if "MTCLKC-MTCLKD" return early if id is signal A or B.

IIUC count0 is always "MTCLKA-MTCLKB", so this exit early check won't
apply in that particular case; check count->id to see which Count we're
handling.

William Breathitt Gray

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