Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v12 2/2] pwm: Add support for RZ/G2L GPT
>
> Hi Biju,
>
> On Wed, Dec 14, 2022 at 2:22 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>
> > ---
> > v11->v12:
> > * Added return code for get_state()
> > * Cache duty cycle/prescale as the driver cannot read the current duty
> > cycle/prescale from the hardware if the hardware is disabled. Cache the
> > last programmed duty cycle/prescale value to return in that case.
> > * Updated rzg2l_gpt_enable to enable the clocks.
> > * Updated rzg2l_gpt_disable to disable the clocks.
> > * Updated rzg2l_gpt_config() to cache duty cucle/prescale value
> > * Updated rzg2l_gpt_get_state to use cached value of duty
> cycle/prescale,If the PWM
> > is disabled.
> > * Simplified rzg2l_gpt_apply()
> > * Added comments in rzg2l_gpt_reset_assert_pm_disable()
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
>
> > +struct rzg2l_gpt_chip {
> > + struct pwm_chip chip;
> > + void __iomem *mmio;
> > + struct reset_control *rstc;
> > + struct clk *clk;
> > + struct mutex lock;
> > + unsigned long rate;
> > + u32 state_period[RZG2L_MAX_HW_CHANNELS];
> > + u32 user_count[RZG2L_MAX_HW_CHANNELS];
> > + /*
> > + * The driver cannot read the current duty cycle/prescale from the
> > + * hardware if the hardware is disabled. Cache the last programmed
> > + * duty cycle/prescale value to return in that case.
> > + */
> > + u8 prescale[RZG2L_MAX_HW_CHANNELS];
> > + unsigned int duty_cycle[RZG2L_MAX_PWM_CHANNELS];
>
> u32? The maximum value stored is U32_MAX.
Agreed.
>
> > +};
>
> > +static int rzg2l_gpt_get_state(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + struct pwm_state *state) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + u32 ch_index = RZG2L_GET_CH_INDEX(pwm->hwpwm);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch_index);
> > + u8 prescale;
> > + u64 tmp;
> > + u32 val;
> > +
> > + pm_runtime_get_sync(chip->dev);
> > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTCR);
> > + state->enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm);
> > + if (state->enabled) {
> > + prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTPR);
> > + tmp = NSEC_PER_SEC * (u64)val;
> > + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate)
> > + << (2 * prescale);
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt,
> > + offs + RZG2L_GTCCR(RZG2L_IS_IOB(pwm-
> >hwpwm)));
> > + tmp = NSEC_PER_SEC * (u64)val;
> > + } else {
> > + /* If the PWM is disabled, use the cached value. */
> > + prescale = rzg2l_gpt->prescale[ch_index];
> > + tmp = NSEC_PER_SEC *
> > + (u64)rzg2l_gpt->duty_cycle[pwm->hwpwm];
>
> Nit: Just set "val = rzg2l_gpt->duty_cycle[pwm->hwpwm];", and factor "tmp =
> NSEC_PER_SEC * (u64)val;" out of the if-statement.
OK.
>
> > + }
> > +
> > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate) << (2 *
> prescale);
> > + state->polarity = PWM_POLARITY_NORMAL;
> > + pm_runtime_put(chip->dev);
> > +
> > + return 0;
> > +}
>
> > +static int __maybe_unused rzg2l_gpt_pm_runtime_suspend(struct device
> > +*dev) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(rzg2l_gpt->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused rzg2l_gpt_pm_runtime_resume(struct device
> > +*dev) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt = dev_get_drvdata(dev);
> > +
> > + clk_prepare_enable(rzg2l_gpt->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops rzg2l_gpt_pm_ops = {
> > + SET_RUNTIME_PM_OPS(rzg2l_gpt_pm_runtime_suspend,
> > +rzg2l_gpt_pm_runtime_resume, NULL) };
>
> DEFINE_RUNTIME_DEV_PM_OPS(), so you can drop the __maybe_unused from the
> callbacks.
Agreed.
>
> > +static struct platform_driver rzg2l_gpt_driver = {
> > + .driver = {
> > + .name = "pwm-rzg2l-gpt",
> > + .pm = &rzg2l_gpt_pm_ops,
>
> pm_ptr(&rzg2l_gpt_pm_ops)?
OK.
>
> > + .of_match_table = of_match_ptr(rzg2l_gpt_of_table),
> > + },
> > + .probe = rzg2l_gpt_probe,
> > +};
> > +module_platform_driver(rzg2l_gpt_driver);
Will send V13 with below changes as well.
1) Replaced the dependency from ARCH_RENESAS->ARCH_RZG2L in Kconfig
2) Sorted the header #include <linux/limits.h> alphabetically
3) Added a comment in mutex_lock to fix check patch warning
4) Removed the unwanted code from rzg2l_gpt_config.
- duty_cycles = state->duty_cycle;
- if (!state->enabled)
- duty_cycles = 0;
-
Cheers,
Biju
