> > -----Original Message-----
> > From: William Breathitt Gray <william.gray@xxxxxxxxxx>
> > Sent: 14 November 2022 03:47
> > 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 Sun, Nov 13, 2022 at 05:15:44PM +0000, Biju Das wrote:
> >
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > ---
> > > +#define RZ_MTU3_GET_HW_CH(id) \
> > > +({ \
> > > + size_t _id = (id); _id = (_id == RZ_MTU3_32_BIT_CH) ? 0 : _id; \
> > > +})
> >
> > I probably missed a discussion about this change in a previous thread;
> > what is the purpose of using a local size_t variable here? Is this due
> > to the "possible side-effects" mentioned in the patch changes note?
>
> Check patch is complaining
> "CHECK: Macro argument reuse 'id' - possible side-effects?"
>
> By using local size_t variable, it fixed the check patch warning.
>
> >
> > > +
> > > +#define SIGNAL_A_ID (0)
> > > +#define SIGNAL_B_ID (1)
> > > +#define SIGNAL_C_ID (2)
> > > +#define SIGNAL_D_ID (3)
> > > +
> > > +/**
> > > + * struct rz_mtu3_cnt - MTU3 counter private data
> > > + *
> > > + * @clk: MTU3 module clock
> > > + * @lock: Lock to prevent concurrent access for ceiling and count
> > > + * @ch: HW channels for the counters
> > > + * @mtu_16bit_max: Cache for 16-bit counters
> > > + * @mtu_32bit_max: Cache for 32-bit counters */ struct rz_mtu3_cnt {
> > > + struct clk *clk;
> > > + struct mutex lock;
> > > + struct rz_mtu3_channel *ch;
> > > + u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> > > + u32 mtu_32bit_max;
> >
> > Does the ceiling set on the device get clobbered when you change
> > between 16- bit and 32-bit phase modes (i.e. writing to TGRALW vs
> > TGRA)? You have a separate cache for the 32-bit ceiling value here,
> > but if it is getting clobbered then as a small optimization you may
> > reimplement this cache as a union such as:
> >
> > union {
> > u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> > u32 mtu_32bit_max;
> > }
>
> Yes, it gets clobbered when we change between 16-bit and 32-bit mode.
>
> For eg: 0xbe1352 value
> Split up into mtu1.TGRA=0xbe and mtu2.TGRA=0x1352.
>
> OK will use the union.
>
> >
> > > +};
> > > +
> >
> > > +
> > > + switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> > > + case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> > > + *function = COUNTER_FUNCTION_QUADRATURE_X4;
> > > + break;
> > > + case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> > > + *function = COUNTER_FUNCTION_PULSE_DIRECTION;
> > > + break;
> > > + case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> > > + *function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> >
> > Sorry if I asked this before: what are counting modes 3 and 5, and are
> > they not supported by this device? If they are not supported, please
> > include a comment stating so in the default case block so that it is
> > clear for future reviewers as well.
>
> Our hardware supports 5 phase counting modes. From that list, I match up some
> of the functions supported by the counter driver.
>
> counting modes 3 and 5 are supported by the Devices, but currently counter
> driver is not supported this.
>
> Please see the attached counting modes 3 and 5.
> https://ibb.co/3YJByG1
>
> OK, I will add a comment for the details for modes not supported by the
> current driver in the default block.
>
> > > +
> > > +static void rz_mtu3_32bit_cnt_setting(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;
> > > +
> > > + /*
> > > + * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
> > > + * counter.
> > > + */
> > > + ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > > + ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> >
> > Can these "function" members be modified from outside this driver? If
> > so, you could have a race condition here.
>
> OK will add channel specific locks to avoid the races.
>
> Do you prefer mutex or spin lock here? As channel selection is based on
> runtime decision For both PWM and counter??
>
> >
> > > +
> > > + /* Phase counting mode 1 is used as default in initialization. */
> > > + rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TMDR1,
> > > +RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > > +
> > > + rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> > > + rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_IC_BOTH);
> > > +
> > > + rz_mtu3_enable(ch1);
> > > + rz_mtu3_enable(ch2);
> > > +}
> > > +
> > > +static void rz_mtu3_16bit_cnt_setting(struct counter_device
> > > +*counter, int id) {
> > > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > + struct rz_mtu3_channel *ch = priv->ch + id;
> > > +
> > > + ch->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> > > +
> > > + /* Phase counting mode 1 is used as default in initialization. */
> > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1,
> > > +RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > > +
> > > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> > > + rz_mtu3_enable(ch);
> > > +}
> > > +
> > > +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
