2024-01-25 03:09:07

by Wenhua Lin

[permalink] [raw]
Subject: [PATCH V2 0/6] pwm: sprd: Modification of UNISOC Platform PWM Driver

Due to new usage scenarios, some upgrades are made to unisoc's pwm driver.
Patch 1 supports the change of channel offset on UMS9620.
Patch 2 supports more brightness levels (duty cycle) for backlight control.
Patch 3 optimizes the calculation method of duty.
Patch 4 converts dt-binding file from txt to yaml
Patch 5-6 update pwm-sprd.yaml according to patch 1 and patch 2.

Change in V2:

-Change dev_err to dev_info in PATCH 2/6.
-Add maintainer to gitconfig.

Wenhua Lin (6):
pwm: sprd: Add support for UMS9620
pwm: sprd: Improve the pwm backlight control function
pwm: sprd: Optimize the calculation method of duty
dt-bindings: pwm: sprd: Convert to YAML
pwm: sprd: Add sprd,ums9620-pwm compatible
dt-bindings: pwm: sprd: Add sprd,mod attribute

.../devicetree/bindings/pwm/pwm-sprd.txt | 40 -------
.../devicetree/bindings/pwm/pwm-sprd.yaml | 106 ++++++++++++++++++
drivers/pwm/pwm-sprd.c | 71 ++++++++++--
3 files changed, 165 insertions(+), 52 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.yaml

--
2.17.1



2024-01-25 03:09:08

by Wenhua Lin

[permalink] [raw]
Subject: [PATCH V2 1/6] pwm: sprd: Add support for UMS9620

The PMW unit on the current Unisoc's SoCs has 4 channels but has different
address offsets. On UMS512, they are 0x0, 0x20, 0x40, 0x60 respectively,
while are 0x0, 0x4000, 0x8000, 0xC000 on UMS9620.

Signed-off-by: Wenhua Lin <[email protected]>
---
drivers/pwm/pwm-sprd.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
index 77939e161006..bc1e3ed13528 100644
--- a/drivers/pwm/pwm-sprd.c
+++ b/drivers/pwm/pwm-sprd.c
@@ -9,6 +9,7 @@
#include <linux/math64.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/pwm.h>

@@ -23,7 +24,6 @@
#define SPRD_PWM_ENABLE_BIT BIT(0)

#define SPRD_PWM_CHN_NUM 4
-#define SPRD_PWM_REGS_SHIFT 5
#define SPRD_PWM_CHN_CLKS_NUM 2
#define SPRD_PWM_CHN_OUTPUT_CLK 1

@@ -32,14 +32,27 @@ struct sprd_pwm_chn {
u32 clk_rate;
};

+struct sprd_pwm_data {
+ int reg_shift;
+};
+
struct sprd_pwm_chip {
void __iomem *base;
struct device *dev;
struct pwm_chip chip;
+ const struct sprd_pwm_data *pdata;
int num_pwms;
struct sprd_pwm_chn chn[SPRD_PWM_CHN_NUM];
};

