Message-ID: <e4289ee5-3fc1-2848-1cae-77d29b5ed93c@ysoft.com>
Date:   Mon, 29 Oct 2018 15:54:27 +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

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@...ft.com>
>>>> ---
>>>> 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

Powered by blists - more mailing lists