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/ |
Attachment:
signature.asc
Description: PGP signature
