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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux