Re: [RCF PATCH v2 2/2] pwm: imx: Configure output to GPIO in disabled state

Thierry Reding
2018-10-12T16:00:45Z

[-- Attachment #1: Type: text/plain, Size: 4634 bytes --]

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
> ___________________________________________ _ _ _ _____________
> |_| |_| |_| |_|
>
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.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;
> };
>
> +
> #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)) {

This is not correct. First, I don't think devm_pinctrl_get() will ever
return NULL, so the !imx_chip->pinctrl is useless. And if it did return
NULL and imx_chip->pinctrl could therefore be NULL, then...

> + dev_info(&pdev->dev, "can not get pinctrl\n");
> + return PTR_ERR(imx_chip->pinctrl);

... this is rubbish because it returns PTR_ERR(NULL) which is 0 and that
represents success.

While at it, dev_info() -> dev_err() might be more appropriate here. Or
if you want to make pinctrl support optional make this dev_dbg() like
the message further below.

> + }
> +
> + 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) {
> + 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)) {
> + dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");

Another option would be to make this (and the above) dev_warn() since we
do want people to update the DTB if at all possible. Without the DTB the
PWM could be susceptible to the issue that we're trying to fix.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]