Hi Uwe,
Thanks for the feedback.
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> Sent: Wednesday, September 25, 2024 4:39 PM
> Subject: Re: [PATCH v21 3/4] pwm: Add support for RZ/G2L GPT
>
> 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);
Agreed. expect rzg2l_gpt_apply() as it will cause
deadlock as rzg2l_gpt_enable acquires same 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 :-)
I agree, this must be round down. Will use DIV_ROUND_DOWN_ULL() like other PWM drivers.
>
> > [...]
> > +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.
I am planning to drop "bootloader_enabled_channels" based on your comment in probe()
which simplifies the driver.
>
> > + 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.
Agreed. This will make the driver simpler like other PWM drivers and no
more PM runtime functionalities needed for PWM as the clock is always on.
Cheers,
Biju
