[PATCH v1 0/2] Enable WDT reload feature

[PATCH v1 0/2] Enable WDT reload feature

[Billy Tsai]

Aspeed PWM controller has the WDT reload feature, which changes the duty
cycle to a preprogrammed value after a WDT/EXTRST#.

Billy Tsai (2):
  hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4
  hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload

 .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 25 +++++++++-
 drivers/hwmon/aspeed-g6-pwm-tach.c            | 49 +++++++++++++++++++
 drivers/pwm/core.c                            |  6 +--
 include/linux/pwm.h                           | 10 ++++
 4 files changed, 86 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v1 1/2] hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4

[Billy Tsai]

Add an option to support #pwm-cells up to 4. The additional cell is used
to enable the WDT reset feature, which is specific to the ASPEED PWM
controller.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Change-Id: Iefcc9622ac3dc684441d3e77aeb53c00f2ce4097
---
 .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 25 ++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
index 9e5ed901ae54..0cc92ce29ece 100644
--- a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
@@ -31,7 +31,11 @@ properties:
     maxItems: 1
 
   "#pwm-cells":
-    const: 3
+    enum: [3, 4]
+    description: |
+      The value should be 4 to enable the WDT reload feature, which will change the duty cycle to
+      a preprogrammed value after WDT/EXTRST#.
+      The range for the fourth cell value supported by this binding is 0 to 255.
 
 patternProperties:
   "^fan-[0-9]+$":
@@ -69,3 +73,22 @@ examples:
         pwms = <&pwm_tach 1 40000 0>;
       };
     };
+  - |
+    #include <dt-bindings/clock/aspeed-clock.h>
+    pwm_tach: pwm-tach-controller@1e610000 {
+      compatible = "aspeed,ast2600-pwm-tach";
+      reg = <0x1e610000 0x100>;
+      clocks = <&syscon ASPEED_CLK_AHB>;
+      resets = <&syscon ASPEED_RESET_PWM>;
+      #pwm-cells = <4>;
+
+      fan-0 {
+        tach-ch = /bits/ 8 <0x0>;
+        pwms = <&pwm_tach 0 40000 0 128>;
+      };
+
+      fan-1 {
+        tach-ch = /bits/ 8 <0x1 0x2>;
+        pwms = <&pwm_tach 1 40000 0 160>;
+      };
+    };
-- 
2.25.1

Re: [PATCH v1 1/2] hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4

[Rob Herring (Arm)]

On Thu, 24 Oct 2024 15:15:47 +0800, Billy Tsai wrote:
> Add an option to support #pwm-cells up to 4. The additional cell is used
> to enable the WDT reset feature, which is specific to the ASPEED PWM
> controller.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Change-Id: Iefcc9622ac3dc684441d3e77aeb53c00f2ce4097
> ---
>  .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 25 ++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.example.dts:54.48-70.11: ERROR (duplicate_label): /example-1/pwm-tach-controller@1e610000: Duplicate label 'pwm_tach' on /example-1/pwm-tach-controller@1e610000 and /example-0/pwm-tach-controller@1e610000
ERROR: Input tree has errors, aborting (use -f to force output)
make[2]: *** [scripts/Makefile.dtbs:129: Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.example.dtb] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1442: dt_binding_check] Error 2
make: *** [Makefile:224: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241024071548.3370363-2-billy_tsai@aspeedtech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 1/2] hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4

[Rob Herring]

On Thu, Oct 24, 2024 at 03:15:47PM +0800, Billy Tsai wrote:
> Add an option to support #pwm-cells up to 4. The additional cell is used
> to enable the WDT reset feature, which is specific to the ASPEED PWM
> controller.

Use subject prefixes matching the subsystem.

> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Change-Id: Iefcc9622ac3dc684441d3e77aeb53c00f2ce4097

Drop.

> ---
>  .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 25 ++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> index 9e5ed901ae54..0cc92ce29ece 100644
> --- a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> @@ -31,7 +31,11 @@ properties:
>      maxItems: 1
>  
>    "#pwm-cells":
> -    const: 3
> +    enum: [3, 4]
> +    description: |
> +      The value should be 4 to enable the WDT reload feature, which will change the duty cycle to
> +      a preprogrammed value after WDT/EXTRST#.
> +      The range for the fourth cell value supported by this binding is 0 to 255.

