pwm: Add pwm driver for Sophgo SG2042
Subsystems (4)
cover pwm: Add pwm driver for Sophgo SG2042
From: Chen Wang <[email protected]>
Add driver for pwm controller of Sophgo SG2042 SoC.
Chen Wang (2):
dt-bindings: pwm: sophgo: add bindings for sg2042
pwm: sophgo: add driver for Sophgo SG2042 PWM
.../bindings/pwm/sophgo,sg2042-pwm.yaml | 52 ++++++ drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sophgo-sg2042.c | 148 ++++++++++++++++++ 4 files changed, 210 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6
Signature · 1 line
2.34.1
PATCH 1/2 dt-bindings: pwm: sophgo: add bindings for sg2042
From: Chen Wang <[email protected]>
Add binding document for sophgo,sg2042-pwm.
--- .../bindings/pwm/sophgo,sg2042-pwm.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +52 -0
@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/sophgo,sg2042-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sophgo SG2042 PWM controller + +maintainers: + - Chen Wang <[email protected]> + +description: | + This controller contains 4 channels which can generate PWM waveforms. + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + const: sophgo,sg2042-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + items: + - const: apb + + "#pwm-cells": + # See pwm.yaml in this directory for a description of the cells format. + const: 2 + +required: + - compatible + - reg + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + pwm@7f006000 { + compatible = "sophgo,sg2042-pwm"; + reg = <0x7f006000 0x1000>; + #pwm-cells = <2>; + clocks = <&clock 67>; + clock-names = "apb"; + };
Signature · 1 line
2.34.1
Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042
Quoted 18 lines · Chen Wang
From: Chen Wang <[email protected]>
Add binding document for sophgo,sg2042-pwm.
Signed-off-by Chen Wang <[email protected]>--- .../bindings/pwm/sophgo,sg2042-pwm.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yamlDocumentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +2
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 12
Quoted 10 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +10
-0+--- +$id: http://devicetree.org/schemas/pwm/sophgo,sg2042-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sophgo SG2042 PWM controller + +maintainers: + - Chen Wang <[email protected]> + +description: |
you can drop | here
Quoted 20 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +20
-0+ This controller contains 4 channels which can generate PWM waveforms. + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + const: sophgo,sg2042-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + items: + - const: apb + + "#pwm-cells":
..
Quoted 1 line
+ # See
pwm.yamlin this directory for a description of the cells format.
I think you can drop this comment, since no useful information added also it's already refered to standard pwm.yaml
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 40
Quoted 7 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +7
-0+ const: 2 + +required: + - compatible + - reg + - clocks + - clock-names
also "#pwm-cells"?
Quoted 15 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +12
-0+ +additionalProperties: false + +examples: + - | + pwm@7f006000 { + compatible = "sophgo,sg2042-pwm"; + reg = <0x7f006000 0x1000>; + #pwm-cells = <2>; + clocks = <&clock 67>; + clock-names = "apb"; + };Signature · 1 line
2.34.1
Signature · 3 lines
Yixun Lan (dlan) Gentoo Linux Developer GPG Key ID AABEFD55
Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042
Quoted 7 lines · Yixun Lan
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 40
Quoted 6 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +6
-0+ +required: + - compatible + - reg + - clocks + - clock-namesalso "#pwm-cells"?
pwm.yaml has it?
Best regards,
Krzysztof
Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042
In reply to Yixun LanQuoted 22 lines · Yixun Lan
Quoted 21 lines · Chen Wang
From: Chen Wang <[email protected]>
Add binding document for sophgo,sg2042-pwm.
Signed-off-by Chen Wang <[email protected]>--- .../bindings/pwm/sophgo,sg2042-pwm.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+)create mode 100644
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yamlDocumentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +5
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/sophgo,sg2042-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
Quoted 8 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 12
Quoted 7 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +7
-0+ +title: Sophgo SG2042 PWM controller + +maintainers: + - Chen Wang <[email protected]> + +description: |you can drop | here
Ack and thanks.
Quoted 21 lines
Quoted 20 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +20
-0+ This controller contains 4 channels which can generate PWM waveforms. + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + const: sophgo,sg2042-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + items: + - const: apb + + "#pwm-cells":..
Quoted 3 lines
Quoted 1 line
+ # See
pwm.yamlin this directory for a description of the cells format.I think you can drop this comment, since no useful information added also it's already refered to standard
pwm.yaml
I add this comment just want to reminder. Ok, anyway, it can be removed to make doc clear.
Quoted 8 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 40
Quoted 7 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +7
-0+ const: 2 + +required: + - compatible + - reg + - clocks + - clock-namesalso "#pwm-cells"?
I think it has been required in pwm.yaml, so no need to required it again here.
Quoted 15 lines
Quoted 15 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +12
-0+ +additionalProperties: false + +examples: + - | + pwm@7f006000 { + compatible = "sophgo,sg2042-pwm"; + reg = <0x7f006000 0x1000>; + #pwm-cells = <2>; + clocks = <&clock 67>; + clock-names = "apb"; + };Signature · 1 line
2.34.1
Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042
In reply to Chen WangQuoted 3 lines · Chen Wang
From: Chen Wang <[email protected]>
Add binding document for sophgo,sg2042-pwm.
A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings. See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Say something useful about hardware instead. The same in commit msg - you keep saying obvious and duplicated commit subject.
Quoted 15 lines
Signed-off-by Chen Wang <[email protected]>--- .../bindings/pwm/sophgo,sg2042-pwm.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yamlDocumentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +2
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 12
Quoted 10 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +10
-0+--- +$id: http://devicetree.org/schemas/pwm/sophgo,sg2042-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sophgo SG2042 PWM controller + +maintainers: + - Chen Wang <[email protected]> + +description: |
drop |
Quoted 11 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +11
-0+ This controller contains 4 channels which can generate PWM waveforms. + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + const: sophgo,sg2042-pwm + + reg: + maxItems: 1
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 33
Quoted 10 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +10
-0+ + clocks: + maxItems: 1 + + clock-names: + items: + - const: apb + + "#pwm-cells": + # See pwm.yaml in this directory for a description of the cells format.
Drop
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 42
Quoted 9 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +9
-0+ const: 2 + +required: + - compatible + - reg + - clocks + - clock-names + +additionalProperties: false
unevaluatedProperties instead
Best regards,
Krzysztof
Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042
Quoted 11 lines · Krzysztof Kozlowski
Quoted 3 lines · Chen Wang
From: Chen Wang <[email protected]>
Add binding document for sophgo,sg2042-pwm.
A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings. See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18Say something useful about hardware instead. The same in commit msg - you keep saying obvious and duplicated commit subject.
OK, thanks
Quoted 17 lines
Quoted 17 lines
Signed-off-by Chen Wang <[email protected]>--- .../bindings/pwm/sophgo,sg2042-pwm.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+)create mode 100644
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yamlDocumentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +5
-0@@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/sophgo,sg2042-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
Quoted 8 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 12
Quoted 7 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +7
-0+ +title: Sophgo SG2042 PWM controller + +maintainers: + - Chen Wang <[email protected]> + +description: |drop |
Ack and thansk.
Quoted 14 lines
Quoted 14 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +14
-0+ This controller contains 4 channels which can generate PWM waveforms. + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + const: sophgo,sg2042-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1
Quoted 8 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 33
Quoted 7 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +7
-0+ + clock-names: + items: + - const: apb + + "#pwm-cells": + # See pwm.yaml in this directory for a description of the cells format.Drop
Ack and thanks.
Quoted 10 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml line 42
Quoted 9 lines
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml new file +9
-0+ const: 2 + +required: + - compatible + - reg + - clocks + - clock-names + +additionalProperties: falseunevaluatedProperties instead
Yes, my mistake, thanks.
Quoted 3 lines
Best regards,
Krzysztof
PATCH 2/2 pwm: sophgo: add driver for Sophgo SG2042 PWM
In reply to Chen WangFrom: Chen Wang <[email protected]>
Add a PWM driver for PWM controller in Sophgo SG2042 SoC.
--- drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sophgo-sg2042.c | 148 ++++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+) create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
drivers/pwm/Kconfig +9 -0
@@ -577,6 +577,15 @@ config PWM_SL28CPLD To compile this driver as a module, choose M here: the module will be called pwm-sl28cpld. +config PWM_SOPHGO_SG2042 + tristate "Sophgo SG2042 PWM support" + depends on ARCH_SOPHGO || COMPILE_TEST + help + PWM driver for Sophgo SG2042 PWM controller. + + To compile this driver as a module, choose M here: the module + will be called pwm_sophgo_sg2042. + config PWM_SPEAR tristate "STMicroelectronics SPEAr PWM support" depends on PLAT_SPEAR || COMPILE_TEST
drivers/pwm/Makefile +1 -0
@@ -52,6 +52,7 @@ obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o +obj-$(CONFIG_PWM_SOPHGO_SG2042) += pwm-sophgo-sg2042.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o obj-$(CONFIG_PWM_STI) += pwm-sti.o
drivers/pwm/pwm-sophgo-sg2042.c new file +148 -0
148 lines not on this page · raw message
Signature · 1 line
2.34.1
Re: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
Hello,
Quoted 97 lines · Chen Wang
From: Chen Wang <[email protected]>
Add a PWM driver for PWM controller in Sophgo SG2042 SoC.
Signed-off-by Chen Wang <[email protected]>--- drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sophgo-sg2042.c | 148 ++++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+) create mode 100644 drivers/pwm/pwm-sophgo-sg2042.cdrivers/pwm/Kconfig +9
-0@@ -577,6 +577,15 @@ config PWM_SL28CPLD To compile this driver as a module, choose M here: the module will be called pwm-sl28cpld. +config PWM_SOPHGO_SG2042 + tristate "Sophgo SG2042 PWM support" + depends on ARCH_SOPHGO || COMPILE_TEST + help + PWM driver for Sophgo SG2042 PWM controller. + + To compile this driver as a module, choose M here: the module + will be called pwm_sophgo_sg2042. + config PWM_SPEAR tristate "STMicroelectronics SPEAr PWM support" depends on PLAT_SPEAR || COMPILE_TESTdrivers/pwm/Makefile +1
-0@@ -52,6 +52,7 @@ obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o +obj-$(CONFIG_PWM_SOPHGO_SG2042) += pwm-sophgo-sg2042.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o obj-$(CONFIG_PWM_STI) += pwm-sti.odrivers/pwm/pwm-sophgo-sg2042.c new file +47
-0@@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Sophgo SG2042 PWM Controller Driver +* +* Copyright (C) 2024 Sophgo Technology Inc. +* Copyright (C) 2024 Chen Wang <[email protected]> +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> + +#include <asm/div64.h> + +/* +* Offset RegisterName +* 0x0000 HLPERIOD0 +* 0x0004 PERIOD0 +* 0x0008 HLPERIOD1 +* 0x000C PERIOD1 +* 0x0010 HLPERIOD2 +* 0x0014 PERIOD2 +* 0x0018 HLPERIOD3 +* 0x001C PERIOD3 +* Four groups and every group is composed of HLPERIOD & PERIOD +*/ +#define REG_HLPERIOD 0x0 +#define REG_PERIOD 0x4 + +#define REG_GROUP 0x8 + +#define SG2042_PWM_CHANNELNUM 4 + +/** +* struct sg2042_pwm_chip - private data of PWM chip +* @base: base address of mapped PWM registers +* @base_clk: base clock used to drive the pwm controller +*/ +struct sg2042_pwm_chip { +void __iomem *base; +struct clk *base_clk; +}; + +static inline
drivers/pwm/pwm-sophgo-sg2042.c line 57
Quoted 10 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +10
-0+struct sg2042_pwm_chip *to_sg2042_pwm_chip (struct pwm_chip *chip) +{ +return pwmchip_get_drvdata (chip); +} + +static void pwm_sg2042_config (void __iomem *base,unsigned int channo,u32 period,u32 hlperiod) +{ +writel (period, base +REG_GROUP * channo +REG_PERIOD ); +writel (hlperiod, base +REG_GROUP * channo +REG_HLPERIOD ); +}
I suggest to use the following instead:
#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
#define SG2042_PERIOD(chan) ((chan) * 8 + 4)
...
static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod)
{
writel(period, base + SG2042_PERIOD(chan));
writel(hlperiod, base + SG2042_HLPERIOD(chan));
}The (subjective?) advantage is that the definition of SG2042_HLPERIOD contains information about all channel's HLPERIOD register and the usage in pwm_sg2042_config is obviously(?) right.
(Another advantage is that the register names have a prefix unique to the driver, so there is no danger of mixing it up with some other hardware's driver that might also have a register named "PERIOD".)
Is this racy? i.e. can it happen that between the two writel the output is defined by the new period and old duty_cycle?
How does the hardware behave on reconfiguration? Does it complete the currently running period? Please document that in a comment at the start of the driver like many other drivers do. (See
sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
)
drivers/pwm/pwm-sophgo-sg2042.c line 71
Quoted 13 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +13
-0+static int pwm_sg2042_apply (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state) +{ +struct sg2042_pwm_chip *sg2042_pwm =to_sg2042_pwm_chip (chip); +u32 hlperiod; +u32 period; +u64 f_clk; +u64 p; + +if (!state->enabled) { +pwm_sg2042_config (sg2042_pwm->base, pwm->hwpwm,0 ,0 ); +return 0 ; + }
Here you're missing (I guess):
if (state->polarity == PWM_POLARITY_INVERSED) return -EINVAL;
drivers/pwm/pwm-sophgo-sg2042.c line 79
Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +7
-0+/* +* Period of High level (duty_cycle) = HLPERIOD x Period_clk +* Period of One Cycle (period) = PERIOD x Period_clk +*/ + f_clk =clk_get_rate (sg2042_pwm->base_clk); + + p = f_clk * state->period;
This might overflow.
drivers/pwm/pwm-sophgo-sg2042.c line 81
Quoted 2 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +2
-0+do_div (p,NSEC_PER_SEC ); + period = (u32 )p;
This gets very wrong if p happens to be bigger than U32_MAX.
If you ensure f_clk <= NSEC_PER_SEC in .probe() (in combination with a call to clk_rate_exclusive_get()), you can do:
period_cycles = min(mul_u64_u64_div_u64(f_clk, state->period, NSEC_PER_SEC), U32_MAX); duty_cycles = min(mul_u64_u64_div_u64(f_clk, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
This would also allow to store the clkrate in the driver private struct and drop the pointer to the clk from there.
drivers/pwm/pwm-sophgo-sg2042.c line 88
Quoted 6 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +6
-0+ p = f_clk * state->duty_cycle; +do_div (p,NSEC_PER_SEC ); + hlperiod = (u32 )p; + +dev_dbg (pwmchip_parent (chip),"chan[%d]: period=%u, hlperiod=%u\n" , + pwm->hwpwm, period, hlperiod);
pwm->hwpwm is an unsigned int, so use %u for it.
drivers/pwm/pwm-sophgo-sg2042.c line 96
Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +7
-0+pwm_sg2042_config (sg2042_pwm->base, pwm->hwpwm, period, hlperiod); + +return 0 ; +} + +static const struct pwm_ops pwm_sg2042_ops = { + .apply = pwm_sg2042_apply,
No .get_state() possible? Please use a single space before =.
drivers/pwm/pwm-sophgo-sg2042.c line 101
Quoted 5 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +5
-0+}; + +static const struct of_device_id sg2042_pwm_match[] = { + { .compatible ="sophgo,sg2042-pwm" }, + { },
Please drop the , after the sentinel entry.
Quoted 21 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +21
-0+}; +MODULE_DEVICE_TABLE (of, sg2042_pwm_match); + +static int pwm_sg2042_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct sg2042_pwm_chip *sg2042_pwm; +struct pwm_chip *chip; +int ret; + + chip =devm_pwmchip_alloc (&pdev->dev,SG2042_PWM_CHANNELNUM ,sizeof (*sg2042_pwm)); +if (IS_ERR (chip)) +return PTR_ERR (chip); + sg2042_pwm =to_sg2042_pwm_chip (chip); + + chip->ops = &pwm_sg2042_ops; + + sg2042_pwm->base =devm_platform_ioremap_resource (pdev,0 ); +if (IS_ERR (sg2042_pwm->base)) +return PTR_ERR (sg2042_pwm->base); +
drivers/pwm/pwm-sophgo-sg2042.c line 132
Quoted 10 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +10
-0+ sg2042_pwm->base_clk =devm_clk_get_enabled (&pdev->dev,"apb" ); +if (IS_ERR (sg2042_pwm->base_clk)) +return dev_err_probe (dev,PTR_ERR (sg2042_pwm->base_clk), +"failed to get base clk\n" ); + + ret =devm_pwmchip_add (&pdev->dev, chip); +if (ret <0 ) +return dev_err_probe (dev, ret,"failed to register PWM chip\n" ); + +platform_set_drvdata (pdev, chip);
This is unused and should/can be dropped.
drivers/pwm/pwm-sophgo-sg2042.c line 144
Quoted 12 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +12
-0+ +return 0 ; +} + +static struct platform_driver pwm_sg2042_driver = { + .driver = { + .name ="sg2042-pwm" , + .of_match_table =of_match_ptr (sg2042_pwm_match), + }, + .probe = pwm_sg2042_probe, +}; +module_platform_driver (pwm_sg2042_driver);
Please use a single space before =.
Quoted 6 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +3
-0+MODULE_AUTHOR ("Chen Wang" ); +MODULE_DESCRIPTION ("Sophgo SG2042 PWM driver" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.34.1
Best regards
Uwe
Re: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
Quoted 3 lines · Uwe Kleine-König
Hello,
On Thu, Sep 05, 2024 at 08:10:42PM +0800, Chen Wang wrote:
[......]
Quoted 18 lines
drivers/pwm/pwm-sophgo-sg2042.c line 57
Quoted 5 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +5
-0+static void pwm_sg2042_config (void __iomem *base,unsigned int channo,u32 period,u32 hlperiod) +{ +writel (period, base +REG_GROUP * channo +REG_PERIOD ); +writel (hlperiod, base +REG_GROUP * channo +REG_HLPERIOD ); +}I suggest to use the following instead:
#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0) #define SG2042_PERIOD(chan) ((chan) * 8 + 4) ... static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod) { writel(period, base + SG2042_PERIOD(chan)); writel(hlperiod, base + SG2042_HLPERIOD(chan)); }
Quoted 7 lines
The (subjective?) advantage is that the definition of
SG2042_HLPERIODcontains information about all channel's HLPERIOD register and the usage inpwm_sg2042_configis obviously(?) right.(Another advantage is that the register names have a prefix unique to the driver, so there is no danger of mixing it up with some other hardware's driver that might also have a register named "PERIOD".)
Agree, will fix this in next version.
Quoted 10 lines
Is this racy? i.e. can it happen that between the two writel the output is defined by the new period and old
duty_cycle?How does the hardware behave on reconfiguration? Does it complete the currently running period? Please document that in a comment at the start of the driver like many other drivers do. (See
sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c)
When hlperiod or period is modified, PWM will start to output waveforms with the new configuration after completing the running period. Will document in a comment as other drivers do.
Quoted 15 lines
drivers/pwm/pwm-sophgo-sg2042.c line 71
Quoted 13 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +13
-0+static int pwm_sg2042_apply (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state) +{ +struct sg2042_pwm_chip *sg2042_pwm =to_sg2042_pwm_chip (chip); +u32 hlperiod; +u32 period; +u64 f_clk; +u64 p; + +if (!state->enabled) { +pwm_sg2042_config (sg2042_pwm->base, pwm->hwpwm,0 ,0 ); +return 0 ; + }Here you're missing (I guess):
Quoted 2 lines
if (state->polarity == PWM_POLARITY_INVERSED) return -EINVAL;
Yes, it is required, will add this in next version, thanks.
Quoted 13 lines
drivers/pwm/pwm-sophgo-sg2042.c line 79
Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +7
-0+/* +* Period of High level (duty_cycle) = HLPERIOD x Period_clk +* Period of One Cycle (period) = PERIOD x Period_clk +*/ + f_clk =clk_get_rate (sg2042_pwm->base_clk); + + p = f_clk * state->period;This might overflow.
drivers/pwm/pwm-sophgo-sg2042.c line 81
Quoted 2 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +2
-0+do_div (p,NSEC_PER_SEC ); + period = (u32 )p;This gets very wrong if p happens to be bigger than
U32_MAX.
Quoted 8 lines
If you ensure
f_clk<=NSEC_PER_SECin .probe() (in combination with a call toclk_rate_exclusive_get()), you can do:period_cycles = min(mul_u64_u64_div_u64(f_clk, state->period, NSEC_PER_SEC), U32_MAX); duty_cycles = min(mul_u64_u64_div_u64(f_clk, state->duty_cycle, NSEC_PER_SEC), U32_MAX);This would also allow to store the clkrate in the driver private struct and drop the pointer to the clk from there.
f_clk should be 100M and <=NSEC_PER_SEC, will improve the code as your suggestion, thanks.
Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c line 88
Quoted 6 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +6
-0+ p = f_clk * state->duty_cycle; +do_div (p,NSEC_PER_SEC ); + hlperiod = (u32 )p; + +dev_dbg (pwmchip_parent (chip),"chan[%d]: period=%u, hlperiod=%u\n" , + pwm->hwpwm, period, hlperiod);
pwm->hwpwmis an unsigned int, so use %u for it.
Ok, will fix.
Quoted 8 lines
drivers/pwm/pwm-sophgo-sg2042.c line 96
Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +7
-0+pwm_sg2042_config (sg2042_pwm->base, pwm->hwpwm, period, hlperiod); + +return 0 ; +} + +static const struct pwm_ops pwm_sg2042_ops = { + .apply = pwm_sg2042_apply,No .get_state() possible? Please use a single space before =.
Will add .get_state() and improve the format.
Quoted 6 lines
drivers/pwm/pwm-sophgo-sg2042.c line 101
Quoted 5 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +5
-0+}; + +static const struct of_device_id sg2042_pwm_match[] = { + { .compatible ="sophgo,sg2042-pwm" }, + { },Please drop the , after the sentinel entry.
OK
Quoted 24 lines
Quoted 24 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +24
-0+}; +MODULE_DEVICE_TABLE (of, sg2042_pwm_match); + +static int pwm_sg2042_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct sg2042_pwm_chip *sg2042_pwm; +struct pwm_chip *chip; +int ret; + + chip =devm_pwmchip_alloc (&pdev->dev,SG2042_PWM_CHANNELNUM ,sizeof (*sg2042_pwm)); +if (IS_ERR (chip)) +return PTR_ERR (chip); + sg2042_pwm =to_sg2042_pwm_chip (chip); + + chip->ops = &pwm_sg2042_ops; + + sg2042_pwm->base =devm_platform_ioremap_resource (pdev,0 ); +if (IS_ERR (sg2042_pwm->base)) +return PTR_ERR (sg2042_pwm->base); + + sg2042_pwm->base_clk =devm_clk_get_enabled (&pdev->dev,"apb" ); +if (IS_ERR (sg2042_pwm->base_clk)) + return dev_err_probe(dev, PTR_ERR(sg2042_pwm->base_clk),
Quoted 8 lines
drivers/pwm/pwm-sophgo-sg2042.c line 132
Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +7
-0+ "failed to get base clk\n"); + + ret =devm_pwmchip_add (&pdev->dev, chip); +if (ret <0 ) +return dev_err_probe (dev, ret,"failed to register PWM chip\n" ); + +platform_set_drvdata (pdev, chip);This is unused and should/can be dropped.
OK, will drop this, thanks for point it out. I should have cleaned it up.
Quoted 5 lines
Quoted 5 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +5
-0+ +return 0 ; +} + +static struct platform_driver pwm_sg2042_driver = {
Quoted 8 lines
drivers/pwm/pwm-sophgo-sg2042.c line 144
Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +7
-0+ .driver = { + .name = "sg2042-pwm", + .of_match_table = of_match_ptr(sg2042_pwm_match), + }, + .probe = pwm_sg2042_probe, +}; +module_platform_driver (pwm_sg2042_driver);Please use a single space before =.
Ok, will improve the format.
Quoted 8 lines
Quoted 6 lines
drivers/pwm/pwm-sophgo-sg2042.c new file +3
-0+MODULE_AUTHOR ("Chen Wang" ); +MODULE_DESCRIPTION ("Sophgo SG2042 PWM driver" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.34.1Best regards
Uwe
kernel test robot
build botRe: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
In reply to Chen Wangkernel test robot noticed the following build warnings:
Hi Chen,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6]
url: https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303 base: 431c1646e1f86b949fa3685efc50b660a364c2b6 patch link: https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/[email protected]/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce: (https://download.01.org/0day-ci/archive/20240908/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=]
99 | static const struct of_device_id sg2042_pwm_match[] = {
| ^~~~~~~~~~~~~~~~vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c
98
> 99 static const struct of_device_id sg2042_pwm_match[] = { 100 { .compatible = "sophgo,sg2042-pwm" },
101 { },
102 };
103 MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
104
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
hi,
I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove this.
Regards,
Chen
Quoted 36 lines · kernel test robot
Hi Chen,
kernel test robot noticed the following build warnings:
[auto build test WARNING on
431c1646e1f86b949fa3685efc50b660a364c2b6]url: https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303 base: 431c1646e1f86b949fa3685efc50b660a364c2b6 patch link: https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/[email protected]/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce: (https://download.01.org/0day-ci/archive/20240908/[email protected]/reproduce)If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/All warnings (new ones prefixed by >>):
Quoted 1 line
Quoted 1 line
drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=]99 | static const struct of_device_id sg2042_pwm_match[] = { | ^~~~~~~~~~~~~~~~vim +/sg2042_pwm_match +99
drivers/pwm/pwm-sophgo-sg2042.c98Quoted 1 line
99 static const struct
of_device_idsg2042_pwm_match[] = {100 { .compatible = "sophgo,sg2042-pwm" }, 101 { }, 102 }; 103 MODULE_DEVICE_TABLE(of, sg2042_pwm_match); 104
Re: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
Hi Chen,
Quoted 2 lines · Chen Wang
I wonder why
CONFIG_PWM_SOPHGO_SG2042is enabeld for m68k? Please remove this.
Because it depends on ARCH_SOPHGO || COMPILE_TEST.
So it can be enabled on all architectures when compile-testing.
Quoted 27 lines · kernel test robot
Quoted 26 lines
kernel test robot noticed the following build warnings:
[auto build test WARNING on
431c1646e1f86b949fa3685efc50b660a364c2b6]url: https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303 base: 431c1646e1f86b949fa3685efc50b660a364c2b6 patch link: https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/[email protected]/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce: (https://download.01.org/0day-ci/archive/20240908/[email protected]/reproduce)If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/All warnings (new ones prefixed by >>):
Quoted 1 line
Quoted 1 line
drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=]99 | static const struct of_device_id sg2042_pwm_match[] = { | ^~~~~~~~~~~~~~~~vim +/sg2042_pwm_match +99
drivers/pwm/pwm-sophgo-sg2042.c
Quoted 7 lines
Quoted 7 lines
98Quoted 1 line
99 static const struct
of_device_idsg2042_pwm_match[] = {100 { .compatible = "sophgo,sg2042-pwm" }, 101 { }, 102 }; 103 MODULE_DEVICE_TABLE(of, sg2042_pwm_match); 104
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
Quoted 7 lines · Geert Uytterhoeven
Hi Chen,
Quoted 2 lines · Chen Wang
I wonder why
CONFIG_PWM_SOPHGO_SG2042is enabeld for m68k? Please remove this.Because it depends on
ARCH_SOPHGO||COMPILE_TEST.
So it can be enabled on all architectures when compile-testing.
Thanks, it's really a potential defect when CONFIG_OF is not set, will fix this in next version.
Quoted 38 lines
Quoted 27 lines · kernel test robot
Quoted 26 lines
kernel test robot noticed the following build warnings:
[auto build test WARNING on
431c1646e1f86b949fa3685efc50b660a364c2b6]url: https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303 base: 431c1646e1f86b949fa3685efc50b660a364c2b6 patch link: https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/[email protected]/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce: (https://download.01.org/0day-ci/archive/20240908/[email protected]/reproduce)If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/All warnings (new ones prefixed by >>):
Quoted 1 line
Quoted 1 line
drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=]99 | static const struct of_device_id sg2042_pwm_match[] = { | ^~~~~~~~~~~~~~~~vim +/sg2042_pwm_match +99
drivers/pwm/pwm-sophgo-sg2042.cQuoted 7 lines
Quoted 7 lines
98 > 99 static const struct of_device_id sg2042_pwm_match[] = { 100 { .compatible = "sophgo,sg2042-pwm" }, 101 { }, 102 }; 103 MODULE_DEVICE_TABLE(of, sg2042_pwm_match); 104Gr{oetje,eeting}s,
Geert
Re: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
Hello Chen,
Quoted 8 lines · Chen Wang
Quoted 5 lines · Geert Uytterhoeven
Quoted 2 lines · Chen Wang
I wonder why
CONFIG_PWM_SOPHGO_SG2042is enabeld for m68k? Please remove this.Because it depends on
ARCH_SOPHGO||COMPILE_TEST.
So it can be enabled on all architectures when compile-testing.Thanks, it's really a potential defect when
CONFIG_OFis not set, will fix this in next version.
BTW, the right approach to fix that is to not use of_match_ptr when initializing pwm_sg2042_driver.driver.of_match_table.
Best regards
Uwe
Re: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
Quoted 9 lines · Uwe Kleine-König
Hello Chen,
Quoted 6 lines · Chen Wang
Quoted 5 lines · Geert Uytterhoeven
Quoted 2 lines · Chen Wang
I wonder why
CONFIG_PWM_SOPHGO_SG2042is enabeld for m68k? Please remove this.Because it depends on
ARCH_SOPHGO||COMPILE_TEST.
So it can be enabled on all architectures when compile-testing.
Quoted 7 lines
Quoted 2 lines
Thanks, it's really a potential defect when
CONFIG_OFis not set, will fix this in next version.BTW, the right approach to fix that is to not use
of_match_ptrwhen initializing pwm_sg2042_driver.driver.of_match_table.Best regards
Uwe
Thank you Uwe, that's what I said potential defect, have fixed here.
Thanks,
Chen