+static const struct sprd_pwm_data ums512_data = {
+ .reg_shift = 5,
+};
+
+static const struct sprd_pwm_data ums9620_data = {
+ .reg_shift = 14,
+};
+
static inline struct sprd_pwm_chip* sprd_pwm_from_chip(struct pwm_chip *chip)
{
return container_of(chip, struct sprd_pwm_chip, chip);
@@ -58,7 +71,7 @@ static const char * const sprd_pwm_clks[] = {

static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
{
- u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
+ u32 offset = reg + (hwid << spc->pdata->reg_shift);

return readl_relaxed(spc->base + offset);
}
@@ -66,7 +79,7 @@ static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
u32 reg, u32 val)
{
- u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
+ u32 offset = reg + (hwid << spc->pdata->reg_shift);

writel_relaxed(val, spc->base + offset);
}
@@ -253,6 +266,7 @@ static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
static int sprd_pwm_probe(struct platform_device *pdev)
{
struct sprd_pwm_chip *spc;
+ const void *priv;
int ret;

spc = devm_kzalloc(&pdev->dev, sizeof(*spc), GFP_KERNEL);
@@ -263,6 +277,11 @@ static int sprd_pwm_probe(struct platform_device *pdev)
if (IS_ERR(spc->base))
return PTR_ERR(spc->base);

+ priv = of_device_get_match_data(&pdev->dev);
+ if (!priv)
+ return dev_err_probe(&pdev->dev, -EINVAL, "get regs shift failed!\n");
+ spc->pdata = priv;
+
spc->dev = &pdev->dev;

ret = sprd_pwm_clk_init(spc);
@@ -281,7 +300,8 @@ static int sprd_pwm_probe(struct platform_device *pdev)
}

static const struct of_device_id sprd_pwm_of_match[] = {
- { .compatible = "sprd,ums512-pwm", },
+ { .compatible = "sprd,ums512-pwm", .data = (void *)&ums512_data},
+ { .compatible = "sprd,ums9620-pwm", .data = (void *)&ums9620_data},
{ },
};
MODULE_DEVICE_TABLE(of, sprd_pwm_of_match);
--
2.17.1


2024-01-25 03:09:50

by Wenhua Lin

[permalink] [raw]
Subject: [PATCH V2 4/6] dt-bindings: pwm: sprd: Convert to YAML

Convert Spreadtrum PWM controller bindings to DT schema.

Signed-off-by: Wenhua Lin <[email protected]>
---
.../devicetree/bindings/pwm/pwm-sprd.txt | 40 --------
.../devicetree/bindings/pwm/pwm-sprd.yaml | 93 +++++++++++++++++++
2 files changed, 93 insertions(+), 40 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.yaml

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
deleted file mode 100644
index 87b206fd0618..000000000000
--- a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
+++ /dev/null
@@ -1,40 +0,0 @@
-Spreadtrum PWM controller
-
-Spreadtrum SoCs PWM controller provides 4 PWM channels.
-
-Required properties:
-- compatible : Should be "sprd,ums512-pwm".
-- reg: Physical base address and length of the controller's registers.
-- clocks: The phandle and specifier referencing the controller's clocks.
-- clock-names: Should contain following entries:
- "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
- "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
-- #pwm-cells: Should be 2. See pwm.yaml in this directory for a description of
- the cells format.
-
-Optional properties:
-- assigned-clocks: Reference to the PWM clock entries.
-- assigned-clock-parents: The phandle of the parent clock of PWM clock.
-
-Example:
- pwms: pwm@32260000 {
- compatible = "sprd,ums512-pwm";
- reg = <0 0x32260000 0 0x10000>;
- clock-names = "pwm0", "enable0",
- "pwm1", "enable1",
- "pwm2", "enable2",
- "pwm3", "enable3";
- clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>,
- <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>,
- <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>,
- <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>;
- assigned-clocks = <&aon_clk CLK_PWM0>,
- <&aon_clk CLK_PWM1>,
- <&aon_clk CLK_PWM2>,
- <&aon_clk CLK_PWM3>;
- assigned-clock-parents = <&ext_26m>,
- <&ext_26m>,
- <&ext_26m>,
- <&ext_26m>;
- #pwm-cells = <2>;
- };
diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
new file mode 100644
index 000000000000..81c5fd688c3c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2023 Unisoc Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-sprd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Spreadtrum PWM controller
+
+maintainers:
+ - Orson Zhai <[email protected]>
+ - Baolin Wang <[email protected]>
+ - Chunyan Zhang <[email protected]>
+
+description: |
+ Spreadtrum SoCs PWM controller provides 4 PWM channels.
+
+allOf:
+ - $ref: pwm.yaml#
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - sprd,ums512-pwm
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ minItems: 8
+ maxItems: 8
+
+ clock-names:
+ items:
+ - const: pwm0
+ - const: enable0
+ - const: pwm1
+ - const: enable1
+ - const: pwm2
+ - const: enable2
+ - const: pwm3
+ - const: enable3
+ description: |
+ Should contain following entries:
+ "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
+ "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
+
+ assigned-clocks:
+ minItems: 4
+ maxItems: 4
+
+ assigned-clock-parents:
+ minItems: 4
+ maxItems: 4
+
+ "#pwm-cells":
+ const: 2
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/sprd,ums512-clk.h>
+ pwms: pwm@32260000 {
+ compatible = "sprd,ums512-pwm";
+ reg = <0x32260000 0x10000>;
+ clock-names = "pwm0", "enable0",
+ "pwm1", "enable1",
+ "pwm2", "enable2",
+ "pwm3", "enable3";
+ clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>,
+ <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>,
+ <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>,
+ <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>;
+ assigned-clocks = <&aon_clk CLK_PWM0>,
+ <&aon_clk CLK_PWM1>,
+ <&aon_clk CLK_PWM2>,
+ <&aon_clk CLK_PWM3>;
+ assigned-clock-parents = <&ext_26m>,
+ <&ext_26m>,
+ <&ext_26m>,
+ <&ext_26m>;
+ #pwm-cells = <2>;
+ };
+
+...
--
2.17.1


2024-01-25 03:10:05

by Wenhua Lin

[permalink] [raw]
Subject: [PATCH V2 3/6] pwm: sprd: Optimize the calculation method of duty

Use DIV_ROUND_CLOSEST_ULL to avoid overflow and improve accuracy
when calculating duty.

Signed-off-by: Wenhua Lin <[email protected]>
---
drivers/pwm/pwm-sprd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
index cc54aa77c7e6..8de3f9e154ce 100644
--- a/drivers/pwm/pwm-sprd.c
+++ b/drivers/pwm/pwm-sprd.c
@@ -156,7 +156,8 @@ static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
* given settings (MOD and input clock).
*/
mod = spc->mod[pwm->hwpwm];
- duty = duty_ns * mod / period_ns;
+ tmp = (u64)duty_ns * mod;
+ duty = DIV_ROUND_CLOSEST_ULL(tmp, period_ns);

tmp = (u64)chn->clk_rate * period_ns;
do_div(tmp, NSEC_PER_SEC);
--
2.17.1


2024-01-25 03:10:17

by Wenhua Lin

[permalink] [raw]
Subject: [PATCH V2 2/6] pwm: sprd: Improve the pwm backlight control function

The pwm-sprd driver support only 8-bit linear control of backlight. Now,
new requests of supporting 9-bit, 10-bit, 11-bit and 12-bit linear
control of backlight are proposed. Besides, different channels of pwm
could be configured into different linear control of backlight. Thus,
sprd,mod attribute is introduced into dts for every channel of pwm
device. This attribute would determine the value of MOD and eventually
realize the new requirements.

Signed-off-by: Wenhua Lin <[email protected]>
---
drivers/pwm/pwm-sprd.c | 42 ++++++++++++++++++++++++++++++++++--------
1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
index bc1e3ed13528..cc54aa77c7e6 100644
--- a/drivers/pwm/pwm-sprd.c
+++ b/drivers/pwm/pwm-sprd.c
@@ -18,7 +18,8 @@
#define SPRD_PWM_DUTY 0x8
#define SPRD_PWM_ENABLE 0x18

-#define SPRD_PWM_MOD_MAX GENMASK(7, 0)
+#define SPRD_PWM_MOD_MAX GENMASK(15, 0)
+#define SPRD_PWM_MOD_DEFAULT GENMASK(9, 0)
#define SPRD_PWM_DUTY_MSK GENMASK(15, 0)
#define SPRD_PWM_PRESCALE_MSK GENMASK(7, 0)
#define SPRD_PWM_ENABLE_BIT BIT(0)
@@ -43,6 +44,7 @@ struct sprd_pwm_chip {
const struct sprd_pwm_data *pdata;
int num_pwms;
struct sprd_pwm_chn chn[SPRD_PWM_CHN_NUM];
+ u32 mod[SPRD_PWM_CHN_NUM];
};

static const struct sprd_pwm_data ums512_data = {
@@ -120,7 +122,7 @@ static int sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
*/
val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
prescale = val & SPRD_PWM_PRESCALE_MSK;
- tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
+ tmp = (prescale + 1) * NSEC_PER_SEC * spc->mod[pwm->hwpwm];
state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);

val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
@@ -140,7 +142,7 @@ static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
int duty_ns, int period_ns)
{
struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
- u32 prescale, duty;
+ u32 prescale, duty, mod;
u64 tmp;

/*
@@ -148,16 +150,21 @@ static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
* The period length is (PRESCALE + 1) * MOD counter steps.
* The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
*
- * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
+ * The value for MOD is obtained from dts.
* The value for PRESCALE is selected such that the resulting period
* gets the maximal length not bigger than the requested one with the
- * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
+ * given settings (MOD and input clock).
*/
- duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
+ mod = spc->mod[pwm->hwpwm];
+ duty = duty_ns * mod / period_ns;

tmp = (u64)chn->clk_rate * period_ns;
do_div(tmp, NSEC_PER_SEC);
- prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
+ prescale = DIV_ROUND_CLOSEST_ULL(tmp, mod);
+ if (prescale < 1)
+ prescale = 1;
+ prescale--;
+
if (prescale > SPRD_PWM_PRESCALE_MSK)
prescale = SPRD_PWM_PRESCALE_MSK;

@@ -170,7 +177,7 @@ static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
* before changing a new configuration to avoid mixed settings.
*/
sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
- sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
+ sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, mod);
sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);

