Hi William Breathitt Gray,
Thanks for the feedback.
> -----Original Message-----
> From: William Breathitt Gray <william.gray@xxxxxxxxxx>
> Sent: 14 November 2022 13:53
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
>
> 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().
It is already taken care while working on cascade_enable.
>
> > > +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.
It is already taken care.
Cheers,
Biju
