Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 19 December 2022 08:40
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: William Breathitt Gray <william.gray@xxxxxxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx; Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>; Prabhakar
> Mahadev Lad <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>; linux-renesas-
> soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v10 4/5] counter: Add Renesas RZ/G2L MTU3a counter
> driver
>
> Hi Biju,
>
> On Fri, Dec 16, 2022 at 9:50 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> 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>
> > ---
> > v9->v10:
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/counter/rz-mtu3-cnt.c
>
> > +static int rz_mtu3_count_read(struct counter_device *counter,
> > + struct counter_count *count, u64 *val) {
> > + struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count-
> >id);
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > + mutex_lock(&priv->lock);
> > + if (ch->is_busy && !priv->count_is_enabled[count->id]) {
> > + mutex_unlock(&priv->lock);
> > + return -EINVAL;
> > + }
> > +
> > + if (rz_mtu3_is_counter_invalid(counter, count->id)) {
> > + mutex_unlock(&priv->lock);
> > + return -EBUSY;
> > + }
>
> As the locking and the above two checks are duplicated multiple times,
> perhaps they can be replaced by an rz_mtu3_lock_if_counter_is_valid()
> helper function?
OK will use the helper function " rz_mtu3_lock_if_counter_is_valid"
+static int rz_mtu3_lock_if_counter_is_valid(struct counter_device *counter,
+ struct rz_mtu3_channel *const ch,
+ struct rz_mtu3_cnt *const priv,
+ int id)
+{
+ if (ch->is_busy && !priv->count_is_enabled[id])
+ return -EINVAL;
+
+ if (rz_mtu3_is_counter_invalid(counter, id))
+ return -EBUSY;
+
+ mutex_lock(&priv->lock);
+
+ return 0;
+}
>
> > +static int rz_mtu3_count_function_read(struct counter_device *counter,
> > + struct counter_count *count,
> > + enum counter_function
> > +*function) {
> > + struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count-
> >id);
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + int ret;
> > +
> > + mutex_lock(&priv->lock);
> > + if (ch->is_busy && !priv->count_is_enabled[count->id]) {
> > + mutex_unlock(&priv->lock);
> > + return -EINVAL;
> > + }
>
> rz_mtu3_lock_if_count_is_disabled() helper?
> (can also be called by rz_mtu3_lock_if_counter_is_valid())
OK will use the helper function "rz_mtu3_lock_if_count_is_enabled"
+static int rz_mtu3_lock_if_count_is_enabled(struct rz_mtu3_channel *const ch,
+ struct rz_mtu3_cnt *const priv,
+ int id)
+{
+ if (ch->is_busy && !priv->count_is_enabled[id])
+ return -EINVAL;
+
+ mutex_lock(&priv->lock);
+
+ return 0;
+}
>
> > +static int rz_mtu3_cascade_counts_enable_set(struct counter_device
> *counter,
> > + u8 cascade_enable) {
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > + mutex_lock(&priv->lock);
> > + if (priv->ch->is_busy && !rz_mtu3_is_ch0_enabled(priv)) {
> > + mutex_unlock(&priv->lock);
> > + return -EINVAL;
> > + }
>
> rz_mtu3_lock_if_count_is_disabled() helper?
OK, Will use the helper function "rz_mtu3_lock_if_ch0_is_enabled"
+static int rz_mtu3_lock_if_ch0_is_enabled(struct rz_mtu3_cnt *const priv)
{
+ if (priv->ch->is_busy && !(priv->count_is_enabled[RZ_MTU3_16_BIT_MTU1_CH] ||
+ priv->count_is_enabled[RZ_MTU3_32_BIT_CH]))
+ return -EINVAL;
+
+ mutex_lock(&priv->lock);
+
+ return 0;
}
Cheers,
Biju
