pwm: add driver for T-THEAD TH1520 SoC
Subsystems (3)
cover pwm: add driver for T-THEAD TH1520 SoC
T-HEAD SoCs such as the TH1520 contain a PWM controller used among other things to control the LCD backlight, fan and so on. Add the PWM driver support for it.
Since the clk part isn't mainlined, so SoC dts(i) changes are not included in this series. However, it can be tested by using fixed-clock.
Jisheng Zhang (2):
dt-bindings: pwm: Add T-HEAD PWM controller
pwm: add T-HEAD PWM driver
.../devicetree/bindings/pwm/pwm-thead.yaml | 44 +++ MAINTAINERS | 1 + drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-thead.c | 289 ++++++++++++++++++ 5 files changed, 346 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-thead.yaml create mode 100644 drivers/pwm/pwm-thead.c
Signature · 1 line
2.40.1
PATCH 1/2 dt-bindings: pwm: Add T-HEAD PWM controller
T-HEAD SoCs such as the TH1520 contain a PWM controller used among other things to control the LCD backlight, fan and so on.
Signed-off-by Jisheng Zhang <[email protected]>
--- .../devicetree/bindings/pwm/pwm-thead.yaml | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-thead.yaml
Documentation/devicetree/bindings/pwm/pwm-thead.yaml new file +44 -0
@@ -0,0 +1,44 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/pwm-thead.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: T-HEAD PWM + +maintainers: + - Jisheng Zhang <[email protected]> + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + enum: + - thead,th1520-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + "#pwm-cells": + const: 2 + +required: + - compatible + - reg + - clocks + +additionalProperties: false + +examples: + - | + + pwm@ec01c000 { + compatible = "thead,th1520-pwm"; + reg = <0xec01c000 0x1000>; + clocks = <&clk 1>; + #pwm-cells = <2>; + };
Signature · 1 line
2.40.1
PATCH 2/2 pwm: add T-HEAD PWM driver
In reply to Jisheng ZhangT-HEAD SoCs such as the TH1520 contain a PWM controller used among other things to control the LCD backlight, fan and so on. Add driver for it.
Signed-off-by Jisheng Zhang <[email protected]>
--- MAINTAINERS | 1 + drivers/pwm/Kconfig | 11 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-thead.c | 289 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 302 insertions(+) create mode 100644 drivers/pwm/pwm-thead.c
MAINTAINERS +1 -0
@@ -18482,6 +18482,7 @@ L: [email protected] S: Maintained F: arch/riscv/boot/dts/thead/ F: drivers/usb/dwc3/dwc3-thead.c +F: drivers/pwm/pwm-thead.c RNBD BLOCK DRIVERS M: Md. Haris Iqbal <[email protected]>
drivers/pwm/Kconfig +11 -0
@@ -637,6 +637,17 @@ config PWM_TEGRA To compile this driver as a module, choose M here: the module will be called pwm-tegra. +config PWM_THEAD + tristate "T-HEAD PWM support" + depends on ARCH_THEAD || COMPILE_TEST + depends on HAS_IOMEM + help + Generic PWM framework driver for the PWFM controller found on THEAD + SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-thead. + config PWM_TIECAP tristate "ECAP PWM support" depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
drivers/pwm/Makefile +1 -0
@@ -59,6 +59,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o +obj-$(CONFIG_PWM_THEAD) += pwm-thead.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o obj-$(CONFIG_PWM_TWL) += pwm-twl.o
drivers/pwm/pwm-thead.c new file +289 -0
289 lines not on this page · raw message
Signature · 1 line
2.40.1
Re: [PATCH 2/2] pwm: add T-HEAD PWM driver
Quoted 97 lines · Jisheng Zhang
T-HEAD SoCs such as the TH1520 contain a PWM controller used among other things to control the LCD backlight, fan and so on. Add driver for it.
Signed-off-by Jisheng Zhang <[email protected]>--- MAINTAINERS | 1 + drivers/pwm/Kconfig | 11 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-thead.c | 289 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 302 insertions(+) create mode 100644 drivers/pwm/pwm-thead.cMAINTAINERS +1
-0@@ -18482,6 +18482,7 @@ L: [email protected] S: Maintained F: arch/riscv/boot/dts/thead/ F: drivers/usb/dwc3/dwc3-thead.c +F: drivers/pwm/pwm-thead.c RNBD BLOCK DRIVERS M: Md. Haris Iqbal <[email protected]>drivers/pwm/Kconfig +11
-0@@ -637,6 +637,17 @@ config PWM_TEGRA To compile this driver as a module, choose M here: the module will be called pwm-tegra. +config PWM_THEAD + tristate "T-HEAD PWM support" + depends on ARCH_THEAD || COMPILE_TEST + depends on HAS_IOMEM + help + Generic PWM framework driver for the PWFM controller found on THEAD + SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-thead. + config PWM_TIECAP tristate "ECAP PWM support" depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TESTdrivers/pwm/Makefile +1
-0@@ -59,6 +59,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o +obj-$(CONFIG_PWM_THEAD) += pwm-thead.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o obj-$(CONFIG_PWM_TWL) += pwm-twl.odrivers/pwm/pwm-thead.c new file +32
-0@@ -0,0 +1,289 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* T-HEAD PWM driver +* +* Copyright (C) 2021 Alibaba Group Holding Limited. +*/ + +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/pwm.h> +#include <linux/slab.h> + +#define MAX_PWM_NUM 6 + +#define LIGHT_PWM_CHN_BASE (n) ((n) * 0x20) +#define LIGHT_PWM_CTRL (n) (LIGHT_PWM_CHN_BASE(n) + 0x00) +#define LIGHT_PWM_RPT (n) (LIGHT_PWM_CHN_BASE(n) + 0x04) +#define LIGHT_PWM_PER (n) (LIGHT_PWM_CHN_BASE(n) + 0x08) +#define LIGHT_PWM_FP (n) (LIGHT_PWM_CHN_BASE(n) + 0x0c) +#define LIGHT_PWM_STATUS (n) (LIGHT_PWM_CHN_BASE(n) + 0x10) + +/* bit definition PWM_CTRL */
drivers/pwm/pwm-thead.c line 42
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+#define PWM_START BIT(0) +#define PWM_SOFT_RST BIT(1) +#define PWM_CFG_UPDATE BIT(2) +#define PWM_INT_EN BIT(3) +#define PWM_ONE_SHOT_MODE BIT(4) +#define PWM_CONTINUOUS_MODE BIT(5) +#define PWM_EVT_RISING_TRIG_UNDER_ONE_SHOT BIT(6) +#define PWM_EVT_FALLING_TRIG_UNDER_ONE_SHOT BIT(7) +#define PWM_FPOUT BIT(8) +#define PWM_INFACTOUT BIT(9)
Hi Jisheng,
I'd be worried that these defines one day clash with the PWM framework. Maybe just keep the LIGHT_PWM_ prefix?
Quoted 8 lines
drivers/pwm/pwm-thead.c new file +8
-0+ +struct thead_pwm_chip { +struct clk *clk; +void __iomem *mmio_base; +struct pwm_chip chip; +}; + +#define to_thead_pwm_chip (chip) container_of(chip, struct thead_pwm_chip, chip)
drivers/pwm/pwm-thead.c line 60
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+ +static int thead_pwm_clk_prepare_enable (struct thead_pwm_chip *pc) +{ +return clk_prepare_enable (pc->clk); +} + +static void thead_pwm_clk_disable_unprepare (struct thead_pwm_chip *pc) +{ +clk_disable_unprepare (pc->clk); +}
These two wrappers don't seem to add a lot of value compared to just writing clk_*(pc->clk) directly and they're not used as callbacks.
Quoted 46 lines
drivers/pwm/pwm-thead.c new file +46
-0+ +static int thead_pwm_enable (struct pwm_chip *chip,struct pwm_device *pwm) +{ +struct thead_pwm_chip *pc =to_thead_pwm_chip (chip); +u32 value; +int ret; + + ret =pm_runtime_get_sync (chip->dev); +if (ret <0 ) { +dev_err (chip->dev,"failed to clock on the pwm device(%d)\n" , ret); +return ret; + } + + value =readl (pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + value |=PWM_START ; +writel (value, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + +return 0 ; +} + +static void thead_pwm_disable (struct pwm_chip *chip,struct pwm_device *pwm) +{ +struct thead_pwm_chip *pc =to_thead_pwm_chip (chip); +u32 value; + + value =readl (pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + value &= ~PWM_START ; +writel (value, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + +pm_runtime_put_sync (chip->dev); +} + +static int thead_pwm_config (struct pwm_chip *chip,struct pwm_device *pwm, +int duty_ns,int period_ns) +{ +struct thead_pwm_chip *pc =to_thead_pwm_chip (chip); +unsigned long rate =clk_get_rate (pc->clk); +unsigned long duty_cycle, period_cycle; +u32 pwm_cfg =PWM_INFACTOUT |PWM_FPOUT |PWM_CONTINUOUS_MODE |PWM_INT_EN ; +int ret; + +if (duty_ns > period_ns) { +dev_err (chip->dev,"invalid pwm configure\n" ); +return -EINVAL ; + } +
drivers/pwm/pwm-thead.c line 116
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+ ret =pm_runtime_get_sync (chip->dev); +if (ret <0 ) { +dev_err (chip->dev,"failed to clock on the pwm device(%d)\n" , ret); +return ret; + } + +writel (pwm_cfg, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + + period_cycle = period_ns * rate; +do_div (period_cycle,NSEC_PER_SEC );
I thought do_div() was only needed to do 64bit by 32bit divisions on 32bit hardware, but hereperiod_cycle is an unsigned long, so only 32bit on 32bit hardware.
drivers/pwm/pwm-thead.c line 120
Quoted 4 lines
drivers/pwm/pwm-thead.c new file +4
-0+writel (period_cycle, pc->mmio_base +LIGHT_PWM_PER (pwm->hwpwm)); + + duty_cycle = duty_ns * rate; +do_div (duty_cycle,NSEC_PER_SEC );
Same as above.
Quoted 98 lines
drivers/pwm/pwm-thead.c new file +98
-0+writel (duty_cycle, pc->mmio_base +LIGHT_PWM_FP (pwm->hwpwm)); + + pwm_cfg =readl (pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + pwm_cfg |=PWM_CFG_UPDATE ; +writel (pwm_cfg, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + +pm_runtime_put_sync (chip->dev); + +return 0 ; +} + +static int thead_pwm_set_polarity (struct pwm_chip *chip, +struct pwm_device *pwm, +enum pwm_polarity polarity) +{ +struct thead_pwm_chip *pc =to_thead_pwm_chip (chip); +u32 value =readl (pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); +int ret; + + ret =pm_runtime_get_sync (chip->dev); +if (ret <0 ) { +dev_err (chip->dev,"failed to clock on the pwm device(%d)\n" , ret); +return ret; + } + +if (polarity ==PWM_POLARITY_NORMAL ) + value |=PWM_FPOUT ; +else + value &= ~PWM_FPOUT ; + +writel (value, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + +pm_runtime_put_sync (chip->dev); + +return 0 ; +} + +static int thead_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state) +{ +int err; +bool enabled = pwm->state.enabled; + +if (state->polarity != pwm->state.polarity) +thead_pwm_set_polarity (chip, pwm, state->polarity); + +if (!state->enabled) { +if (enabled) +thead_pwm_disable (chip, pwm); +return 0 ; + } + + err =thead_pwm_config (chip, pwm, state->duty_cycle, state->period); +if (err) +return err; + +if (!enabled) +return thead_pwm_enable (chip, pwm); + +return 0 ; +} + +static const struct pwm_ops thead_pwm_ops = { + .apply = thead_pwm_apply, + .owner =THIS_MODULE , +}; + +static int __maybe_unused thead_pwm_runtime_suspend (struct device *dev) +{ +struct thead_pwm_chip *pc =dev_get_drvdata (dev); + +thead_pwm_clk_disable_unprepare (pc); + +return 0 ; +} + +static int __maybe_unused thead_pwm_runtime_resume (struct device *dev) +{ +struct thead_pwm_chip *pc =dev_get_drvdata (dev); +int ret; + + ret =thead_pwm_clk_prepare_enable (pc); +if (ret) { +dev_err (dev,"failed to enable pwm clock(%d)\n" , ret); +return ret; + } + +return 0 ; +} + +static int thead_pwm_probe (struct platform_device *pdev) +{ +struct thead_pwm_chip *pc; +int ret; + + pc =devm_kzalloc (&pdev->dev,sizeof (*pc),GFP_KERNEL ); +if (!pc) +return -ENOMEM ;
drivers/pwm/pwm-thead.c line 228
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+ +platform_set_drvdata (pdev, pc); + + pc->mmio_base =devm_platform_ioremap_resource (pdev,0 ); +if (IS_ERR (pc->mmio_base)) +return PTR_ERR (pc->mmio_base); + + pc->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (pc->clk)) +return PTR_ERR (pc->clk);
A lot of other drivers have something like
return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk), "failed to get clock\n"); when failing to get clocks.
Quoted 15 lines
drivers/pwm/pwm-thead.c new file +15
-0+ +pm_runtime_enable (&pdev->dev); +pm_runtime_get_noresume (&pdev->dev); + ret =thead_pwm_clk_prepare_enable (pc); +if (ret) { +dev_err (&pdev->dev,"failed to enable pwm clock(%d)\n" , ret); + goto err_pm_disable; + } + + pc->chip.ops = &thead_pwm_ops; + pc->chip.dev = &pdev->dev; + pc->chip.npwm =MAX_PWM_NUM ; + + ret =pwmchip_add (&pc->chip); +if (ret)
drivers/pwm/pwm-thead.c line 253
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+ goto err_clk_disable; + +pm_runtime_put (&pdev->dev); + +return 0 ; + +err_clk_disable: +thead_pwm_clk_disable_unprepare (pc); +err_pm_disable: +pm_runtime_disable (&pdev->dev);
Here you disable the clock and then pm_runtime_disable()..
drivers/pwm/pwm-thead.c line 262
Quoted 9 lines
drivers/pwm/pwm-thead.c new file +9
-0+return ret; +} + +static void thead_pwm_remove (struct platform_device *pdev) +{ +struct thead_pwm_chip *pc =platform_get_drvdata (pdev); + +pm_runtime_disable (&pdev->dev); +thead_pwm_clk_disable_unprepare (pc);
..but here it's the other way around. It may not make a difference, but let's be consistent anyway.
Quoted 14 lines
drivers/pwm/pwm-thead.c new file +14
-0+pwmchip_remove (&pc->chip); +} + +static const struct of_device_id thead_pwm_dt_ids[] = { + {.compatible ="thead,th1520-pwm" ,}, + {/* sentinel */ } +}; + +static const struct dev_pm_ops thead_pwm_pm_ops = { +SET_RUNTIME_PM_OPS (thead_pwm_runtime_suspend, thead_pwm_runtime_resume,NULL ) +}; + +static struct platform_driver thead_pwm_driver = { + .driver = {
drivers/pwm/pwm-thead.c line 286
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+ .name = "thead-pwm", + .of_match_table = thead_pwm_dt_ids, + .pm = &thead_pwm_pm_ops, + }, + .probe = thead_pwm_probe, + .remove_new = thead_pwm_remove, +}; +module_platform_driver (thead_pwm_driver); + +MODULE_AUTHOR ("wei.liu <[email protected]>" );
wei.liu is a very uncommon name, do you mean Wei Liu?
Quoted 11 lines
drivers/pwm/pwm-thead.c new file +3
-1+MODULE_AUTHOR ("Jisheng Zhang <[email protected]>" ); +MODULE_DESCRIPTION ("T-HEAD pwm driver" ); +MODULE_LICENSE ("GPL v2" ); --2.40.1
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH 1/2] dt-bindings: pwm: Add T-HEAD PWM controller
In reply to Jisheng ZhangHey,
Quoted 2 lines · Jisheng Zhang
T-HEAD SoCs such as the TH1520 contain a PWM controller used among other things to control the LCD backlight, fan and so on.
"such as", are there others?
Quoted 13 lines
Signed-off-by Jisheng Zhang <[email protected]>--- .../devicetree/bindings/pwm/pwm-thead.yaml | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-thead.yamlDocumentation/devicetree/bindings/pwm/pwm-thead.yaml new file +0
-0
Documentation/devicetree/bindings/pwm/pwm-thead.yaml line 4
Quoted 4 lines
Documentation/devicetree/bindings/pwm/pwm-thead.yaml new file +4
-0@@ -0,0 +1,44 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/pwm-thead.yaml#
The filename etc should match the compatible, especially when afaict there's only one applicable SoC.
Otherwise, this looks good to me.
Thanks,
Conor.
Quoted 43 lines
Documentation/devicetree/bindings/pwm/pwm-thead.yaml new file +40
-0+$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: T-HEAD PWM + +maintainers: + - Jisheng Zhang <[email protected]> + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + enum: + - thead,th1520-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + "#pwm-cells": + const: 2 + +required: + - compatible + - reg + - clocks + +additionalProperties: false + +examples: + - | + + pwm@ec01c000 { + compatible = "thead,th1520-pwm"; + reg = <0xec01c000 0x1000>; + clocks = <&clk 1>; + #pwm-cells = <2>; + };Signature · 1 line
2.40.1
Re: [PATCH 2/2] pwm: add T-HEAD PWM driver
In reply to Jisheng ZhangHello,
Quoted 65 lines · Jisheng Zhang
T-HEAD SoCs such as the TH1520 contain a PWM controller used among other things to control the LCD backlight, fan and so on. Add driver for it.
Signed-off-by Jisheng Zhang <[email protected]>--- MAINTAINERS | 1 + drivers/pwm/Kconfig | 11 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-thead.c | 289 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 302 insertions(+) create mode 100644 drivers/pwm/pwm-thead.cMAINTAINERS +1
-0@@ -18482,6 +18482,7 @@ L: [email protected] S: Maintained F: arch/riscv/boot/dts/thead/ F: drivers/usb/dwc3/dwc3-thead.c +F: drivers/pwm/pwm-thead.c RNBD BLOCK DRIVERS M: Md. Haris Iqbal <[email protected]>drivers/pwm/Kconfig +11
-0@@ -637,6 +637,17 @@ config PWM_TEGRA To compile this driver as a module, choose M here: the module will be called pwm-tegra. +config PWM_THEAD + tristate "T-HEAD PWM support" + depends on ARCH_THEAD || COMPILE_TEST + depends on HAS_IOMEM + help + Generic PWM framework driver for the PWFM controller found on THEAD + SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-thead. + config PWM_TIECAP tristate "ECAP PWM support" depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TESTdrivers/pwm/Makefile +1
-0@@ -59,6 +59,7 @@ obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o +obj-$(CONFIG_PWM_THEAD) += pwm-thead.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o obj-$(CONFIG_PWM_TWL) += pwm-twl.odrivers/pwm/pwm-thead.c new file +0
-0
drivers/pwm/pwm-thead.c line 5
Quoted 5 lines
drivers/pwm/pwm-thead.c new file +5
-0@@ -0,0 +1,289 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* T-HEAD PWM driver +* +* Copyright (C) 2021 Alibaba Group Holding Limited.
If you have a public link to a reference manual, please mention this here. Also please add a section that describes hardware (and software) limitations sticking to the format in other drivers. The idea is that
sed -rn '/Limitations:/,/\*\/?$/p'
tells you about (at least) about the following properties:
- Can the PWM be updated atomically?
- Does it complete the current running period before switching to the new configuration?
- How does the output behave when disabled?
Quoted 27 lines
drivers/pwm/pwm-thead.c new file +27
-0+*/ + +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/pwm.h> +#include <linux/slab.h> + +#define MAX_PWM_NUM 6 + +#define LIGHT_PWM_CHN_BASE (n) ((n) * 0x20) +#define LIGHT_PWM_CTRL (n) (LIGHT_PWM_CHN_BASE(n) + 0x00) +#define LIGHT_PWM_RPT (n) (LIGHT_PWM_CHN_BASE(n) + 0x04) +#define LIGHT_PWM_PER (n) (LIGHT_PWM_CHN_BASE(n) + 0x08) +#define LIGHT_PWM_FP (n) (LIGHT_PWM_CHN_BASE(n) + 0x0c) +#define LIGHT_PWM_STATUS (n) (LIGHT_PWM_CHN_BASE(n) + 0x10) + +/* bit definition PWM_CTRL */
drivers/pwm/pwm-thead.c line 42
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+#define PWM_START BIT(0) +#define PWM_SOFT_RST BIT(1) +#define PWM_CFG_UPDATE BIT(2) +#define PWM_INT_EN BIT(3) +#define PWM_ONE_SHOT_MODE BIT(4) +#define PWM_CONTINUOUS_MODE BIT(5) +#define PWM_EVT_RISING_TRIG_UNDER_ONE_SHOT BIT(6) +#define PWM_EVT_FALLING_TRIG_UNDER_ONE_SHOT BIT(7) +#define PWM_FPOUT BIT(8) +#define PWM_INFACTOUT BIT(9)
Emil already criticised the naming here (Thanks btw for the review!). I also want you to use a consistent prefix, but I wonder about "LIGHT_", I would have expected "THEAD_" matching the driver name?!
drivers/pwm/pwm-thead.c line 50
Quoted 7 lines
drivers/pwm/pwm-thead.c new file +7
-0+struct thead_pwm_chip { +struct clk *clk; +void __iomem *mmio_base; +struct pwm_chip chip; +}; + +#define to_thead_pwm_chip (chip) container_of(chip, struct thead_pwm_chip, chip)
This is wrong. Try:
struct pwm_chip *mychip; struct thead_pwm_chip *pc = to_thead_pwm_chip(mychip);
. (Of course you shouldn't name a pwm_chip "mychip", "chip" is fine. Still this is too ugly.) I suggest a static inline here, ideally one that has a name starting with "thead_pwm_". thead_pwm_from_chip() comes to mind.
drivers/pwm/pwm-thead.c line 60
Quoted 9 lines
drivers/pwm/pwm-thead.c new file +9
-0+static int thead_pwm_clk_prepare_enable (struct thead_pwm_chip *pc) +{ +return clk_prepare_enable (pc->clk); +} + +static void thead_pwm_clk_disable_unprepare (struct thead_pwm_chip *pc) +{ +clk_disable_unprepare (pc->clk); +}
I agree to Emil here that these wrappers are too thin. (Also you might get rid of them completely, see below.)
drivers/pwm/pwm-thead.c line 68
Quoted 7 lines
drivers/pwm/pwm-thead.c new file +7
-0+static int thead_pwm_enable (struct pwm_chip *chip,struct pwm_device *pwm) +{ +struct thead_pwm_chip *pc =to_thead_pwm_chip (chip); +u32 value; +int ret; + + ret =pm_runtime_get_sync (chip->dev);
This function is typically called directly after thead_pwm_config() which ended with calling pm_runtime_put_sync(). If you put the logic of .apply() in a single function (or move runtime pm handling in there) you can prevent some put/get ping-pong.
drivers/pwm/pwm-thead.c line 67
Quoted 2 lines
drivers/pwm/pwm-thead.c new file +2
-0+if (ret <0 ) { +dev_err (chip->dev,"failed to clock on the pwm device(%d)\n" , ret);
Please use %pe for better readable error messages.
Quoted 24 lines
drivers/pwm/pwm-thead.c new file +24
-0+return ret; + } + + value =readl (pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + value |=PWM_START ; +writel (value, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + +return 0 ; +} + +static void thead_pwm_disable (struct pwm_chip *chip,struct pwm_device *pwm) +{ +struct thead_pwm_chip *pc =to_thead_pwm_chip (chip); +u32 value; + + value =readl (pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + value &= ~PWM_START ; +writel (value, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + +pm_runtime_put_sync (chip->dev); +} + +static int thead_pwm_config (struct pwm_chip *chip,struct pwm_device *pwm, +int duty_ns,int period_ns)
drivers/pwm/pwm-thead.c line 104
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+{ +struct thead_pwm_chip *pc =to_thead_pwm_chip (chip); +unsigned long rate =clk_get_rate (pc->clk); +unsigned long duty_cycle, period_cycle; +u32 pwm_cfg =PWM_INFACTOUT |PWM_FPOUT |PWM_CONTINUOUS_MODE |PWM_INT_EN ; +int ret; + +if (duty_ns > period_ns) { +dev_err (chip->dev,"invalid pwm configure\n" ); +return -EINVAL ;
You can assume that won't happen (once you fixed the implicit type cast in the call to thead_pwm_config pointed out below).
drivers/pwm/pwm-thead.c line 115
Quoted 11 lines
drivers/pwm/pwm-thead.c new file +11
-0+ } + + ret =pm_runtime_get_sync (chip->dev); +if (ret <0 ) { +dev_err (chip->dev,"failed to clock on the pwm device(%d)\n" , ret); +return ret; + } + +writel (pwm_cfg, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + + period_cycle = period_ns * rate;
This might overflow. Please use mul_u64_u64_div_u64 here together with making sure that rate <= NSEC_PER_SEC.
drivers/pwm/pwm-thead.c line 125
Quoted 9 lines
drivers/pwm/pwm-thead.c new file +9
-0+do_div (period_cycle,NSEC_PER_SEC ); +writel (period_cycle, pc->mmio_base +LIGHT_PWM_PER (pwm->hwpwm)); + duty_cycle = duty_ns * rate; +do_div (duty_cycle,NSEC_PER_SEC ); +writel (duty_cycle, pc->mmio_base +LIGHT_PWM_FP (pwm->hwpwm)); + + pwm_cfg =readl (pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); + pwm_cfg |=PWM_CFG_UPDATE ; +writel (pwm_cfg, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm));
I assume this means period and duty_cycle are updated atomically (i.e. at no time the hardware is configured to emit a wave that has the new period but the old duty_cycle)?
Is this needed even if the PWM is currently off and you write PWM_START a moment later? Can writing PWM_CFG_UPDATE and PWM_START be done in a single step?
Quoted 6 lines
drivers/pwm/pwm-thead.c new file +6
-0+pm_runtime_put_sync (chip->dev); + +return 0 ; +} + +static int thead_pwm_set_polarity(struct pwm_chip *chip,
drivers/pwm/pwm-thead.c line 142
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+struct pwm_device *pwm, + enum pwm_polarity polarity) +{ + struct thead_pwm_chip *pc = to_thead_pwm_chip(chip); +u32 value =readl (pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm)); +int ret; + + ret =pm_runtime_get_sync (chip->dev); +if (ret <0 ) { +dev_err (chip->dev,"failed to clock on the pwm device(%d)\n" , ret);
Please don't make .apply() emit an error message.
drivers/pwm/pwm-thead.c line 151
Quoted 9 lines
drivers/pwm/pwm-thead.c new file +9
-0+return ret; + } + +if (polarity ==PWM_POLARITY_NORMAL ) + value |=PWM_FPOUT ; +else + value &= ~PWM_FPOUT ; + +writel (value, pc->mmio_base +LIGHT_PWM_CTRL (pwm->hwpwm));
Oh, no PWM_CFG_UPDATE here? So writing the polarity takes effect immediately? If so, please note this is the Limitations section I asked for above.
Quoted 11 lines
drivers/pwm/pwm-thead.c new file +11
-0+pm_runtime_put_sync (chip->dev); + +return 0 ; +} + +static int thead_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state) +{ +int err; +bool enabled = pwm->state.enabled; +
drivers/pwm/pwm-thead.c line 173
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+if (state->polarity != pwm->state.polarity) +thead_pwm_set_polarity (chip, pwm, state->polarity); + +if (!state->enabled) { +if (enabled) +thead_pwm_disable (chip, pwm); +return 0 ; + } + + err =thead_pwm_config (chip, pwm, state->duty_cycle, state->period);
state->duty_cycle is an u64, but thead_pwm_config takes an int ...
drivers/pwm/pwm-thead.c line 184
Quoted 11 lines
drivers/pwm/pwm-thead.c new file +11
-0+if (err) +return err; + +if (!enabled) +return thead_pwm_enable (chip, pwm); + +return 0 ; +} + +static const struct pwm_ops thead_pwm_ops = { + .apply = thead_pwm_apply,
Please implement .get_state() and test with PWM_DEBUG enabled.
Quoted 32 lines
drivers/pwm/pwm-thead.c new file +32
-0+ .owner = THIS_MODULE, +}; + +static int __maybe_unused thead_pwm_runtime_suspend (struct device *dev) +{ +struct thead_pwm_chip *pc =dev_get_drvdata (dev); + +thead_pwm_clk_disable_unprepare (pc); + +return 0 ; +} + +static int __maybe_unused thead_pwm_runtime_resume (struct device *dev) +{ +struct thead_pwm_chip *pc =dev_get_drvdata (dev); +int ret; + + ret =thead_pwm_clk_prepare_enable (pc); +if (ret) { +dev_err (dev,"failed to enable pwm clock(%d)\n" , ret); +return ret; + } + +return 0 ; +} + +static int thead_pwm_probe (struct platform_device *pdev) +{ +struct thead_pwm_chip *pc; +int ret; + + pc =devm_kzalloc (&pdev->dev,sizeof (*pc),GFP_KERNEL );
drivers/pwm/pwm-thead.c line 226
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+if (!pc) +return -ENOMEM ; + +platform_set_drvdata (pdev, pc); + + pc->mmio_base =devm_platform_ioremap_resource (pdev,0 ); +if (IS_ERR (pc->mmio_base)) +return PTR_ERR (pc->mmio_base); + + pc->clk =devm_clk_get (&pdev->dev,NULL );
devm_clk_get_enabled()
Quoted 6 lines
drivers/pwm/pwm-thead.c new file +6
-0+if (IS_ERR (pc->clk)) +return PTR_ERR (pc->clk); + +pm_runtime_enable (&pdev->dev); +pm_runtime_get_noresume (&pdev->dev); + ret =thead_pwm_clk_prepare_enable (pc);
drivers/pwm/pwm-thead.c line 242
Quoted 10 lines
drivers/pwm/pwm-thead.c new file +10
-0+if (ret) { +dev_err (&pdev->dev,"failed to enable pwm clock(%d)\n" , ret); + goto err_pm_disable; + } + + pc->chip.ops = &thead_pwm_ops; + pc->chip.dev = &pdev->dev; + pc->chip.npwm =MAX_PWM_NUM ; + + ret =pwmchip_add (&pc->chip);
devm_pwmchip_add()
drivers/pwm/pwm-thead.c line 255
Quoted 13 lines
drivers/pwm/pwm-thead.c new file +13
-0+if (ret) + goto err_clk_disable; + +pm_runtime_put (&pdev->dev); + +return 0 ; + +err_clk_disable: +thead_pwm_clk_disable_unprepare (pc); +err_pm_disable: +pm_runtime_disable (&pdev->dev); +return ret; +}
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |