update synopsys driver for platform/devicetree support

update synopsys driver for platform/devicetree support

[Ben Dooks]

This is a series to allow the Synopsys PWM driver to be built for
device-tree/platform usage.

An open question is whether there should be some form of standard
property for the number of PWMs that each device supports.

Note, the patches 5 and 6 could be grouped together as patch
6 undoes some of the bits in 5 for configuration of clock rate.

[PATCH 1/7] pwm: change &pci->dev to dev in probe

[Ben Dooks]

The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
&pci->dev throughout the function. Change these all to the be
'dev' variable to make lines shorter.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 7568300bb11e..c706ef9a7ba1 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -202,14 +202,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	struct dwc_pwm *dwc;
 	int ret;
 
-	dwc = devm_kzalloc(&pci->dev, sizeof(*dwc), GFP_KERNEL);
+	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
 	if (!dwc)
 		return -ENOMEM;
 
 	ret = pcim_enable_device(pci);
 	if (ret) {
-		dev_err(&pci->dev,
-			"Failed to enable device (%pe)\n", ERR_PTR(ret));
+		dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
 		return ret;
 	}
 
@@ -217,14 +216,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 
 	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
 	if (ret) {
-		dev_err(&pci->dev,
-			"Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
+		dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
 		return ret;
 	}
 
 	dwc->base = pcim_iomap_table(pci)[0];
 	if (!dwc->base) {
-		dev_err(&pci->dev, "Base address missing\n");
+		dev_err(dev, "Base address missing\n");
 		return -ENOMEM;
 	}
 
-- 
2.35.1

Re: [PATCH 1/7] pwm: change &pci->dev to dev in probe

[Uwe Kleine-König]

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

On Tue, Jul 12, 2022 at 11:01:07AM +0100, Ben Dooks wrote:
> The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
> &pci->dev throughout the function. Change these all to the be
> 'dev' variable to make lines shorter.

Looks reasonable.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/7] pwm: change &pci->dev to dev in probe

[Ben Dooks]

On 13/07/2022 09:16, Uwe Kleine-König wrote:
> On Tue, Jul 12, 2022 at 11:01:07AM +0100, Ben Dooks wrote:
>> The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
>> &pci->dev throughout the function. Change these all to the be
>> 'dev' variable to make lines shorter.
> 
> Looks reasonable.
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for the reviews, I'll get a new series out tomorrow.

I might avoid the last patch about number of pwms for now
and just get the basic driver updates sorted.

-- 
Ben

Re: [PATCH 1/7] pwm: change &pci->dev to dev in probe

[Ben Dooks]

On 13/07/2022 09:16, Uwe Kleine-König wrote:
> On Tue, Jul 12, 2022 at 11:01:07AM +0100, Ben Dooks wrote:
>> The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
>> &pci->dev throughout the function. Change these all to the be
>> 'dev' variable to make lines shorter.
> 
> Looks reasonable.
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

ack for 1/7 or the series?
I'll repost v2 this week once we've dealt with polarity.

> 

Re: [PATCH 1/7] pwm: change &pci->dev to dev in probe

[Uwe Kleine-König]

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

On Mon, Jul 18, 2022 at 08:19:16AM +0100, Ben Dooks wrote:
> On 13/07/2022 09:16, Uwe Kleine-König wrote:
> > On Tue, Jul 12, 2022 at 11:01:07AM +0100, Ben Dooks wrote:
> > > The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
> > > &pci->dev throughout the function. Change these all to the be
> > > 'dev' variable to make lines shorter.
> > 
> > Looks reasonable.
> > 
> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> ack for 1/7 or the series?

The former. For the other patches I assume they will change in v2.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/7] pwm: change &pci->dev to dev in probe

[Uwe Kleine-König]

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

Hello,

On Mon, Jul 18, 2022 at 09:49:54AM +0200, Uwe Kleine-König wrote:
> On Mon, Jul 18, 2022 at 08:19:16AM +0100, Ben Dooks wrote:
> > On 13/07/2022 09:16, Uwe Kleine-König wrote:
> > > On Tue, Jul 12, 2022 at 11:01:07AM +0100, Ben Dooks wrote:
> > > > The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
> > > > &pci->dev throughout the function. Change these all to the be
> > > > 'dev' variable to make lines shorter.
> > > 
> > > Looks reasonable.
> > > 
> > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > ack for 1/7 or the series?
> 
> The former. For the other patches I assume they will change in v2.

Oh well, and 2/7 is so obviously a preparation patch that I'll wait to
see v2 for that one, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[PATCH 2/7] pwm: move dwc memory alloc to own function

[Ben Dooks]

