RE: [PATCH 6.1.y-cip 5/8] counter: Add Renesas RZ/G2L MTU3a counter driver

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

Hi Pavel,

> Subject: Re: [PATCH 6.1.y-cip 5/8] counter: Add Renesas RZ/G2L MTU3a
> counter driver
> 
> Hi!
> 
> > commit 0be8907359df4c62319f5cb2c6981ff0d9ebf35a upstream.
> >
> > Add RZ/G2L MTU3a counter driver. This IP supports the following phase
> > counting modes on MTU1 and MTU2 channels
> >
> > 1) 16-bit phase counting modes on MTU1 and MTU2 channels.
> > 2) 32-bit phase counting mode by cascading MTU1 and MTU2 channels.
> >
> > This patch adds 3 counter value channels.
> > 	count0: 16-bit phase counter value channel on MTU1
> > 	count1: 16-bit phase counter value channel on MTU2
> > 	count2: 32-bit phase counter value channel by cascading
> >                 MTU1 and MTU2 channels.
> >
> > The external input phase clock pin for the counter value channels are
> > as follows:
> > 	count0: "MTCLKA-MTCLKB"
> > 	count1: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> > 	count2: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> >
> > Use the sysfs variable "external_input_phase_clock_select" to select
> > the external input phase clock pin and "cascade_counts_enable" to
> > enable/ disable cascading of channels.
> 
> > +static bool rz_mtu3_is_counter_invalid(struct counter_device
> > +*counter, int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	unsigned long tmdr;
> > +
> > +	pm_runtime_get_sync(priv->ch->dev);
> > +	tmdr = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> > +	pm_runtime_put(priv->ch->dev);
> 
> pm_runtime_get/put is unusually fine-grained here.
> 
> Do we need error handling for pm_runtime_get?

No, it is ok. For error handing, if needed, I need to use pm_runtime_resume_and_get.

> 
> > +static int rz_mtu3_count_ceiling_write(struct counter_device
> *counter,
> > +				       struct counter_count *count,
> > +				       u64 ceiling)
> > +{
> ...
> > +	ret = rz_mtu3_lock_if_counter_is_valid(counter, ch, priv, count-
> >id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (count->id) {
> > +	case RZ_MTU3_16_BIT_MTU1_CH:
> > +	case RZ_MTU3_16_BIT_MTU2_CH:
> > +		if (ceiling > U16_MAX)
> > +			return -ERANGE;
> 
> Missing mutex_unlock here.

OK, will backport the patch [1]
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/counter/rz-mtu3-cnt.c?h=v6.4-rc5&id=1e28dfdf40d7293882ba7eb70a3bc0e9ab625377

> 
> > +		priv->mtu_16bit_max[ch_id] = ceiling;
> > +		break;
> > +	case RZ_MTU3_32_BIT_CH:
> > +		if (ceiling > U32_MAX)
> > +			return -ERANGE;
> 
> And here.

Same as above.

> 
> > +static int rz_mtu3_count_enable_write(struct counter_device *counter,
> > +				      struct counter_count *count, u8 enable) {
> > +	struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count-
> >id);
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		pm_runtime_get_sync(ch->dev);
> > +		mutex_lock(&priv->lock);
> > +		ret = rz_mtu3_initialize_counter(counter, count->id);
> > +		if (ret == 0)
> > +			priv->count_is_enabled[count->id] = true;
> > +		mutex_unlock(&priv->lock);
> 
> Elsewhere you did pm_runtime_get inside the lock. (Which might enable
> code cleanups).

Mutex for protecting shared resource

And

PM for register access.

Both are having different functions. Ideally, it should have
done inside the lock as you said for consistency.

> 
> > +static int rz_mtu3_cnt_probe(struct platform_device *pdev) {
> ...
> > +	mutex_init(&priv->lock);
> > +	platform_set_drvdata(pdev, priv->clk);
> > +	clk_prepare_enable(priv->clk);
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +	ret = devm_add_action_or_reset(&pdev->dev, rz_mtu3_cnt_pm_disable,
> dev);
> > +	if (ret < 0)
> > +		goto disable_clock;
> 
> Should we have dev_err_probe here?

I guess, it is not required unless there is a valid reason.

> 
> > +disable_clock:
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return ret;
> > +}
> 
> Do we need pm_runtime_disable here?

Not required. On error case, the action_or_reset will call rz_mtu3_cnt_pm_disable.

Cheers,
Biju