return 0;
@@ -263,6 +270,21 @@ static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
return 0;
}

+static int sprd_pwm_get_mod(struct platform_device *pdev)
+{
+ int i, ret;
+ struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
+
+ ret = of_property_read_u32_array(pdev->dev.of_node,
+ "sprd,mod", spc->mod, spc->num_pwms);
+ if (ret) {
+ for (i = 0; i < spc->num_pwms; i++)
+ spc->mod[i] = SPRD_PWM_MOD_DEFAULT;
+ }
+
+ return ret;
+}
+
static int sprd_pwm_probe(struct platform_device *pdev)
{
struct sprd_pwm_chip *spc;
@@ -288,6 +310,10 @@ static int sprd_pwm_probe(struct platform_device *pdev)
if (ret)
return ret;

+ ret = sprd_pwm_get_mod(pdev);
+ if (ret)
+ dev_info(&pdev->dev, "get pwm mod failed! Use default setting\n");
+
spc->chip.dev = &pdev->dev;
spc->chip.ops = &sprd_pwm_ops;
spc->chip.npwm = spc->num_pwms;
--
2.17.1


2024-01-25 03:10:18

by Wenhua Lin

[permalink] [raw]
Subject: [PATCH V2 5/6] pwm: sprd: Add sprd,ums9620-pwm compatible

Add sprd,ums9620-pwm compatible string to binding document.

Signed-off-by: Wenhua Lin <[email protected]>
---
Documentation/devicetree/bindings/pwm/pwm-sprd.yaml | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
index 81c5fd688c3c..02e039fee3b4 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
@@ -23,6 +23,7 @@ properties:
items:
- enum:
- sprd,ums512-pwm
+ - sprd,ums9620-pwm

reg:
maxItems: 1
--
2.17.1


2024-01-25 03:11:13

by Wenhua Lin

[permalink] [raw]
Subject: [PATCH V2 6/6] dt-bindings: pwm: sprd: Add sprd,mod attribute

Add sprd,mod attribute, which set the number of different
duty cycles that PWM's waveform could output, to dts.

Signed-off-by: Wenhua Lin <[email protected]>
---
Documentation/devicetree/bindings/pwm/pwm-sprd.yaml | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
index 02e039fee3b4..7c956b840fa1 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
@@ -55,6 +55,16 @@ properties:
minItems: 4
maxItems: 4

+ sprd,mod:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 4
+ maxItems: 4
+ items:
+ minimum: 0xFF
+ maximum: 0xFFF
+ description: |
+ The number of different duty cycles that could be set for PWM's waveform output.
+
"#pwm-cells":
const: 2

@@ -63,6 +73,7 @@ required:
- reg
- clocks
- clock-names
+ - sprd,mod

additionalProperties: false

@@ -88,6 +99,7 @@ examples:
<&ext_26m>,
<&ext_26m>,
<&ext_26m>;
+ sprd,mod = <0xFF 0x1FF 0x3FF 0xFFF>;
#pwm-cells = <2>;
};

