Received-SPF: pass (google.com: domain of linux-kernel+bounces-29070-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 17 Jan 2024 15:18:27 +0100
From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <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
Message-ID: <rph3di2izoqdlxbj2dzdt7nydvmze373pazeopmim5bny4d7qi@d2fus3wzuhj7>
References: <20240115201222.1423626-1-tgamblin@baylibre.com>
 <20240115201222.1423626-3-tgamblin@baylibre.com>
 <gbessnmierg5gvdguhwauoe2mxr3krwcfk2afhazrqvz45md64@itbchezepncg>
 <e6fbf553-f2f8-47ea-8781-cf01d37196a5@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="az43kqsaevecs6w2"
Content-Disposition: inline
In-Reply-To: <e6fbf553-f2f8-47ea-8781-cf01d37196a5@baylibre.com>


--az43kqsaevecs6w2
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello Trevor,

On Wed, Jan 17, 2024 at 07:51:26AM -0500, Trevor Gamblin wrote:
> On 2024-01-15 16:18, Uwe Kleine-K=F6nig 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?
>=20
> 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 =3D DIV_ROUND_UP_ULL(state->period * clk_rate_hz, NSEC_P=
ER_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.
> >=20
> > 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 =3D UINT_MAX here.
> >=20
> > Instead you should probably error out on period_cnt =3D=3D 0.
> >=20
> > > +	pwm->ch_period[ch] =3D period_cnt;
> > > +	pwm->ch_enabled[ch] =3D state->enabled;
> > > +	ret =3D regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), state->enab=
led ? period_cnt : 0);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	duty_cnt =3D DIV_ROUND_UP_ULL(state->duty_cycle * clk_rate_hz, NSEC=
_PER_SEC);
> > > +	ret =3D 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?!
> >=20
> > Does this wait for the currently running period to complete before
> > switching to the new configuration?
> >=20
> > Please document these two hardware properties in a "Limitations"
> > paragraph at the top of the driver. See other drivers for the format.
>=20
> The registers are shadow registers and changes don't take effect right aw=
ay.
> 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_de=
vice *device,
> > > +				struct pwm_state *state)
> > > +{
> > > +	struct axi_pwmgen *pwm =3D to_axi_pwmgen(chip);
> > > +	unsigned long clk_rate_hz =3D clk_get_rate(pwm->clk);
> > > +	struct regmap *regmap =3D pwm->regmap;
> > > +	unsigned int ch =3D 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 =3D pwm->ch_enabled[ch];
> > > +
> > > +	if (state->enabled) {
> > > +		ret =3D regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
> > > +		if (ret)
> > > +			return ret;
> > > +	} else {
> > > +		cnt =3D 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.
> >=20
> > Also ch_enabled shouldn't be needed. Just reporting
> > AXI_PWMGEN_CHX_PERIOD(ch) =3D=3D 0 as disabled should work fine?!
> >=20
> > 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 =3D 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.
> >=20
> > > +	pwm->chip.dev =3D &pdev->dev;
> > > +	pwm->chip.ops =3D &axi_pwmgen_pwm_ops;
> > > +	pwm->chip.base =3D -1;
> > Don't assign .base.
> Alright. I will set pwm->chip.atomic as per Sean's comment as well.

Sounds good.

Best regards
Uwe

--=20
Pengutronix e.K.                           | Uwe Kleine-K=F6nig            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

--az43kqsaevecs6w2
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmWn4bIACgkQj4D7WH0S
/k54cwf/VyTy7fh72MUwhCHWYDFU0aaQNBu38femzw3/fReWeSnnTuvZUVAzloej
m6x0XoXw8jiJBDyxKXvSA3OETB94CKLDZc5iuREBneSx0rcKGYPxekzdNqwMgp4l
5Du7drDCvISRaUWn9mreacHTraxkroHl6XmZ2szEHagKAxx40ZiiV/yuGqMLlS2q
tuErCfNKVDzMLknNE9qNRRI4+0Qdq2r62u2xFDbQhV3RgfkfKO8g35NrFg1rGlyr
a/kaWf5WgaSXwbNB6iR0JaLc16I0cSueok3cFZUbIo0dNsXUnYoyul7hfySZEQjG
GKbW+wQyNyST4rC7PqpiKMFgJ0+1bg==
=ImPA
-----END PGP SIGNATURE-----

--az43kqsaevecs6w2--