In preparation for adding other bus support, move the allocation
of the pwm struct out of the main driver code.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index c706ef9a7ba1..61f11e0a9319 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -196,13 +196,29 @@ static const struct pwm_ops dwc_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
+{
+	struct dwc_pwm *dwc;
+
+	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+	if (!dwc)
+		return NULL;
+
+	dwc->chip.dev = dev;
+	dwc->chip.ops = &dwc_pwm_ops;
+	dwc->chip.npwm = DWC_TIMERS_TOTAL;
+
+	dev_set_drvdata(dev, dwc);
+	return dwc;
+}
+
 static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
 	struct device *dev = &pci->dev;
 	struct dwc_pwm *dwc;
 	int ret;
 
-	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+	dwc = dwc_pwm_alloc(dev);
 	if (!dwc)
 		return -ENOMEM;
 
@@ -226,12 +242,6 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return -ENOMEM;
 	}
 
-	pci_set_drvdata(pci, dwc);
-
-	dwc->chip.dev = dev;
-	dwc->chip.ops = &dwc_pwm_ops;
-	dwc->chip.npwm = DWC_TIMERS_TOTAL;
-
 	ret = pwmchip_add(&dwc->chip);
 	if (ret)
 		return ret;
-- 
2.35.1

[PATCH 3/7] pwm: dwc: add of/platform support

[Ben Dooks]

The dwc pwm controller can be used in non-PCI systems, so allow
either platform or OF based probing.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++
 drivers/pwm/Kconfig                           |  5 +-
 drivers/pwm/pwm-dwc.c                         | 53 +++++++++++++++++++
 3 files changed, 96 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-synposys.yaml

diff --git a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
new file mode 100644
index 000000000000..38ac0da75272
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 SiFive, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-synposys.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys PWM controller
+
+maintainers:
+   - Ben Dooks <ben.dooks@sifive.com>
+
+properties:
+  "#pwm-cells":
+    description: |
+      See pwm.yaml in this directory for a description of the cells format.
+
+  clocks:
+    items:
+      - description: Interface bus clock
+      - description: PWM reference clock
+
+  clock-names:
+    items:
+      - const: bus
+      - const: timer
+
+  compatible:
+    oneOf:
+      - items:
+        - const: snps,pwm
+
+required:
+  - "#pwm-cells"
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 904de8d61828..e1aa645c1084 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -166,9 +166,10 @@ config PWM_CROS_EC
 
 config PWM_DWC
 	tristate "DesignWare PWM Controller"
-	depends on PCI
+	depends on PCI || OF
 	help
-	  PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
+	  PWM driver for Synopsys DWC PWM Controller attached to either a
+	  PCI or platform bus.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-dwc.
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 61f11e0a9319..235cb730c888 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
 
@@ -319,6 +320,58 @@ static struct pci_driver dwc_pwm_driver = {
 
 module_pci_driver(dwc_pwm_driver);
 
+#ifdef CONFIG_OF
+static int dwc_pwm_plat_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dwc_pwm *dwc;
+	int ret;
+
+	dwc = dwc_pwm_alloc(dev);
+	if (!dwc)
+		return -ENOMEM;
+
+	dwc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dwc->base))
+		return dev_err_probe(dev, PTR_ERR(dwc->base),
+				     "failed to map IO\n");
+
+	ret = pwmchip_add(&dwc->chip);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int dwc_pwm_plat_remove(struct platform_device *pdev)
+{
+	struct dwc_pwm *dwc = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&dwc->chip);
+	return 0;
+}
+
+static const struct of_device_id dwc_pwm_dt_ids[] = {
+	{ .compatible = "snps,pwm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
+
+static struct platform_driver dwc_pwm_plat_driver = {
+	.driver = {
+		.name		= "dwc-pwm",
+		.of_match_table  = dwc_pwm_dt_ids,
+	},
+	.probe	= dwc_pwm_plat_probe,
+	.remove	= dwc_pwm_plat_remove,
+};
+
+module_platform_driver(dwc_pwm_plat_driver);
+
+MODULE_ALIAS("platform:dwc-pwm");
+#endif /* CONFIG_OF */
+
+
 MODULE_AUTHOR("Felipe Balbi (Intel)");
 MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@linux.intel.com>");
 MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
-- 
2.35.1

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Krzysztof Kozlowski]

On 12/07/2022 12:01, Ben Dooks wrote:
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++

Bindings must be a separate patch. Preferably first in the series. Use
proper subject prefix matching the subsystem.

Best regards,
Krzysztof

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Ben Dooks]

On 12/07/2022 11:08, Krzysztof Kozlowski wrote:
> On 12/07/2022 12:01, Ben Dooks wrote:
>> The dwc pwm controller can be used in non-PCI systems, so allow
>> either platform or OF based probing.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>>   .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++
> 
> Bindings must be a separate patch. Preferably first in the series. Use
> proper subject prefix matching the subsystem.
> 
> Best regards,
> Krzysztof

Thanks, seems counter-intuitive that one change is split up like that.

-- 
Ben

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Krzysztof Kozlowski]

