Hi William Breathitt Gray,
Thanks for the feedback.
> Subject: Re: [PATCH v8 4/5] counter: Add Renesas RZ/G2L MTU3a counter
> driver
>
> On Sat, Dec 10, 2022 at 10:21:09AM +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 channels.
> >
> > This patch adds 3 counter value channels.
> > count0: 16-bit phase counter value channel on MTU1
> > count1: 16-bit phase counter value channel on MTU2
> > count2: 32-bit phase counter value channel by cascading
> > MTU1 and MTU2 channels.
> >
> > The external input phase clock pin for the counter value channels are
> > as follows:
> > count0: "MTCLKA-MTCLKB"
> > count1: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> > count2: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> >
> > Use the sysfs variable "external_input_phase_clock_select" to select
> > the external input phase clock pin and "cascade_counts_enable" to
> > enable/ disable cascading of channels.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Hi Biju,
>
> I see you use rz_mtu3_request_channel()/rz_mtu3_release_channel() to share
> access to the channels between the drivers. The Counter sysfs attributes
> can still be accessed when a channel is released, so should
> ch->is_busy be checked before every Counter callback to ensure we do not
> try to access a busy channel?
OK will introduce "count_is_enabled[3]" in priv and
use
(ch->is_busy && !priv->count_is_enabled[count->id]) to make sure
is_busy is because of pwm acquired channel.
Also, will add pm_sync and pm_put for register_{read, write} in all callbacks
So example usage is
echo "MTCLKA-MTCLKB" > /sys/bus/counter/devices/counter0/external_input_phase_clock_select
echo 1 > /sys/bus/counter/devices/counter0/cascade_counts_enable
echo 0 > /sys/bus/counter/devices/counter0/count1/count
echo 20 > /sys/bus/counter/devices/counter0/count1/ceiling
echo 1 > /sys/bus/counter/devices/counter0/count1/enable
cat /sys/bus/counter/devices/counter0/count1/direction
cat /sys/bus/counter/devices/counter0/count1/count
> > +static inline struct rz_mtu3_channel * rz_mtu3_get_ch(struct
> > +counter_device *counter, int id)
>
> I'm not sure why this is split between two lines but you can put it all on
> one.
It comes about 93 columns. As checkpatch is ok with this. I will make it to
one line.
>
> > +static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter,
> > +int id)
>
> It doesn't look like you're using the 'id' parameter in this function so
> you might as well remove it.
Agreed.
>
> > + switch (id) {
> > + case RZ_MTU3_16_BIT_MTU1_CH:
> > + case RZ_MTU3_16_BIT_MTU2_CH:
> > + if (!rz_mtu3_request_channel(ch))
> > + return -EBUSY;
> > +
> > + rz_mtu3_16bit_cnt_setting(counter, id);
> > +
> > + break;
> > + case RZ_MTU3_32_BIT_CH:
> > + /*
> > + * 32-bit phase counting need MTU1 and MTU2 to create 32-bit
> > + * cascade counter.
> > + */
> > + if (!rz_mtu3_request_channel(ch1))
> > + return -EBUSY;
> > +
> > + if (!rz_mtu3_request_channel(ch2)) {
> > + rz_mtu3_release_channel(ch1);
> > + return -EBUSY;
> > + }
> > +
> > + rz_mtu3_32bit_cnt_setting(counter, id);
> > + break;
> > + default:
> > + /* should never reach this path */
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
>
> Instead of the two 'break' statements in the switch block above, replace
> them both with 'return 0' and then you can get rid of this 'return 0'
> at the end.
Agreed.
>
> > + if ((mtclkc_mtclkd && (synapse->signal->id == SIGNAL_A_ID ||
> > + synapse->signal->id == SIGNAL_B_ID)) ||
> > + (!mtclkc_mtclkd && (synapse->signal->id == SIGNAL_C_ID ||
> > + synapse->signal->id == SIGNAL_D_ID))) {
>
> That's a lot of expressions to evaluate, so it's easy for someone to get
> lost in what's happening here. It'll be good to refactor by spinning off
> the signal check to a bool variable. For example:
>
> const bool is_signal_ab = (synapse->signal->id == SIGNAL_A_ID) ||
> (synapse->signal->id == SIGNAL_B_ID);
> ...
> if ((mtclkc_mtclkd && is_signal_ab) ||
> (!mtclkc_mtclkd && !is_signal_ab)) {
> mutex_unlock(&priv->lock
> return 0;
> }
Agreed
Cheers,
Biju
