Message-ID: <4fbb7307-df01-d7bd-f2e2-e05e6d17807d@ysoft.com>
Date:   Thu, 8 Nov 2018 15:21:44 +0000
From:   Vokáč Michal <Michal.Vokac@...ft.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     Thierry Reding <thierry.reding@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lukasz Majewski <l.majewski@...ess.pl>,
        Fabio Estevam <fabio.estevam@....com>,
        Lothar Waßmann <LW@...o-electronics.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state

Hi Uwe,

On 7.11.2018 16:01, Uwe Kleine-König wrote:
>> Interesting idea. I just wonder why nobody else did not come up with such
>> a simple solution before.
> 
> I think I mentioned it already in this thread, but it went unnoticed :-)

I meant it like "How happened this was not invented years ago, when people
first noticed the issue with using inverted PWM for backlight on i.MX6."
In our project, this issue dates back to 2015 :(

> Then the patch isn't correct yet. The idea is always keep the hardware
> running and only disable it if it's uninverted.

OK, I got the point.

> In imx_pwm_probe it's not yet known what the polarity is supposed to be,
> right?

Not really. It can already be known but currently there is no way how to
pass the information to the probe function. I, as a creator of the device
(and author of a DTS file) know that the circuit needs inverted PWM signal.
And I know that the circuit needs to be disabled until I say it can be
enabled. How I say that can warry. It may be default brightness level > 0
in DTS file or from a userspace program using PWM sysfs interface.

> So the right thing to do there is to not touch the configuration
> of the pwm. I think all states that are problematic then are also
> problematic with the gpio/pinmux approach.

I think my use-case I already presented before is an example where
involving pinctrl solves the problem while the "leave PWM enabled
for inverted users" does not. That is all the time between
imx_pwm_probe() and imx_pwm_apply_v2().
  
>> In probe you do not have any users yet. So you do not know the requested
>> output polarity. With "default" pinctrl the PWM output would be muxed to
>> the selected pin and since the PWM chip is most probably disabled
>> (unless you enabled it in bootloader) you would get low level on the pin.
>> That means your backlight is fully enabled until the first call to
>> imx_pwm_apply_v2(). On my system this is 2 seconds.
> 
> With the gpio/pinmux approach you don't know the intended polarity
> either and maybe enable the display, too.

You know it because the pinctrl solution is optional. So if you use it,
you use it on purpose to override the default PWM output level in PWM
disabled state. It is very useful in cases where you need inverted and
disabled PWM signal from power-up to userspace. Or until some kernel
driver (backlight, led, fan..) enables it. For this it is the only
solution I think.

It allows you to boot with disabled PWM that has normal polarity set
by default. Later on from your userspace program you configure the PWM
to desired period/duty, set PWM output to inversed and enable it.
Until this point the circuit is disabled with my solution.

> For both the solution is to let the bootloader enable the pwm with
> the right output level. Am I missing something?

Bootloader is only a small part of the whole solution I think. And I
suppose you meant: "enable the *GPIO* with the right output level".

  - Even if you use GPIO in bootloader to set the required level the
    time frame from imx_pwm_probe to imx_pwm_apply is not covered.

  - Currently there is no support in Linux pwm-imx driver to detect
    the PWM chip is already enabled at probe time. I actually send
    patches for this a month ago [1]. No response yet.

  - Inverted PWM does not work in U-Boot (on imx at least). And it
    does not seam like it can be fixed easily. I do not know what is
    the situation in other bootloaders.

So my current bootloader solution is one of:
  - Set the pin to the appropriate (HIGH) level using GPIO.
  - Do not touch the pin at all, it has 100k pull-up by default.

>> The other thing is I would prefer to make the change optional. With your
>> approach you are changing the behavior for all current users of inverted
>> PWM. I do not think all imx6 users are aware of the problem so they might
>> not be OK with the sudden change in the behavior.
> 
> Isn't my change an improvement for all users? What state do you have in
> mind that make things worse than they are now?

Lets say that the user:
- Needs inverted PWM signal.
- Needs it to be disabled all the time unless he enable it.

Current situation:
H|____________________
L|                    \_________________________________________________
  +-------+------------+-----------------+-----------------+-------------+
  | reset | bootloader | default pinctrl | PWM enable 100% | PWM disable |

What you propose (for all users of inverted PWM):
H|____________________                                     _____________
L|                    \___________________________________/
  +-------+------------+-----------------+-----------------+-------------+
  | reset | bootloader | default pinctrl | PWM enable 100% | PWM disable |

My solution (for those who want it):
H|______________________________________                   _____________
L|                                      \_________________/
  +-------+------------+-----------------+-----------------+-------------+
  | reset | bootloader | default pinctrl | PWM enable 100% | PWM disable |

So your solution at least allows the user to really disable the circuit.
I can not really think of cases where this might not be good for current
users. Maybe that they simply expect that no matter what polarity is set,
the output in disabled state is always low. And they may have HW that
get already used to that and does not like the change :)

And it reminds me of something similar I have done for OLED display reset
recently [2]. I tried to fix active-low reset sequence that is hardcoded
in the driver. So you are supposed to use GPIO_ACTIVE_HIGH in DT to make
the active-low reset work. It was rejected. The reason was backward DTB
compatibility [3]. In other words: "Users of newer kernels expect that
the reset still work the same if they do not update DTBs on their boards".
I think this is kind of similar?

[1] http://patchwork.ozlabs.org/project/linux-pwm/list/?series=68445
[2] https://patchwork.kernel.org/project/linux-fbdev/list/?series=23775
[3] https://lkml.org/lkml/2018/10/9/135

Best regards,
Michal

Powered by blists - more mailing lists