[{"id":3674706,"web_url":"http://patchwork.ozlabs.org/comment/3674706/","msgid":"<8030cac3703f9aa1b7a8b476ad92aeae@trvn.ru>","list_archive_url":null,"date":"2026-04-08T10:42:46","subject":"Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for\n constant output levels","submitter":{"id":82201,"url":"http://patchwork.ozlabs.org/api/people/82201/","name":"Nikita Travkin","email":"nikita@trvn.ru"},"content":"Xilin Wu писал(а) 08.04.2026 15:07:\n> The clk-pwm driver cannot guarantee a defined output level when the\n> PWM is disabled or when 0%/100% duty cycle is requested, because the\n> pin state when the clock is stopped is hardware-dependent.\n> \n> Add optional GPIO and pinctrl support: when a GPIO descriptor and\n> pinctrl states (\"default\" for clock mux, \"gpio\" for GPIO mode) are\n> provided in the device tree, the driver switches the pin to GPIO mode\n> and drives the appropriate level for disabled/0%/100% states. For\n> normal PWM output, the pin is switched back to its clock function mux.\n> \n> If no GPIO is provided, the driver falls back to the original\n> clock-only behavior.\n> \n> Signed-off-by: Xilin Wu <sophon@radxa.com>\n> ---\n>  drivers/pwm/pwm-clk.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++---\n>  1 file changed, 80 insertions(+), 4 deletions(-)\n> \n> diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c\n> index f8f5af57acba..d7d8d2c2dd0f 100644\n> --- a/drivers/pwm/pwm-clk.c\n> +++ b/drivers/pwm/pwm-clk.c\n> @@ -11,11 +11,20 @@\n>   * - Due to the fact that exact behavior depends on the underlying\n>   *   clock driver, various limitations are possible.\n>   * - Underlying clock may not be able to give 0% or 100% duty cycle\n> - *   (constant off or on), exact behavior will depend on the clock.\n> + *   (constant off or on), exact behavior will depend on the clock,\n> + *   unless a gpio pinctrl state is supplied.\n>   * - When the PWM is disabled, the clock will be disabled as well,\n> - *   line state will depend on the clock.\n> + *   line state will depend on the clock, unless a gpio pinctrl\n> + *   state is supplied.\n>   * - The clk API doesn't expose the necessary calls to implement\n>   *   .get_state().\n> + *\n> + * Optionally, a GPIO descriptor and pinctrl states (\"default\" and\n> + * \"gpio\") can be provided. When a constant output level is needed\n> + * (0% duty, 100% duty, or disabled), the driver switches the pin to\n> + * GPIO mode and drives the appropriate level. For normal PWM output\n> + * the pin is switched back to its clock function mux. If no GPIO is\n> + * provided, the driver falls back to the original clock-only behavior.\n>   */\n>  \n>  #include <linux/kernel.h>\n> @@ -25,11 +34,17 @@\n>  #include <linux/of.h>\n>  #include <linux/platform_device.h>\n>  #include <linux/clk.h>\n> +#include <linux/gpio/consumer.h>\n> +#include <linux/pinctrl/consumer.h>\n>  #include <linux/pwm.h>\n>  \n>  struct pwm_clk_chip {\n>  \tstruct clk *clk;\n>  \tbool clk_enabled;\n> +\tstruct pinctrl *pinctrl;\n> +\tstruct pinctrl_state *pins_default;  /* clock function mux */\n> +\tstruct pinctrl_state *pins_gpio;     /* GPIO mode */\n> +\tstruct gpio_desc *gpiod;\n>  };\n>  \n>  static inline struct pwm_clk_chip *to_pwm_clk_chip(struct pwm_chip *chip)\n> @@ -45,14 +60,36 @@ static int pwm_clk_apply(struct pwm_chip *chip, struct pwm_device *pwm,\n>  \tu32 rate;\n>  \tu64 period = state->period;\n>  \tu64 duty_cycle = state->duty_cycle;\n> +\tbool constant_level = false;\n> +\tint gpio_value = 0;\n>  \n>  \tif (!state->enabled) {\n> -\t\tif (pwm->state.enabled) {\n> +\t\tconstant_level = true;\n> +\t\tgpio_value = 0;\n> +\t} else if (state->duty_cycle == 0) {\n> +\t\tconstant_level = true;\n> +\t\tgpio_value = (state->polarity == PWM_POLARITY_INVERSED) ? 1 : 0;\n> +\t} else if (state->duty_cycle >= state->period) {\n> +\t\tconstant_level = true;\n> +\t\tgpio_value = (state->polarity == PWM_POLARITY_INVERSED) ? 0 : 1;\n> +\t}\n> +\n\nSo I'm looking at it again, and I'm a bit confused.\n\nOld behavior was:\n - pwm was enabled and being disabled -> stop the clock and hope state is 0;\n - pwm is still enabled but\n                            - duty=0%   -> set clk duty to 0%\n                            - duty=100% -> set clk duty to 100%\n\nNew behavior if we have gpio:\n - pwm was enabled and being disabled -> constant 0\n - pwm is still enabled but\n                            - duty=0%   -> constant 0\n                            - duty=100% -> constant 1\n\nNew behavior if we don't have gpio:\nSame as above but\n  - if we need constant 0 -> clock is halted and we pray it's 0\n  - if we need constant 1 -> clock is halted and we pray it's 1 (??)\n\nPer my recollection, when I wrote this driver 5 years ago, I've manually\nverified that at least on qcom setting duty cycle to 0% and 100% worked\nproperly, so this feels like it would regress it if left as-is...\n\n(Btw I wonder what's the platform you need this for?)\n\n> +\tif (constant_level) {\n> +\t\tif (pcchip->gpiod) {\n> +\t\t\tgpiod_direction_output(pcchip->gpiod, gpio_value);\n> +\t\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);\n> +\t\t}\n> +\t\tif (pcchip->clk_enabled) {\n>  \t\t\tclk_disable(pcchip->clk);\n>  \t\t\tpcchip->clk_enabled = false;\n>  \t\t}\n>  \t\treturn 0;\n> -\t} else if (!pwm->state.enabled) {\n> +\t}\n> +\n> +\tif (pcchip->gpiod)\n> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_default);\n> +\n> +\tif (!pcchip->clk_enabled) {\n>  \t\tret = clk_enable(pcchip->clk);\n>  \t\tif (ret)\n>  \t\t\treturn ret;\n> @@ -97,6 +134,45 @@ static int pwm_clk_probe(struct platform_device *pdev)\n>  \t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcchip->clk),\n>  \t\t\t\t     \"Failed to get clock\\n\");\n>  \n> +\tpcchip->pinctrl = devm_pinctrl_get(&pdev->dev);\n> +\tif (IS_ERR(pcchip->pinctrl)) {\n> +\t\tret = PTR_ERR(pcchip->pinctrl);\n> +\t\tpcchip->pinctrl = NULL;\n> +\t\tif (ret == -EPROBE_DEFER)\n> +\t\t\treturn ret;\n> +\t} else {\n> +\t\tpcchip->pins_default = pinctrl_lookup_state(pcchip->pinctrl,\n> +\t\t\t\t\t\t\t    PINCTRL_STATE_DEFAULT);\n> +\t\tpcchip->pins_gpio = pinctrl_lookup_state(pcchip->pinctrl,\n> +\t\t\t\t\t\t\t \"gpio\");\n> +\t\tif (IS_ERR(pcchip->pins_default) || IS_ERR(pcchip->pins_gpio))\n> +\t\t\tpcchip->pinctrl = NULL;\n> +\t}\n> +\n> +\t/*\n> +\t * Switch to GPIO pinctrl state before requesting the GPIO.\n> +\t * The driver core has already applied the \"default\" state, which\n> +\t * muxes the pin to the clock function and claims it.  We must\n> +\t * release that claim first so that gpiolib can request the pin.\n> +\t */\n> +\tif (pcchip->pinctrl)\n> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);\n> +\n> +\tpcchip->gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, GPIOD_ASIS);\n> +\tif (IS_ERR(pcchip->gpiod))\n> +\t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcchip->gpiod),\n> +\t\t\t\t     \"Failed to get gpio\\n\");\n> +\n> +\t/*\n> +\t * If pinctrl states were found but no GPIO was provided, the pin is\n> +\t * stuck in GPIO mode from the switch above.  Restore the default\n> +\t * (clock-function) mux and fall back to clock-only operation.\n> +\t */\n\nFeels slightly weird to silently allow \"broken\" DT, it would make no sense\nfor it to have \"gpio\" pinctrl and not have a gpio defined, would it?\n\nPerhaps it makes more sense to put getting a gpio under having pins_gpio\nand make it strict, so two allowed states for the driver would be either\nno pinctrl-1 and no gpio, or having both at the same time?\n\n(maybe then also worth adding cross dependency of pinctrl-1 and gpio in\nthe binding, it's one way only currently, not sure what's the correct\nway to describe it tho)\n\nNikita\n\n> +\tif (pcchip->pinctrl && !pcchip->gpiod) {\n> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_default);\n> +\t\tpcchip->pinctrl = NULL;\n> +\t}\n> +\n>  \tchip->ops = &pwm_clk_ops;\n>  \n>  \tret = pwmchip_add(chip);","headers":{"Return-Path":"\n <linux-pwm+bounces-8521-incoming=patchwork.ozlabs.org@vger.kernel.org>","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pwm@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=trvn.ru header.i=@trvn.ru header.a=rsa-sha256\n header.s=mail header.b=ISGQ7Jq2;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=172.234.253.10; helo=sea.lore.kernel.org;\n envelope-from=linux-pwm+bounces-8521-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=trvn.ru header.i=@trvn.ru\n header.b=\"ISGQ7Jq2\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=45.141.101.25","smtp.subspace.kernel.org;\n dmarc=pass (p=quarantine dis=none) header.from=trvn.ru","smtp.subspace.kernel.org;\n spf=pass smtp.mailfrom=trvn.ru"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org [172.234.253.10])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4frKS95sfrz1yD3\n\tfor <incoming@patchwork.ozlabs.org>; Wed, 08 Apr 2026 20:44:09 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id 05B613036777\n\tfor <incoming@patchwork.ozlabs.org>; Wed,  8 Apr 2026 10:42:58 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 941A43B38BD;\n\tWed,  8 Apr 2026 10:42:56 +0000 (UTC)","from box.trvn.ru (box.trvn.ru [45.141.101.25])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id E4D0F37F75C;\n\tWed,  8 Apr 2026 10:42:54 +0000 (UTC)","from authenticated-user (box.trvn.ru [45.141.101.25])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest\n SHA256)\n\t(No client certificate requested)\n\tby box.trvn.ru (Postfix) with ESMTPSA id A5F396E95D;\n\tWed,  8 Apr 2026 15:42:46 +0500 (+05)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1775644976; cv=none;\n b=TR1e4iU57wvLq1WGQnsaym3tvwPEh9lyaDmj9eKXgnXb+oMfAk6clIuUpcJYQYHhHLBaYO5n+gChrRV+a5+wa1E3a5f7Zxtfu24B+0Jas0fBsaRoJRfBzDKal/2D6AKyzShOwYl+yI+rylSEOHwpw06PFRHqR/WsB9/fvTZ9m2Q=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1775644976; c=relaxed/simple;\n\tbh=RW860wbOyJPK1lrUCMSC83VTWhl2YZIfD3CZo/XgccI=;\n\th=MIME-Version:Date:From:To:Cc:Subject:In-Reply-To:References:\n\t Message-ID:Content-Type;\n b=Vb/T8ejzIhGoC4b7c4bUeuvVzzingMdbhg2BJ5pJxZzxbO91RjGifmCBufgBvLvGXVMPIz+E1wFP54d8LIpDSyxCkxYg6/OcUP88HTpxAmuUOkyNavzK9mg7coCqQd51n3k+ORfKdIUkpWV4VmkKsirQuU9Szc6Gy9FvWG4Vkcc=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dmarc=pass (p=quarantine dis=none) header.from=trvn.ru;\n spf=pass smtp.mailfrom=trvn.ru;\n dkim=pass (2048-bit key) header.d=trvn.ru header.i=@trvn.ru\n header.b=ISGQ7Jq2; arc=none smtp.client-ip=45.141.101.25","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=trvn.ru; s=mail;\n\tt=1775644966; bh=RW860wbOyJPK1lrUCMSC83VTWhl2YZIfD3CZo/XgccI=;\n\th=Date:From:To:Cc:Subject:In-Reply-To:References:From;\n\tb=ISGQ7Jq2xxmMcbIJpib4DTRURf1zAjSnyAsOLbVV/IIDk3nBMOPXpMl6WF40+lkLX\n\t HcQFRKt3Qyu1XnHlRhloxPUzcqtM4vu0cFDt25/QN6VoFoTX9ohlB4vCoHGDhWq78D\n\t exF/MRsMw6WGEBQ96e0Qj9tX3NdZTsOFy4iDdLrZDUhyIn2Yw9yBjH953Rn38yqa49\n\t ER07pe9NUDgT9sasmx5/uOj7myxr9RJz+BzldAQcDMKYT3yHdP89FseBiPELM9aFru\n\t /I5LlkjHmPfWZBCNglv37NP+VYPX1lOymihmtAJEKyg8PDQhdLFzyiNRlZKKYx4U4K\n\t et5xs4YsUdoig==","Precedence":"bulk","X-Mailing-List":"linux-pwm@vger.kernel.org","List-Id":"<linux-pwm.vger.kernel.org>","List-Subscribe":"<mailto:linux-pwm+subscribe@vger.kernel.org>","List-Unsubscribe":"<mailto:linux-pwm+unsubscribe@vger.kernel.org>","MIME-Version":"1.0","Date":"Wed, 08 Apr 2026 15:42:46 +0500","From":"Nikita Travkin <nikita@trvn.ru>","To":"Xilin Wu <sophon@radxa.com>","Cc":"=?utf-8?q?Uwe_Kleine-K=C3=B6nig?= <ukleinek@kernel.org>,\n Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>,\n Conor Dooley <conor+dt@kernel.org>, linux-pwm@vger.kernel.org,\n devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,\n linux-arm-msm@vger.kernel.org","Subject":"Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for\n constant output levels","In-Reply-To":"<20260408-clk-pwm-gpio-v2-2-d22f1f3498a0@radxa.com>","References":"<20260408-clk-pwm-gpio-v2-0-d22f1f3498a0@radxa.com>\n <20260408-clk-pwm-gpio-v2-2-d22f1f3498a0@radxa.com>","Message-ID":"<8030cac3703f9aa1b7a8b476ad92aeae@trvn.ru>","X-Sender":"nikita@trvn.ru","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit"}},{"id":3674766,"web_url":"http://patchwork.ozlabs.org/comment/3674766/","msgid":"<7BA26FC036D1C9AF+64204287-21b5-4664-ae75-be3dd54ec092@radxa.com>","list_archive_url":null,"date":"2026-04-08T13:19:19","subject":"Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for\n constant output levels","submitter":{"id":90715,"url":"http://patchwork.ozlabs.org/api/people/90715/","name":"Xilin Wu","email":"sophon@radxa.com"},"content":"On 4/8/2026 6:42 PM, Nikita Travkin wrote:\n> Xilin Wu писал(а) 08.04.2026 15:07:\n>> The clk-pwm driver cannot guarantee a defined output level when the\n>> PWM is disabled or when 0%/100% duty cycle is requested, because the\n>> pin state when the clock is stopped is hardware-dependent.\n>>\n>> Add optional GPIO and pinctrl support: when a GPIO descriptor and\n>> pinctrl states (\"default\" for clock mux, \"gpio\" for GPIO mode) are\n>> provided in the device tree, the driver switches the pin to GPIO mode\n>> and drives the appropriate level for disabled/0%/100% states. For\n>> normal PWM output, the pin is switched back to its clock function mux.\n>>\n>> If no GPIO is provided, the driver falls back to the original\n>> clock-only behavior.\n>>\n>> Signed-off-by: Xilin Wu <sophon@radxa.com>\n>> ---\n>>   drivers/pwm/pwm-clk.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++---\n>>   1 file changed, 80 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c\n>> index f8f5af57acba..d7d8d2c2dd0f 100644\n>> --- a/drivers/pwm/pwm-clk.c\n>> +++ b/drivers/pwm/pwm-clk.c\n>> @@ -11,11 +11,20 @@\n>>    * - Due to the fact that exact behavior depends on the underlying\n>>    *   clock driver, various limitations are possible.\n>>    * - Underlying clock may not be able to give 0% or 100% duty cycle\n>> - *   (constant off or on), exact behavior will depend on the clock.\n>> + *   (constant off or on), exact behavior will depend on the clock,\n>> + *   unless a gpio pinctrl state is supplied.\n>>    * - When the PWM is disabled, the clock will be disabled as well,\n>> - *   line state will depend on the clock.\n>> + *   line state will depend on the clock, unless a gpio pinctrl\n>> + *   state is supplied.\n>>    * - The clk API doesn't expose the necessary calls to implement\n>>    *   .get_state().\n>> + *\n>> + * Optionally, a GPIO descriptor and pinctrl states (\"default\" and\n>> + * \"gpio\") can be provided. When a constant output level is needed\n>> + * (0% duty, 100% duty, or disabled), the driver switches the pin to\n>> + * GPIO mode and drives the appropriate level. For normal PWM output\n>> + * the pin is switched back to its clock function mux. If no GPIO is\n>> + * provided, the driver falls back to the original clock-only behavior.\n>>    */\n>>   \n>>   #include <linux/kernel.h>\n>> @@ -25,11 +34,17 @@\n>>   #include <linux/of.h>\n>>   #include <linux/platform_device.h>\n>>   #include <linux/clk.h>\n>> +#include <linux/gpio/consumer.h>\n>> +#include <linux/pinctrl/consumer.h>\n>>   #include <linux/pwm.h>\n>>   \n>>   struct pwm_clk_chip {\n>>   \tstruct clk *clk;\n>>   \tbool clk_enabled;\n>> +\tstruct pinctrl *pinctrl;\n>> +\tstruct pinctrl_state *pins_default;  /* clock function mux */\n>> +\tstruct pinctrl_state *pins_gpio;     /* GPIO mode */\n>> +\tstruct gpio_desc *gpiod;\n>>   };\n>>   \n>>   static inline struct pwm_clk_chip *to_pwm_clk_chip(struct pwm_chip *chip)\n>> @@ -45,14 +60,36 @@ static int pwm_clk_apply(struct pwm_chip *chip, struct pwm_device *pwm,\n>>   \tu32 rate;\n>>   \tu64 period = state->period;\n>>   \tu64 duty_cycle = state->duty_cycle;\n>> +\tbool constant_level = false;\n>> +\tint gpio_value = 0;\n>>   \n>>   \tif (!state->enabled) {\n>> -\t\tif (pwm->state.enabled) {\n>> +\t\tconstant_level = true;\n>> +\t\tgpio_value = 0;\n>> +\t} else if (state->duty_cycle == 0) {\n>> +\t\tconstant_level = true;\n>> +\t\tgpio_value = (state->polarity == PWM_POLARITY_INVERSED) ? 1 : 0;\n>> +\t} else if (state->duty_cycle >= state->period) {\n>> +\t\tconstant_level = true;\n>> +\t\tgpio_value = (state->polarity == PWM_POLARITY_INVERSED) ? 0 : 1;\n>> +\t}\n>> +\n> \n> So I'm looking at it again, and I'm a bit confused.\n> \n> Old behavior was:\n>   - pwm was enabled and being disabled -> stop the clock and hope state is 0;\n>   - pwm is still enabled but\n>                              - duty=0%   -> set clk duty to 0%\n>                              - duty=100% -> set clk duty to 100%\n> \n> New behavior if we have gpio:\n>   - pwm was enabled and being disabled -> constant 0\n>   - pwm is still enabled but\n>                              - duty=0%   -> constant 0\n>                              - duty=100% -> constant 1\n> \n> New behavior if we don't have gpio:\n> Same as above but\n>    - if we need constant 0 -> clock is halted and we pray it's 0\n>    - if we need constant 1 -> clock is halted and we pray it's 1 (??)\n> \n> Per my recollection, when I wrote this driver 5 years ago, I've manually\n> verified that at least on qcom setting duty cycle to 0% and 100% worked\n> properly, so this feels like it would regress it if left as-is...\n> \n> (Btw I wonder what's the platform you need this for?)\n> \n\nI took a careful look at clk_rcg2_set_duty_cycle() in \ndrivers/clk/qcom/clk-rcg2.c, and I believe the Qualcomm RCG2 MND counter \ncannot produce a true 0% or 100% duty cycle. For a 0% duty request, the \nactual duty cycle can become very small, but never exactly zero. \nLikewise, for a 100% duty request, it can get very close to 100%, but \nnot exactly 100%.\n\nI agree that the current change may cause a regression. Do you think it \nwould make more sense to keep the old behavior when no GPIO is \navailable, and still set the clock duty cycle to 0% or 100% in that case?\n\nWe need this for many of our future Qualcomm-based products, because the \nPMIC that comes with the SoC usually provides only one PWM output.\n\n>> +\tif (constant_level) {\n>> +\t\tif (pcchip->gpiod) {\n>> +\t\t\tgpiod_direction_output(pcchip->gpiod, gpio_value);\n>> +\t\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);\n>> +\t\t}\n>> +\t\tif (pcchip->clk_enabled) {\n>>   \t\t\tclk_disable(pcchip->clk);\n>>   \t\t\tpcchip->clk_enabled = false;\n>>   \t\t}\n>>   \t\treturn 0;\n>> -\t} else if (!pwm->state.enabled) {\n>> +\t}\n>> +\n>> +\tif (pcchip->gpiod)\n>> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_default);\n>> +\n>> +\tif (!pcchip->clk_enabled) {\n>>   \t\tret = clk_enable(pcchip->clk);\n>>   \t\tif (ret)\n>>   \t\t\treturn ret;\n>> @@ -97,6 +134,45 @@ static int pwm_clk_probe(struct platform_device *pdev)\n>>   \t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcchip->clk),\n>>   \t\t\t\t     \"Failed to get clock\\n\");\n>>   \n>> +\tpcchip->pinctrl = devm_pinctrl_get(&pdev->dev);\n>> +\tif (IS_ERR(pcchip->pinctrl)) {\n>> +\t\tret = PTR_ERR(pcchip->pinctrl);\n>> +\t\tpcchip->pinctrl = NULL;\n>> +\t\tif (ret == -EPROBE_DEFER)\n>> +\t\t\treturn ret;\n>> +\t} else {\n>> +\t\tpcchip->pins_default = pinctrl_lookup_state(pcchip->pinctrl,\n>> +\t\t\t\t\t\t\t    PINCTRL_STATE_DEFAULT);\n>> +\t\tpcchip->pins_gpio = pinctrl_lookup_state(pcchip->pinctrl,\n>> +\t\t\t\t\t\t\t \"gpio\");\n>> +\t\tif (IS_ERR(pcchip->pins_default) || IS_ERR(pcchip->pins_gpio))\n>> +\t\t\tpcchip->pinctrl = NULL;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Switch to GPIO pinctrl state before requesting the GPIO.\n>> +\t * The driver core has already applied the \"default\" state, which\n>> +\t * muxes the pin to the clock function and claims it.  We must\n>> +\t * release that claim first so that gpiolib can request the pin.\n>> +\t */\n>> +\tif (pcchip->pinctrl)\n>> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);\n>> +\n>> +\tpcchip->gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, GPIOD_ASIS);\n>> +\tif (IS_ERR(pcchip->gpiod))\n>> +\t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcchip->gpiod),\n>> +\t\t\t\t     \"Failed to get gpio\\n\");\n>> +\n>> +\t/*\n>> +\t * If pinctrl states were found but no GPIO was provided, the pin is\n>> +\t * stuck in GPIO mode from the switch above.  Restore the default\n>> +\t * (clock-function) mux and fall back to clock-only operation.\n>> +\t */\n> \n> Feels slightly weird to silently allow \"broken\" DT, it would make no sense\n> for it to have \"gpio\" pinctrl and not have a gpio defined, would it?\n> \n> Perhaps it makes more sense to put getting a gpio under having pins_gpio\n> and make it strict, so two allowed states for the driver would be either\n> no pinctrl-1 and no gpio, or having both at the same time?\n> \n> (maybe then also worth adding cross dependency of pinctrl-1 and gpio in\n> the binding, it's one way only currently, not sure what's the correct\n> way to describe it tho)\n> \n> Nikita\n> \n\nYeah, good point. Having a gpio pinctrl state without an actual gpio \nproperty is indeed a broken DT and there's no reason to silently work \naround it. Do you think the following change would work?\n\n\tif (pcchip->pinctrl) {\n\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);\n\n\t\tpcchip->gpiod = devm_gpiod_get(&pdev->dev, NULL, GPIOD_ASIS);\n\t\tif (IS_ERR(pcchip->gpiod))\n\t\t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcchip->gpiod),\n\t\t\t\t\t     \"GPIO required when 'gpio' pinctrl state is present\\n\");\n\t}\n\n>> +\tif (pcchip->pinctrl && !pcchip->gpiod) {\n>> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_default);\n>> +\t\tpcchip->pinctrl = NULL;\n>> +\t}\n>> +\n>>   \tchip->ops = &pwm_clk_ops;\n>>   \n>>   \tret = pwmchip_add(chip);\n>","headers":{"Return-Path":"\n <linux-pwm+bounces-8522-incoming=patchwork.ozlabs.org@vger.kernel.org>","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pwm@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c0a:e001:db::12fc:5321; helo=sea.lore.kernel.org;\n envelope-from=linux-pwm+bounces-8522-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=54.207.19.206","smtp.subspace.kernel.org;\n dmarc=pass (p=none dis=none) header.from=radxa.com","smtp.subspace.kernel.org;\n spf=pass smtp.mailfrom=radxa.com"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org\n [IPv6:2600:3c0a:e001:db::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4frP4N29QZz1xy1\n\tfor <incoming@patchwork.ozlabs.org>; Wed, 08 Apr 2026 23:27:16 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id A5FDA30C7AD8\n\tfor <incoming@patchwork.ozlabs.org>; Wed,  8 Apr 2026 13:20:14 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 163543264FA;\n\tWed,  8 Apr 2026 13:20:09 +0000 (UTC)","from smtpbgbr1.qq.com (smtpbgbr1.qq.com [54.207.19.206])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 6EBEB313E36;\n\tWed,  8 Apr 2026 13:20:01 +0000 (UTC)","from [127.0.0.1] ( [116.234.85.158])\n\tby bizesmtp.qq.com (ESMTP) with\n\tid ; Wed, 08 Apr 2026 21:19:21 +0800 (CST)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1775654409; cv=none;\n b=X9PBi7f3ry6YDbKTn72mx78JbfqpmNPb4GStmWNvgICxbzKFW++L/mbGN+1e0H8qGQHPyHPCcp3iuaoRqGN4qt16j7dmzm1GTJCO0p0rvoFqb9t7OlUNwZAZpnfE3p1BQt9yvanHqvrdYbSx6OpyresBp6jycONYzoDyKOgp7yA=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1775654409; c=relaxed/simple;\n\tbh=Qwpbhww7CrXXunhU8ku2p7hNzmTqVyIvdYsDejjvfsM=;\n\th=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:\n\t In-Reply-To:Content-Type;\n b=lfWSDabGaaAnljuFUu1QHjr/oT+UnKCXBngDw1klXEFf9YqhTsh2fxYloLyCtvihW8JcEHAhGnBD3OcdUmfnc7DvaRP5EaS7GRd35R0PTupU0HkwFv/ohygyB9Az0CHtNAz84bLFWYrKm0oeleQsMoh13bxkXfC+JpxXE3KYe3M=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dmarc=pass (p=none dis=none) header.from=radxa.com;\n spf=pass smtp.mailfrom=radxa.com; arc=none smtp.client-ip=54.207.19.206","X-QQ-mid":"esmtpsz18t1775654363t3cb4073c","X-QQ-Originating-IP":"gZbJhug9qvNVdH4zYW+DyL7LZUKCNEJAVtc2edbWdQA=","X-QQ-SSF":"0000000000000000000000000000000","X-QQ-GoodBg":"0","X-BIZMAIL-ID":"9970830700309624208","Message-ID":"<7BA26FC036D1C9AF+64204287-21b5-4664-ae75-be3dd54ec092@radxa.com>","Date":"Wed, 8 Apr 2026 21:19:19 +0800","Precedence":"bulk","X-Mailing-List":"linux-pwm@vger.kernel.org","List-Id":"<linux-pwm.vger.kernel.org>","List-Subscribe":"<mailto:linux-pwm+subscribe@vger.kernel.org>","List-Unsubscribe":"<mailto:linux-pwm+unsubscribe@vger.kernel.org>","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for\n constant output levels","To":"Nikita Travkin <nikita@trvn.ru>","Cc":"=?utf-8?q?Uwe_Kleine-K=C3=B6nig?= <ukleinek@kernel.org>,\n Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>,\n Conor Dooley <conor+dt@kernel.org>, linux-pwm@vger.kernel.org,\n devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,\n linux-arm-msm@vger.kernel.org","References":"<20260408-clk-pwm-gpio-v2-0-d22f1f3498a0@radxa.com>\n <20260408-clk-pwm-gpio-v2-2-d22f1f3498a0@radxa.com>\n <8030cac3703f9aa1b7a8b476ad92aeae@trvn.ru>","Content-Language":"en-US","From":"Xilin Wu <sophon@radxa.com>","In-Reply-To":"<8030cac3703f9aa1b7a8b476ad92aeae@trvn.ru>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-QQ-SENDSIZE":"520","Feedback-ID":"esmtpsz:radxa.com:qybglogicsvrsz:qybglogicsvrsz3b-0","X-QQ-XMAILINFO":"NvjhxCSDgXICj91atmTlnbUBY1+dVlCNR7IDUDdEfp3KvGNGjxPVNsDo\n\turwzpIJKfXKahcDXxbOx/8S3xnUUsppvftsmv/qBRmvlvO8tNMZpR1qRcBV3zlDXvLnEPlx\n\tNPvAcb88zwvZgw+2BPMR01uIhdgmJ4HtK7eYmPKDezPl+UVVD2AOzN55SkgTQcShRxAt6zr\n\tiqzz0NqAAbEclWMhZm/FpYtHEFwA/X8Wlh/IIPNKhDXcgCCz290LyineUzbYQeKaekFoW7k\n\tG/c1LKxHHyWxigGfMRmB10XzS2irKPrjV9I+uxl4mGrPQ7IZ1GbsERGgR0EcUSXUuEc5xYT\n\tFbHG4aFmQ2AsaZ/uO95DXqnt9WXbNJl1y5+AkvvMamHWJhho0QQ9ZoB+Zun3qdasOUF/Yq0\n\t/85ghtMIPFJCZmfvY7KKtjyWDb9Vk8qoEDITwdRINBZ8yphiu2nmqqTbjLKIuvIr3MP9tyk\n\t8srS4B9pVrwAY9r8+DFKC5bzJ5sgDmq6geK5U2QVyiuTKQZRPLlp1SziQGNRcCZ7ZTFP5Z8\n\tDivwzSYeBtHKKJFVrtdDz7X9HHBpZfrQSXvKnTUO8PUDc+fCaI9QfC/kJCVyIK8felIzWU4\n\t55VyfbBp+3dZgd2ge0cXW5ltjmmoPoyIqIM1RGIwDBtiASEwcBtH3YIEeO0toqXZFqOdLZl\n\t0MzxvhyGXCkT9tvBjLCzT/QIsZaGwtMLV7ZROlboMwOW0SxZo7zvRxR38WnGnG0KljCbe+H\n\tOTCxVrlRN/mA7qBBqsxAwGBCXXvlBVjVDqYhTjDlOHMfoxQGJ45a+bGejdSe2tKuRAQY89X\n\tg7nDJinK67u6H7ao8PQEtXXBSl3vMR1AW+Wvmwa5vbRuDiyTLVrbFtOCWQ4K6CA77igfLiQ\n\tRndIcy/1+AP+oq46o/e9+os6lfoWQXpyy2j+gGuOJTxXFYZho1jiWyK4YUw59k94Nr9lw0N\n\tf2RLOzbqTmNmAqkWOxiK4s+9/KUHZmLB5/hlctNJcIMjavocMBPATLqmpvk71ZcpNN2Yfg1\n\traq8Fs0Q==","X-QQ-XMRINFO":"OWPUhxQsoeAVwkVaQIEGSKwwgKCxK/fD5g==","X-QQ-RECHKSPAM":"0"}},{"id":3674787,"web_url":"http://patchwork.ozlabs.org/comment/3674787/","msgid":"<f7f1b27731c54e65f52d6fb8e347c878@trvn.ru>","list_archive_url":null,"date":"2026-04-08T13:53:35","subject":"Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for\n constant output levels","submitter":{"id":82201,"url":"http://patchwork.ozlabs.org/api/people/82201/","name":"Nikita Travkin","email":"nikita@trvn.ru"},"content":"Xilin Wu писал(а) 08.04.2026 18:19:\n> On 4/8/2026 6:42 PM, Nikita Travkin wrote:\n>> Xilin Wu писал(а) 08.04.2026 15:07:\n>>> The clk-pwm driver cannot guarantee a defined output level when the\n>>> PWM is disabled or when 0%/100% duty cycle is requested, because the\n>>> pin state when the clock is stopped is hardware-dependent.\n>>>\n>>> Add optional GPIO and pinctrl support: when a GPIO descriptor and\n>>> pinctrl states (\"default\" for clock mux, \"gpio\" for GPIO mode) are\n>>> provided in the device tree, the driver switches the pin to GPIO mode\n>>> and drives the appropriate level for disabled/0%/100% states. For\n>>> normal PWM output, the pin is switched back to its clock function mux.\n>>>\n>>> If no GPIO is provided, the driver falls back to the original\n>>> clock-only behavior.\n>>>\n>>> Signed-off-by: Xilin Wu <sophon@radxa.com>\n>>> ---\n>>>   drivers/pwm/pwm-clk.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++---\n>>>   1 file changed, 80 insertions(+), 4 deletions(-)\n>>>\n>>> diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c\n>>> index f8f5af57acba..d7d8d2c2dd0f 100644\n>>> --- a/drivers/pwm/pwm-clk.c\n>>> +++ b/drivers/pwm/pwm-clk.c\n>>> @@ -11,11 +11,20 @@\n>>>    * - Due to the fact that exact behavior depends on the underlying\n>>>    *   clock driver, various limitations are possible.\n>>>    * - Underlying clock may not be able to give 0% or 100% duty cycle\n>>> - *   (constant off or on), exact behavior will depend on the clock.\n>>> + *   (constant off or on), exact behavior will depend on the clock,\n>>> + *   unless a gpio pinctrl state is supplied.\n>>>    * - When the PWM is disabled, the clock will be disabled as well,\n>>> - *   line state will depend on the clock.\n>>> + *   line state will depend on the clock, unless a gpio pinctrl\n>>> + *   state is supplied.\n>>>    * - The clk API doesn't expose the necessary calls to implement\n>>>    *   .get_state().\n>>> + *\n>>> + * Optionally, a GPIO descriptor and pinctrl states (\"default\" and\n>>> + * \"gpio\") can be provided. When a constant output level is needed\n>>> + * (0% duty, 100% duty, or disabled), the driver switches the pin to\n>>> + * GPIO mode and drives the appropriate level. For normal PWM output\n>>> + * the pin is switched back to its clock function mux. If no GPIO is\n>>> + * provided, the driver falls back to the original clock-only behavior.\n>>>    */\n>>>     #include <linux/kernel.h>\n>>> @@ -25,11 +34,17 @@\n>>>   #include <linux/of.h>\n>>>   #include <linux/platform_device.h>\n>>>   #include <linux/clk.h>\n>>> +#include <linux/gpio/consumer.h>\n>>> +#include <linux/pinctrl/consumer.h>\n>>>   #include <linux/pwm.h>\n>>>     struct pwm_clk_chip {\n>>>   \tstruct clk *clk;\n>>>   \tbool clk_enabled;\n>>> +\tstruct pinctrl *pinctrl;\n>>> +\tstruct pinctrl_state *pins_default;  /* clock function mux */\n>>> +\tstruct pinctrl_state *pins_gpio;     /* GPIO mode */\n>>> +\tstruct gpio_desc *gpiod;\n>>>   };\n>>>     static inline struct pwm_clk_chip *to_pwm_clk_chip(struct pwm_chip *chip)\n>>> @@ -45,14 +60,36 @@ static int pwm_clk_apply(struct pwm_chip *chip, struct pwm_device *pwm,\n>>>   \tu32 rate;\n>>>   \tu64 period = state->period;\n>>>   \tu64 duty_cycle = state->duty_cycle;\n>>> +\tbool constant_level = false;\n>>> +\tint gpio_value = 0;\n>>>     \tif (!state->enabled) {\n>>> -\t\tif (pwm->state.enabled) {\n>>> +\t\tconstant_level = true;\n>>> +\t\tgpio_value = 0;\n>>> +\t} else if (state->duty_cycle == 0) {\n>>> +\t\tconstant_level = true;\n>>> +\t\tgpio_value = (state->polarity == PWM_POLARITY_INVERSED) ? 1 : 0;\n>>> +\t} else if (state->duty_cycle >= state->period) {\n>>> +\t\tconstant_level = true;\n>>> +\t\tgpio_value = (state->polarity == PWM_POLARITY_INVERSED) ? 0 : 1;\n>>> +\t}\n>>> +\n>> \n>> So I'm looking at it again, and I'm a bit confused.\n>> \n>> Old behavior was:\n>>   - pwm was enabled and being disabled -> stop the clock and hope state is 0;\n>>   - pwm is still enabled but\n>>                              - duty=0%   -> set clk duty to 0%\n>>                              - duty=100% -> set clk duty to 100%\n>> \n>> New behavior if we have gpio:\n>>   - pwm was enabled and being disabled -> constant 0\n>>   - pwm is still enabled but\n>>                              - duty=0%   -> constant 0\n>>                              - duty=100% -> constant 1\n>> \n>> New behavior if we don't have gpio:\n>> Same as above but\n>>    - if we need constant 0 -> clock is halted and we pray it's 0\n>>    - if we need constant 1 -> clock is halted and we pray it's 1 (??)\n>> \n>> Per my recollection, when I wrote this driver 5 years ago, I've manually\n>> verified that at least on qcom setting duty cycle to 0% and 100% worked\n>> properly, so this feels like it would regress it if left as-is...\n>> \n>> (Btw I wonder what's the platform you need this for?)\n>> \n> \n> I took a careful look at clk_rcg2_set_duty_cycle() in drivers/clk/qcom/clk-rcg2.c, and I believe the Qualcomm RCG2 MND counter cannot produce a true 0% or 100% duty cycle. For a 0% duty request, the actual duty cycle can become very small, but never exactly zero. Likewise, for a 100% duty request, it can get very close to 100%, but not exactly 100%.\n> \n\nAre you aware of the hardware quick of the clock [1] where you can't get\nfull range if your dividers aren't configured properly? I don't know if\nnew hardware is different in that regard comapred to the old sd410 I was\nworking with, but I recall spending a while with oscilloscope until I've\nfigured out why I wasn't getting full range from 0 to 100%.\n\nI'm pretty convinced I saw full coverage (i.e. flat 0 when clock is at\n0% and flat 1 when clock is at 100%) but perhaps I was measuring it wrong\nor I misremember as it was long ago... I still think your solution here\nis clever though, as long as you don't accidentally mask bugged gcc config.\n\n[1] https://elixir.bootlin.com/linux/v6.19/source/drivers/clk/qcom/gcc-msm8916.c#L958-L973\n\n> I agree that the current change may cause a regression. Do you think it would make more sense to keep the old behavior when no GPIO is available, and still set the clock duty cycle to 0% or 100% in that case?\n> \n\nYes please, keep the old behavior when there is no gpio. There are\ncertainly a few existing users for this and it would be sad to have\nsomeone's backlight go out when they set it to 100% xD\n\n> We need this for many of our future Qualcomm-based products, because the PMIC that comes with the SoC usually provides only one PWM output.\n> \n>>> +\tif (constant_level) {\n>>> +\t\tif (pcchip->gpiod) {\n>>> +\t\t\tgpiod_direction_output(pcchip->gpiod, gpio_value);\n>>> +\t\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);\n>>> +\t\t}\n>>> +\t\tif (pcchip->clk_enabled) {\n>>>   \t\t\tclk_disable(pcchip->clk);\n>>>   \t\t\tpcchip->clk_enabled = false;\n>>>   \t\t}\n>>>   \t\treturn 0;\n>>> -\t} else if (!pwm->state.enabled) {\n>>> +\t}\n>>> +\n>>> +\tif (pcchip->gpiod)\n>>> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_default);\n>>> +\n>>> +\tif (!pcchip->clk_enabled) {\n>>>   \t\tret = clk_enable(pcchip->clk);\n>>>   \t\tif (ret)\n>>>   \t\t\treturn ret;\n>>> @@ -97,6 +134,45 @@ static int pwm_clk_probe(struct platform_device *pdev)\n>>>   \t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcchip->clk),\n>>>   \t\t\t\t     \"Failed to get clock\\n\");\n>>>   +\tpcchip->pinctrl = devm_pinctrl_get(&pdev->dev);\n>>> +\tif (IS_ERR(pcchip->pinctrl)) {\n>>> +\t\tret = PTR_ERR(pcchip->pinctrl);\n>>> +\t\tpcchip->pinctrl = NULL;\n>>> +\t\tif (ret == -EPROBE_DEFER)\n>>> +\t\t\treturn ret;\n>>> +\t} else {\n>>> +\t\tpcchip->pins_default = pinctrl_lookup_state(pcchip->pinctrl,\n>>> +\t\t\t\t\t\t\t    PINCTRL_STATE_DEFAULT);\n>>> +\t\tpcchip->pins_gpio = pinctrl_lookup_state(pcchip->pinctrl,\n>>> +\t\t\t\t\t\t\t \"gpio\");\n>>> +\t\tif (IS_ERR(pcchip->pins_default) || IS_ERR(pcchip->pins_gpio))\n>>> +\t\t\tpcchip->pinctrl = NULL;\n>>> +\t}\n>>> +\n>>> +\t/*\n>>> +\t * Switch to GPIO pinctrl state before requesting the GPIO.\n>>> +\t * The driver core has already applied the \"default\" state, which\n>>> +\t * muxes the pin to the clock function and claims it.  We must\n>>> +\t * release that claim first so that gpiolib can request the pin.\n>>> +\t */\n>>> +\tif (pcchip->pinctrl)\n>>> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);\n>>> +\n>>> +\tpcchip->gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, GPIOD_ASIS);\n>>> +\tif (IS_ERR(pcchip->gpiod))\n>>> +\t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcchip->gpiod),\n>>> +\t\t\t\t     \"Failed to get gpio\\n\");\n>>> +\n>>> +\t/*\n>>> +\t * If pinctrl states were found but no GPIO was provided, the pin is\n>>> +\t * stuck in GPIO mode from the switch above.  Restore the default\n>>> +\t * (clock-function) mux and fall back to clock-only operation.\n>>> +\t */\n>> \n>> Feels slightly weird to silently allow \"broken\" DT, it would make no sense\n>> for it to have \"gpio\" pinctrl and not have a gpio defined, would it?\n>> \n>> Perhaps it makes more sense to put getting a gpio under having pins_gpio\n>> and make it strict, so two allowed states for the driver would be either\n>> no pinctrl-1 and no gpio, or having both at the same time?\n>> \n>> (maybe then also worth adding cross dependency of pinctrl-1 and gpio in\n>> the binding, it's one way only currently, not sure what's the correct\n>> way to describe it tho)\n>> \n>> Nikita\n>> \n> \n> Yeah, good point. Having a gpio pinctrl state without an actual gpio property is indeed a broken DT and there's no reason to silently work around it. Do you think the following change would work?\n> \n> \tif (pcchip->pinctrl) {\n> \t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);\n> \n> \t\tpcchip->gpiod = devm_gpiod_get(&pdev->dev, NULL, GPIOD_ASIS);\n> \t\tif (IS_ERR(pcchip->gpiod))\n> \t\t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcchip->gpiod),\n> \t\t\t\t\t     \"GPIO required when 'gpio' pinctrl state is present\\n\");\n> \t}\n> \n\nThis makes sense to me, yes.\n\nNikita\n\n>>> +\tif (pcchip->pinctrl && !pcchip->gpiod) {\n>>> +\t\tpinctrl_select_state(pcchip->pinctrl, pcchip->pins_default);\n>>> +\t\tpcchip->pinctrl = NULL;\n>>> +\t}\n>>> +\n>>>   \tchip->ops = &pwm_clk_ops;\n>>>     \tret = pwmchip_add(chip);\n>>","headers":{"Return-Path":"\n <linux-pwm+bounces-8524-incoming=patchwork.ozlabs.org@vger.kernel.org>","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pwm@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=trvn.ru header.i=@trvn.ru header.a=rsa-sha256\n header.s=mail header.b=gaVcCYgY;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c0a:e001:db::12fc:5321; helo=sea.lore.kernel.org;\n envelope-from=linux-pwm+bounces-8524-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=trvn.ru header.i=@trvn.ru\n header.b=\"gaVcCYgY\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=45.141.101.25","smtp.subspace.kernel.org;\n dmarc=pass (p=quarantine dis=none) header.from=trvn.ru","smtp.subspace.kernel.org;\n spf=pass smtp.mailfrom=trvn.ru"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org\n [IPv6:2600:3c0a:e001:db::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4frPkc0WCzz1xy1\n\tfor <incoming@patchwork.ozlabs.org>; Wed, 08 Apr 2026 23:56:56 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id E99003028834\n\tfor <incoming@patchwork.ozlabs.org>; Wed,  8 Apr 2026 13:53:41 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id B63753BC691;\n\tWed,  8 Apr 2026 13:53:40 +0000 (UTC)","from box.trvn.ru (box.trvn.ru [45.141.101.25])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id EB2EE2F3C3E;\n\tWed,  8 Apr 2026 13:53:38 +0000 (UTC)","from authenticated-user (box.trvn.ru [45.141.101.25])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest\n SHA256)\n\t(No client certificate requested)\n\tby box.trvn.ru (Postfix) with ESMTPSA id E8FF06E992;\n\tWed,  8 Apr 2026 18:53:35 +0500 (+05)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1775656420; cv=none;\n b=N63ezgLjDTg88e7e8vEM2cemfbgSbD3xyG+CfysGRlmepuzSEguQ1uDoZfoV7tYSyZ4H2WWs7R4PvLBJB1GDSIQwzzCZgbc/atTifxIhQlaI6OsBY1dMqyvUyYIPUTlWkg0SyZjXr/pw+13IIoP/Ds5XmE8rzk5bzTfTaS8yy6c=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1775656420; c=relaxed/simple;\n\tbh=s872tDRo2JQyY0cBLCjimL+w/HTgFFhiUZgRAxQGINo=;\n\th=MIME-Version:Date:From:To:Cc:Subject:In-Reply-To:References:\n\t Message-ID:Content-Type;\n b=cQYdzW5WJJ0r9XQo0vtvjO88iJzJ3Vi3+pWfJtTU4JlCsuUpPgdg856LlKQGPAqZqCSl05d5BZ40yml8naWMtVID5vZSPx0anNCH5fJ9nr5b9s7b3jEWeNUBfXdMlgXpWS5e1lMOFlZ3B01zNPBSMeLz+sIY0nyFdOivh/nmA6g=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dmarc=pass (p=quarantine dis=none) header.from=trvn.ru;\n spf=pass smtp.mailfrom=trvn.ru;\n dkim=pass (2048-bit key) header.d=trvn.ru header.i=@trvn.ru\n header.b=gaVcCYgY; arc=none smtp.client-ip=45.141.101.25","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=trvn.ru; s=mail;\n\tt=1775656416; bh=s872tDRo2JQyY0cBLCjimL+w/HTgFFhiUZgRAxQGINo=;\n\th=Date:From:To:Cc:Subject:In-Reply-To:References:From;\n\tb=gaVcCYgYTW4n0IDuBGz7Mj9hKld8hAnfvyHnogVzvN3G+l9CmjpGXU2U5DhUTV3EW\n\t eWtWBZw5O8rYVHrevpi6RZHMH3oFOl/5y/jkf0CMouqV+SjknuRTSCwGZIxe5MTlIC\n\t lFOHzKDMyPRjpOYC9Jm5qvgo7MNSThYXtW+EltfSL4MU2cmBuFETqFWuniCn101GU3\n\t okr0qsvTicwrQn61NL4pBq+IVP79mDhaQnEGtAOImqDf903bdlB4ow4K/FtbbJZI87\n\t vAW2MD4B7+pWu1gwPVt6EfMJD1n1TT5VMNxRwWzd4B9gdJ/jMhwNeRxqZUr/DnwTU8\n\t nu/+NR9iHUqSA==","Precedence":"bulk","X-Mailing-List":"linux-pwm@vger.kernel.org","List-Id":"<linux-pwm.vger.kernel.org>","List-Subscribe":"<mailto:linux-pwm+subscribe@vger.kernel.org>","List-Unsubscribe":"<mailto:linux-pwm+unsubscribe@vger.kernel.org>","MIME-Version":"1.0","Date":"Wed, 08 Apr 2026 18:53:35 +0500","From":"Nikita Travkin <nikita@trvn.ru>","To":"Xilin Wu <sophon@radxa.com>","Cc":"=?utf-8?q?Uwe_Kleine-K=C3=B6nig?= <ukleinek@kernel.org>,\n Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>,\n Conor Dooley <conor+dt@kernel.org>, linux-pwm@vger.kernel.org,\n devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,\n linux-arm-msm@vger.kernel.org","Subject":"Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for\n constant output levels","In-Reply-To":"<7BA26FC036D1C9AF+64204287-21b5-4664-ae75-be3dd54ec092@radxa.com>","References":"<20260408-clk-pwm-gpio-v2-0-d22f1f3498a0@radxa.com>\n <20260408-clk-pwm-gpio-v2-2-d22f1f3498a0@radxa.com>\n <8030cac3703f9aa1b7a8b476ad92aeae@trvn.ru>\n <7BA26FC036D1C9AF+64204287-21b5-4664-ae75-be3dd54ec092@radxa.com>","Message-ID":"<f7f1b27731c54e65f52d6fb8e347c878@trvn.ru>","X-Sender":"nikita@trvn.ru","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit"}}]