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

[Clemens Gruber <clemens.gruber@pqgruber.com>]

Hi Uwe,

On Mon, Mar 01, 2021 at 10:52:48PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Feb 01, 2021 at 06:24:02PM +0100, Clemens Gruber wrote:
> > 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.
> 
> I would prefer to drop the reset. If the bootloader left with an invalid
> state, this is active for sure until the PWM driver is loaded. If you
> don't reset, the time is extended (usually) until the consumer comes
> along and corrects the setting. So the downside of not resetting is
> quite limited, but if you disable the PWM in .probe() the effect can be
> worse. And consistency dictates to not reset.
> 
> > Removing the resets could then be left as something to discuss further
> > in the future and something that belongs in a separate patch series?
> 
> That would be fine for me, too.

Great, then I will prepare a new series next week.

Thanks,
Clemens