2021-01-29 16:39:33

by Clemens Gruber

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

Hi Sven,

On Fri, Jan 29, 2021 at 08:42:13AM -0500, Sven Van Asbroeck wrote:
> On Mon, Jan 11, 2021 at 3:35 PM Uwe Kleine-K?nig
> <[email protected]> wrote:
> >
> > My position here is: A consumer should disable a PWM before calling
> > pwm_put. The driver should however not enforce this and so should not
> > modify the hardware state in .free().
> >
> > Also .probe should not change the PWM configuration.
>
> I agree that this is the most user-friendly behaviour.
>
> The problem however with the pca9685 is that it has many degrees of
> freedom: there are many possible register values which produce the same
> physical chip outputs.
>
> This could lead to a situation where, if .probe() does not reset the register
> values, subsequent writes may lead to different outputs than expected.
>
> One possible solution is to write .get_state() so that it always reads the
> correct state, even if "unconventional" register settings are present, i.e.
> those written by an outside entity, e.g. a bootloader. Then write that
> state back using driver conventions.
>
> This may be trickier than it sounds - after all we've learnt that the pca9685
> looks simple on the surface, but is actually quite challenging to get right.
>
> Clemens, Uwe, what do you think?

Ok, so you suggest we extend our get_state logic to deal with cases
like the following:
If neither full OFF nor full ON is set && ON == OFF, we should probably
set the full OFF bit to disable the PWM and log a warning message?
(e.g. "invalid register setting detected: pwm disabled" ?)
If the ON registers are set and the nxp,staggered-outputs property is
not, I'd calculate (off - on) & 4095, set the OFF register to that value
and clear the ON register.

And then call our get_state in .probe, followed by a write of the
resulting / fixed-up state?

This would definitely solve the problem of invalid/unconventional values
set by the bootloader and avoid inconsistencies.
Sounds good to me!

If Thierry and Uwe have no objections, I can send out a new round of
patches in the upcoming weeks.

My current goal is to get the changes into 5.13.

Thanks,
Clemens


2021-01-29 18:08:21

by Sven Van Asbroeck

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

Hi Clemens,

On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
<[email protected]> wrote:
>
> Ok, so you suggest we extend our get_state logic to deal with cases
> like the following:

Kind of. We can't control how other actors (bootloaders etc) program the
chip. As far as I know, there are many, many different register settings that
result in the same physical chip outputs. So if .probe() wants to preserve the
existing chip settings, .get_state() has to be able to deal with every possible
setting. Even invalid ones.

In addition, .apply() cannot make any assumptions as to which bits are
already set/cleared on the chip. Including preserved, invalid settings.

This might get quite complex.

However if we reset the chip in .probe() to a known state (a normalized state,
in the mathematical sense), then both .get_state() and .apply() become
much simpler. because they only need to deal with known, normalized states.

In short, it's a tradeoff between code complexity, and user friendliness/
features.

Sven

2021-01-29 20:40:52

by Clemens Gruber

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

Hi Sven,

On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens,
>
> On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> <[email protected]> wrote:
> >
> > Ok, so you suggest we extend our get_state logic to deal with cases
> > like the following:
>
> Kind of. We can't control how other actors (bootloaders etc) program the
> chip. As far as I know, there are many, many different register settings that
> result in the same physical chip outputs. So if .probe() wants to preserve the
> existing chip settings, .get_state() has to be able to deal with every possible
> setting. Even invalid ones.

Is the driver really responsible for bootloaders that program the chip
with invalid values?
The chip comes out of PoR with sane default values. If the bootloader of
a user messes them up, isn't that a bootloader problem instead of a
Linux kernel driver problem?

> In addition, .apply() cannot make any assumptions as to which bits are
> already set/cleared on the chip. Including preserved, invalid settings.
>
> This might get quite complex.
>
> However if we reset the chip in .probe() to a known state (a normalized state,
> in the mathematical sense), then both .get_state() and .apply() become
> much simpler. because they only need to deal with known, normalized states.

