Hi Biju,
On Tue, Jul 19, 2022 at 5:59 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
> (GPT32E). It supports the following functions
> * 32 bits × 8 channels
> * Up-counting or down-counting (saw waves) or up/down-counting
> (triangle waves) for each counter.
> * Clock sources independently selectable for each channel
> * Two I/O pins per channel
> * Two output compare/input capture registers per channel
> * For the two output compare/input capture registers of each channel,
> four registers are provided as buffer registers and are capable of
> operating as comparison registers when buffering is not in use.
> * In output compare operation, buffer switching can be at crests or
> troughs, enabling the generation of laterally asymmetric PWM waveforms.
> * Registers for setting up frame cycles in each channel (with capability
> for generating interrupts at overflow or underflow)
> * Generation of dead times in PWM operation
> * Synchronous starting, stopping and clearing counters for arbitrary
> channels
> * Starting, stopping, clearing and up/down counters in response to input
> level comparison
> * Starting, clearing, stopping and up/down counters in response to a
> maximum of four external triggers
> * Output pin disable function by dead time error and detected
> short-circuits between output pins
> * A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
> * Enables the noise filter for input capture and external trigger
> operation
>
> This patch adds basic pwm support for RZ/G2L GPT driver by creating
> separate logical channels for each IOs.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v2->v3:
Thanks for the update!
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> +static u8 rzg2l_calculate_prescale(struct rzg2l_gpt_chip *pc, u64 period_cycles)
> +{
> + u64 prescaled_period_cycles;
> + u8 prescale;
> + u16 i;
Why do prescale and i differ in type?
Why not use unsigned int?
> +
> + prescaled_period_cycles = period_cycles >> 32;
So prescaled_period_cycles can be u32?
> + prescale = 5;
> + /* prescale 1, 4, 16, 64, 256 and 1024 */
> + for (i = 0; i < 6; i++) {
> + if ((1 << (2 * i)) > prescaled_period_cycles) {
> + prescale = i;
> + break;
> + }
> + }
> +
> + return prescale;
> +}
> +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + u64 duty_ns, u64 period_ns)
> +{
> + struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> + struct gpt_pwm_device *gpt = &pc->gpt[pwm->hwpwm];
> + unsigned long rate, pv, dc;
> + u64 period_cycles;
> + u8 prescale;
> +
> + rate = clk_get_rate(pc->clk);
> + /*
> + * Refuse clk rates > 1 GHz to prevent overflowing the following
> + * calculation.
> + */
> + if (rate > NSEC_PER_SEC)
> + return -EINVAL;
> +
> + period_cycles = mul_u64_u64_div_u64(rate, period_ns, NSEC_PER_SEC);
As NSEC_PER_SEC fits in 32-bit, and we know rate does, too, you can
use the cheaper mul_u64_u32_div(period_ns, rate, NSEC_PER_SEC) instead.
> + prescale = rzg2l_calculate_prescale(pc, period_cycles);
> +
> + pv = round_down(period_cycles >> (2 * prescale), 1 << (2 * prescale));
> + period_cycles = mul_u64_u64_div_u64(rate, duty_ns, NSEC_PER_SEC);
Likewise.
> + dc = round_down(period_cycles >> (2 * prescale), 1 << (2 * prescale));
Why are pv and dc unsigned long, which is either 32-bit or 64-bit?
> +static const struct of_device_id rzg2l_gpt_of_table[] = {
> + { .compatible = "renesas,rzg2l-gpt", },
> + { /* Sentinel */ },
Please drop the comma after the sentinel.
> +};
> +MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
> +static int rzg2l_gpt_probe(struct platform_device *pdev)
> +{
> + struct rzg2l_gpt_chip *rzg2l_gpt;
> + struct clk *clk;
> + int ret;
> +
> + rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt), GFP_KERNEL);
> + if (!rzg2l_gpt)
> + return -ENOMEM;
> +
> + rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(rzg2l_gpt->mmio))
> + return PTR_ERR(rzg2l_gpt->mmio);
> +
> + rzg2l_gpt->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> + if (IS_ERR(rzg2l_gpt->rstc))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->rstc),
> + "get reset failed\n");
> +
> + rzg2l_gpt->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(rzg2l_gpt->clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->clk),
> + "cannot get clock\n");
> +
> + platform_set_drvdata(pdev, rzg2l_gpt);
> +
> + ret = reset_control_deassert(rzg2l_gpt->rstc);
> + if (ret) {
> + dev_err(&pdev->dev, "cannot deassert reset control: %pe\n",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> +
> + ret = devm_add_action_or_reset(&pdev->dev,
> + rzg2l_gpt_reset_assert_pm_disable,
> + rzg2l_gpt);
> + if (ret < 0)
> + return ret;
> +
> + clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
Why optional?
You already have a pointer to this clock?
> + if (IS_ERR(clk)) {
> + rzg2l_gpt_reset_assert_pm_disable(rzg2l_gpt);
Isn't this called automatically, due to devm_add_action_or_reset()?
> + return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> + "clk operation failed");
> + }
> +
> + if (!(rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST))
> + devm_clk_put(&pdev->dev, clk);
This is done to keep the clock enabled in case it was enabled before?
I think this deserves a comment.
> +
> + rzg2l_gpt->chip.dev = &pdev->dev;
> + rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> + rzg2l_gpt->chip.npwm = RZG2L_GPT_IO_PER_CHANNEL;
> +
> + return devm_pwmchip_add(&pdev->dev, &rzg2l_gpt->chip);
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
