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

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi William Breathitt Gray,

Thanks for the feedback.

> -----Original Message-----
> From: William Breathitt Gray <william.gray@xxxxxxxxxx>
> Sent: 15 November 2022 04: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
> 
> On Mon, Nov 14, 2022 at 05:52:11PM +0000, Biju Das wrote:
> > > > > +static int rz_mtu3_initialize_counter(struct counter_device
> > > > > +*counter, int id) {
> > > > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > > > +	struct rz_mtu3_channel *ch1 = priv->ch;
> > > > > +	struct rz_mtu3_channel *ch2 = ch1 + 1;
> > > >
> > > > No need to complicate this, just use priv->ch[0], priv->ch[1],
> and
> > > > priv->ch[id]. Same advice applies to the other functions as
> well.
> > >
> > > I get below error when I use array susbscripts. "*ch1 = priv-
> >ch[0];"
> >
> > > drivers/counter/rz-mtu3-cnt.c:291:32: error: incompatible types
> when
> > > initialising type 'struct rz_mtu3_channel *' using type 'struct
> > > rz_mtu3_channel'
> > >   291 |  struct rz_mtu3_channel *ch1 = priv->ch[0];
> > >
> >
> > I could use "*ch1 = &priv->ch[0];" please let me know is it ok?
> >
> > Cheers,
> > Biju
> 
> Hi Biju,
> 
> I meant to use the array subscripts inline (e.g. priv-
> >ch[id].function).
> However, I can see the benefit of using the ch1 and ch2 local
> variables, so perhaps something like this would be clearer to read:
> 
>     struct rz_mtu3_chanel *const ch = priv->ch;
>     struct rz_mtu3_chanel *const ch1 = &ch[0];
>     struct rz_mtu3_chanel *const ch2 = &ch[1];
>     ...
>     case RZ_MTU3_16_BIT_MTU1_CH:
>     case RZ_MTU3_16_BIT_MTU2_CH:
>             if (ch[id].function != RZ_MTU3_NORMAL) {
>     ...


OK, I have added below inline function which simplifies the code
in each function. Is it ok?

For eg:

+static inline struct rz_mtu3_channel *
+rz_mtu3_get_ch(struct counter_device *counter, int id)
+{
+       struct rz_mtu3_cnt *const priv = counter_priv(counter);
+       const size_t ch_id = RZ_MTU3_GET_HW_CH(id);
+
+       return &priv->ch[ch_id];
+}


@@ -154,11 +163,10 @@ static int rz_mtu3_count_function_read(struct counter_device *counter,
                                       struct counter_count *count,
                                       enum counter_function *function)
 {
-       struct rz_mtu3_cnt *const priv = counter_priv(counter);
-       const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
+       struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count->id);


Cheers,
Biju