Hi William Breathitt Gray,
Thanks for the feedback.
> Subject: Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver
>
> On Sat, Oct 08, 2022 at 09:01:21AM +0000, Biju Das wrote:
> > Hi William Breathitt Gray,
> >
> > Thanks for the feedback.
>
> Hello Biju,
>
> I see that you have already released a v4, so some of my comments may
> no longer apply, but I want to respond here to continue our
> discussions; I'll reiterate any relevant suggestions when I review v4
> in the coming days.
>
> By the way, if you agree with a review comment there is no need to
> reply with "OK"; just delete the parts you agree with from your
> response and I'll know those are okay. Doing this will reduce the
> amount of text we have to scroll through and thus allow us to focus on
> just the questions we have remaining. ;-)
OK.
> > > Looks like something different is done when ceiling is set to 0.
> > > Would you explain what's happening in this case and why it's
> > > different that then else case below; in other words, what's the
> > > difference between RZ_MTU3_TCR_CCLR_NONE and
> RZ_MTU3_TCR_CCLR_TGRA?
> >
> > RZ_MTU3_TCR_CCLR_TGRA --> for triggering counter count using Z-Phase
> signal.
> > RZ_MTU3_TCR_CCLR_NONE --> No clearing.
>
> Does the Z-Phase signal trigger a reset of the counter count back to
> the ceiling value?
No, It resets to 0.
Does the count loop back to 0 when it passes the
> ceiling value,
Yes, it loopback to 0.
or does it remain at the ceiling until the direction
> changes?
No.
> By "no clearing" do you mean that the ceiling is disabled in this case
> and the Counter count increases without limit?
Counter clear source disabled. So the counter won't reset to 0 by applying
Z-Phase signal.
>
> In the Counter subsystem, the "ceiling" Count extension puts an upper
> limit on the Count value. This means that setting "ceiling" to 0 would
> put the upper limit at 0, effectively restricting the Count value to 0
> until the value of "ceiling" is raised.
>
> If the device is unable to support a ceiling value of 0, you should
> return -ERANGE rather than disable the ceiling.
OK, will check this.
>
> > > > +static void rz_mtu3_16bit_cnt_setting(struct counter_device
> > > *counter,
> > > > +int id) {
> > > > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > > +
> > > > + priv->ch[id]->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> > >
> > > If 16-bit phase counting is selected for one 16-bit counter, does
> > > the other 16-bit counter need to be configured as well?
> >
> > Not required I guess, as it is run time decision.
> >
> > After this, if user tries to enable 16-bit on other channel, we will
> > configure that channel. otherwise, we will return error, if user
> tries
> > to enable 32-bit channel.
> >
> > Are you ok with this?
>
> Because the phase mode affects how the device interprets multiple
> channels rather than a specific one, maybe it's better to save this
> state as an enum rz_mtu3_function member of struct rz_mtu3_cnt. Or if
> this is affecting the entire device, move it to your struct rz_mut3
> and share a pointer to that for your Counter and PWM drivers.
It is the same pointer allocated by parent and shared to both Counter
and PWM drivers. At runtime any device can claim the channel by checking
RZ_MTU3_NORMAL function.
>
> It makes me wonder if the rz_mut3_cnt structure is necessary for this
> Counter driver at all when you could pass a pointer your existing
> rz_mut3 structure instead in order to access the channels.
It is a shared pointer. For easy handling I have added
only 2 channels that are relevant for the counter in rz_mut3_cnt.
Similar case for pwm.
>
> > > > + int ret = 0;
> > > > +
> > > > + if (enable) {
> > > > + pm_runtime_get_sync(ch->dev);
> > > > + ret = rz_mtu3_initialize_counter(counter, count->id);
> > >
> > > Are you using the Count's "enable" extension to switch between
> > > 16-bit and 32-bit phase modes?
> >
> > No. But will use that for switching on the next version.
>
> Sorry, I wasn't clear with my question. Please do not implement the
> "enable" Count extensions as a way to toggle between the 16-bit and
> 32-bit phase modes. The purpose of "enable" is to provide a
> pause/resume mechanism for a Count: the existing count value should be
> preserved when a Count is disabled, and should continue where it left
> off when the Count is enabled.
I use below sample program to test 16-bit and 32-bit phase mode
Counting with trigger for clear count by grounding the Z-phase pin.
16-bit phase testing
------------------
echo 1 > /sys/bus/counter/devices/counter0/count0/enable
echo 0 > /sys/bus/counter/devices/counter0/count0/count
echo 20 > /sys/bus/counter/devices/counter0/count0/ceiling
echo 1 > /sys/bus/counter/devices/counter0/count1/enable
echo 0 > /sys/bus/counter/devices/counter0/count1/count
echo 20 > /sys/bus/counter/devices/counter0/count1/ceiling
for i in {1..200};
do
cat /sys/bus/counter/devices/counter0/count0/count;
cat /sys/bus/counter/devices/counter0/count1/count;
done
echo 0 > /sys/bus/counter/devices/counter0/count1/enable
echo 0 > /sys/bus/counter/devices/counter0/count0/enable
32-bit phase count testing
--------------------------
echo 1 > /sys/bus/counter/devices/counter0/count2/enable
echo 0 > /sys/bus/counter/devices/counter0/count2/count
echo 20 > /sys/bus/counter/devices/counter0/count2/ceiling
for i in {1..200};
do
cat /sys/bus/counter/devices/counter0/count2/count;
done
echo 0 > /sys/bus/counter/devices/counter0/count2/enable
> To support the phase mode selection, implement a Counter device
> extension for that specific purpose. You can use DEFINE_COUNTER_ENUM()
> and COUNTER_COMP_DEVICE_ENUM() to create a device extension that will
> allow users to toggle between "16-bit" and "32-bit" phase modes. If
> you need help with these macros, just let me know.
Yes please, that will be helpful.
Cheers,
Biju
