RE: [PATCH 6.1.y-cip 3/8] mfd: Add Renesas RZ/G2L MTU3a core driver

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Pavel,

Thanks for the feedback.

> Subject: Re: [PATCH 6.1.y-cip 3/8] mfd: Add Renesas RZ/G2L MTU3a core
> driver
> 
> Hi!
> 
> > 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 and Clock event.
> 
> 
> 
> > +static void rz_mtu3_start_stop_ch(struct rz_mtu3_channel *ch, bool
> > +start) {
> > +	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> > +	struct rz_mtu3_priv *priv = mtu->priv_data;
> > +	unsigned long flags, tstr;
> > +	u16 offset;
> > +	u8 bitpos;
> > +
> > +	/* start stop register shared by multiple timer channels */
> > +	raw_spin_lock_irqsave(&priv->lock, flags);
> > +
> > +	offset = rz_mtu3_get_tstr_offset(ch);
> > +	bitpos = rz_mtu3_get_tstr_bit_pos(ch);
> 
> These two can be done outside raw_spin_lock protection.

Agreed.

> 
> > +	tstr = rz_mtu3_shared_reg_read(ch, offset);
> > +	__assign_bit(bitpos, &tstr, start);
> > +	rz_mtu3_shared_reg_write(ch, offset, tstr);
> > +
> > +	raw_spin_unlock_irqrestore(&priv->lock, flags); }
> > +
> > +bool rz_mtu3_is_enabled(struct rz_mtu3_channel *ch) {
> > +	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> > +	struct rz_mtu3_priv *priv = mtu->priv_data;
> > +	unsigned long flags, tstr;
> > +	bool ret = false;
> > +	u16 offset;
> > +	u8 bitpos;
> > +
> > +	/* start stop register shared by multiple timer channels */
> > +	raw_spin_lock_irqsave(&priv->lock, flags);
> > +
> > +	offset = rz_mtu3_get_tstr_offset(ch);
> > +	bitpos = rz_mtu3_get_tstr_bit_pos(ch);
> 
> These two can be done outside raw_spin_lock protection.

Agreed.
> 
> > +	tstr = rz_mtu3_shared_reg_read(ch, offset);
> > +	ret = tstr & BIT(bitpos);
> 
> As can be the ret computation.

Agreed. You mean "return tstr & BIT(bitpos);"??

Cheers,
Biju