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. > Signed-off-by: Michal Vokáč > --- > 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 > #include > #include > +#include > #include > #include > #include > #include > #include > +#include > #include > #include > #include > @@ -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. > + 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 (. > + } > + > 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