Wrap lines at 80.

>  
>  patternProperties:
>    "^fan-[0-9]+$":
> @@ -69,3 +73,22 @@ examples:
>          pwms = <&pwm_tach 1 40000 0>;
>        };
>      };
> +  - |
> +    #include <dt-bindings/clock/aspeed-clock.h>
> +    pwm_tach: pwm-tach-controller@1e610000 {
> +      compatible = "aspeed,ast2600-pwm-tach";
> +      reg = <0x1e610000 0x100>;
> +      clocks = <&syscon ASPEED_CLK_AHB>;
> +      resets = <&syscon ASPEED_RESET_PWM>;
> +      #pwm-cells = <4>;
> +
> +      fan-0 {
> +        tach-ch = /bits/ 8 <0x0>;
> +        pwms = <&pwm_tach 0 40000 0 128>;
> +      };
> +
> +      fan-1 {
> +        tach-ch = /bits/ 8 <0x1 0x2>;
> +        pwms = <&pwm_tach 1 40000 0 160>;
> +      };
> +    };
> -- 
> 2.25.1
> 

[PATCH v1 2/2] hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload

[Billy Tsai]

Use the DTS property #pwm-cells to determine if the PWM controller needs
to enable the WDT reload feature, which changes the duty cycle to a
preprogrammed value after a WDT/EXTRST#. When #pwm-cells = <4>, the
feature will be enabled, and the PWM consumer can use the 4th argument to
set the reload duty cycle [0-255].

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Change-Id: Ided520f73220581e3b37819a106ec81ebf9bb5a6
---
 drivers/hwmon/aspeed-g6-pwm-tach.c | 49 ++++++++++++++++++++++++++++++
 drivers/pwm/core.c                 |  6 ++--
 include/linux/pwm.h                | 10 ++++++
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c
index 75eadda738ab..df47f9aa8ee6 100644
--- a/drivers/hwmon/aspeed-g6-pwm-tach.c
+++ b/drivers/hwmon/aspeed-g6-pwm-tach.c
@@ -56,6 +56,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
+#include <dt-bindings/pwm/pwm.h>
 #include <linux/reset.h>
 #include <linux/sysfs.h>
 
@@ -452,6 +453,51 @@ static void aspeed_pwm_tach_reset_assert(void *data)
 	reset_control_assert(rst);
 }
 
+static void aspeed_pwm_set_wdt_reload(struct pwm_chip *chip,
+				      struct pwm_device *pwm,
+				      u64 reload_duty_cycle)
+{
+	struct aspeed_pwm_tach_data *priv = aspeed_pwm_chip_to_data(chip);
+	u32 hwpwm = pwm->hwpwm, val;
+
+	val = readl(priv->base + PWM_ASPEED_DUTY_CYCLE(hwpwm));
+	val &= ~PWM_ASPEED_DUTY_CYCLE_POINT_AS_WDT;
+	val |= FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_POINT_AS_WDT,
+			  reload_duty_cycle);
+	writel(val, priv->base + PWM_ASPEED_DUTY_CYCLE(hwpwm));
+
+	val = readl(priv->base + PWM_ASPEED_CTRL(hwpwm));
+	val |= PWM_ASPEED_CTRL_DUTY_LOAD_AS_WDT_ENABLE;
+	writel(val, priv->base + PWM_ASPEED_CTRL(hwpwm));
+}
+
+static struct pwm_device *
+aspeed_pwm_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	/* flags in the fourth cell are optional */
+	if (args->args_count < 3)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args[0] >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm->args.period = args->args[1];
+	pwm->args.polarity = PWM_POLARITY_NORMAL;
+	if (args->args[2] & PWM_POLARITY_INVERTED)
+		pwm->args.polarity = PWM_POLARITY_INVERSED;
+
+	if (args->args_count > 3 && args->args[3] < U8_MAX)
+		aspeed_pwm_set_wdt_reload(chip, pwm, args->args[3]);
+
+	return pwm;
+}
+
 static int aspeed_pwm_tach_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev, *hwmon;
