Hello,

On Thu, Mar 30, 2023 at 12:16:32PM +0100, Biju Das wrote:
> +static struct rz_mtu3_pwm_channel *
> +rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
> +{
> +	struct rz_mtu3_pwm_channel *priv = rz_mtu3_pwm->channel_data;
> +	unsigned int ch;
> +
> +	for (ch = 0; ch < RZ_MTU3_MAX_HW_CHANNELS; ch++, priv++) {
> +		if (priv->map->channel + priv->map->num_channel_ios >  hwpwm)

s/  / /

> +			break;
> +	}
> +
> +	return priv;
> +}
> +
> +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
> +				      u32 hwpwm)
> +{
> +	struct rz_mtu3_pwm_channel *priv;
> +	bool is_channel_en;
> +	u8 val;
> +
> +	priv = rz_mtu3_get_channel(rz_mtu3_pwm, hwpwm);
> +	is_channel_en = rz_mtu3_is_enabled(priv->mtu);
> +	if (!is_channel_en)
> +		return false;
> +
> +	if (priv->map->channel == hwpwm)
> +		val = rz_mtu3_8bit_ch_read(priv->mtu, RZ_MTU3_TIORH);
> +	else
> +		val = rz_mtu3_8bit_ch_read(priv->mtu, RZ_MTU3_TIORL);
> +
> +	return (is_channel_en && (val & RZ_MTU3_TIOR_IOA));

Here you already know that is_channel_en == true, so it can be dropped
here.

> +}
> +
> [...]
> +static int rz_mtu3_pwm_enable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
> +			      struct pwm_device *pwm)
> +{
> +	struct rz_mtu3_pwm_channel *priv;
> +	u32 ch;
> +	u8 val;
> +	int rc;
> +
> +	rc = pm_runtime_resume_and_get(rz_mtu3_pwm->chip.dev);
> +	if (rc)
> +		return rc;
> +
> +	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> +	ch = priv - rz_mtu3_pwm->channel_data;
> +	val = RZ_MTU3_TIOR_OC_IOB_TOGGLE | RZ_MTU3_TIOR_OC_IOA_H_COMP_MATCH;
> +
> +	rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_PWMMODE1);
> +	if (priv->map->channel == pwm->hwpwm)

This condition identifies the first PWM of a channel. I was surprised
here that the channel numbering has a hole after each channel that
manages two IOs. OTOH, in the comment at the top of the driver there is:

	(The channels are MTU{0..4, 6, 7}.)

which I would have expected to see in channel_map. I think the first
member of this struct is really the base pwm number and so is misnamed?

> +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORH, val);
> +	else
> +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORL, val);
> +
> +	mutex_lock(&rz_mtu3_pwm->lock);
> +	if (!rz_mtu3_pwm->enable_count[ch])
> +		rz_mtu3_enable(priv->mtu);
> +
> +	rz_mtu3_pwm->enable_count[ch]++;
> +	mutex_unlock(&rz_mtu3_pwm->lock);
> +
> +	return 0;
> +}
> +
> +static void rz_mtu3_pwm_disable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
> +				struct pwm_device *pwm)
> +{
> +	struct rz_mtu3_pwm_channel *priv;
> +	u32 ch;
> +
> +	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> +	ch = priv - rz_mtu3_pwm->channel_data;
> +
> +	/* Return to normal mode and disable output pins of MTU3 channel */
> +	if (rz_mtu3_pwm->user_count[ch] <= 1)
> +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_NORMAL);

This never triggers if both PWMs of a channel are requested, even if
both are disabled. Is this intended?

> +	if (priv->map->channel == pwm->hwpwm)
> +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORH, RZ_MTU3_TIOR_OC_RETAIN);
> +	else
> +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORL, RZ_MTU3_TIOR_OC_RETAIN);
> +
> +	mutex_lock(&rz_mtu3_pwm->lock);
> +	rz_mtu3_pwm->enable_count[ch]--;
> +	if (!rz_mtu3_pwm->enable_count[ch])
> +		rz_mtu3_disable(priv->mtu);
> +
> +	mutex_unlock(&rz_mtu3_pwm->lock);
> +
> +	pm_runtime_put_sync(rz_mtu3_pwm->chip.dev);
> +}
> +
> [...]
> +static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      const struct pwm_state *state)
> +{
> +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
> +	struct rz_mtu3_pwm_channel *priv;
> +	unsigned long pv, dc;
> +	u64 period_cycles;
> +	u64 duty_cycles;
> +	u8 prescale;
> +	u8 val;
> +
> +	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> +	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
> +					NSEC_PER_SEC);
> +	prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm, period_cycles);
> +
> +	if (period_cycles >> (2 * prescale) <= U16_MAX)
> +		pv = period_cycles >> (2 * prescale);
> +	else
> +		pv = U16_MAX;
> +
> +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm->rate,
> +				      NSEC_PER_SEC);
> +	if (duty_cycles >> (2 * prescale) <= U16_MAX)
> +		dc = duty_cycles >> (2 * prescale);
> +	else
> +		dc = U16_MAX;
> +
> +	/*
> +	 * If the PWM channel is disabled, make sure to turn on the clock
> +	 * before writing the register.
> +	 */
> +	if (!pwm->state.enabled)
> +		pm_runtime_get_sync(chip->dev);
> +
> +	val = RZ_MTU3_TCR_CKEG_RISING | prescale;
> +	if (priv->map->channel == pwm->hwpwm) {
> +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> +				      RZ_MTU3_TCR_CCLR_TGRA | val);

If the sibling PWM on the same channel is on, you're overwriting its
prescale value here, are you not?

> +		rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRB, dc);
> +		rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRA, pv);
> +	} else {
> +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> +				      RZ_MTU3_TCR_CCLR_TGRC | val);
> +		rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRD, dc);
> +		rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRC, pv);
> +	}
> +
> +	/* If the PWM is not enabled, turn the clock off again to save power. */
> +	if (!pwm->state.enabled)
> +		pm_runtime_put(chip->dev);
> +
> +	return 0;
> +}
> +
> [...]
> +static int rz_mtu3_pwm_probe(struct platform_device *pdev)
> +{
> +	struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);

Nitpick: I would have called that parent_ddata.

> +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm;
> +	struct device *dev = &pdev->dev;
> +	unsigned int i, j = 0;
> +	int ret;
> +
> +	rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm), GFP_KERNEL);
> +	if (!rz_mtu3_pwm)
> +		return -ENOMEM;
> +
> +	rz_mtu3_pwm->clk = ddata->clk;
> +
> +	for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) {
> +		if (i == RZ_MTU3_CHAN_5 || i == RZ_MTU3_CHAN_8)
> +			continue;
> +
> +		rz_mtu3_pwm->channel_data[j].mtu = &ddata->channels[i];
> +		rz_mtu3_pwm->channel_data[j].mtu->dev = dev;
> +		rz_mtu3_pwm->channel_data[j].map = &channel_map[j];
> +		j++;
> +	}
> +

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |