Received-SPF: pass (google.com: domain of linux-kernel+bounces-140977-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 11 Apr 2024 17:37:26 +0200
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, 
	dlechner@baylibre.com, Drew Fustini <dfustini@baylibre.com>, 
	Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Subject: Re: [PATCH 2/2 v4] pwm: Add driver for AXI PWM generator
Message-ID: <7i44urixaxohcae44gleqm6tvgterwjz7kerbozijxzrau7czd@bl6rkkmpes3u>
References: <20240301173343.1086332-1-tgamblin@baylibre.com>
 <20240301173343.1086332-3-tgamblin@baylibre.com>
 <zwer5e7jmrqfi3yeooandpfyhs663i3adcaxjeizmwgmfxltp3@gnpimhyoaqjf>
 <f2579349-2cb0-434a-bae1-493218a62d53@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="z5pq6rhmfbngiav5"
Content-Disposition: inline
In-Reply-To: <f2579349-2cb0-434a-bae1-493218a62d53@baylibre.com>


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

Hello,

On Thu, Apr 11, 2024 at 10:07:54AM -0400, Trevor Gamblin wrote:
> On 2024-04-11 12:59, Uwe Kleine-K=F6nig wrote:
> > > + * - Writing LOAD_CONFIG also has the effect of re-synchronizing all
> > > + *   enabled channels, which could cause glitching on other channels=
=2E It
> > > + *   is therefore expected that channels are assigned harmonic perio=
ds
> > > + *   and all have a single user coordinating this.
> > What does "re-synchronize" mean here? Are all counters reset to zero?
> > "harmonic" means that all channels should use the same period length?
> Yes, it means that all counters are reset to zero. Harmonic in this case
> means that channels can have different periods, but they should be integer
> multiples of each other. Should I rewrite the comment to be more explicit?

I hesitate to say "yes, please be more specific" because I think it's
mood. If all pwm lines restart with their counter =3D 0 as soon as one
line is reconfigured (without completing the current period) being a
multiple of each other doesn't help at all. So I think the right thing
to write there is:

 - Reconfiguring a channel doesn't complete the currently running period
   and resets the counters of all other channels and so very likely
   introduces glitches on these unrelated outputs.

(Even if the period was completed, and only assuming configuration
updates that don't modify the period, all channels that don't have a
period that is a divider of the just configured line (might) glitch. So
if you have one PWM with period =3D 200 and another with period =3D 400,
everything is fine if you update the latter, however updating the former
might make the latter glitch. So essentially you need to have a single
period for all channels. That's why I asked if "harmonic" means that all
channels should use the same period.)

> > Reading https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen I would
> > have expected:
> >=20
> > 	/* ch in { 0, ... 15 } */
> > 	#define AXI_PWMGEN_REG_PULSE_X_PERIOD(ch)	(0x40 + 4 * (ch))
> > 	#define AXI_PWMGEN_REG_PULSE_X_WIDTH(ch)	(0x80 + 4 * (ch))
> > 	#define AXI_PWMGEN_REG_PULSE_X_OFFSET		(0xc0 + 4 * (ch))
>=20
> The regmap you find there now reflects v2 of the pwmgen IP; v1 used a step
> of 12 instead of 4. The v2 series sent a little bit later on adds this ex=
tra
> support: https://lore.kernel.org/linux-pwm/20240314204722.1291993-1-tgamb=
lin@baylibre.com/
>=20
> I've added support for both versions since v1 of the IP could still be in
> use on some devices. Would it be better to have the two patch series
> squashed together into a v5 of the axi-pwmgen driver?

Not necessarily squashed, but I suggest to send them in a single series.
(Note, this doesn't mean "Don't squash". I didn't look at the other
series yet, so make a sensible choice yourself (or wait until I come
around reviewing that other series and hope that I remember the context
to comment about this question. :-)
=20
Best regards
Uwe

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

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

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

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmYYA7UACgkQj4D7WH0S
/k42rAf/dYYgtWgQCwgoIgEbZ3xDBVCMESJkV0AhyE6HM01lBRbUSA5YWTE3kV6b
opqkoV8PJn0Vc4l0cUkKrW/dtLYdn53XYIzu0G04IJLGN2qpj73UxAl/mcl3r+HW
5UcYF9SQiXF+xcalszH3x2q7g9Lk4YA2j19Jv1322HWp0PIQ59uD5a/MsDhzvsnr
e9ZlQ8nnvFVIDozffW+Dzi8TwisbywGvo8IEBtyW8/PQPlNtmvdKfTBiR/1Cjg8p
wS6wmlYNqqVBIeCMRWr377gE/16YAUCs7uzksJ33B5BuNT8NlkfmAI+2U5QD5jni
4B2Pqu0G5QpOwMipw/RLhIftklyaeQ==
=H5Ix
-----END PGP SIGNATURE-----

--z5pq6rhmfbngiav5--