| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20181206135902.un2nbreqfmi6mpd6@pengutronix.de>
Date: Thu, 6 Dec 2018 14:59:02 +0100
From: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
To: Vokáč Michal <Michal.Vokac@...ft.com>
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>,
Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [RFC PATCH v3 2/2] pwm: imx: Configure output to GPIO in
disabled state
On Thu, Dec 06, 2018 at 01:41:31PM +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 "pwm" and a "gpio" pinctrl states. The pwm pinctrl
> state is selected when PWM is enabled and the gpio pinctrl state is
> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
> configured as input and the internal pull-up resistor is used to pull the
> output level high.
>
> If all the pinctrl states and the pwm-gpios GPIO are not correctly
> specified in DT the PWM work as usual.
>
> 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.
>
> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
>
> +--------------+------------+---------------+----------- +-------------+
> | After reset | Bootloader | PWM probe | PWM | PWM |
> | 100k pull-up | | | enable 30% | disable |
> +--------------+------------+---------------+------------+-------------+
> | pinctrl | none | default | default | default |
> | out H __________________ __ __ |
> | out L \_________________/ \_/ \_/\____________ |
> | ^ ^ ^ |
> +--------------+------------+---------------+------------+-------------+
> | pinctrl | none | gpio | pwm | gpio |
> | out H __________________________________ __ __ _____________ |
> | out L \_/ \_/ \_/ |
> | ^ ^ ^ |
> +----------------------------------------------------------------------+
>
> Signed-off-by: Michal Vokáč <michal.vokac@...ft.com>
> ---
> Changes in v3:
> - Commit message update.
> - Minor fix in code comment (Uwe)
> - Align function arguments to the opening parentheses. (Uwe)
> - Do not test devm_pinctrl_get for NULL. (Thierry)
> - Convert all messages to dev_dbg() (Thierry)
> - Do not actively drive the pin in gpio state. Configure it as input
> and rely solely on the internal pull-up. (Thierry)
>
> 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 | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 1d5242c..84eaec8 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -12,10 +12,12 @@
> #include <linux/err.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/io.h>
> #include <linux/pwm.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>
> /* i.MX1 and i.MX21 share the same PWM function block: */
>
> @@ -52,10 +54,44 @@ 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)
Please indent the follow up line to the opening parenthesis.
> +{
> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(imx_chip->pinctrl)) {
> + dev_dbg(&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) {
> + 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");
> + devm_pinctrl_put(imx_chip->pinctrl);
> + imx_chip->pinctrl = NULL;
Can it happen, that pinctrl_pins_pwm is PTR_ERR(-EPROBE_DEFER)?
Maybe you only want to ignore PTR_ERR(-ENODEV) and for example propagate
-EIO? I think you want to put the gpio if the failure is because there
is a pinctrl related error.
> + }
> +
> + return 0;
> +}
> +
> static int imx_pwm_config_v1(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> @@ -216,7 +252,25 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> cr |= MX3_PWMCR_POUTC;
>
> 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. Let's use
> + * pinctrl to switch the output pin to GPIO functon and keep
> + * the output at the same level as for duty-cycle = 0.
> + */
> + if (imx->pinctrl)
> + pinctrl_select_state(imx->pinctrl,
> + imx->pinctrl_pins_gpio);
> +
> writel(0, imx->mmio_base + MX3_PWMCR);
>
> clk_disable_unprepare(imx->clk_per);
> @@ -263,6 +317,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;
> @@ -294,6 +349,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))
> @@ -303,6 +362,24 @@ 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 {
> + pinctrl_select_state(imx->pinctrl,
> + imx->pinctrl_pins_gpio);
> + }
> + }
> +
ISTR that there was a patch that implements get_state for imx. Is there
a dependency on that one? Otherwise the state returned by
pwm_get_state() might not be what is actually configured.
Do you know if this is required for the old i.MX pwm, e.g. on i.MX21?
I ask because of https://patchwork.ozlabs.org/patch/1000071/
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Powered by blists - more mailing lists