Hello Trevor,
On Wed, Jan 17, 2024 at 07:51:26AM -0500, Trevor Gamblin wrote:
> On 2024-01-15 16:18, Uwe Kleine-König wrote:
> > On Mon, Jan 15, 2024 at 03:12:21PM -0500, Trevor Gamblin wrote:
> > > +#define AXI_PWMGEN_TEST_DATA 0x5A0F0081
> > Is this a documented constant, or just a random (as in xkcd#221) value?
>
> This is just a random (as in xkcd#221) value to write to the scratch
> register.
Then I suggest to add a comment telling that.
> > > + period_cnt = DIV_ROUND_UP_ULL(state->period * clk_rate_hz, NSEC_PER_SEC);
> > The multiplication might overflow. Please use mul_u64_u64_div_u64() (or
> > one of its variant) and error out on clk_rate_hz > NSEC_PER_SEC.
> >
> > Also round-up is wrong. I would expect that enabling PWM_DEBUG and
> > enough testing should tell you that. .apply is supposed to implement the
> > biggest period not bigger than the requested one. So you have to round
> > down here.
> To be clear, I should use mul_u64_u64_div_u64 only, or should I round down
> afterwards with another function as well?
mul_u64_u64_div_u64() already rounds down.
> > > + if (period_cnt > UINT_MAX)
> > > + return -EINVAL;
> > That's wrong. Please continue with period_cnd = UINT_MAX here.
> >
> > Instead you should probably error out on period_cnt == 0.
> >
> > > + pwm->ch_period[ch] = period_cnt;
> > > + pwm->ch_enabled[ch] = state->enabled;
> > > + ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), state->enabled ? period_cnt : 0);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + duty_cnt = DIV_ROUND_UP_ULL(state->duty_cycle * clk_rate_hz, NSEC_PER_SEC);
> > > + ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
> > I assume this means that the writes above are to shadow registers and on
> > this write they are latched into the hardware. So there is no glitch?!
> >
> > Does this wait for the currently running period to complete before
> > switching to the new configuration?
> >
> > Please document these two hardware properties in a "Limitations"
> > paragraph at the top of the driver. See other drivers for the format.
>
> The registers are shadow registers and changes don't take effect right away.
> It also keeps the phase offset in sync between outputs.
I don't understand that. This only makes sense if the different PWMs
share the same period length, doesn't it?
Please add this to the Limitations paragraph, too.
> > > +static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *device,
> > > + struct pwm_state *state)
> > > +{
> > > + struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
> > > + unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
> > > + struct regmap *regmap = pwm->regmap;
> > > + unsigned int ch = device->hwpwm;
> > > + u32 cnt;
> > > + int ret;
> > > +
> > > + if (!clk_rate_hz) {
> > > + dev_err(device->chip->dev, "axi pwm clock has no frequency\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + state->enabled = pwm->ch_enabled[ch];
> > > +
> > > + if (state->enabled) {
> > > + ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
> > > + if (ret)
> > > + return ret;
> > > + } else {
> > > + cnt = pwm->ch_period[ch];
> > > + }
> > If state->enabled is false, state->period is (or should) be ignored by
> > the caller, so there shouldn't be a need to track ch_period.
> >
> > Also ch_enabled shouldn't be needed. Just reporting
> > AXI_PWMGEN_CHX_PERIOD(ch) == 0 as disabled should work fine?!
> >
> > I think then you also don't need to artificially limit npwm to four.
> The only concern I have with not tracking ch_period is that it might not be
> clear what period to restore after disabling and re-enabling the device,
> unless I'm missing something.
A consumer that reenables the device should use pwm_apply_* which gets
the complete state that should be configured. (And for the few other
cases, this is solved in the core.)
> > > + pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > > + if (IS_ERR(pwm->clk))
> > > + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk), "failed to get clock\n");
> > Please call clk_rate_exclusive_get() on pwm->clk and cache the rate in
> > struct axi_pwmgen.
> >
> > > + pwm->chip.dev = &pdev->dev;
> > > + pwm->chip.ops = &axi_pwmgen_pwm_ops;
> > > + pwm->chip.base = -1;
> > Don't assign .base.
> Alright. I will set pwm->chip.atomic as per Sean's comment as well.
Sounds good.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