Yes, I agree. This would however make it impossible to do a flicker-free
transition from bootloader to kernel, but that's not really a usecase I
have so I can live without it.

Another point in favor of resetting is that the driver already does it.
Removing the reset of the OFF register may break some boards who rely on
that behaviour.
My version only extended the reset to include the ON register.

>
> In short, it's a tradeoff between code complexity, and user friendliness/
> features.
>
> Sven

Thierry, Uwe, what's your take on this?

Thierry: Would you accept it if we continue to reset the registers in
.probe?

Thanks,
Clemens

2021-01-29 21:27:50

by Sven Van Asbroeck

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

Hi Clemens,

On Fri, Jan 29, 2021 at 3:37 PM Clemens Gruber
<[email protected]> wrote:
>
> Is the driver really responsible for bootloaders that program the chip
> with invalid values?

No, but it's responsible for correcting invalid values. Otherwise the driver
doesn't work.

> The chip comes out of PoR with sane default values. If the bootloader of
> a user messes them up, isn't that a bootloader problem instead of a
> Linux kernel driver problem?

Invalid values are only half the problem. The other half is that two valid
values might produce the same output, e.g.:

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.

2021-01-29 22:21:26

by Sven Van Asbroeck

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

Hi Clemens,

On Fri, Jan 29, 2021 at 4:24 PM Sven Van Asbroeck <[email protected]> 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 :)

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.

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.

Sven

2021-02-01 17:28:40

by Clemens Gruber

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

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 <[email protected]> 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

2021-02-14 14:48:27

by Clemens Gruber

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

Hi all,

On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> Hi Sven,
>
> On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> > Hi Clemens,
> >
> > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> > <[email protected]> wrote:
> > >
> > > Ok, so you suggest we extend our get_state logic to deal with cases
> > > like the following:
> >
> > Kind of. We can't control how other actors (bootloaders etc) program the
> > chip. As far as I know, there are many, many different register settings that
> > result in the same physical chip outputs. So if .probe() wants to preserve the
> > existing chip settings, .get_state() has to be able to deal with every possible
> > setting. Even invalid ones.
>
> Is the driver really responsible for bootloaders that program the chip
> with invalid values?
> The chip comes out of PoR with sane default values. If the bootloader of
> a user messes them up, isn't that a bootloader problem instead of a
> Linux kernel driver problem?
>
> > In addition, .apply() cannot make any assumptions as to which bits are
> > already set/cleared on the chip. Including preserved, invalid settings.
> >
> > This might get quite complex.
> >
> > However if we reset the chip in .probe() to a known state (a normalized state,
> > in the mathematical sense), then both .get_state() and .apply() become
> > much simpler. because they only need to deal with known, normalized states.
>
> Yes, I agree. This would however make it impossible to do a flicker-free
> transition from bootloader to kernel, but that's not really a usecase I
> have so I can live without it.
>
> Another point in favor of resetting is that the driver already does it.
> Removing the reset of the OFF register may break some boards who rely on
> that behaviour.
> My version only extended the reset to include the ON register.
>
> >
> > In short, it's a tradeoff between code complexity, and user friendliness/
> > features.
> >
> > Sven
>
> Thierry, Uwe, what's your take on this?
>
> Thierry: Would you accept it if we continue to reset the registers in
> .probe?
>
> Thanks,
> Clemens

I realize that it is a difficult time at the moment, but it is a little
bit frustrating not getting any response from the maintainer.

I think the best way forward is to just keep the register resets in
probe as they are. If this is to be changed, I think it should be done
in a separate patchset and by someone who has a usecase requiring it.

Best regards,
Clemens

2021-03-02 17:41:43

by Uwe Kleine-König

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

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 <[email protected]> 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.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |


Attachments:
(No filename) (3.01 kB)
signature.asc (499.00 B)
Download all attachments

