> -----Original Message-----
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Sent: 09 January 2023 14:28
> To: Geert Uytterhoeven <geert@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 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;
> +}
It will be updated as,
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)
{
mutex_lock(&priv->lock);
if (ch->is_busy && !priv->count_is_enabled[id]) {
mutex_unlock(&priv->lock);
return -EINVAL;
}
if (rz_mtu3_is_counter_invalid(counter, id)) {
mutex_unlock(&priv->lock);
return -EBUSY;
}
return 0;
}
Cheers,
Biju