On 13/07/2022 11:21, Ben Dooks wrote:
> On 12/07/2022 11:08, Krzysztof Kozlowski wrote:
>> On 12/07/2022 12:01, Ben Dooks wrote:
>>> The dwc pwm controller can be used in non-PCI systems, so allow
>>> either platform or OF based probing.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>> ---
>>>   .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++
>>
>> Bindings must be a separate patch. Preferably first in the series. Use
>> proper subject prefix matching the subsystem.
>>
>> Best regards,
>> Krzysztof
> 
> Thanks, seems counter-intuitive that one change is split up like that.

Why? Bindings are not really related to driver implementation, they are
shared with other projects. Binding reviewers usually care less about
implementation, so combining it with drivers forces me to read carefully
each one of 300 emails I receive...

Anyway, that's the policy since years, so nothing new here.

Best regards,
Krzysztof

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Rob Herring]

On Tue, 12 Jul 2022 11:01:09 +0100, Ben Dooks wrote:
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++
>  drivers/pwm/Kconfig                           |  5 +-
>  drivers/pwm/pwm-dwc.c                         | 53 +++++++++++++++++++
>  3 files changed, 96 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/pwm/pwm-synposys.yaml:11:4: [warning] wrong indentation: expected 2 but found 3 (indentation)
./Documentation/devicetree/bindings/pwm/pwm-synposys.yaml:31:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Ben Dooks]

On 12/07/2022 15:26, Rob Herring wrote:
> On Tue, 12 Jul 2022 11:01:09 +0100, Ben Dooks wrote:
>> The dwc pwm controller can be used in non-PCI systems, so allow
>> either platform or OF based probing.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>>   .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++
>>   drivers/pwm/Kconfig                           |  5 +-
>>   drivers/pwm/pwm-dwc.c                         | 53 +++++++++++++++++++
>>   3 files changed, 96 insertions(+), 2 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):

Is the following equivalent:

> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pwm/snps,pwm.yaml 


> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/pwm/pwm-synposys.yaml:11:4: [warning] wrong indentation: expected 2 but found 3 (indentation)
> ./Documentation/devicetree/bindings/pwm/pwm-synposys.yaml:31:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
> 
> dtschema/dtc warnings/errors:
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/patch/
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> 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:

Whoops, turns out I hadn't installed yamllint.

> pip3 install dtschema --upgrade
> 
> Please check and re-submit.
> 

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Rob Herring]

On Tue, Jul 12, 2022 at 11:01:09AM +0100, Ben Dooks wrote:
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++

Use compatible string for filename.

>  drivers/pwm/Kconfig                           |  5 +-
>  drivers/pwm/pwm-dwc.c                         | 53 +++++++++++++++++++
>  3 files changed, 96 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> new file mode 100644
> index 000000000000..38ac0da75272
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2022 SiFive, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/pwm-synposys.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys PWM controller
> +
> +maintainers:
> +   - Ben Dooks <ben.dooks@sifive.com>
> +
> +properties:
> +  "#pwm-cells":
> +    description: |
> +      See pwm.yaml in this directory for a description of the cells format.

pwm.yaml doesn't define how many cells. You need to. And you don't need 
generic descriptions.

> +
> +  clocks:
> +    items:
> +      - description: Interface bus clock
> +      - description: PWM reference clock
> +
> +  clock-names:
> +    items:
> +      - const: bus
> +      - const: timer
> +
> +  compatible:

Convention is compatible comes first in the list.

> +    oneOf:
> +      - items:
> +        - const: snps,pwm

Don't need oneOf or items. Just 'const: snps,pwm'.

That's pretty generic for a compatible. There's only 1 version of the IP 
or is the version discoverable?

> +
> +required:
> +  - "#pwm-cells"
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Ben Dooks]

On 12/07/2022 23:17, Rob Herring wrote:
> On Tue, Jul 12, 2022 at 11:01:09AM +0100, Ben Dooks wrote:
>> The dwc pwm controller can be used in non-PCI systems, so allow
>> either platform or OF based probing.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>>   .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++
> 
> Use compatible string for filename.

ok, will fix.

>>   drivers/pwm/Kconfig                           |  5 +-
>>   drivers/pwm/pwm-dwc.c                         | 53 +++++++++++++++++++
>>   3 files changed, 96 insertions(+), 2 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
>> new file mode 100644
>> index 000000000000..38ac0da75272
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
>> @@ -0,0 +1,40 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2022 SiFive, Inc.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/pwm-synposys.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Synopsys PWM controller
>> +
>> +maintainers:
>> +   - Ben Dooks <ben.dooks@sifive.com>
>> +
>> +properties:
>> +  "#pwm-cells":
>> +    description: |
>> +      See pwm.yaml in this directory for a description of the cells format.
> 
> pwm.yaml doesn't define how many cells. You need to. And you don't need
> generic descriptions.

  "#pwm-cells":
     const: 1

should be sufficient then?

>> +
>> +  clocks:
>> +    items:
>> +      - description: Interface bus clock
>> +      - description: PWM reference clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: bus
>> +      - const: timer
>> +
>> +  compatible:
> 
> Convention is compatible comes first in the list.

ok, fixed.