2021-03-04 17:29:19

by Clemens Gruber

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

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 <[email protected]> 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

2021-03-22 09:16:01

by Thierry Reding

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

On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens,
>
> On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> <[email protected]> wrote:
> >
> > Ok, so you suggest we extend our get_state logic to deal with cases
> > like the following:
>
> Kind of. We can't control how other actors (bootloaders etc) program the
> chip. As far as I know, there are many, many different register settings that
> result in the same physical chip outputs. So if .probe() wants to preserve the
> existing chip settings, .get_state() has to be able to deal with every possible
> setting. Even invalid ones.

I said earlier that the PWM state is a snapshot of the current hardware
settings and that's not entirely accurate because it isn't actually a
complete representation of the hardware state. It's merely a
representation of the PWM software state that's currently applied to the
hardware.

This is simpler from an API point of view than completely representing
the actual hardware state, but it's also sufficient for most use-cases
because we don't care about the exact programming as long as it yields
the result represented by the atomic state. Although it's still vitally
important that the amount of state that we have is accurately
represented (i.e. duty-cycle/period values must not be collapsed to 0
when the PWM is off), otherwise the API isn't usable.

One good thing that comes from this simplification is that it gives us
a bit more flexibility in hardware readout because you can collapse a
large amount of variation into the couple of values that we have. So if
your bootloaders program weird values, you can canonicalize them as long
as they still yield the same result.

So roughly what should be guaranteed from an atomic API point of view is
that doing the following is glitch-free and doesn't cause a change in
the physical PWM signal:

chip->ops->get_state(chip, pwm, &state);
pwm_apply_state(pwm, &state);

Ideally we'd even be able to, though we don't do it at present, to
optimize that out as a no-op by comparing the new state with the current
state and just not doing anything if they are equal.

And just to clarify: glitch-free above means: to the extent possible. In
some cases it might not be possible to set PWM hardware state in a
completely glitch-free way. If so, there's not a lot we can do and it's
better to do something even if it's not ideal. The rationale behind this
is that nobody will select a chip that doesn't meet requirements to
perform a given task, so it's highly unlikely that a chip that glitches
during transitions will ever be used in a setup where it's required not
to glitch. We should obviously always do our best to keep glitches to a
minimum, but software can't change hardware...

> In addition, .apply() cannot make any assumptions as to which bits are
> already set/cleared on the chip. Including preserved, invalid settings.
>
> This might get quite complex.
>
> However if we reset the chip in .probe() to a known state (a normalized state,
> in the mathematical sense), then both .get_state() and .apply() become
> much simpler. because they only need to deal with known, normalized states.

As was mentioned before, this does restrict the usability of the driver.
In some cases you really want to avoid resetting the chip. But I'm also
okay with leaving this as-is because it's the status quo.

So what I'd propose is to take this forward and keep the reset during
probe for now and then follow up with a separate, simple patch that
removes the reset. That way we can easily back it out, or revert it, if
it causes any breakage, but it won't hold up this series.

Thierry


Attachments:
(No filename) (3.65 kB)
signature.asc (849.00 B)
Download all attachments

2021-03-22 09:21:08

by Thierry Reding

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

On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> Hi Sven,
>
> On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> > Hi Clemens,
> >
> > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> > <[email protected]> wrote:
> > >
> > > Ok, so you suggest we extend our get_state logic to deal with cases
> > > like the following:
> >
> > Kind of. We can't control how other actors (bootloaders etc) program the
> > chip. As far as I know, there are many, many different register settings that
> > result in the same physical chip outputs. So if .probe() wants to preserve the
> > existing chip settings, .get_state() has to be able to deal with every possible
> > setting. Even invalid ones.
>
> Is the driver really responsible for bootloaders that program the chip
> with invalid values?
> The chip comes out of PoR with sane default values. If the bootloader of
> a user messes them up, isn't that a bootloader problem instead of a
> Linux kernel driver problem?

