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áč <michal.vokac@xxxxxxxxx>
> ---
> 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.
> + 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
Attachment:
signature.asc
Description: PGP signature