>> +    oneOf:
>> +      - items:
>> +        - const: snps,pwm
> 
> Don't need oneOf or items. Just 'const: snps,pwm'.

ok, fixed.


> That's pretty generic for a compatible. There's only 1 version of the IP
> or is the version discoverable?

This IP block doesn't seem to have evolved much since being included so
I don't think so at this point, most of the revision list is for the
internal options and cleanup.

I will go through and check if all the compile-time properties are deal
with, but that'll be adding more properiteis if they are needed.

>> +
>> +required:
>> +  - "#pwm-cells"
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Uwe Kleine-König]

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

On Wed, Jul 13, 2022 at 12:56:55PM +0100, Ben Dooks wrote:
> On 12/07/2022 23:17, Rob Herring wrote:
> > On Tue, Jul 12, 2022 at 11:01:09AM +0100, Ben Dooks wrote:
> > > The dwc pwm controller can be used in non-PCI systems, so allow
> > > either platform or OF based probing.
> > > 
> > > Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> > > ---
> > >   .../devicetree/bindings/pwm/pwm-synposys.yaml | 40 ++++++++++++++
> > 
> > Use compatible string for filename.
> 
> ok, will fix.
> 
> > >   drivers/pwm/Kconfig                           |  5 +-
> > >   drivers/pwm/pwm-dwc.c                         | 53 +++++++++++++++++++
> > >   3 files changed, 96 insertions(+), 2 deletions(-)
> > >   create mode 100644 Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> > > new file mode 100644
> > > index 000000000000..38ac0da75272
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> > > @@ -0,0 +1,40 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +# Copyright (C) 2022 SiFive, Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pwm/pwm-synposys.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Synopsys PWM controller
> > > +
> > > +maintainers:
> > > +   - Ben Dooks <ben.dooks@sifive.com>
> > > +
> > > +properties:
> > > +  "#pwm-cells":
> > > +    description: |
> > > +      See pwm.yaml in this directory for a description of the cells format.
> > 
> > pwm.yaml doesn't define how many cells. You need to. And you don't need
> > generic descriptions.
> 
>  "#pwm-cells":
>     const: 1
> 
> should be sufficient then?

I would expect a value of (at least) 2 or (better) 3.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Ben Dooks]

On 13/07/2022 14:52, Uwe Kleine-König wrote:
> On Wed, Jul 13, 2022 at 12:56:55PM +0100, Ben Dooks wrote:
>> On 12/07/2022 23:17, Rob Herring wrote:
>>> On Tue, Jul 12, 2022 at 11:01:09AM +0100, Ben Dooks wrote:
>>>> The dwc pwm controller can be used in non-PCI systems, so allow
>>>> either platform or OF based probing.
>>>>
>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>

[snip]

>>>> +properties:
>>>> +  "#pwm-cells":
>>>> +    description: |
>>>> +      See pwm.yaml in this directory for a description of the cells format.
>>>
>>> pwm.yaml doesn't define how many cells. You need to. And you don't need
>>> generic descriptions.
>>
>>   "#pwm-cells":
>>      const: 1
>>
>> should be sufficient then?
> 
> I would expect a value of (at least) 2 or (better) 3.

OOPS, forgot the phandle.

I will have to check if we have any support yet for dealing
with any of the pwm flags yet.

> Best regards
> Uwe
> 

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Uwe Kleine-König]

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

On Wed, Jul 13, 2022 at 03:30:07PM +0100, Ben Dooks wrote:
> On 13/07/2022 14:52, Uwe Kleine-König wrote:
> > On Wed, Jul 13, 2022 at 12:56:55PM +0100, Ben Dooks wrote:
> > > On 12/07/2022 23:17, Rob Herring wrote:
> > > > On Tue, Jul 12, 2022 at 11:01:09AM +0100, Ben Dooks wrote:
> > > > > The dwc pwm controller can be used in non-PCI systems, so allow
> > > > > either platform or OF based probing.
> > > > > 
> > > > > Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> 
> [snip]
> 
> > > > > +properties:
> > > > > +  "#pwm-cells":
> > > > > +    description: |
> > > > > +      See pwm.yaml in this directory for a description of the cells format.
> > > > 
> > > > pwm.yaml doesn't define how many cells. You need to. And you don't need
> > > > generic descriptions.
> > > 
> > >   "#pwm-cells":
> > >      const: 1
> > > 
> > > should be sufficient then?
> > 
> > I would expect a value of (at least) 2 or (better) 3.
> 
> OOPS, forgot the phandle.
> 
> I will have to check if we have any support yet for dealing
> with any of the pwm flags yet.

I didn't double check, but given that the driver only supports inversed
polarity it might not even work without passing the flag for inversed
polarity. Having said that, I expect you have to only add "#pwm-cells =
<3>;" to your dts and then everything should work just fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Ben Dooks]

