Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v7 2/5] clocksource/drivers: Add Renesas RZ/G2L MTU3a
> core driver
>
> Hi Biju,
>
> On Thu, Nov 24, 2022 at 6:00 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
> > the Renesas RZ/G2L family SoCs. It consists of eight 16-bit timer
> > channels and one 32-bit timer channel. It supports the following
> > functions
> > - Counter
> > - Timer
> > - PWM
> >
> > The 8/16/32 bit registers are mixed in each channel.
> >
> > Add MTU3a core driver for RZ/G2L SoC. The core driver shares the clk
> > and channel register access for the other child devices like Counter,
> > PWM, Clock Source, and Clock event.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v6->v7:
> > * Added channel specific mutex to avoid races between child devices
> > (for eg: pwm and counter)
> > * Added rz_mtu3_shared_reg_update_bit() to update bit.
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/clocksource/rz-mtu3.c
>
> > +void rz_mtu3_shared_reg_update_bit(struct rz_mtu3_channel *ch, u16 off,
> > + u16 pos, u8 val) {
> > + unsigned long tmdr;
> > +
> > + tmdr = rz_mtu3_shared_reg_read(ch, off);
> > + __assign_bit(pos, &tmdr, !!val);
> > + rz_mtu3_shared_reg_write(ch, off, tmdr);
>
> As this is RMW on a shared register, probably this needs locking, here or
> in the callers?
OK will add locking here.
>
> > +}
> > +EXPORT_SYMBOL_GPL(rz_mtu3_shared_reg_update_bit);
>
> > --- /dev/null
> > +++ b/include/clocksource/rz-mtu3.h
>
> > +enum rz_mtu3_functions {
> > + RZ_MTU3_NORMAL,
>
> IIRC, this state means the channel is idle?
> RZ_MTU3_IDLE?
>
> > + RZ_MTU3_16BIT_PHASE_COUNTING,
> > + RZ_MTU3_32BIT_PHASE_COUNTING,
> > + RZ_MTU3_PWM_MODE_1,
>
> Do you need to know the actual function, or is it sufficient to just know
> if the channel is busy? ...
Actually not required. Channel busy is enough.
>
> > +};
> > +
> > +/**
> > + * struct rz_mtu3_channel - MTU3 channel private data
> > + *
> > + * @dev: device handle
> > + * @index: channel index
> > + * @base: channel base address
> > + * @lock: Lock to protect channel function
> > + * @function: channel function
> > + */
> > +struct rz_mtu3_channel {
> > + struct device *dev;
> > + unsigned int index;
> > + void __iomem *base;
> > + struct mutex lock; /* Protect channel function */
> > + enum rz_mtu3_functions function;
>
> ... so perhaps a simple busy flag is sufficient?
>
> Regardless, to avoid replicating the channel function in every subdriver,
> I would add a simple API (can be inline) to request and release a channel.
Agreed, it will simplifies a lot.
Cheers,
Biju
>
> > +};
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
>
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
> -- Linus Torvalds