It is ultimately a problem of the bootloader and where possible the
bootloader should be fixed. However, fixing bootloaders sometimes isn't
possible, or impractical, so the kernel has to be able to deal with
hardware that's been badly programmed by the bootloader. Within reason,
of course. Sometimes this can't be done in any other way than forcing a
hard reset of the chip, but it should always be a last resort.

> > In addition, .apply() cannot make any assumptions as to which bits are
> > already set/cleared on the chip. Including preserved, invalid settings.
> >
> > This might get quite complex.
> >
> > However if we reset the chip in .probe() to a known state (a normalized state,
> > in the mathematical sense), then both .get_state() and .apply() become
> > much simpler. because they only need to deal with known, normalized states.
>
> Yes, I agree. This would however make it impossible to do a flicker-free
> transition from bootloader to kernel, but that's not really a usecase I
> have so I can live without it.
>
> Another point in favor of resetting is that the driver already does it.
> Removing the reset of the OFF register may break some boards who rely on
> that behaviour.
> My version only extended the reset to include the ON register.
>
> >
> > In short, it's a tradeoff between code complexity, and user friendliness/
> > features.
> >
> > Sven
>
> Thierry, Uwe, what's your take on this?
>
> 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 behaviour.

Thierry


Attachments:
(No filename) (3.17 kB)
signature.asc (849.00 B)
Download all attachments

2021-03-27 16:07:23

by Clemens Gruber

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

Hi Thierry,

On Mon, Mar 22, 2021 at 10:19:22AM +0100, Thierry Reding wrote:
> On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > Hi Sven,
> >
> > On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> > > Hi Clemens,
> > >
> > > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> > > <[email protected]> wrote:
> > > >
> > > > Ok, so you suggest we extend our get_state logic to deal with cases
> > > > like the following:
> > >
> > > Kind of. We can't control how other actors (bootloaders etc) program the
> > > chip. As far as I know, there are many, many different register settings that
> > > result in the same physical chip outputs. So if .probe() wants to preserve the
> > > existing chip settings, .get_state() has to be able to deal with every possible
> > > setting. Even invalid ones.
> >
> > Is the driver really responsible for bootloaders that program the chip
> > with invalid values?
> > The chip comes out of PoR with sane default values. If the bootloader of
> > a user messes them up, isn't that a bootloader problem instead of a
> > Linux kernel driver problem?
>
> It is ultimately a problem of the bootloader and where possible the
> bootloader should be fixed. However, fixing bootloaders sometimes isn't
> possible, or impractical, so the kernel has to be able to deal with
> hardware that's been badly programmed by the bootloader. Within reason,
> of course. Sometimes this can't be done in any other way than forcing a
> hard reset of the chip, but it should always be a last resort.
>
> > > In addition, .apply() cannot make any assumptions as to which bits are
> > > already set/cleared on the chip. Including preserved, invalid settings.
> > >
> > > This might get quite complex.
> > >
> > > However if we reset the chip in .probe() to a known state (a normalized state,
> > > in the mathematical sense), then both .get_state() and .apply() become
> > > much simpler. because they only need to deal with known, normalized states.
> >
> > Yes, I agree. This would however make it impossible to do a flicker-free
> > transition from bootloader to kernel, but that's not really a usecase I
> > have so I can live without it.
> >
> > Another point in favor of resetting is that the driver already does it.
> > Removing the reset of the OFF register may break some boards who rely on
> > that behaviour.
> > My version only extended the reset to include the ON register.
> >
> > >
> > > In short, it's a tradeoff between code complexity, and user friendliness/
> > > features.
> > >
> > > Sven
> >
> > Thierry, Uwe, what's your take on this?
> >
> > 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 behaviour.

Thanks for your input!

Sounds good to me. I am currently preparing a new revision of the
series. As soon as that is reviewed and good to go, I will look into
removing the resets.

Clemens