Hello Thierry,

On Fri, Nov 16, 2018 at 10:51:24AM +0100, Thierry Reding wrote:

On Thu, Nov 15, 2018 at 09:37:33PM +0100, Uwe Kleine-König wrote:

quoted

On Thu, Nov 15, 2018 at 04:25:45PM +0100, Thierry Reding wrote:

quoted

On Wed, Nov 14, 2018 at 10:51:20PM +0100, Uwe Kleine-König wrote:

quoted

On Wed, Nov 14, 2018 at 12:34:49PM +0100, Thierry Reding wrote:

quoted

On Fri, Nov 09, 2018 at 05:55:55PM +0100, Uwe Kleine-König wrote:

quoted

On Fri, Nov 09, 2018 at 02:24:42PM +0000, Vokáč Michal wrote:

quoted

On 8.11.2018 20:18, Uwe Kleine-König wrote:

quoted

Taking your example with the backlight device you specify an "init" and
a "default" pinctrl and only "default" contains the muxing for the PWM
pin everything should be as smooth as necessary: The pwm is only muxed
when the backlight device is successfully bound.

Have you tried that Uwe? The bad news is I tested that before and now
again and it does not work like that. We already discussed that earlier.

The key is that the pinmux setting for the PWM pin should be part of the
bl pinctrl, not the pwm pinctrl. Then "default" is only setup when the
bl device is successfully bound which is after the bl's .probe callback
called pwm_apply().

No, that's not at all correct. Pinmux settings should reside with the
consumer of the pin. In this case, the PWM is the consumer of the pin,
whereas the backlight is the consumer of the *PWM*.

This is news to me. Adding Linus W. to Cc, maybe he can comment?!

Grepping through the arm device trees it really seems common to put the
pinctrl for the pwm pin into the pwm device. I didn't search in depth,
but I didn't find a counter example.

For GPIOs it is common that the pinmuxing is included in the GPIO's
consumer pinctrl. Ditto for mdio busses whose pinctrl is included in the
ethernet pinctrl.

GPIO is different from PWM in that the GPIO is already the pin itself
and is otherwise generic. So typically you put the pinmuxing options
into the device tree node for the consumer of the GPIO, because it is
only when the consumer uses the GPIO that you need to configure that
pin as GPIO.

For PWM, however, the PWM consumer is only the consumer of the PWM, but
the PWM device itself is the real consumer of the pin that outputs the
PWM signal. So the PWM determines when the pinmux states need to be
applied, whereas the consumer of the PWM only deals with the PWM.

For MDIO busses, I think they are usually part of, and driven by, the
ethernet controller, so again it makes sense to put the pinmux into the
node of the ethernet controller, because the ethernet controller is the
user of the pins.

Maybe it was a bad idea to broaden the discussion to talk about gpios
and ethernet stuff here. I'd still consider it a valid construct to put
the pwm pin into the backlight's pinctrl unless Linux W. disagrees.

But why? The backlight doesn't care about the specific pinmuxing of the
PWM pin. All it cares about is the PWM signal. That's the level of
abstraction that the PWM consumer expects, anything lower level belongs
in the PWM driver.

The backlight driver cares about the PWM pin muxing because if it's
wrongly muxed the backlight doesn't work as intended.

quoted

quoted

quoted

quoted

The problem with making the PWM mode the "default" pinctrl state is that
the default state will be applied before the driver is even probed. That
makes it unsuitable for this case. I think what we really want here is
explicitly "active" and "inactive" states for pinctrl where the PWM
driver controls when exactly each state is applied.

Note that this problem goes away nicely if the pwm pin is attached to
the backlight. Because it's the backlight's driver that "knows" when the
pwm is configured correctly and so the already existing mechanisms that
setup the mux when the bl is correctly probed do the right thing at the
right time.

Actually that's not exactly true. The default pinctrl state will be
applied before the driver's ->probe() implementation, so the pinctrl
state will be active some time before even the backlight driver gets
around to setting up the PWM. If you look at drivers/base/dd.c you'll
see that really_probe() calls pinctrl_bind_pins() before calling the
driver's ->probe() and will select the default state (unless there's
also an "init" state defined, in which case that will get applied and
only after successful probe will the default state be selected).

So if you use only a default state, then you could even get into a
situation where ->probe() return -EPROBE_DEFER and it would potentially
take several seconds before the driver is reprobed, during which time
the pinmux will already be set up but the PWM not configured properly
and potentially outputting the wrong level.

If you reread my suggestion to Michal completely you will notice I got
that right.

quoted

quoted

quoted

This solves the problem quite nicely because by default the pinctrl
state isn't touched. For the case where the bootloader didn't initialize
the PWM pin at all, the driver core won't do anything and keep it at the
100k pull-up default.

Ditto if the pwm pinctrl is attached to the consumer without having to
introduce new pwm-specific stuff.

Well yes, but you'd obviously also have to avoid using the "default"
state, otherwise you'd run into the issues that I described above.

I'd need "default" and "init", right.

quoted

quoted

quoted

quoted

quoted

quoted

No I meant the pwm. Well, it's as easy as that: Whenever with your
approach you configure the pin as GPIO with the output set to low,
instead configure the pwm with duty_cycle to zero (or disable it).
Whenever with your approach you configure the pin as GPIO with the
output set to high, configure the pwm with duty_cycle to 100%. (Keeping
out inverted PWMs for the ease of discussion, but the procedure can be
adapted accordingly.) The only difference then is that with your
approach you already "know" in pwm-imx's .probe the idle level and can
configure the GPIO accordingly. With my approach you just have to wait
until the first pwm_apply which (as described above) works just as well.

