On 12.10.2018 18:08, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Oct 12, 2018 at 03:04:48PM +0000, Vokáč Michal wrote:
>> On 12.10.2018 10:57, Uwe Kleine-König wrote:
>>> On Wed, Oct 10, 2018 at 09:33:26AM +0000, Vokáč Michal wrote:
>>>> Normally the PWM output is held LOW when PWM is disabled. This can cause
>>>> problems when inverted PWM signal polarity is needed. With this behavior
>>>> the connected circuit is fed by 100% duty cycle instead of being shut-off.
>>>>
>>>> Allow users to define a "gpio" and a "pwm" pinctrl states. The pwm pinctrl
>>>> state is then selected when PWM is enabled and the gpio pinctrl state is
>>>> selected when PWM is disabled. Also add a new pwm-gpios GPIO that is used
>>>> to drive the output in the gpio state.
>>>>
>>>> If all the pinctrl states and the pwm-gpios are not correctly specified
>>>> in DT the logic will work as before.
>>>>
>>>> As an example, with this patch a PWM controlled backlight with inversed
>>>> signal polarity can be used in full brightness range. Without this patch
>>>> the backlight can not be turned off as brightness = 0 disables the PWM
>>>> and that in turn set PWM output LOW, that is full brightness.
>>>>
>>>> Output of the PWM with "default" pinctrl and with "pwm"+"gpio" pinctrl:
>>>>
>>>> +--------------+------------+---------------+---------------------------+
>>>> | After reset | Bootloader | Linux pinctrl | User (sysfs, backlight..) |
>>>> | 100k pull-up | (not used) | | enable | disable |
>>>> +--------------+------------+---------------+---------------------------+
>>>> ___________________________ default _ _ _
>>>> |_________________| |_| |_| |_|_____________
>>>>
>>>> pwm + gpio
>>>> ___________________________________________ _ _ _ _____________
>>>> |_| |_| |_| |_|
>>>
>>> I was made aware of this patch by Thierry while discussion about a patch
>>> opportunity. I already pointed out some stuff I don't like about this
>>> patch in the repective thread, but I repeat it here to have it at the
>>> right place.
>>
>> Thank you for taking time to comment on this one as well Uwe.
>>
>>>> Signed-off-by: Michal Vokáč <michal.vokac@xxxxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>> - Utilize the "pwm" and "gpio" pinctrl states.
>>>> - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
>>>> - Select the right pinctrl state in probe.
>>>>
>>>> drivers/pwm/pwm-imx.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 86 insertions(+)
>>>>
>>>> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>>>> index 6cd3b72..3502123 100644
>>>> --- a/drivers/pwm/pwm-imx.c
>>>> +++ b/drivers/pwm/pwm-imx.c
>>>> @@ -10,11 +10,13 @@
>>>> #include <linux/clk.h>
>>>> #include <linux/delay.h>
>>>> #include <linux/err.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> #include <linux/io.h>
>>>> #include <linux/kernel.h>
>>>> #include <linux/module.h>
>>>> #include <linux/of.h>
>>>> #include <linux/of_device.h>
>>>> +#include <linux/pinctrl/consumer.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/pwm.h>
>>>> #include <linux/slab.h>
>>>> @@ -92,10 +94,45 @@ struct imx_chip {
>>>> void __iomem *mmio_base;
>>>>
>>>> struct pwm_chip chip;
>>>> +
>>>> + struct pinctrl *pinctrl;
>>>> + struct pinctrl_state *pinctrl_pins_gpio;
>>>> + struct pinctrl_state *pinctrl_pins_pwm;
>>>> + struct gpio_desc *pwm_gpiod;
>>>
>>> The pinctrl framework already knows about "init" and "default". These
>>> should be enough. i.e. "init" configures as gpio and "default" als pwm.
>>
>> That is totally new information for me. I've never seen any usage of
>> "init" pinctrl state before. The total number of users is 6 (rockchip)
>> so that explains a lot.
>>
>> I think that it would not work though. The basic idea behind my
>> solution is that the pinctrl driver does not know what is the correct
>> pin configuration at probe whereas the pwm-imx driver does.
>>
>> With the "init" and "default" states the pinctrl driver will always
>> select the "default" if it find the pins in "init" state.
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/core.c#L1469
>>
>> But we want the pins to stay in the "init" GPIO state unless a client
>> enables the PWM.
>
> Note I don't know this very well either. What should work then is to
> have "init" as GPIO and "active" as PWM. Well, though this breaks if you
> intend to probe a running pwm without stopping it.
I'm not sure what you mean by "active". I tried to find more info about
that and it looks like it is not a natively supported pinctrl state.
Only "init", "default" and "sleep" are, right? I could not find any current
users of "active". Few TI dra7 boards pretend to use it for dcan1 pinctrl
but I did not find any code handling that state.
Or do you propose to rename what I call "pwm" state to "active"?
Anyway, as you said - this still does not work as I would like to have
an option not to stop PWM that is already running.
As a side note - while greping the "active" state, I found
a pwm-renesas-tpu driver that is doing something very "pinctrl-ish" to
switch between pin states. Though the PWM chip has a built-in HW support
for that.
https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-renesas-tpu.c#L108
>>>> };
>>>>
>>>> +
>>>> #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
>>>>
>>>> +static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
>>>> + struct platform_device *pdev)
>>>> +{
>>>> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
>>>> + if (!imx_chip->pinctrl || IS_ERR(imx_chip->pinctrl)) {
>>>> + dev_info(&pdev->dev, "can not get pinctrl\n");
>>>> + return PTR_ERR(imx_chip->pinctrl);
>>>> + }
>>>> +
>>>> + imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
>>>> + "pwm");
>>>> + imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
>>>> + "gpio");
>>>> + imx_chip->pwm_gpiod = devm_gpiod_get_optional(&pdev->dev, "pwm",
>>>> + GPIOD_IN);
>>>> +
>>>> + if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
>>>
>>> You must not use PTR_ERR on a value that might not contain an error
>>> pointer.
>>
>> OK, thank you for valuable info.
>> So it seems like the I2C folks are in troubles as well:
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-imx.c#L996
>
> correct. I cannot find this documented though.
>
>>>> + * pinctrl to switch the output pin to GPIO functon and keep
>>>> + * the output at the same level as for duty-cycle = 0.
>>>
>>> Is it obvious that using a GPIO is more efficient/better/worth the
>>> complexity than just enabling the PWM with duty-cycle 0 and the right
>>> polarity?
>>
>> To me, it does more than just setting duty-cycle = 0. I think that the
>> user expect to find the PWM chip in disabled state after this. So that
>> when you read the HW registers you see the chip is disabled.
>
> The PWM API user doesn't (have to) care how the lowlevel driver
> implements the .disable() callback.
Sure, I agree the PWM API doesn't care. What I wanted to say is that I,
as a real person/user/developer kind of expect the PWM HW to be really
disabled when I do $(echo 0 > pwm0/enabled) as I know the HW is capable
of being disabled. But yeah, better is not to expect anything.
> I personally would prefer to keep the driver simple (without pinctrl and
> gpio stuff which also simplifies matters for dts authors) and spend that
> little energy keeping the HW on.
>
>>> There might be mechanisms in pincontrol that automatically mux the pin
>>> if it's configured as gpio, I didn't follow the details though.
>>>
>>> Also it should be possible to configure the GPIO as output immediatly.
>>> If the pinmuxing is set to the PWM function this doesn't have a visible
>>> side effect.
>>
>> This could be the first time that we want to disable the PWM as you
>> might start with PWM enabled from probe. And hence the GPIO is still
>> configured as input.
>
> I fail to follow your case. Do you mean the pwm is already running when
> the driver probes? Then it is save to set the gpio to output as this has
> no effect as long as the pin is muxed in its pwm function.
Sorry, I admit my skills to explain things in English are not the best.
Maybe I misunderstood what you meant by "configure the GPIO as output
immediately". I read that as: "You do not need gpiod_set_value(0), use
pinctrl_select_state(gpio) straight away.".
Now when I think about that again - you are right.
I actually did it like that in v1. But I did not use pwm-gpios at all.
I was relying on the input and pull-up setting from reset. But that is
not right as the bootloader might change it.
Now, when I use devm_gpiod_get_optional("pwm", GPIOD_IN), it is ensured
that the pin is configured as input so the pull-up keeps the right level.
>>>> + pinctrl_select_state(imx->pinctrl,
>>>> + imx->pinctrl_pins_gpio);
>>>
>>> Usually align function arguments to the opening (.
>>
>> Oops, I will fix those. There is more than this one..
>>
>>>> + }
>>>> +
>>>> writel(0, imx->mmio_base + MX3_PWMCR);
>>>>
>>>> clk_disable_unprepare(imx->clk_per);
>>>> @@ -354,6 +415,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
>>>> const struct of_device_id *of_id =
>>>> of_match_device(imx_pwm_dt_ids, &pdev->dev);
>>>> const struct imx_pwm_data *data;
>>>> + struct pwm_state cstate;
>>>> struct imx_chip *imx;
>>>> struct resource *r;
>>>> int ret = 0;
>>>> @@ -385,6 +447,10 @@ static int imx_pwm_probe(struct platform_device *pdev)
>>>> imx->chip.of_pwm_n_cells = 3;
>>>> }
>>>>
>>>> + ret = imx_pwm_init_pinctrl_info(imx, pdev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
>>>> if (IS_ERR(imx->mmio_base))
>>>> @@ -394,6 +460,26 @@ static int imx_pwm_probe(struct platform_device *pdev)
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> + if (imx->pinctrl) {
>>>> + /*
>>>> + * Update cstate after pwmchip_add() call as the core might
>>>> + * call the get_state() function to read the PWM registers
>>>> + * to get the actual HW state.
>>>> + */
>>>> + pwm_get_state(imx->chip.pwms, &cstate);
>>>> + if (cstate.enabled) {
>>>> + dev_dbg(&pdev->dev,
>>>> + "PWM entered probe in enabled state\n");
>>>> + pinctrl_select_state(imx->pinctrl,
>>>> + imx->pinctrl_pins_pwm);
>>>> + } else {
>>>> + gpiod_set_value_cansleep(imx->pwm_gpiod, 0);
>>>> + pinctrl_select_state(imx->pinctrl,
>>>> + imx->pinctrl_pins_gpio);
>>>> +
>>>> + }
>>>> + }
>>>> +
>>>> platform_set_drvdata(pdev, imx);
>>>> return 0;
>>>> }
>>>
>>> There is nothing in this patch that would prevent this code to live in a
>>> place where other drivers could reuse this. (But attention, there are
>>> dragons: Thierry already replied on my topic that his view is different
>>> in this aspect compared to other maintainers though. His POV is that as
>>> long as there is only a single driver known that has a problem this
>>> should be handled in driver specific code.)
>>
>> When I tripped over that issue I also thought it is i.MX specific.
>> It never came to my mind that something like that should be done in
>> higher layer.
>>
>> But I have to admit that my current understanding of the overall
>> architecture is at such level that I will just do it how the maintainer
>> wants me to do it. Given that the solution improves the situation and
>> solves my original problem. And the pinctrl solution was already
>> suggested by Fabio Estevam a year ago [1] so I went that way.
>>
>> [1] https://patchwork.ozlabs.org/patch/839834/#1819865
>
> In message 15 both Lothar and Fabio agree that they would prefer a
> solution without messing with pin configs. But also note that they
> discussed about suspend, too, not only pwm_disable().
Sorry, I did not want it to look like I said: "Fabio wants it to be done
the pinctrl way". I came up with the pinctrl solution on my own. Then when
I was searching in archives if someone have the same problem I tripped
over that thread. So I was happy that they discussed that as a possible
solution. Fabio did not need it for his suspend case though.
So I thought involving pinctrl is probably the last option to solve
the problem at pwm-imx driver level. So far, Thierry seems he support
that solution.
All the best,
Michal