On 13/07/2022 16:07, Uwe Kleine-König wrote:
> On Wed, Jul 13, 2022 at 03:30:07PM +0100, Ben Dooks wrote:
>> On 13/07/2022 14:52, Uwe Kleine-König wrote:
>>> On Wed, Jul 13, 2022 at 12:56:55PM +0100, Ben Dooks wrote:
>>>> On 12/07/2022 23:17, Rob Herring wrote:
>>>>> On Tue, Jul 12, 2022 at 11:01:09AM +0100, Ben Dooks wrote:
>>>>>> The dwc pwm controller can be used in non-PCI systems, so allow
>>>>>> either platform or OF based probing.
>>>>>>
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>
>> [snip]
>>
>>>>>> +properties:
>>>>>> +  "#pwm-cells":
>>>>>> +    description: |
>>>>>> +      See pwm.yaml in this directory for a description of the cells format.
>>>>>
>>>>> pwm.yaml doesn't define how many cells. You need to. And you don't need
>>>>> generic descriptions.
>>>>
>>>>    "#pwm-cells":
>>>>       const: 1
>>>>
>>>> should be sufficient then?
>>>
>>> I would expect a value of (at least) 2 or (better) 3.
>>
>> OOPS, forgot the phandle.
>>
>> I will have to check if we have any support yet for dealing
>> with any of the pwm flags yet.
> 
> I didn't double check, but given that the driver only supports inversed
> polarity it might not even work without passing the flag for inversed
> polarity. Having said that, I expect you have to only add "#pwm-cells =
> <3>;" to your dts and then everything should work just fine.

I hadn't noticed, but we've been testing with the sysfs interface
as we're using qemu.

Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Ben Dooks]

On 13/07/2022 16:07, Uwe Kleine-König wrote:
> On Wed, Jul 13, 2022 at 03:30:07PM +0100, Ben Dooks wrote:
>> On 13/07/2022 14:52, Uwe Kleine-König wrote:
>>> On Wed, Jul 13, 2022 at 12:56:55PM +0100, Ben Dooks wrote:
>>>> On 12/07/2022 23:17, Rob Herring wrote:
>>>>> On Tue, Jul 12, 2022 at 11:01:09AM +0100, Ben Dooks wrote:
>>>>>> The dwc pwm controller can be used in non-PCI systems, so allow
>>>>>> either platform or OF based probing.
>>>>>>
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>
>> [snip]
>>
>>>>>> +properties:
>>>>>> +  "#pwm-cells":
>>>>>> +    description: |
>>>>>> +      See pwm.yaml in this directory for a description of the cells format.
>>>>>
>>>>> pwm.yaml doesn't define how many cells. You need to. And you don't need
>>>>> generic descriptions.
>>>>
>>>>    "#pwm-cells":
>>>>       const: 1
>>>>
>>>> should be sufficient then?
>>>
>>> I would expect a value of (at least) 2 or (better) 3.
>>
>> OOPS, forgot the phandle.
>>
>> I will have to check if we have any support yet for dealing
>> with any of the pwm flags yet.
> 
> I didn't double check, but given that the driver only supports inversed
> polarity it might not even work without passing the flag for inversed
> polarity. Having said that, I expect you have to only add "#pwm-cells =
> <3>;" to your dts and then everything should work just fine.

I've gone back over the documentation we have for the block, and it
should have a count for high and a count for low in the PWM mode the
driver puts it into. I have no idea /why/ the driver is reporting it
as inversed, unless the PCI version has this automatically set....

I will go back and talk with the engineer who did the testing of the
PWM to get the test-bench re-set and check this, however my expectation
is we could easily do both and for the of/plat case we should just
report normal polarity (and we could deal with the inversed by simply
swapping the low and high values).

I also noted the v2 block supports 0 and 100% by setting a bit in the
control and the timers to a given value, so that can also be added to
the series (although this requires an IP generation option to be
set) which we can also add.

Thnak you for pointing this out, hopefully we can have this sorted
today and if so we will need to change this to a range of 2..3 for
the PWM cells.


> Best regards
> Uwe
> 

[PATCH 4/7] pwm: dwc: allow driver to be built with COMPILE_TEST

[Ben Dooks]

Allow dwc driver to be built with COMPILE_TEST should allow
better coverage when build testing.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e1aa645c1084..e654655c4b3f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -166,7 +166,7 @@ config PWM_CROS_EC
 
 config PWM_DWC
 	tristate "DesignWare PWM Controller"
-	depends on PCI || OF
+	depends on PCI || OF || COMPILE_TEST
 	help
 	  PWM driver for Synopsys DWC PWM Controller attached to either a
 	  PCI or platform bus.
-- 
2.35.1

[PATCH 5/7] pwm: dwc: add timer clock

[Ben Dooks]

Add a configurable clock base rate for the pwm as when
being built for non-PCI the block may be sourced from
an internal clock.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 drivers/pwm/pwm-dwc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 235cb730c888..aa0486b89bdd 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
@@ -35,7 +36,12 @@
 #define DWC_TIMERS_COMP_VERSION	0xac
 
 #define DWC_TIMERS_TOTAL	8