While here I am quite confident you are talking about kernel code, right?
If yes, then your approach is clear to me.

The problem is I am quite sure your approach does not solve the cases
the pinctrl solution does. And according to my tests so far it does not
work at all because the "init" and "default" states does not work as you
are saying.

That's as pointed out above, because you're looking at the pwm's pinctrl
and I at the pwm-consumer's pinctrl.

Note that a sysfs consumer cannot be operated smoothly here, because
there is no pinctrl node to add the PWM mode to that only gets active
after the first configuration. This however is something that should not
be addressed in the imx driver but in the pwm core (if at all).

With the pinctrl-based solution outlined above you can even operate a
sysfs consumer properly. The pinctrl states are where they belong, with
the PWM device and therefore they can be properly set when the PWM is
used, rather than waiting for a PWM consumer to muck with the pinmux.

Note how all the pieces are suddenly falling into place. In my
experience that's usually a good indication that you're on the right
track.

OK, sysfs is the only point where the "put pinctrl stuff into the pwm
core (or driver)" is superior to the already existing and otherwise
completely working status quo. (Apart from bugs that need fixing in
your scenario, too.)

Nope, sorry. It's superior in all of the other cases as well. You've
said elsewhere already that the prerequisite for the current solution to
support inverse polarity with the i.MX driver is to keep the driver
running, even after the PWM is no longer used. Sorry but that's just not
an option for me.

You want that after pwm_disable() the pin still keeps the idle level. As
the hardware doesn't provide this feature "as is" something has to be
done about it. This can be reached either by operating the pin as PWM
with 0% duty cycle or by switching to GPIO that is configured to the
desired level. From the PWM driver's POV the first is the more natural,
as this can be accomplished with the registers this driver cares about
anyhow.

We've been over this before. Yes, as long as you operate the pin as PWM
it's okay to just actively drive it. But once you no longer use the pin,
why would you want to still actively drive it?

This is because you say the pin should keep its level as inactive even
though that's not what the hardware does without keeping care.

I say this is strange: The consumer specifies if the pwm should be
inverted or not because the pwm alone doesn't know that. Then with the
consumer gone *you* want the pwm to "remember" its last user requested
inversion and so the pin should stay at 1.

To answer your question: I don't want to actively drive the pin when the
user is gone. That's a requirement comming from you. If it was my
decision, I'd say: If the backlight driver calls

	pwm_config(pwm, 0, 100);
	pwm_disable(pwm);

I'd interpret that as: The consumer doesn't use the pin any more, so I'm
not bound to keep the pin at a certain level. If however after
pwm_disable the consumer is still considered to use the pin, then
implementing it the same way as pwm_config(pwm, 0, 100) is the right
thing to do. This applies then to all pwm implementations and so should
be solved in the pwm core, not in the imx driver. In this case the
concept of "disabling a PWM" can go away completely.

In another mail you wrote:

Your example of keeping an LED in the current state is actually an
example of where the consumer still needs it. As long as you want to use
the LED you need to keep the LED driver around, and as long as the LED
driver is around you have a consumer for the PWM.

With an analog reasoning I'd say: As long as the backlight driver cares
about the backlight being off, it should not disable (or put) the PWM.

quoted

Also note this is similar in the pwm-bcm-kona driver that doesn't seem
to have the concept of "disable" at all. kona_pwmc_disable() just sets
up 0% duty cycle. In my eyes this is an argument that is good enough to
at least nack the imx-specific implementation of that pinctrl stuff.

It's not a good enough argument for me. It's certainly possible that not
all PWM driver can be made to behave exactly as needed. pwm-bcm-kona
might be one of those, but that doesn't mean that everybody else should
be restricted to the same behaviour. If we can make i.MX behave exactly
right, then we should do that.

And if we can make the imx-specific implementation right in a generic
way in the pwm core that might help the bcm-kona driver for free.

quoted

If the pinctrl idea is implemented in the pwm core, I won't object.

Let me see if I get this straight: you're not objecting to the idea of
implementing the pinctrl solution, your only objection is to put it in
the i.MX driver?

Given that the pinctrl solution is a generic solution that might help
other drivers, too, I think it should not go into the imx specific
driver just because for now this is the only driver that might benefit
from it.

I still don't think it's the best solution for the imx problem but as I
care more for imx in general than for pwm in general I'm interested in
keeping the imx driver focused to the imx specific parts. I won't repeat
the advantages of putting generic stuff into a generic location instead
of its first user.

quoted

quoted

quoted

Also dts writes don't need to lookup the needed GPIO numbers and pinctrl.

Just to clarify: I don't think that we need to get the GPIO number
involved in this case, because we don't have to reconfigure the pin as
GPIO to make this work. The only reason that Michal's proposal did that
is because that was believed to be necessary. But if the pin can just be
configured with a 100k pull-up, that's enough to pull the pin high when
we need it.

Unless the gpio happens to be configured as output at the wrong value.
Further I'm not sure if the pwm in disabled state actively pulls to 0
and if in this state the PU of the pin is good enough to ensure a one
here. That would need verification first.

The idea is to *not* configure the GPIO as output and output the wrong
value. The idea is to not use the GPIO at all and instead use whatever
the hardware default is that makes it such that the backlight is off by
default at boot.

Which might be possible with Lothar's idea for some machines, but not
for all users of the pwm-imx driver.

Also note that you don't include the poor souls where there is no
hardware pullup into the right direction.

Best regards
Uwe

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