--
2.17.1


2024-01-25 17:24:48

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH V2 6/6] dt-bindings: pwm: sprd: Add sprd,mod attribute

On Thu, Jan 25, 2024 at 10:55:33AM +0800, Wenhua Lin wrote:
> Add sprd,mod attribute, which set the number of different
> duty cycles that PWM's waveform could output, to dts.
>
> Signed-off-by: Wenhua Lin <[email protected]>
> ---
> Documentation/devicetree/bindings/pwm/pwm-sprd.yaml | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
> index 02e039fee3b4..7c956b840fa1 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
> @@ -55,6 +55,16 @@ properties:
> minItems: 4
> maxItems: 4
>
> + sprd,mod:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 4
> + maxItems: 4
> + items:
> + minimum: 0xFF
> + maximum: 0xFFF
> + description: |
> + The number of different duty cycles that could be set for PWM's waveform output.

Why is this not a fixed value for a given SoC? Given the description, it
certainly sounds like something that does not vary on a per device
basis.

Thanks,
Conor.

> +
> "#pwm-cells":
> const: 2
>
> @@ -63,6 +73,7 @@ required:
> - reg
> - clocks
> - clock-names
> + - sprd,mod
>
> additionalProperties: false
>
> @@ -88,6 +99,7 @@ examples:
> <&ext_26m>,
> <&ext_26m>,
> <&ext_26m>;
> + sprd,mod = <0xFF 0x1FF 0x3FF 0xFFF>;
> #pwm-cells = <2>;
> };
>
> --
> 2.17.1
>


Attachments:
(No filename) (1.63 kB)
signature.asc (235.00 B)
Download all attachments

2024-01-25 17:32:53

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH V2 4/6] dt-bindings: pwm: sprd: Convert to YAML

Hey,

On Thu, Jan 25, 2024 at 10:55:31AM +0800, Wenhua Lin wrote:
> Convert Spreadtrum PWM controller bindings to DT schema.
>
> Signed-off-by: Wenhua Lin <[email protected]>
> ---
> .../devicetree/bindings/pwm/pwm-sprd.txt | 40 --------
> .../devicetree/bindings/pwm/pwm-sprd.yaml | 93 +++++++++++++++++++
> 2 files changed, 93 insertions(+), 40 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> deleted file mode 100644
> index 87b206fd0618..000000000000
> --- a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> +++ /dev/null
> @@ -1,40 +0,0 @@
> -Spreadtrum PWM controller
> -
> -Spreadtrum SoCs PWM controller provides 4 PWM channels.
> -
> -Required properties:
> -- compatible : Should be "sprd,ums512-pwm".
> -- reg: Physical base address and length of the controller's registers.
> -- clocks: The phandle and specifier referencing the controller's clocks.
> -- clock-names: Should contain following entries:
> - "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
> - "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
> -- #pwm-cells: Should be 2. See pwm.yaml in this directory for a description of
> - the cells format.
> -
> -Optional properties:
> -- assigned-clocks: Reference to the PWM clock entries.
> -- assigned-clock-parents: The phandle of the parent clock of PWM clock.
> -
> -Example:
> - pwms: pwm@32260000 {
> - compatible = "sprd,ums512-pwm";
> - reg = <0 0x32260000 0 0x10000>;
> - clock-names = "pwm0", "enable0",
> - "pwm1", "enable1",
> - "pwm2", "enable2",
> - "pwm3", "enable3";
> - clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>,
> - <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>,
> - <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>,
> - <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>;
> - assigned-clocks = <&aon_clk CLK_PWM0>,
> - <&aon_clk CLK_PWM1>,
> - <&aon_clk CLK_PWM2>,
> - <&aon_clk CLK_PWM3>;
> - assigned-clock-parents = <&ext_26m>,
> - <&ext_26m>,
> - <&ext_26m>,
> - <&ext_26m>;
> - #pwm-cells = <2>;
> - };
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
> new file mode 100644
> index 000000000000..81c5fd688c3c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2023 Unisoc Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/pwm-sprd.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Spreadtrum PWM controller
> +
> +maintainers:
> + - Orson Zhai <[email protected]>
> + - Baolin Wang <[email protected]>
> + - Chunyan Zhang <[email protected]>
> +
> +description: |

The | here is not need, you have no formatting to preserve.

> + Spreadtrum SoCs PWM controller provides 4 PWM channels.
> +
> +allOf:
> + - $ref: pwm.yaml#
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - sprd,ums512-pwm

this is just

compatible:
const: sprd,ums512-pwm

> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + minItems: 8
> + maxItems: 8
> +
> + clock-names:
> + items:
> + - const: pwm0
> + - const: enable0
> + - const: pwm1
> + - const: enable1
> + - const: pwm2
> + - const: enable2
> + - const: pwm3
> + - const: enable3

> + description: |
> + Should contain following entries:
> + "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
> + "enablen": for PWM channel n enable clock (n range: 0 ~ 3).

I would drop this description from here, and create an items list under
clocks, describing each clock. You can then drop the "minItems: 8" &
"maxItems: 8" from there.

> +
> + assigned-clocks:
> + minItems: 4
> + maxItems: 4
> +
> + assigned-clock-parents:
> + minItems: 4
> + maxItems: 4
> +
> + "#pwm-cells":
> + const: 2
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/sprd,ums512-clk.h>