@@ -493,6 +539,9 @@ static int aspeed_pwm_tach_probe(struct platform_device *pdev)
 	pwmchip_set_drvdata(chip, priv);
 	chip->ops = &aspeed_pwm_ops;
 
+	if (IS_ENABLED(CONFIG_OF))
+		chip->of_xlate = aspeed_pwm_xlate;
+
 	ret = devm_pwmchip_add(dev, chip);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6e752e148b98..8251f7b361ab 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -422,9 +422,8 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
  * chip. A negative error code is returned if the index is not valid for the
  * specified PWM chip or if the PWM device cannot be requested.
  */
-static struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
-						unsigned int index,
-						const char *label)
+struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
+					 unsigned int index, const char *label)
 {
 	struct pwm_device *pwm;
 	int err;
@@ -442,6 +441,7 @@ static struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 
 	return pwm;
 }
+EXPORT_SYMBOL_GPL(pwm_request_from_chip);
 
 struct pwm_device *
 of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *args)
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 8acd60b53f58..95ae885f65c3 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -405,6 +405,8 @@ void pwmchip_remove(struct pwm_chip *chip);
 int __devm_pwmchip_add(struct device *dev, struct pwm_chip *chip, struct module *owner);
 #define devm_pwmchip_add(dev, chip) __devm_pwmchip_add(dev, chip, THIS_MODULE)
 
+struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
+					 unsigned int index, const char *label);
 struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *chip,
 		const struct of_phandle_args *args);
 struct pwm_device *of_pwm_single_xlate(struct pwm_chip *chip,
@@ -504,6 +506,14 @@ static inline void pwm_put(struct pwm_device *pwm)
 	might_sleep();
 }
 
