Hello Biju,
just a few minor issues left---see below ...
On Thu, Aug 08, 2024 at 02:16:19PM +0100, Biju Das wrote:
> +static int rzg2l_gpt_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> + u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> +
> + mutex_lock(&rzg2l_gpt->lock);
> + rzg2l_gpt->user_count[ch]++;
> + mutex_unlock(&rzg2l_gpt->lock);
Please consider using guard(mutex)(&rzg2l_gpt->lock);
> +
> + return 0;
> +}
> +
> [...]
> +static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8 prescale)
> +{
> + return min_t(u64, (period_or_duty_cycle + (1 << (2 * prescale)) - 1) >> (2 * prescale),
> + U32_MAX);
> +}
Hmm, why does this round up? This is used during .apply() and converts a
nanosecond value to a register value. This should be rounded down. What
am I missing? (Maybe a code comment with an explanation :-)
> [...]
> +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> + bool enabled = pwm->state.enabled;
> + int ret;
> +
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + /*
> + * Probe() sets bootloader_enabled_channels. In such case,
.probe() ?
> + * clearing the flag will avoid errors during unbind.
> + */
> + if (enabled && rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm])
> + rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = false;
Hmm, not 100% sure, but I think if rzg2l_gpt_config() below fails,
cleaning this flag is wrong.
Does rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] == true imply
enabled == true? If so, the if condition can be simplified to just the
right hand side of the &&. Then even an unconditional assignment works,
because
rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = false;
is a nop if the flag is already false.
> + if (!state->enabled) {
> + if (enabled) {
> + rzg2l_gpt_disable(rzg2l_gpt, pwm);
> + pm_runtime_put_sync(pwmchip_parent(chip));
> + }
> +
> + return 0;
> + }
> +
> + if (!enabled) {
> + ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
> + if (ret)
> + return ret;
> + }
> +
> + mutex_lock(&rzg2l_gpt->lock);
> + ret = rzg2l_gpt_config(chip, pwm, state);
> + mutex_unlock(&rzg2l_gpt->lock);
> + if (!ret && !enabled)
> + ret = rzg2l_gpt_enable(rzg2l_gpt, pwm);
> +
> + return ret;
> +}
> +
> [...]
> +
> +static int rzg2l_gpt_probe(struct platform_device *pdev)
> +{
> + struct rzg2l_gpt_chip *rzg2l_gpt;
> + struct device *dev = &pdev->dev;
> + struct reset_control *rstc;
> + struct pwm_chip *chip;
> + unsigned long rate;
> + struct clk *clk;
> + int ret;
> + u32 i;
> +
> + chip = devm_pwmchip_alloc(dev, RZG2L_MAX_PWM_CHANNELS, sizeof(*rzg2l_gpt));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> + rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> +
> + rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(rzg2l_gpt->mmio))
> + return PTR_ERR(rzg2l_gpt->mmio);
> +
> + rstc = devm_reset_control_get_exclusive(dev, NULL);
> + if (IS_ERR(rstc))
> + return dev_err_probe(dev, PTR_ERR(rstc), "get reset failed\n");
> +
> + clk = devm_clk_get(dev, NULL);
Please use devm_clk_get_enabled() here. Otherwise the result of
clk_get_rate() isn't well-defined.
> + if (IS_ERR(clk))
> + return dev_err_probe(dev, PTR_ERR(clk), "cannot get clock\n");
> +
> + ret = reset_control_deassert(rstc);
> + if (ret)
> + return dev_err_probe(dev, ret, "cannot deassert reset control\n");
> +
> + ret = devm_add_action_or_reset(dev, rzg2l_gpt_reset_assert, rstc);
> + if (ret)
> + return ret;
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return ret;
> [...]
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