nit: newline here please.

> + pwms: pwm@32260000 {

The "pwms" label here is not used and should be dropped.

Thanks,
Conor.


> + compatible = "sprd,ums512-pwm";
> + reg = <0x32260000 0x10000>;
> + clock-names = "pwm0", "enable0",
> + "pwm1", "enable1",
> + "pwm2", "enable2",
> + "pwm3", "enable3";
> + clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>,
> + <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>,
> + <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>,
> + <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>;
> + assigned-clocks = <&aon_clk CLK_PWM0>,
> + <&aon_clk CLK_PWM1>,
> + <&aon_clk CLK_PWM2>,
> + <&aon_clk CLK_PWM3>;
> + assigned-clock-parents = <&ext_26m>,
> + <&ext_26m>,
> + <&ext_26m>,
> + <&ext_26m>;
> + #pwm-cells = <2>;
> + };
> +
> +...
> --
> 2.17.1
>


Attachments:
(No filename) (5.59 kB)
signature.asc (235.00 B)
Download all attachments

2024-01-25 21:14:21

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH V2 2/6] pwm: sprd: Improve the pwm backlight control function

Hello,

On Thu, Jan 25, 2024 at 10:55:29AM +0800, Wenhua Lin wrote:
> The pwm-sprd driver support only 8-bit linear control of backlight. Now,
> new requests of supporting 9-bit, 10-bit, 11-bit and 12-bit linear
> control of backlight are proposed.

I would expect that you can determine a sensible value for mod at
runtime. Also adding this to the device tree isn't hardware description,
is it?

Best regards
Uwe

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


Attachments:
(No filename) (586.00 B)
signature.asc (499.00 B)
Download all attachments

2024-01-25 21:18:06

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH V2 5/6] pwm: sprd: Add sprd,ums9620-pwm compatible

Hello,

On Thu, Jan 25, 2024 at 10:55:32AM +0800, Wenhua Lin wrote:
> Add sprd,ums9620-pwm compatible string to binding document.
>
> Signed-off-by: Wenhua Lin <[email protected]>
> ---
> Documentation/devicetree/bindings/pwm/pwm-sprd.yaml | 1 +
> 1 file changed, 1 insertion(+)

Please use a Subject prefix that makes the dt maintainers spot this
patch in their inbox. The right one here would be:

dt-bindings: pwm: sprd: ...

Best regards
Uwe

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


Attachments:
(No filename) (630.00 B)
signature.asc (499.00 B)
Download all attachments

2024-01-26 08:57:56

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH V2 1/6] pwm: sprd: Add support for UMS9620

Hello,

On Thu, Jan 25, 2024 at 10:55:28AM +0800, Wenhua Lin wrote:
> The PMW unit on the current Unisoc's SoCs has 4 channels but has different
> address offsets. On UMS512, they are 0x0, 0x20, 0x40, 0x60 respectively,
> while are 0x0, 0x4000, 0x8000, 0xC000 on UMS9620.

I just sent feedback to (implicit) v1 of this patch and only realized
now there is a v2 already. My feedback given there also applies to this
patch. So I'm marking also this patch as "changes requested" in
patchwork, please look at
https://lore.kernel.org/linux-pwm/bvnhi4qeczrmlmaog6drlztg4x6ubozjzu57sukpejme7xecqc@724g62vjgxrq/T/#u
for the details.

Best regards
Uwe

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


Attachments:
(No filename) (818.00 B)
signature.asc (499.00 B)
Download all attachments

2024-01-26 08:59:44

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH V2 3/6] pwm: sprd: Optimize the calculation method of duty

Hello,

On Thu, Jan 25, 2024 at 10:55:30AM +0800, Wenhua Lin wrote:
> diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> index cc54aa77c7e6..8de3f9e154ce 100644
> --- a/drivers/pwm/pwm-sprd.c
> +++ b/drivers/pwm/pwm-sprd.c
> @@ -156,7 +156,8 @@ static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> * given settings (MOD and input clock).
> */
> mod = spc->mod[pwm->hwpwm];
> - duty = duty_ns * mod / period_ns;
> + tmp = (u64)duty_ns * mod;
> + duty = DIV_ROUND_CLOSEST_ULL(tmp, period_ns);

Please stick to rounding down in .apply() (and so sprd_pwm_config()).
Given that duty_ns is an u64 in .apply(), you're loosing precision
anyhow. Look at how the microchip-core driver uses mul_u64_u64_div_u64()
for how to do that properly.