+
+#ifndef CONFIG_OF
 #define DWC_CLK_PERIOD_NS	10
+#else
+#define DWC_CLK_PERIOD_NS	dwc->clk_ns
+#endif
 
 /* Timer Control Register */
 #define DWC_TIM_CTRL_EN		BIT(0)
@@ -54,6 +60,8 @@ struct dwc_pwm_ctx {
 struct dwc_pwm {
 	struct pwm_chip chip;
 	void __iomem *base;
+	struct clk *clk;
+	unsigned int clk_ns;
 	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
 };
 #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
@@ -336,6 +344,14 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(dwc->base),
 				     "failed to map IO\n");
 
+	dwc->clk = devm_clk_get(dev, "timer");
+	if (IS_ERR(dwc->clk))
+		return dev_err_probe(dev, PTR_ERR(dwc->clk),
+				     "failed to get timer clock\n");
+
+	clk_prepare_enable(dwc->clk);
+	dwc->clk_ns = 1000000000 /clk_get_rate(dwc->clk);
+
 	ret = pwmchip_add(&dwc->chip);
 	if (ret)
 		return ret;
@@ -347,6 +363,7 @@ static int dwc_pwm_plat_remove(struct platform_device *pdev)
 {
 	struct dwc_pwm *dwc = platform_get_drvdata(pdev);
 
+	clk_disable_unprepare(dwc->clk);
 	pwmchip_remove(&dwc->chip);
 	return 0;
 }
-- 
2.35.1

Re: [PATCH 5/7] pwm: dwc: add timer clock

[Uwe Kleine-König]

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

On Tue, Jul 12, 2022 at 11:01:11AM +0100, Ben Dooks wrote:
> Add a configurable clock base rate for the pwm as when
> being built for non-PCI the block may be sourced from
> an internal clock.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  drivers/pwm/pwm-dwc.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index 235cb730c888..aa0486b89bdd 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -18,6 +18,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/clk.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
> @@ -35,7 +36,12 @@
>  #define DWC_TIMERS_COMP_VERSION	0xac
>  
>  #define DWC_TIMERS_TOTAL	8
> +
> +#ifndef CONFIG_OF
>  #define DWC_CLK_PERIOD_NS	10
> +#else
> +#define DWC_CLK_PERIOD_NS	dwc->clk_ns
> +#endif

Hmm, that looks wrong. If you have CONFIG_OF but use the pci device ...

IMHO it would help readability if you used ifdef. When there is an #else
branch anyhow, there is no reason to use the negative variant.

>  /* Timer Control Register */
>  #define DWC_TIM_CTRL_EN		BIT(0)
> @@ -54,6 +60,8 @@ struct dwc_pwm_ctx {
>  struct dwc_pwm {
>  	struct pwm_chip chip;
>  	void __iomem *base;
> +	struct clk *clk;
> +	unsigned int clk_ns;
>  	struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
>  };
>  #define to_dwc_pwm(p)	(container_of((p), struct dwc_pwm, chip))
> @@ -336,6 +344,14 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(dwc->base),
>  				     "failed to map IO\n");
>  
> +	dwc->clk = devm_clk_get(dev, "timer");
> +	if (IS_ERR(dwc->clk))
> +		return dev_err_probe(dev, PTR_ERR(dwc->clk),
> +				     "failed to get timer clock\n");
> +
> +	clk_prepare_enable(dwc->clk);
> +	dwc->clk_ns = 1000000000 /clk_get_rate(dwc->clk);
> +
>  	ret = pwmchip_add(&dwc->chip);
>  	if (ret)
>  		return ret;

Here you're missing clk_disable_unprepare(). (Alternatively use
devm_clk_get_enabled().)

> @@ -347,6 +363,7 @@ static int dwc_pwm_plat_remove(struct platform_device *pdev)
>  {
>  	struct dwc_pwm *dwc = platform_get_drvdata(pdev);
>  
> +	clk_disable_unprepare(dwc->clk);
>  	pwmchip_remove(&dwc->chip);

The order is wrong here. You must only stop the clk when the pwmchip is
removed. Until that the PWM is supposed to stay functional.

>  	return 0;
>  }

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[PATCH 6/7] pwm: dwc: remove the CONFIG_OF in timer clock

[Ben Dooks]

We should probably change from the #ifdef added earlier in
49a0f4692a8752c7b03cb26d54282bee5c8c71bb ("wm: dwc: add timer clock")
and just have it always in the dwc data so if we have a system with
both PCI and OF probing it should work

-- consider merging with original patch
---
 drivers/pwm/pwm-dwc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index aa0486b89bdd..6a4364a5d137 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -37,12 +37,6 @@
 
 #define DWC_TIMERS_TOTAL	8
 
-#ifndef CONFIG_OF
-#define DWC_CLK_PERIOD_NS	10
-#else
-#define DWC_CLK_PERIOD_NS	dwc->clk_ns
-#endif
-
 /* Timer Control Register */
 #define DWC_TIM_CTRL_EN		BIT(0)
 #define DWC_TIM_CTRL_MODE	BIT(1)
