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

[William Breathitt Gray <william.gray@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 3179 bytes --]

On Tue, Nov 15, 2022 at 10:38:32AM +0000, Biju Das wrote:
> Hi William Breathitt Gray,
> 
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: William Breathitt Gray <william.gray@linaro.org>
> > Sent: 15 November 2022 04:53
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: linux-iio@vger.kernel.org; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Chris Paterson
> > <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad
> > <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas-
> > soc@vger.kernel.org
> > 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

Sure, I think that function will be okay to use.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]