Hello Andy,
On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote:
> On Monday, March 22, 2021, Thierry Reding <[email protected]> wrote:
> > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > > Thierry: Would you accept it if we continue to reset the registers in
> > > .probe?
> >
> > Yes, I think it's fine to continue to reset the registers since that's
> > basically what the driver already does. It'd be great if you could
> > follow up with a patch that removes the reset and leaves the hardware in
> > whatever state the bootloader has set up. Then we can take that patch
> > for a ride and see if there are any complains about it breaking. If
> > there are we can always try to fix them, but as a last resort we can
> > also revert, which then may be something we have to live with. But I
> > think we should at least try to make this consistent with how other
> > drivers do this so that people don't stumble over this particular
> > driver's
>
> I guess we may miss (a PCB / silicon design flaw or warm boot case) when
> boot loader left device completely untouched and device either in wrong
> state because if failed reset (saw this on PCA9555 which has a
> corresponding errata), or simply we have done a warm reset of the system.
> So, we also have to understand how to properly exit.
I don't think that not resetting is a real problem. My argumentation
goes as follows:
When the PWM driver is loaded and the PWM configuration is invalid, it
was already invalid for the time between power up (or warm start) and
PWM driver load time. Then it doesn't really hurt to keep the PWM
in this invalid state for a little moment longer until the consumer of
the PWM becomes active.
Together with the use cases where not resetting is the right thing to
do, I'm convinced not resetting is the better strategy.
> Another point, CCF has a bit “is critical”, and u guess PWM may get the
> same and make the all assumptions much easier.
So I think complicating the PWM framework for this isn't the right thing
to do.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-König
<[email protected]> wrote:
> On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote:
> > On Monday, March 22, 2021, Thierry Reding <[email protected]> wrote:
> > > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > > > Thierry: Would you accept it if we continue to reset the registers in
> > > > .probe?
> > >
> > > Yes, I think it's fine to continue to reset the registers since that's
> > > basically what the driver already does. It'd be great if you could
> > > follow up with a patch that removes the reset and leaves the hardware in
> > > whatever state the bootloader has set up. Then we can take that patch
> > > for a ride and see if there are any complains about it breaking. If
> > > there are we can always try to fix them, but as a last resort we can
> > > also revert, which then may be something we have to live with. But I
> > > think we should at least try to make this consistent with how other
> > > drivers do this so that people don't stumble over this particular
> > > driver's
> >
> > I guess we may miss (a PCB / silicon design flaw or warm boot case) when
> > boot loader left device completely untouched and device either in wrong
> > state because if failed reset (saw this on PCA9555 which has a
> > corresponding errata), or simply we have done a warm reset of the system.
> > So, we also have to understand how to properly exit.
>
> I don't think that not resetting is a real problem. My argumentation
> goes as follows:
>
> When the PWM driver is loaded and the PWM configuration is invalid, it
> was already invalid for the time between power up (or warm start) and
> PWM driver load time. Then it doesn't really hurt to keep the PWM
> in this invalid state for a little moment longer until the consumer of
> the PWM becomes active.
But this won't work in the cases when we have a chip with a shared
settings for period and/or duty cycle. You will never have a user come
due to -EBUSY.
> Together with the use cases where not resetting is the right thing to
> do, I'm convinced not resetting is the better strategy.
I'm on the opposite side, although I know the cases that resetting
maybe harmful (to some extent).
We definitely need a hint if we may or may not touch 1 or more
channels of a given PWM.
> > Another point, CCF has a bit “is critical”, and u guess PWM may get the
> > same and make the all assumptions much easier.
>
> So I think complicating the PWM framework for this isn't the right thing
> to do.
Again, I'm on opposite side here b/c see above.
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 22, 2021 at 01:40:57PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote:
> > > On Monday, March 22, 2021, Thierry Reding <[email protected]> wrote:
> > > > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > > > > Thierry: Would you accept it if we continue to reset the registers in
> > > > > .probe?
> > > >
> > > > Yes, I think it's fine to continue to reset the registers since that's
> > > > basically what the driver already does. It'd be great if you could
> > > > follow up with a patch that removes the reset and leaves the hardware in
> > > > whatever state the bootloader has set up. Then we can take that patch
> > > > for a ride and see if there are any complains about it breaking. If
> > > > there are we can always try to fix them, but as a last resort we can
> > > > also revert, which then may be something we have to live with. But I
> > > > think we should at least try to make this consistent with how other
> > > > drivers do this so that people don't stumble over this particular
> > > > driver's
> > >
> > > I guess we may miss (a PCB / silicon design flaw or warm boot case) when
> > > boot loader left device completely untouched and device either in wrong
> > > state because if failed reset (saw this on PCA9555 which has a
> > > corresponding errata), or simply we have done a warm reset of the system.
> > > So, we also have to understand how to properly exit.
> >
> > I don't think that not resetting is a real problem. My argumentation
> > goes as follows:
> >
> > When the PWM driver is loaded and the PWM configuration is invalid, it
> > was already invalid for the time between power up (or warm start) and
> > PWM driver load time. Then it doesn't really hurt to keep the PWM
> > in this invalid state for a little moment longer until the consumer of
> > the PWM becomes active.
>
> But this won't work in the cases when we have a chip with a shared
> settings for period and/or duty cycle. You will never have a user come
> due to -EBUSY.
That's wrong, the first consumer to enable the PWM (in software) is
supposed to be able to change the settings.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Mar 22, 2021 at 1:48 PM Uwe Kleine-König
<[email protected]> wrote:
> On Mon, Mar 22, 2021 at 01:40:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-König
> > <[email protected]> wrote:
> > > On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote:
> > > > On Monday, March 22, 2021, Thierry Reding <[email protected]> wrote:
> > > > > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > > > > > Thierry: Would you accept it if we continue to reset the registers in
> > > > > > .probe?
> > > > >
> > > > > Yes, I think it's fine to continue to reset the registers since that's
> > > > > basically what the driver already does. It'd be great if you could
> > > > > follow up with a patch that removes the reset and leaves the hardware in
> > > > > whatever state the bootloader has set up. Then we can take that patch
> > > > > for a ride and see if there are any complains about it breaking. If
> > > > > there are we can always try to fix them, but as a last resort we can
> > > > > also revert, which then may be something we have to live with. But I
> > > > > think we should at least try to make this consistent with how other
> > > > > drivers do this so that people don't stumble over this particular
> > > > > driver's
> > > >
> > > > I guess we may miss (a PCB / silicon design flaw or warm boot case) when
> > > > boot loader left device completely untouched and device either in wrong
> > > > state because if failed reset (saw this on PCA9555 which has a
> > > > corresponding errata), or simply we have done a warm reset of the system.
> > > > So, we also have to understand how to properly exit.
> > >
> > > I don't think that not resetting is a real problem. My argumentation
> > > goes as follows:
> > >
> > > When the PWM driver is loaded and the PWM configuration is invalid, it
> > > was already invalid for the time between power up (or warm start) and
> > > PWM driver load time. Then it doesn't really hurt to keep the PWM
> > > in this invalid state for a little moment longer until the consumer of
> > > the PWM becomes active.
> >
> > But this won't work in the cases when we have a chip with a shared
> > settings for period and/or duty cycle. You will never have a user come
> > due to -EBUSY.
>
> That's wrong, the first consumer to enable the PWM (in software) is
> supposed to be able to change the settings.
If it's a critical PWM, how can you be allowed to do that?
And if so, what is the difference between resetting the device in this
case? You may consider it as a change to the settings by the first
consumer.
--
With Best Regards,
Andy Shevchenko
Hello,
On Mon, Mar 22, 2021 at 02:15:08PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 22, 2021 at 1:48 PM Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Mon, Mar 22, 2021 at 01:40:57PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-K?nig
> > > <[email protected]> wrote:
> > > > When the PWM driver is loaded and the PWM configuration is invalid, it
> > > > was already invalid for the time between power up (or warm start) and
> > > > PWM driver load time. Then it doesn't really hurt to keep the PWM
> > > > in this invalid state for a little moment longer until the consumer of
> > > > the PWM becomes active.
> > >
> > > But this won't work in the cases when we have a chip with a shared
> > > settings for period and/or duty cycle. You will never have a user come
> > > due to -EBUSY.
> >
> > That's wrong, the first consumer to enable the PWM (in software) is
> > supposed to be able to change the settings.
>
> If it's a critical PWM, how can you be allowed to do that?
You seem to have a tight concept of a critical PWM. I don't, so I have
problems following you. What is your picture about what is to be
allowed/denied for a critical PWM?
> And if so, what is the difference between resetting the device in this
> case?
The difference is that we have a consumer that knows what to do with the
PWM then.
> You may consider it as a change to the settings by the first
> consumer.
.. but without knowing if the first consumer is a backlight driver or a
motor control it's hard to know if disabling the PWM is OK. So I like
the concept of not doing anything until a process comes along that knows
better.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |