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:
signature.asc
Description: PGP signature
