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 <thesve...@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