You tested your patch with CONFIG_PWM_DEBUG enabled, right?

Best regards
Uwe

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


Attachments:
(No filename) (1.02 kB)
signature.asc (499.00 B)
Download all attachments

2024-01-30 22:18:12

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH V2 4/6] dt-bindings: pwm: sprd: Convert to YAML

On Thu, Jan 25, 2024 at 05:31:48PM +0000, Conor Dooley wrote:
> Hey,
>
> On Thu, Jan 25, 2024 at 10:55:31AM +0800, Wenhua Lin wrote:
> > Convert Spreadtrum PWM controller bindings to DT schema.
> >
> > Signed-off-by: Wenhua Lin <[email protected]>
> > ---
> > .../devicetree/bindings/pwm/pwm-sprd.txt | 40 --------
> > .../devicetree/bindings/pwm/pwm-sprd.yaml | 93 +++++++++++++++++++
> > 2 files changed, 93 insertions(+), 40 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> > deleted file mode 100644
> > index 87b206fd0618..000000000000
> > --- a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> > +++ /dev/null
> > @@ -1,40 +0,0 @@
> > -Spreadtrum PWM controller
> > -
> > -Spreadtrum SoCs PWM controller provides 4 PWM channels.
> > -
> > -Required properties:
> > -- compatible : Should be "sprd,ums512-pwm".
> > -- reg: Physical base address and length of the controller's registers.
> > -- clocks: The phandle and specifier referencing the controller's clocks.
> > -- clock-names: Should contain following entries:
> > - "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
> > - "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
> > -- #pwm-cells: Should be 2. See pwm.yaml in this directory for a description of
> > - the cells format.
> > -
> > -Optional properties:
> > -- assigned-clocks: Reference to the PWM clock entries.
> > -- assigned-clock-parents: The phandle of the parent clock of PWM clock.
> > -
> > -Example:
> > - pwms: pwm@32260000 {
> > - compatible = "sprd,ums512-pwm";
> > - reg = <0 0x32260000 0 0x10000>;
> > - clock-names = "pwm0", "enable0",
> > - "pwm1", "enable1",
> > - "pwm2", "enable2",
> > - "pwm3", "enable3";
> > - clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>,
> > - <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>,
> > - <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>,
> > - <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>;
> > - assigned-clocks = <&aon_clk CLK_PWM0>,
> > - <&aon_clk CLK_PWM1>,
> > - <&aon_clk CLK_PWM2>,
> > - <&aon_clk CLK_PWM3>;
> > - assigned-clock-parents = <&ext_26m>,
> > - <&ext_26m>,
> > - <&ext_26m>,
> > - <&ext_26m>;
> > - #pwm-cells = <2>;
> > - };
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
> > new file mode 100644
> > index 000000000000..81c5fd688c3c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.yaml
> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2023 Unisoc Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pwm/pwm-sprd.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Spreadtrum PWM controller
> > +
> > +maintainers:
> > + - Orson Zhai <[email protected]>
> > + - Baolin Wang <[email protected]>
> > + - Chunyan Zhang <[email protected]>
> > +
> > +description: |
>
> The | here is not need, you have no formatting to preserve.
>
> > + Spreadtrum SoCs PWM controller provides 4 PWM channels.
> > +
> > +allOf:
> > + - $ref: pwm.yaml#
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - sprd,ums512-pwm
>
> this is just
>
> compatible:
> const: sprd,ums512-pwm
>
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + clocks:
> > + minItems: 8
> > + maxItems: 8
> > +
> > + clock-names:
> > + items:
> > + - const: pwm0
> > + - const: enable0
> > + - const: pwm1
> > + - const: enable1
> > + - const: pwm2
> > + - const: enable2
> > + - const: pwm3
> > + - const: enable3
>
> > + description: |
> > + Should contain following entries:
> > + "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
> > + "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
>
> I would drop this description from here, and create an items list under
> clocks, describing each clock. You can then drop the "minItems: 8" &
> "maxItems: 8" from there.
>
> > +
> > + assigned-clocks:
> > + minItems: 4
> > + maxItems: 4
> > +
> > + assigned-clock-parents:
> > + minItems: 4
> > + maxItems: 4

Also, drop assigned-clock* here and from the example. It's outside the
scope of bindings.

Rob