+static inline struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
+						       unsigned int index,
+						       const char *label)
+{
+	might_sleep();
+	return ERR_PTR(-ENODEV);
+}
+
 static inline struct pwm_device *devm_pwm_get(struct device *dev,
 					      const char *consumer)
 {
-- 
2.25.1

Re: [PATCH v1 0/2] Enable WDT reload feature

[Uwe Kleine-König]

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

Hello,

On Thu, Oct 24, 2024 at 03:15:46PM +0800, Billy Tsai wrote:
> Aspeed PWM controller has the WDT reload feature, which changes the duty
> cycle to a preprogrammed value after a WDT/EXTRST#.
> 
> Billy Tsai (2):
>   hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4
>   hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload

Huh, I'm not convinced that extending #pwm-cells for that feature is a
good idea. Unless I'm missing something none of the other supported PWM
chips can do that, so I hesitate to change a standard for it. I suggest
to make this a separate property instead.

Best regards
Uwe

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

Re: [PATCH v1 0/2] Enable WDT reload feature

[Guenter Roeck]

On 10/24/24 08:40, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Oct 24, 2024 at 03:15:46PM +0800, Billy Tsai wrote:
>> Aspeed PWM controller has the WDT reload feature, which changes the duty
>> cycle to a preprogrammed value after a WDT/EXTRST#.
>>
>> Billy Tsai (2):
>>    hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4
>>    hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload
> 
> Huh, I'm not convinced that extending #pwm-cells for that feature is a
> good idea. Unless I'm missing something none of the other supported PWM
> chips can do that, so I hesitate to change a standard for it. I suggest
> to make this a separate property instead.
> 

Agreed.
Guenter

Re: [PATCH v1 0/2] Enable WDT reload feature

[Billy Tsai]

> On 10/24/24 08:40, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Thu, Oct 24, 2024 at 03:15:46PM +0800, Billy Tsai wrote:
> >> Aspeed PWM controller has the WDT reload feature, which changes the duty
> >> cycle to a preprogrammed value after a WDT/EXTRST#.
> >>
> >> Billy Tsai (2):
> >>    hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4
> >>    hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload
> >
> > Huh, I'm not convinced that extending #pwm-cells for that feature is a
> > good idea. Unless I'm missing something none of the other supported PWM
> > chips can do that, so I hesitate to change a standard for it. I suggest
> > to make this a separate property instead.
> >

> Agreed.
> Guenter

Hi Uwe and Guenter,

Using a separate property to enable this feature is a straightforward method, but I don’t understand why extending #pwm-cells isn’t a good idea in my situation. The feature ‘WDT reload’ can be set for individual PWM channels, and the PWM subsystem has the of_xlate callback hook, which allows each driver to define its arguments for the PWM consumer. I’m unsure if I misunderstood this callback usage, as I couldn’t find examples. If my understanding is correct, this method is better for adding our specific feature, rather than using child nodes or separate properties to indicate which PWM channel should enable this feature with the corresponding duty cycle values. I think using separate properties to achieve this feature would be quite cumbersome.
As I know the arguments for this usage are as follows:
First: PWM channel index
Second: PWM period in ns
Third: PWM polarity
Therefore, I extended our feature to a fourth argument to avoid any confusion regarding usage and added the description in our yaml file.

If my thinking is incorrect or doesn’t make sense, please let me know.

Thanks

Re: [PATCH v1 0/2] Enable WDT reload feature

[Uwe Kleine-König]

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

Hello Billy,

On Fri, Oct 25, 2024 at 02:00:39AM +0000, Billy Tsai wrote:
> > On 10/24/24 08:40, Uwe Kleine-König wrote:
> > > On Thu, Oct 24, 2024 at 03:15:46PM +0800, Billy Tsai wrote:
> > >> Aspeed PWM controller has the WDT reload feature, which changes the duty
> > >> cycle to a preprogrammed value after a WDT/EXTRST#.
> > >>
> > >> Billy Tsai (2):
> > >>    hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4
> > >>    hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload
> > >
> > > Huh, I'm not convinced that extending #pwm-cells for that feature is a
> > > good idea. Unless I'm missing something none of the other supported PWM
> > > chips can do that, so I hesitate to change a standard for it. I suggest
> > > to make this a separate property instead.
>
> Using a separate property to enable this feature is a straightforward
> method, but I don’t understand why extending #pwm-cells isn’t a good
> idea in my situation. The feature ‘WDT reload’ can be set for
> individual PWM channels, and the PWM subsystem has the of_xlate
> callback hook, which allows each driver to define its arguments for
> the PWM consumer. I’m unsure if I misunderstood this callback usage,
> as I couldn’t find examples. If my understanding is correct, this
> method is better for adding our specific feature, rather than using
> child nodes or separate properties to indicate which PWM channel
> should enable this feature with the corresponding duty cycle values. I
> think using separate properties to achieve this feature would be quite
> cumbersome.
> As I know the arguments for this usage are as follows:
> First: PWM channel index
> Second: PWM period in ns
> Third: PWM polarity
> Therefore, I extended our feature to a fourth argument to avoid any confusion regarding usage and added the description in our yaml file.
> 
> If my thinking is incorrect or doesn’t make sense, please let me know.

It might make sense if your bubble only contains that single PWM
hardware. However if you extend the pwm cells semantic for your PWM to
mean "period if the PWM watchdog triggers", i can hardly refuse the next
developer who wants to extend for "period of the secondary output pin of
the PWM" or a step counter or some property that defines how the duty
cycle is modulated over time. And should the next one also use the 4th
u32 for his purpose, or should we consider it reserved now for your
purpose such that the duty_cycle modulation goes into the 7th cell?

Today the bindings are (well nearly) used in the same way for all
hardwares and I want to keep it that way. If your PWM has a special
feature, give it a speaking name that the occasional dts reader has a
chance to understand without reading HW docs or dt bindings.

Best regards
Uwe

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

Re: [PATCH v1 0/2] Enable WDT reload feature

[Billy Tsai]

> > > On 10/24/24 08:40, Uwe Kleine-König wrote:
> > > > On Thu, Oct 24, 2024 at 03:15:46PM +0800, Billy Tsai wrote:
> > > >> Aspeed PWM controller has the WDT reload feature, which changes the duty
> > > >> cycle to a preprogrammed value after a WDT/EXTRST#.
> > > >>
> > > >> Billy Tsai (2):
> > > >>    hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4
> > > >>    hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload
> > > >
> > > > Huh, I'm not convinced that extending #pwm-cells for that feature is a
> > > > good idea. Unless I'm missing something none of the other supported PWM
> > > > chips can do that, so I hesitate to change a standard for it. I suggest
> > > > to make this a separate property instead.
> >
> > Using a separate property to enable this feature is a straightforward
> > method, but I don’t understand why extending #pwm-cells isn’t a good
> > idea in my situation. The feature ‘WDT reload’ can be set for
> > individual PWM channels, and the PWM subsystem has the of_xlate
> > callback hook, which allows each driver to define its arguments for
> > the PWM consumer. I’m unsure if I misunderstood this callback usage,
> > as I couldn’t find examples. If my understanding is correct, this
> > method is better for adding our specific feature, rather than using
> > child nodes or separate properties to indicate which PWM channel
> > should enable this feature with the corresponding duty cycle values. I
> > think using separate properties to achieve this feature would be quite
> > cumbersome.
> > As I know the arguments for this usage are as follows:
> > First: PWM channel index
> > Second: PWM period in ns
> > Third: PWM polarity
> > Therefore, I extended our feature to a fourth argument to avoid any confusion regarding usage and added the description in our yaml file.
> >
> > If my thinking is incorrect or doesn’t make sense, please let me know.

> It might make sense if your bubble only contains that single PWM
> hardware. However if you extend the pwm cells semantic for your PWM to
> mean "period if the PWM watchdog triggers", i can hardly refuse the next
> developer who wants to extend for "period of the secondary output pin of
> the PWM" or a step counter or some property that defines how the duty
> cycle is modulated over time. And should the next one also use the 4th
> u32 for his purpose, or should we consider it reserved now for your
> purpose such that the duty_cycle modulation goes into the 7th cell?

Hi Uwe,

In my view, the order of arguments—such as PWM number, PWM period in ns, and PWM polarity—is just
a convention for PWM consumers to follow. Even if another driver doesn’t adhere strictly to this
rule, it shouldn’t be considered an error if the YAML file documents the usage of each argument.
For example, some PWM controllers set #pwm-cells to 1, where the first argument isn’t necessarily
the PWM number. In google,cros-ec-pwm.yaml, it’s treated as the PWM index, while in marvell,pxa-pwm.yaml,
it represents the period in ns.

If users want to work with these PWM controllers, they should confirm the purpose of each argument from the
YAML file, rather than assuming the PWM driver follows a conventional argument order. If the YAML file doesn’t
specify details, it can be treated as described in pwm.yaml, which is fine. However, if there are any differences,
I think recording them in their own YAML file is sufficient (Like the example google,cros-ec-pwm.yaml and  marvell,pxa-pwm.yaml).

> Today the bindings are (well nearly) used in the same way for all
> hardwares and I want to keep it that way. If your PWM has a special
> feature, give it a speaking name that the occasional dts reader has a
> chance to understand without reading HW docs or dt bindings.

Using another DTS property to achieve this isn’t as elegant as utilizing PWM arguments, which will only
be applied when the PWM consumer uses it. This is another reason I want to extend the PWM cells semantics.

Thanks

Best regards
Billy Tsai

Re: [PATCH v1 0/2] Enable WDT reload feature

[Uwe Kleine-König]

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

Hello Billy,

On Fri, Oct 25, 2024 at 09:37:53AM +0000, Billy Tsai wrote:
> > > > On 10/24/24 08:40, Uwe Kleine-König wrote:
> > > > > On Thu, Oct 24, 2024 at 03:15:46PM +0800, Billy Tsai wrote:
> > > > >> Aspeed PWM controller has the WDT reload feature, which changes the duty
> > > > >> cycle to a preprogrammed value after a WDT/EXTRST#.
> > > > >>
> > > > >> Billy Tsai (2):
> > > > >>    hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4
> > > > >>    hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload
> > > > >
> > > > > Huh, I'm not convinced that extending #pwm-cells for that feature is a
> > > > > good idea. Unless I'm missing something none of the other supported PWM
> > > > > chips can do that, so I hesitate to change a standard for it. I suggest
> > > > > to make this a separate property instead.
> > >
> > > Using a separate property to enable this feature is a straightforward
> > > method, but I don’t understand why extending #pwm-cells isn’t a good
> > > idea in my situation. The feature ‘WDT reload’ can be set for
> > > individual PWM channels, and the PWM subsystem has the of_xlate
> > > callback hook, which allows each driver to define its arguments for
> > > the PWM consumer. I’m unsure if I misunderstood this callback usage,
> > > as I couldn’t find examples. If my understanding is correct, this
> > > method is better for adding our specific feature, rather than using
> > > child nodes or separate properties to indicate which PWM channel
> > > should enable this feature with the corresponding duty cycle values. I
> > > think using separate properties to achieve this feature would be quite
> > > cumbersome.
> > > As I know the arguments for this usage are as follows:
> > > First: PWM channel index
> > > Second: PWM period in ns
> > > Third: PWM polarity
> > > Therefore, I extended our feature to a fourth argument to avoid any confusion regarding usage and added the description in our yaml file.
> > >
> > > If my thinking is incorrect or doesn’t make sense, please let me know.
> 
> > It might make sense if your bubble only contains that single PWM
> > hardware. However if you extend the pwm cells semantic for your PWM to
> > mean "period if the PWM watchdog triggers", i can hardly refuse the next
> > developer who wants to extend for "period of the secondary output pin of
> > the PWM" or a step counter or some property that defines how the duty
> > cycle is modulated over time. And should the next one also use the 4th
> > u32 for his purpose, or should we consider it reserved now for your
> > purpose such that the duty_cycle modulation goes into the 7th cell?
> 
> In my view, the order of arguments—such as PWM number, PWM period in ns, and PWM polarity—is just
> a convention for PWM consumers to follow. Even if another driver doesn’t adhere strictly to this
> rule, it shouldn’t be considered an error if the YAML file documents the usage of each argument.

And it's a good idea to follow known conventions. There must be a good
reason to deviate because each deviation adds burden to the developers
making use of that device. And to patch authors, patch reviewers and
maintainers.

So I'd not say, extending pwm cells is an error. But it's an action
where the advantages don't outweight the disadvantages.

> For example, some PWM controllers set #pwm-cells to 1, where the first argument isn’t necessarily
> the PWM number. In google,cros-ec-pwm.yaml, it’s treated as the PWM index, while in marvell,pxa-pwm.yaml,
> it represents the period in ns.

Agreed. That is historic ballast that cannot be changed without breaking
dt compatibility. New drivers are supposed to use #pwm-cells = <3> with
the known semantics.

And if I would design the pwm references today, I'd just mandate
#pwm-cells = <1>; and don't specify a default period and flags in the
reference which I think don't really belong there.

> If users want to work with these PWM controllers, they should confirm the purpose of each argument from the
> YAML file, rather than assuming the PWM driver follows a conventional argument order.

Yes they should. However doing surprising stuff cannot be excused by
documenting it and then pointing to that documentation.

> If the YAML file doesn’t specify details, it can be treated as
> described in pwm.yaml, which is fine. However, if there are any
> differences, I think recording them in their own YAML file is
> sufficient (Like google,cros-ec-pwm.yaml and  marvell,pxa-pwm.yaml).
> 
> > Today the bindings are (well nearly) used in the same way for all
> > hardwares and I want to keep it that way. If your PWM has a special
> > feature, give it a speaking name that the occasional dts reader has a
> > chance to understand without reading HW docs or dt bindings.
> 
> Using another DTS property to achieve this isn’t as elegant as utilizing PWM arguments, which will only
> be applied when the PWM consumer uses it. This is another reason I want to extend the PWM cells semantic.

It seems elegance is subjective as I don't agree.

Best regards
Uwe

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