@@ -104,13 +98,13 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
 	 * periods and check are the result within HW limits between 1 and
 	 * 2^32 periods.
 	 */
-	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_NS);
+	tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	low = tmp - 1;
 
 	tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
-				    DWC_CLK_PERIOD_NS);
+				    dwc->clk_ns);
 	if (tmp < 1 || tmp > (1ULL << 32))
 		return -ERANGE;
 	high = tmp - 1;
@@ -185,12 +179,12 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
 	duty += 1;
-	duty *= DWC_CLK_PERIOD_NS;
+	duty *= dwc->clk_ns;
 	state->duty_cycle = duty;
 
 	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
 	period += 1;
-	period *= DWC_CLK_PERIOD_NS;
+	period *= dwc->clk_ns;
 	period += duty;
 	state->period = period;
 
@@ -213,6 +207,7 @@ static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
 	if (!dwc)
 		return NULL;
 
+	dwc->clk_ns = 10;
 	dwc->chip.dev = dev;
 	dwc->chip.ops = &dwc_pwm_ops;
 	dwc->chip.npwm = DWC_TIMERS_TOTAL;
-- 
2.35.1

Re: [PATCH 6/7] pwm: dwc: remove the CONFIG_OF in timer clock

[Krzysztof Kozlowski]

On 12/07/2022 12:01, Ben Dooks wrote:
> We should probably change from the #ifdef added earlier in
> 49a0f4692a8752c7b03cb26d54282bee5c8c71bb ("wm: dwc: add timer clock")
> and just have it always in the dwc data so if we have a system with
> both PCI and OF probing it should work
> 
> -- consider merging with original patch

Missing SoB. Please run checkpatch.

Best regards,
Krzysztof

Re: [PATCH 6/7] pwm: dwc: remove the CONFIG_OF in timer clock

[Ben Dooks]

On 12/07/2022 11:09, Krzysztof Kozlowski wrote:
> On 12/07/2022 12:01, Ben Dooks wrote:
>> We should probably change from the #ifdef added earlier in
>> 49a0f4692a8752c7b03cb26d54282bee5c8c71bb ("wm: dwc: add timer clock")
>> and just have it always in the dwc data so if we have a system with
>> both PCI and OF probing it should work
>>
>> -- consider merging with original patch
> 
> Missing SoB. Please run checkpatch.

This was meant to be an RFC about whether it should be a single patch
or merged back into the previous one.

Re: [PATCH 6/7] pwm: dwc: remove the CONFIG_OF in timer clock

[Uwe Kleine-König]

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

On Tue, Jul 12, 2022 at 11:20:23AM +0100, Ben Dooks wrote:
> On 12/07/2022 11:09, Krzysztof Kozlowski wrote:
> > On 12/07/2022 12:01, Ben Dooks wrote:
> > > We should probably change from the #ifdef added earlier in
> > > 49a0f4692a8752c7b03cb26d54282bee5c8c71bb ("wm: dwc: add timer clock")
> > > and just have it always in the dwc data so if we have a system with
> > > both PCI and OF probing it should work
> > > 
> > > -- consider merging with original patch
> > 
> > Missing SoB. Please run checkpatch.
> 
> This was meant to be an RFC about whether it should be a single patch
> or merged back into the previous one.

+1 for merging these

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 6/7] pwm: dwc: remove the CONFIG_OF in timer clock

[Ben Dooks]

On 13/07/2022 07:11, Uwe Kleine-König wrote:
> On Tue, Jul 12, 2022 at 11:20:23AM +0100, Ben Dooks wrote:
>> On 12/07/2022 11:09, Krzysztof Kozlowski wrote:
>>> On 12/07/2022 12:01, Ben Dooks wrote:
>>>> We should probably change from the #ifdef added earlier in
>>>> 49a0f4692a8752c7b03cb26d54282bee5c8c71bb ("wm: dwc: add timer clock")
>>>> and just have it always in the dwc data so if we have a system with
>>>> both PCI and OF probing it should work
>>>>
>>>> -- consider merging with original patch
>>>
>>> Missing SoB. Please run checkpatch.
>>
>> This was meant to be an RFC about whether it should be a single patch
>> or merged back into the previous one.
> 
> +1 for merging these

Yes, done for v2.

Re: [PATCH 6/7] pwm: dwc: remove the CONFIG_OF in timer clock

[Ben Dooks]

On 12/07/2022 11:09, Krzysztof Kozlowski wrote:
> On 12/07/2022 12:01, Ben Dooks wrote:
>> We should probably change from the #ifdef added earlier in
>> 49a0f4692a8752c7b03cb26d54282bee5c8c71bb ("wm: dwc: add timer clock")
>> and just have it always in the dwc data so if we have a system with
>> both PCI and OF probing it should work
>>
>> -- consider merging with original patch
> 
> Missing SoB. Please run checkpatch.
> 
> Best regards,
> Krzysztof

