Message-ID: <X8+DI7ZN7mXtsxv9@ulmo>
Date:   Tue, 8 Dec 2020 14:44:03 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-pwm@...r.kernel.org,
        Lee Jones <lee.jones@...aro.org>, linux-kernel@...r.kernel.org,
        Sven Van Asbroeck <TheSven73@...il.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        David Jander <david@...tonic.nl>
Subject: Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

On Tue, Dec 08, 2020 at 11:12:18AM +0100, Clemens Gruber wrote:
> Hello Uwe,
> 
> On Tue, Dec 08, 2020 at 10:10:33AM +0100, Uwe Kleine-König wrote:
> > Hello Clemens,
> > 
> > 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önig wrote:
> > > > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote:
> > > > > The hardware readout may return slightly different values than those
> > > > > that were set in apply due to the limited range of possible prescale and
> > > > > counter register values. If one channel is reconfigured with new duty
> > > > > cycle and period, the others will keep the same relative duty cycle to
> > > > > period ratio as they had before, even though the per-chip / global
> > > > > frequency changed. (The PCA9685 has only one prescaler!)
> > > > 
> > > > 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 consumers
> > > > must live with that. (Also the first must be denied modifying the period
> > > > if a second consumer has enabled its PWM.)
> > > 
> > > Good idea, but is it OK to potentially break users relying on the old
> > > behavior ("the last one who changes the period wins") ?
> > 
> > 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.)
> 
> 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)
> 
> In the previous kernel versions, this would work, but with your
> suggested change, (2) would fail and the motor would no longer work.
> 
> We are basically changing "the last one to set the period wins" to "the
> first one to set the period wins".
> 
> 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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists