From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Trevor Gamblin <tgamblin@baylibre.com>
Cc: linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org,
michael.hennerich@analog.com, nuno.sa@analog.com,
devicetree@vger.kernel.org, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org
Subject: Re: [PATCH 2/2] pwm: Add driver for AXI PWM generator
Date: Wed, 17 Jan 2024 15:18:27 +0100 [thread overview]
Message-ID: <rph3di2izoqdlxbj2dzdt7nydvmze373pazeopmim5bny4d7qi@d2fus3wzuhj7> (raw)
In-Reply-To: <e6fbf553-f2f8-47ea-8781-cf01d37196a5@baylibre.com>
[-- Attachment #1: Type: text/plain, Size: 4795 bytes --]
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
prev parent reply other threads:[~2024-01-17 14:18 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-15 20:12 [PATCH 0/2] pwm: add axi-pwm-gen driver Trevor Gamblin
2024-01-15 20:12 ` [PATCH 1/2] dt-bindings: pwm: Add bindings for AXI PWM generator Trevor Gamblin
2024-01-16 7:38 ` Krzysztof Kozlowski
2024-01-15 20:12 ` [PATCH 2/2] pwm: Add driver " Trevor Gamblin
2024-01-15 21:18 ` Uwe Kleine-König
2024-01-16 8:28 ` Sean Young
2024-01-16 12:49 ` Uwe Kleine-König
2024-01-17 12:51 ` Trevor Gamblin
2024-01-17 14:18 ` Uwe Kleine-König [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=rph3di2izoqdlxbj2dzdt7nydvmze373pazeopmim5bny4d7qi@d2fus3wzuhj7 \
--to=u.kleine-koenig@pengutronix.de \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=michael.hennerich@analog.com \
--cc=nuno.sa@analog.com \
--cc=robh+dt@kernel.org \
--cc=tgamblin@baylibre.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).