On 2023/2/28 22:34, Krzysztof Kozlowski wrote: > On 28/02/2023 10:13, William Qiu wrote: >> Add Pulse Width Modulation driver support for StarFive >> JH7110 soc. >> >> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx> >> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx> >> --- >> MAINTAINERS | 7 + >> drivers/pwm/Kconfig | 10 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-starfive-ptc.c | 256 +++++++++++++++++++++++++++++++++ >> 4 files changed, 274 insertions(+) >> create mode 100644 drivers/pwm/pwm-starfive-ptc.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index ac151975d0d3..05b59605d864 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -19929,6 +19929,13 @@ F: drivers/pinctrl/starfive/pinctrl-starfive-jh71* >> F: include/dt-bindings/pinctrl/pinctrl-starfive-jh7100.h >> F: include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h >> >> +STARFIVE JH71X0 PWM DRIVERS >> +M: William Qiu <william.qiu@xxxxxxxxxxxxxxxx> >> +M: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx> >> +S: Supported >> +F: Documentation/devicetree/bindings/pwm/pwm-starfive.yaml >> +F: drivers/pwm/pwm-starfive-ptc.c >> + >> STARFIVE JH71X0 RESET CONTROLLER DRIVERS >> M: Emil Renner Berthing <kernel@xxxxxxxx> >> M: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index dae023d783a2..2307a0099994 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -536,6 +536,16 @@ config PWM_SPRD >> To compile this driver as a module, choose M here: the module >> will be called pwm-sprd. >> >> +config PWM_STARFIVE_PTC >> + tristate "StarFive PWM PTC support" >> + depends on OF >> + depends on COMMON_CLK >> + help >> + Generic PWM framework driver for StarFive SoCs. >> + >> + To compile this driver as a module, choose M here: the module >> + will be called pwm-starfive-ptc. >> + >> config PWM_STI >> tristate "STiH4xx PWM support" >> depends on ARCH_STI || COMPILE_TEST >> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile >> index 7bf1a29f02b8..577f69904baa 100644 >> --- a/drivers/pwm/Makefile >> +++ b/drivers/pwm/Makefile >> @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o >> obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o >> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o >> obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o >> +obj-$(CONFIG_PWM_STARFIVE_PTC) += pwm-starfive-ptc.o >> obj-$(CONFIG_PWM_STI) += pwm-sti.o >> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o >> obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o >> diff --git a/drivers/pwm/pwm-starfive-ptc.c b/drivers/pwm/pwm-starfive-ptc.c >> new file mode 100644 >> index 000000000000..58831c600168 >> --- /dev/null >> +++ b/drivers/pwm/pwm-starfive-ptc.c >> @@ -0,0 +1,256 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PWM driver for the StarFive JH7110 SoC >> + * >> + * Copyright (C) 2018 StarFive Technology Co., Ltd. >> + */ >> + >> +#include <dt-bindings/pwm/pwm.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/pwm.h> >> +#include <linux/slab.h> >> +#include <linux/clk.h> >> +#include <linux/reset.h> >> +#include <linux/io.h> >> + >> +/* how many parameters can be transferred to ptc */ >> +#define OF_PWM_N_CELLS 3 >> + >> +/* PTC Register offsets */ >> +#define REG_RPTC_CNTR 0x0 >> +#define REG_RPTC_HRC 0x4 >> +#define REG_RPTC_LRC 0x8 >> +#define REG_RPTC_CTRL 0xC >> + >> +/* Bit for PWM clock */ >> +#define BIT_PWM_CLOCK_EN 31 >> + >> +/* Bit for clock gen soft reset */ >> +#define BIT_CLK_GEN_SOFT_RESET 13 >> + >> +#define NS_PER_SECOND 1000000000 >> +#define DEFAULT_FREQ_HZ 2000000 > > Drop unused defines. > Will drop. >> + >> +/* >> + * Access PTC register (cntr hrc lrc and ctrl), >> + * need to replace PWM_BASE_ADDR >> + */ >> +#define REG_PTC_BASE_ADDR_SUB(base, N) \ >> +((base) + (((N) > 3) ? (((N) % 4) * 0x10 + (1 << 15)) : ((N) * 0x10))) >> +#define REG_PTC_RPTC_CNTR(base, N) (REG_PTC_BASE_ADDR_SUB(base, N)) >> +#define REG_PTC_RPTC_HRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x4) >> +#define REG_PTC_RPTC_LRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x8) >> +#define REG_PTC_RPTC_CTRL(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0xC) >> + >> +/* PTC_RPTC_CTRL */ >> +#define PTC_EN BIT(0) >> +#define PTC_ECLK BIT(1) >> +#define PTC_NEC BIT(2) >> +#define PTC_OE BIT(3) >> +#define PTC_SIGNLE BIT(4) >> +#define PTC_INTE BIT(5) >> +#define PTC_INT BIT(6) >> +#define PTC_CNTRRST BIT(7) >> +#define PTC_CAPTE BIT(8) >> + >> +struct starfive_pwm_ptc_device { >> + struct pwm_chip chip; >> + struct clk *clk; >> + struct reset_control *rst; >> + void __iomem *regs; >> + int irq; >> + /*pwm apb clock frequency*/ > > Missing spaces. Use Linux coding style. > Will fix. >> + unsigned int approx_freq; >> +}; >> + >> +static inline struct starfive_pwm_ptc_device * >> + chip_to_starfive_ptc(struct pwm_chip *c) >> +{ >> + return container_of(c, struct starfive_pwm_ptc_device, chip); >> +} >> + > > (...) > >> +static int starfive_pwm_ptc_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct starfive_pwm_ptc_device *pwm; >> + struct pwm_chip *chip; >> + struct resource *res; >> + unsigned int clk_apb_freq; >> + int ret; >> + >> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL); >> + if (!pwm) >> + return -ENOMEM; >> + >> + chip = &pwm->chip; >> + chip->dev = dev; >> + chip->ops = &starfive_pwm_ptc_ops; >> + chip->npwm = 8; >> + >> + chip->of_pwm_n_cells = OF_PWM_N_CELLS; >> + chip->base = -1; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + pwm->regs = devm_ioremap_resource(dev, res); > > Combine these two, there is a helper for it. > Will update. >> + if (IS_ERR(pwm->regs)) { >> + dev_err(dev, "Unable to map IO resources\n"); > > return dev_err_probe(), everywhere probably. > Will fix. >> + return PTR_ERR(pwm->regs); >> + } >> + >> + pwm->clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(pwm->clk)) { >> + dev_err(dev, "Unable to get pwm clock\n"); >> + return PTR_ERR(pwm->clk); >> + } >> + >> + pwm->rst = devm_reset_control_get_exclusive(dev, NULL); >> + if (IS_ERR(pwm->rst)) { >> + dev_err(dev, "Unable to get pwm reset\n"); >> + return PTR_ERR(pwm->rst); >> + } >> + >> + ret = clk_prepare_enable(pwm->clk); >> + if (ret) { >> + dev_err(dev, >> + "Failed to enable pwm clock, %d\n", ret); >> + return ret; >> + } >> + >> + reset_control_deassert(pwm->rst); >> + >> + clk_apb_freq = (unsigned int)clk_get_rate(pwm->clk); > > Why do you need this local variable? And why the cast? > Will drop this local variable. >> + if (!clk_apb_freq) >> + dev_warn(dev, >> + "get pwm apb clock rate failed.\n"); > > and pwm->approx_freq stays 0 which you later use for dividing. Did you > actually test it? It should produce big splat... > Will update. >> + else >> + pwm->approx_freq = clk_apb_freq; >> + >> + ret = pwmchip_add(chip); > > devm > Will update. >> + if (ret < 0) { >> + dev_err(dev, "cannot register PTC: %d\n", ret); >> + clk_disable_unprepare(pwm->clk); >> + return ret; >> + } >> + >> + platform_set_drvdata(pdev, pwm); >> + >> + return 0; >> +} >> + >> +static int starfive_pwm_ptc_remove(struct platform_device *dev) >> +{ >> + struct starfive_pwm_ptc_device *pwm = platform_get_drvdata(dev); >> + struct pwm_chip *chip = &pwm->chip; >> + >> + pwmchip_remove(chip); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id starfive_pwm_ptc_of_match[] = { >> + { .compatible = "starfive,jh7110-pwm" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, starfive_pwm_ptc_of_match); >> + >> +static struct platform_driver starfive_pwm_ptc_driver = { >> + .probe = starfive_pwm_ptc_probe, >> + .remove = starfive_pwm_ptc_remove, >> + .driver = { >> + .name = "pwm-starfive-ptc", >> + .of_match_table = of_match_ptr(starfive_pwm_ptc_of_match), > > of_match_ptr goes with maybe_unused, which you do not have. Anyway I am > not sure what's the benefit of having it here, so just drop it. > Will drop. Thank you for spending time reviewing and providing helpful comments for this driver Best regards William > Best regards, > Krzysztof >