Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

From: Clemens Gruber <clemens.gruber@pqgruber.com>
To: Sven Van Asbroeck <thesven73@gmail.com>,
	Thierry Reding <thierry.reding@gmail.com>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-pwm@vger.kernel.org
Subject: Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
Date: Mon, 1 Feb 2021 18:24:02 +0100	[thread overview]
Message-ID: <YBg5MlvJQ0N2u+j6@workstation.tuxnet> (raw)
In-Reply-To: <CAGngYiV5GGJvHTwG7k6mv76uR1RLnHOJoO8+d2ofiZAQi3K0BA@mail.gmail.com>

Hi Sven, Thierry, Uwe,

On Fri, Jan 29, 2021 at 05:16:51PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens,
> 
> On Fri, Jan 29, 2021 at 4:24 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> >
> > LEN_ON = 409, LED_OFF = 1228 and
> > LED_ON = 419, LED_OFF = 1238
> > produce the same result. you can't see the difference between the two
> > when scoping the channel. there are probably more ways to do this,
> > some might surprise us. It's a tricky chip.
> 
> Please ignore this example, it's bogus. In my defence, it's a Friday
> afternoon here :)

Happens to the best of us :)

> 
> But consider the following: imagine the bootloader has enabled a few
> pwm channels, and the driver's .probe() has left them on/unchanged.
> Then the user enables another pwm channel, and tries to change the
> period/prescaler. How would pca9685_may_change_prescaler() know
> if changing the prescaler is allowed?
> 
> And the following: imagine the bootloader has enabled a few
> pwm channels, and the driver's .probe() has left them on/unchanged.
> After .probe(), the runtime_pm will immediately put the chip to sleep,
> because it's unaware that some channels are alive.

(We could read out the state in .probe. If a pwm is already enabled by
the bootloader, then the user can't change the period. Also, the chip
would not be put to sleep.

The user then can export channels and see if they are enabled. If he
wants to change the period, he needs to find the one enabled by the
bootloader and change the period there, before he requests more.
If the bootloader enabled more than one, then he has to disable all but
one to change the period.

Or did I miss something?)

> 
> I'm sure I'm overlooking a few complications here. probe not changing
> the existing configuration, will add a lot of complexity to the driver.
> I'm not saying this is necessarily bad, just a tradeoff. Or, a management
> decision.

But I agree that it is simpler if we keep the resets in probe. It would
also avoid a potentially breaking change for users that do not reset
their pca9685 chips in their bootloader code.
There might be users out there that depend on the driver to reset the
OFF registers in .probe.

If Thierry agrees / allows it, I can keep the resets for now.

Removing the resets could then be left as something to discuss further
in the future and something that belongs in a separate patch series?

Clemens