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

[Pavel Machek <pavel@denx.de>]

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

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?

> +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.

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

And here.

> +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). 

> +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?

> +disable_clock:
> +	clk_disable_unprepare(priv->clk);
> +
> +	return ret;
> +}

Do we need pm_runtime_disable here?

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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