Will be merging this with the previous patch now, thanks.

[PATCH 7/7] pwm: dwc: add snps,pwm-number to limit pwm count

[Ben Dooks]

Add snps,pwm-number property to indicate if the block does not have
all 8 of the PWM blocks.

Not sure if this should be a general PWM property consider optional
for all PWM types, so have added a specific one here (there is only
one other controller with a property for PWM count at the moment)

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 Documentation/devicetree/bindings/pwm/pwm-synposys.yaml | 5 +++++
 drivers/pwm/pwm-dwc.c                                   | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
index 38ac0da75272..15bdf764b46a 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
@@ -30,11 +30,16 @@ properties:
       - items:
         - const: snps,pwm
 
+  snps,pwm-number:
+    $ref: '/schemas/types.yaml#/definitions/uint32'
+    description: u32 value representing the number of PWM devices
+
 required:
   - "#pwm-cells"
   - compatible
   - reg
   - clocks
   - clock-names
+  - snps,pwm-number
 
 additionalProperties: false
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 6a4364a5d137..abdde83452ad 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -328,12 +328,20 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dwc_pwm *dwc;
+	u32 nr_pwm;
 	int ret;
 
 	dwc = dwc_pwm_alloc(dev);
 	if (!dwc)
 		return -ENOMEM;
 
+	if (!device_property_read_u32(dev, "snps,pwm-number", &nr_pwm)) {
+		if (nr_pwm > DWC_TIMERS_TOTAL)
+			dev_err(dev, "too many PWMs specified (%d)\n", nr_pwm);
+		else
+			dwc->chip.npwm = nr_pwm;
+	}
+
 	dwc->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(dwc->base))
 		return dev_err_probe(dev, PTR_ERR(dwc->base),
-- 
2.35.1

Re: [PATCH 7/7] pwm: dwc: add snps,pwm-number to limit pwm count

[Krzysztof Kozlowski]

On 12/07/2022 12:01, Ben Dooks wrote:
> Add snps,pwm-number property to indicate if the block does not have
> all 8 of the PWM blocks.
> 
> Not sure if this should be a general PWM property consider optional
> for all PWM types, so have added a specific one here (there is only
> one other controller with a property for PWM count at the moment)
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-synposys.yaml | 5 +++++

No. Please do not hide bindings patches in the code. They must be
separate and checkpatch complains about it, so please be sure you run
checkpatch.


Best regards,
Krzysztof

Re: [PATCH 7/7] pwm: dwc: add snps,pwm-number to limit pwm count

[Rob Herring]

On Tue, Jul 12, 2022 at 11:01:13AM +0100, Ben Dooks wrote:
> Add snps,pwm-number property to indicate if the block does not have
> all 8 of the PWM blocks.
> 
> Not sure if this should be a general PWM property consider optional
> for all PWM types, so have added a specific one here (there is only
> one other controller with a property for PWM count at the moment)
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-synposys.yaml | 5 +++++
>  drivers/pwm/pwm-dwc.c                                   | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> index 38ac0da75272..15bdf764b46a 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
> @@ -30,11 +30,16 @@ properties:
>        - items:
>          - const: snps,pwm
>  
> +  snps,pwm-number:
> +    $ref: '/schemas/types.yaml#/definitions/uint32'
> +    description: u32 value representing the number of PWM devices

Why do we need to know this? Are you going to have a consumer to a 
non-existent PWM? If you do need to know how many, it should be implied 
by the compatible string.

Rob

Re: [PATCH 7/7] pwm: dwc: add snps,pwm-number to limit pwm count

[Ben Dooks]

On 18/07/2022 21:08, Rob Herring wrote:
> On Tue, Jul 12, 2022 at 11:01:13AM +0100, Ben Dooks wrote:
>> Add snps,pwm-number property to indicate if the block does not have
>> all 8 of the PWM blocks.
>>
>> Not sure if this should be a general PWM property consider optional
>> for all PWM types, so have added a specific one here (there is only
>> one other controller with a property for PWM count at the moment)
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>>   Documentation/devicetree/bindings/pwm/pwm-synposys.yaml | 5 +++++
>>   drivers/pwm/pwm-dwc.c                                   | 8 ++++++++
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
>> index 38ac0da75272..15bdf764b46a 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-synposys.yaml
>> @@ -30,11 +30,16 @@ properties:
>>         - items:
>>           - const: snps,pwm
>>   
>> +  snps,pwm-number:
>> +    $ref: '/schemas/types.yaml#/definitions/uint32'
>> +    description: u32 value representing the number of PWM devices
> 
> Why do we need to know this? Are you going to have a consumer to a
> non-existent PWM? If you do need to know how many, it should be implied
> by the compatible string.

For this IP block it is a build time option for 1..8 timers
so I thought it best we don't register non-existant timers


The system we are working on only has 1 PWM timer per block.

-- 
Ben