Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 8 Dec 2020 14:44:03 +0100
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Clemens Gruber <clemens.gruber@pqgruber.com>
Cc:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-pwm@vger.kernel.org,
        Lee Jones <lee.jones@linaro.org>, linux-kernel@vger.kernel.org,
        Sven Van Asbroeck <TheSven73@gmail.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        David Jander <david@protonic.nl>
Subject: Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
Message-ID: <X8+DI7ZN7mXtsxv9@ulmo>
References: <20201207193629.493241-1-clemens.gruber@pqgruber.com>
 <20201207220025.42b6g76wq7ph5nvb@pengutronix.de>
 <X863KNo0IaekkU7q@workstation.tuxnet>
 <20201208091033.bxzrlad7mjbe3dsp@pengutronix.de>
 <X89RgpTb3sBBI++w@workstation.tuxnet>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="mrYU85tcYWs+7OgA"
Content-Disposition: inline
In-Reply-To: <X89RgpTb3sBBI++w@workstation.tuxnet>
User-Agent: Mutt/2.0.2 (d9268908) (2020-11-20)
Precedence: bulk


--mrYU85tcYWs+7OgA
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Dec 08, 2020 at 11:12:18AM +0100, Clemens Gruber wrote:
> Hello Uwe,
>=20
> On Tue, Dec 08, 2020 at 10:10:33AM +0100, Uwe Kleine-K=C3=B6nig wrote:
> > Hello Clemens,
> >=20
> > On Tue, Dec 08, 2020 at 12:13:44AM +0100, Clemens Gruber wrote:
> > > On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-K=C3=B6nig wrote:
> > > > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote:
> > > > > The hardware readout may return slightly different values than th=
ose
> > > > > that were set in apply due to the limited range of possible presc=
ale and
> > > > > counter register values. If one channel is reconfigured with new =
duty
> > > > > cycle and period, the others will keep the same relative duty cyc=
le to
> > > > > period ratio as they had before, even though the per-chip / global
> > > > > frequency changed. (The PCA9685 has only one prescaler!)
> > > >=20
> > > > This is not acceptable, if you have two PWM outputs and a consumer
> > > > modifies one of them the other must change. So if this chip only
> > > > supports a single period length of all channels, the first consumer
> > > > enabling a channel defines the period to be used. All later consume=
rs
> > > > must live with that. (Also the first must be denied modifying the p=
eriod
> > > > if a second consumer has enabled its PWM.)
> > >=20
> > > Good idea, but is it OK to potentially break users relying on the old
> > > behavior ("the last one who changes the period wins") ?
> >=20
> > If this is already in the old code, this probably warrants a separate
> > fix, and yes, I consider this a severe bug. (Consider one channel
> > driving a motor and reconfiguring an LED modifies the motor's speed.)
>=20
> Yes, but a user could also be relying on the old behavior as follows:
> 1. Requests & enables pwm 0 for a backlight, using a period of 5000000ns
>    (does not care about the frequency as long as it does not flicker)
> 2. Requests & enables pwm 1 for a motor, using a period of 1000000ns
>    (does care about the frequency)
>=20
> In the previous kernel versions, this would work, but with your
> suggested change, (2) would fail and the motor would no longer work.
>=20
> We are basically changing "the last one to set the period wins" to "the
> first one to set the period wins".
>=20
> If we do it like this, I'll split it out so we can at least revert it if
> someone complains that it breaks his application, without reverting the
> whole series.

Yes, that makes sense to me. We do want to make sure that we don't have
these kinds of races for PWM controllers and other drivers already have
corresponding checks in place.

But I agree that if this is preserving the status quo, then yes, we
should follow up with a separate patch to add that check so that it can
be easily reverted if this indeed break.

Although, if we do get failures after this check has been introduced,
they should be considered bugs and fixed in the right place. Ultimately
this is something that board designers have hopefully already thought
about and if there are two PWM consumers they will usually be able to
run at a common period, in which case fixing these should be as easy as
finding that common period and, well, using it for both consumers.

Thierry

--mrYU85tcYWs+7OgA
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAl/PgyEACgkQ3SOs138+
s6H0Hg/+IFCQPNudxTHOV8jXYr7yxNaRilxcSAkFrjMAUHpYuF+PW5lU4HiXlFrH
nKABwDGqHD8+KsGvQ+5VUVPVjC5/b6S4nckFRM07iJ3KZ9MdF9ShRaF0RWi3TSkG
zrXalJ6+lirrmB3Z2Io5uj1EA+HXMjPxuRuqKQ8l5vs2OpUK58ohDz+Fysjw0cpI
hj2T0Aq6lvyf/ZROI36yjYfVUYe6wBYCXDvViQTx4WcIdV2LfDJKiRRRiSICOCcp
hUUWBEprQrK77+ftRvDcvYFgiOoERo58NxDZYaLhi7foH8OF+xKIglJqOf5Kr0tM
FK/I+ag4V/NwKBV820O9Ahc6e/C5zMEcxxxcQ0kJBoIIRoRVi+r6b6fclPn8LZ2K
mCcuf4F+jjlIn8XSRHTwt9JgGkek25vDnyswOUe9R55td+H4oB+qUYAieWnvSmyn
c4l7rTlUiVmT3qnspGWfVoc4W+NprSqJIWVXcdLOUDLvz7VjHWpywf2BNYO/3jqy
XlV0MoummWi+41ie0IbF29SXhnepld1Wc6ErA9rHgVYbWeGOzrPxFZxYgP1oVaaq
a3keRmlEa4bkp/Mg6EehKcQvVF1QkhgmCzgCMdbgZWupKaZEQE6NX48lN+76VwN3
Z+Ym12DZ3bkLzTj+INa+Z4zC8iJeE+v8hCoCpIzJ5Sn1JIpbtv4=
=irwy
-----END PGP SIGNATURE-----

--mrYU85tcYWs+7OgA--