Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state
From: Uwe Kleine-König <hidden>
Date: 2018-10-12 09:04:06
Also in:
linux-pwm, lkml
Hello, 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.
quoted hunk ↗ jump to hunk
Signed-off-by: Michal Vokáč <redacted> --- 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.
};
+
#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.
+ return -EPROBE_DEFER;
+ } else if (IS_ERR(imx_chip->pwm_gpiod) ||
+ IS_ERR(imx_chip->pinctrl_pins_pwm) ||
+ IS_ERR(imx_chip->pinctrl_pins_gpio)) {Would it be more correct to handle imx_chip->pinctrl_pins_pwm == ERR_PTR(-EPROBE_DEFER) similar to imx_chip->pwm_gpiod == ERR_PTR(-EPROBE_DEFER)?
+ dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");
I wouldn't call that "incomplete". It's incomplete for the gpio switching trick, but enough in general.
quoted hunk ↗ jump to hunk
+ devm_pinctrl_put(imx_chip->pinctrl); + imx_chip->pinctrl = NULL; + } + + return 0; +} + static void imx_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) {@@ -306,7 +343,31 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm, MX3_PWMCR_POUTC_INVERTED); writel(cr, imx->mmio_base + MX3_PWMCR); + + /* + * If we are in charge of pinctrl then switch output to + * the PWM signal. + */ + if (imx->pinctrl) + pinctrl_select_state(imx->pinctrl, + imx->pinctrl_pins_pwm); } else if (cstate.enabled) { + /* + * PWM block will be disabled. Normally its output will be set + * low no matter what output polarity is configured. Lets use
s/Lets/Let's/
+ * 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?
+ * First set the GPIO to the desired level, then switch the
+ * muxing and at last disable PWM. In that order we do not get
+ * unwanted logic level changes on the output.
+ */
+ if (imx->pinctrl) {
+ gpiod_set_value_cansleep(imx->pwm_gpiod, 0);You must call gpiod_direction_output for this to have any effect. 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.
+ pinctrl_select_state(imx->pinctrl, + imx->pinctrl_pins_gpio);
Usually align function arguments to the opening (.
quoted hunk ↗ jump to hunk
+ } + 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.) Best regards Uwe
Attachments
- signature.asc [application/pgp-signature] 488 bytes