diff mbox series

[v5,2/4] pwm: starfive: Add PWM driver support

Message ID 20230922092848.72664-3-william.qiu@starfivetech.com
State New
Headers
Series StarFive's Pulse Width Modulation driver support |

Commit Message

William Qiu Sept. 22, 2023, 9:28 a.m. UTC 
  Add Pulse Width Modulation driver support for StarFive
JH7100 and JH7110 SoC.

Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 MAINTAINERS                |   7 ++
 drivers/pwm/Kconfig        |   9 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-starfive.c | 190 +++++++++++++++++++++++++++++++++++++
 4 files changed, 207 insertions(+)
 create mode 100644 drivers/pwm/pwm-starfive.c

Comments

Emil Renner Berthing Sept. 23, 2023, 12:08 p.m. UTC | #1 
William Qiu wrote:
> Add Pulse Width Modulation driver support for StarFive
> JH7100 and JH7110 SoC.
>
> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  MAINTAINERS                |   7 ++
>  drivers/pwm/Kconfig        |   9 ++
>  drivers/pwm/Makefile       |   1 +
>  drivers/pwm/pwm-starfive.c | 190 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 207 insertions(+)
>  create mode 100644 drivers/pwm/pwm-starfive.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bf0f54c24f81..bc2155bd2712 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20495,6 +20495,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@starfivetech.com>
> +M:	Hal Feng <hal.feng@starfivetech.com>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/pwm/starfive,jh7100-pwm.yaml
> +F:	drivers/pwm/pwm-starfive-ptc.c
> +
>  STARFIVE JH71X0 RESET CONTROLLER DRIVERS
>  M:	Emil Renner Berthing <kernel@esmil.dk>
>  M:	Hal Feng <hal.feng@starfivetech.com>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8ebcddf91f7b..e2ee0169f6e4 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -569,6 +569,15 @@ config PWM_SPRD
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sprd.
>
> +config PWM_STARFIVE
> +	tristate "StarFive PWM support"
> +	depends on ARCH_STARFIVE || COMPILE_TEST
> +	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.
> +
>  config PWM_STI
>  	tristate "STiH4xx PWM support"
>  	depends on ARCH_STI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c822389c2a24..93b954376873 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -52,6 +52,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)	+= pwm-starfive.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.c b/drivers/pwm/pwm-starfive.c

Hi William,

You never answered my questions about what PTC is short for and if there are
other PWMs on the JH7110. You just removed -ptc from the name of this file..

> new file mode 100644
> index 000000000000..d390349fc95d
> --- /dev/null
> +++ b/drivers/pwm/pwm-starfive.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PWM driver for the StarFive JH71x0 SoC
> + *
> + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +/* Access PTC register (CNTR, HRC, LRC and CTRL) */
> +#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)

..but these defines

> +
> +/* PTC_RPTC_CTRL register bits*/
> +#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)

..and these defines are still prefixed with *PTC where I'd expect something like
STARFIVE_PWM_, and below structs and function names are also still
using starfive_pwm_ptc_
where I'd expect starfive_pwm_. Please be consistant in your naming.

> +struct starfive_pwm_ptc_device {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	void __iomem *regs;
> +	u32 clk_rate; /* PWM APB clock frequency */
> +};
> +
> +static inline struct starfive_pwm_ptc_device *
> +chip_to_starfive_ptc(struct pwm_chip *chip)
> +
> +{
> +	return container_of(chip, struct starfive_pwm_ptc_device, chip);
> +}
> +
> +static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
> +				      struct pwm_device *dev,
> +				      struct pwm_state *state)
> +{
> +	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
> +	u32 period_data, duty_data, ctrl_data;
> +
> +	period_data = readl(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
> +	duty_data = readl(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
> +	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
> +
> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
> +	state->polarity = PWM_POLARITY_INVERSED;
> +	state->enabled = (ctrl_data & PTC_EN) ? true : false;
> +
> +	return 0;
> +}
> +
> +static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
> +				  struct pwm_device *dev,
> +				  const struct pwm_state *state)
> +{
> +	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
> +	u32 period_data, duty_data, ctrl_data = 0;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,
> +					    NSEC_PER_SEC);
> +	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
> +					  NSEC_PER_SEC);
> +
> +	writel(period_data, REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
> +	writel(duty_data, REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
> +	writel(0,  REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm));
> +
> +	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
> +	if (state->enabled)
> +		writel(ctrl_data | PTC_EN | PTC_OE, REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
> +	else
> +		writel(ctrl_data & ~(PTC_EN | PTC_OE), REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops starfive_pwm_ptc_ops = {
> +	.get_state	= starfive_pwm_ptc_get_state,
> +	.apply		= starfive_pwm_ptc_apply,
> +	.owner		= THIS_MODULE,
> +};
> +
> +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;
> +	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 = 3;
> +
> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pwm->regs))
> +		return dev_err_probe(dev, PTR_ERR(pwm->regs),
> +				     "Unable to map IO resources\n");
> +
> +	pwm->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(pwm->clk),
> +				     "Unable to get pwm's clock\n");
> +
> +	pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(pwm->rst))
> +		return dev_err_probe(dev, PTR_ERR(pwm->rst),
> +				     "Unable to get pwm's reset\n");
> +
> +	ret = reset_control_deassert(pwm->rst);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock for pwm: %d\n", ret);
> +		return ret;
> +	}
> +
> +	pwm->clk_rate = clk_get_rate(pwm->clk);
> +	if (pwm->clk_rate <= 0) {
> +		dev_warn(dev, "Failed to get APB clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot register PTC: %d\n", ret);
> +		clk_disable_unprepare(pwm->clk);
> +		reset_control_assert(pwm->rst);
> +		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);
> +
> +	reset_control_assert(pwm->rst);
> +	clk_disable_unprepare(pwm->clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id starfive_pwm_ptc_of_match[] = {
> +	{ .compatible = "starfive,jh7100-pwm" },
> +	{ .compatible = "starfive,jh7110-pwm" },
> +	{ /* sentinel */ }
> +};
> +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",

Here

> +		.of_match_table = starfive_pwm_ptc_of_match,
> +	},
> +};
> +module_platform_driver(starfive_pwm_ptc_driver);
> +
> +MODULE_AUTHOR("Jieqin Chen");
> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
> +MODULE_DESCRIPTION("StarFive PWM PTC driver");

..and here you're also still calling the driver PTC without explaining why.

> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
William Qiu Sept. 25, 2023, 10:27 a.m. UTC | #2 
On 2023/9/23 20:08, Emil Renner Berthing wrote:
> William Qiu wrote:
>> Add Pulse Width Modulation driver support for StarFive
>> JH7100 and JH7110 SoC.
>>
>> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  MAINTAINERS                |   7 ++
>>  drivers/pwm/Kconfig        |   9 ++
>>  drivers/pwm/Makefile       |   1 +
>>  drivers/pwm/pwm-starfive.c | 190 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 207 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-starfive.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index bf0f54c24f81..bc2155bd2712 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20495,6 +20495,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@starfivetech.com>
>> +M:	Hal Feng <hal.feng@starfivetech.com>
>> +S:	Supported
>> +F:	Documentation/devicetree/bindings/pwm/starfive,jh7100-pwm.yaml
>> +F:	drivers/pwm/pwm-starfive-ptc.c
>> +
>>  STARFIVE JH71X0 RESET CONTROLLER DRIVERS
>>  M:	Emil Renner Berthing <kernel@esmil.dk>
>>  M:	Hal Feng <hal.feng@starfivetech.com>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 8ebcddf91f7b..e2ee0169f6e4 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -569,6 +569,15 @@ config PWM_SPRD
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-sprd.
>>
>> +config PWM_STARFIVE
>> +	tristate "StarFive PWM support"
>> +	depends on ARCH_STARFIVE || COMPILE_TEST
>> +	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.
>> +
>>  config PWM_STI
>>  	tristate "STiH4xx PWM support"
>>  	depends on ARCH_STI || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index c822389c2a24..93b954376873 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -52,6 +52,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)	+= pwm-starfive.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.c b/drivers/pwm/pwm-starfive.c
> 
> Hi William,
> 
> You never answered my questions about what PTC is short for and if there are
> other PWMs on the JH7110. You just removed -ptc from the name of this file..
> 
Hi Emil,

The PTC, short for PWM/TIMER/CONUTER, comes from OpenCore's ip, but only PWM
mode is used in the JH7110. So the register still has the word "PTC".
s the best way to change all the prefix to STARFIVE?

Best regards,
William
>> new file mode 100644
>> index 000000000000..d390349fc95d
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-starfive.c
>> @@ -0,0 +1,190 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PWM driver for the StarFive JH71x0 SoC
>> + *
>> + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +/* Access PTC register (CNTR, HRC, LRC and CTRL) */
>> +#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)
> 
> ..but these defines
> 
>> +
>> +/* PTC_RPTC_CTRL register bits*/
>> +#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)
> 
> ..and these defines are still prefixed with *PTC where I'd expect something like
> STARFIVE_PWM_, and below structs and function names are also still
> using starfive_pwm_ptc_
> where I'd expect starfive_pwm_. Please be consistant in your naming.
> 
>> +struct starfive_pwm_ptc_device {
>> +	struct pwm_chip chip;
>> +	struct clk *clk;
>> +	struct reset_control *rst;
>> +	void __iomem *regs;
>> +	u32 clk_rate; /* PWM APB clock frequency */
>> +};
>> +
>> +static inline struct starfive_pwm_ptc_device *
>> +chip_to_starfive_ptc(struct pwm_chip *chip)
>> +
>> +{
>> +	return container_of(chip, struct starfive_pwm_ptc_device, chip);
>> +}
>> +
>> +static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
>> +				      struct pwm_device *dev,
>> +				      struct pwm_state *state)
>> +{
>> +	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
>> +	u32 period_data, duty_data, ctrl_data;
>> +
>> +	period_data = readl(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
>> +	duty_data = readl(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
>> +	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
>> +
>> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
>> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
>> +	state->polarity = PWM_POLARITY_INVERSED;
>> +	state->enabled = (ctrl_data & PTC_EN) ? true : false;
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
>> +				  struct pwm_device *dev,
>> +				  const struct pwm_state *state)
>> +{
>> +	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
>> +	u32 period_data, duty_data, ctrl_data = 0;
>> +
>> +	if (state->polarity != PWM_POLARITY_INVERSED)
>> +		return -EINVAL;
>> +
>> +	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,
>> +					    NSEC_PER_SEC);
>> +	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
>> +					  NSEC_PER_SEC);
>> +
>> +	writel(period_data, REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
>> +	writel(duty_data, REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
>> +	writel(0,  REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm));
>> +
>> +	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
>> +	if (state->enabled)
>> +		writel(ctrl_data | PTC_EN | PTC_OE, REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
>> +	else
>> +		writel(ctrl_data & ~(PTC_EN | PTC_OE), REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pwm_ops starfive_pwm_ptc_ops = {
>> +	.get_state	= starfive_pwm_ptc_get_state,
>> +	.apply		= starfive_pwm_ptc_apply,
>> +	.owner		= THIS_MODULE,
>> +};
>> +
>> +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;
>> +	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 = 3;
>> +
>> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(pwm->regs))
>> +		return dev_err_probe(dev, PTR_ERR(pwm->regs),
>> +				     "Unable to map IO resources\n");
>> +
>> +	pwm->clk = devm_clk_get_enabled(dev, NULL);
>> +	if (IS_ERR(pwm->clk))
>> +		return dev_err_probe(dev, PTR_ERR(pwm->clk),
>> +				     "Unable to get pwm's clock\n");
>> +
>> +	pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
>> +	if (IS_ERR(pwm->rst))
>> +		return dev_err_probe(dev, PTR_ERR(pwm->rst),
>> +				     "Unable to get pwm's reset\n");
>> +
>> +	ret = reset_control_deassert(pwm->rst);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable clock for pwm: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	pwm->clk_rate = clk_get_rate(pwm->clk);
>> +	if (pwm->clk_rate <= 0) {
>> +		dev_warn(dev, "Failed to get APB clock rate\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = devm_pwmchip_add(dev, chip);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Cannot register PTC: %d\n", ret);
>> +		clk_disable_unprepare(pwm->clk);
>> +		reset_control_assert(pwm->rst);
>> +		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);
>> +
>> +	reset_control_assert(pwm->rst);
>> +	clk_disable_unprepare(pwm->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id starfive_pwm_ptc_of_match[] = {
>> +	{ .compatible = "starfive,jh7100-pwm" },
>> +	{ .compatible = "starfive,jh7110-pwm" },
>> +	{ /* sentinel */ }
>> +};
>> +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",
> 
> Here
> 
>> +		.of_match_table = starfive_pwm_ptc_of_match,
>> +	},
>> +};
>> +module_platform_driver(starfive_pwm_ptc_driver);
>> +
>> +MODULE_AUTHOR("Jieqin Chen");
>> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
>> +MODULE_DESCRIPTION("StarFive PWM PTC driver");
> 
> ..and here you're also still calling the driver PTC without explaining why.
> 
>> +MODULE_LICENSE("GPL");
>> --
>> 2.34.1
>>
Emil Renner Berthing Sept. 25, 2023, 10:31 a.m. UTC | #3 
William Qiu wrote:
>
>
> On 2023/9/23 20:08, Emil Renner Berthing wrote:
> > William Qiu wrote:
> >> Add Pulse Width Modulation driver support for StarFive
> >> JH7100 and JH7110 SoC.
> >>
> >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> >> ---
> >>  MAINTAINERS                |   7 ++
> >>  drivers/pwm/Kconfig        |   9 ++
> >>  drivers/pwm/Makefile       |   1 +
> >>  drivers/pwm/pwm-starfive.c | 190 +++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 207 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-starfive.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index bf0f54c24f81..bc2155bd2712 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -20495,6 +20495,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@starfivetech.com>
> >> +M:	Hal Feng <hal.feng@starfivetech.com>
> >> +S:	Supported
> >> +F:	Documentation/devicetree/bindings/pwm/starfive,jh7100-pwm.yaml
> >> +F:	drivers/pwm/pwm-starfive-ptc.c
> >> +
> >>  STARFIVE JH71X0 RESET CONTROLLER DRIVERS
> >>  M:	Emil Renner Berthing <kernel@esmil.dk>
> >>  M:	Hal Feng <hal.feng@starfivetech.com>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 8ebcddf91f7b..e2ee0169f6e4 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -569,6 +569,15 @@ config PWM_SPRD
> >>  	  To compile this driver as a module, choose M here: the module
> >>  	  will be called pwm-sprd.
> >>
> >> +config PWM_STARFIVE
> >> +	tristate "StarFive PWM support"
> >> +	depends on ARCH_STARFIVE || COMPILE_TEST
> >> +	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.
> >> +
> >>  config PWM_STI
> >>  	tristate "STiH4xx PWM support"
> >>  	depends on ARCH_STI || COMPILE_TEST
> >> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> >> index c822389c2a24..93b954376873 100644
> >> --- a/drivers/pwm/Makefile
> >> +++ b/drivers/pwm/Makefile
> >> @@ -52,6 +52,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)	+= pwm-starfive.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.c b/drivers/pwm/pwm-starfive.c
> >
> > Hi William,
> >
> > You never answered my questions about what PTC is short for and if there are
> > other PWMs on the JH7110. You just removed -ptc from the name of this file..
> >
> Hi Emil,
>
> The PTC, short for PWM/TIMER/CONUTER, comes from OpenCore's ip, but only PWM
> mode is used in the JH7110. So the register still has the word "PTC".
> s the best way to change all the prefix to STARFIVE?

I see. Yeah, since you're only using the P from PTC the PTC name doesn't make a
lot of sense anymore. I'd just call this whole driver
STARFIVE_PWM_/starfive_pwm_ consistently.

>
> Best regards,
> William
> >> new file mode 100644
> >> index 000000000000..d390349fc95d
> >> --- /dev/null
> >> +++ b/drivers/pwm/pwm-starfive.c
> >> @@ -0,0 +1,190 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * PWM driver for the StarFive JH71x0 SoC
> >> + *
> >> + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/io.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pwm.h>
> >> +#include <linux/reset.h>
> >> +#include <linux/slab.h>
> >> +
> >> +/* Access PTC register (CNTR, HRC, LRC and CTRL) */
> >> +#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)
> >
> > ..but these defines
> >
> >> +
> >> +/* PTC_RPTC_CTRL register bits*/
> >> +#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)
> >
> > ..and these defines are still prefixed with *PTC where I'd expect something like
> > STARFIVE_PWM_, and below structs and function names are also still
> > using starfive_pwm_ptc_
> > where I'd expect starfive_pwm_. Please be consistant in your naming.
> >
> >> +struct starfive_pwm_ptc_device {
> >> +	struct pwm_chip chip;
> >> +	struct clk *clk;
> >> +	struct reset_control *rst;
> >> +	void __iomem *regs;
> >> +	u32 clk_rate; /* PWM APB clock frequency */
> >> +};
> >> +
> >> +static inline struct starfive_pwm_ptc_device *
> >> +chip_to_starfive_ptc(struct pwm_chip *chip)
> >> +
> >> +{
> >> +	return container_of(chip, struct starfive_pwm_ptc_device, chip);
> >> +}
> >> +
> >> +static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
> >> +				      struct pwm_device *dev,
> >> +				      struct pwm_state *state)
> >> +{
> >> +	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
> >> +	u32 period_data, duty_data, ctrl_data;
> >> +
> >> +	period_data = readl(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
> >> +	duty_data = readl(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
> >> +	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
> >> +
> >> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
> >> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
> >> +	state->polarity = PWM_POLARITY_INVERSED;
> >> +	state->enabled = (ctrl_data & PTC_EN) ? true : false;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
> >> +				  struct pwm_device *dev,
> >> +				  const struct pwm_state *state)
> >> +{
> >> +	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
> >> +	u32 period_data, duty_data, ctrl_data = 0;
> >> +
> >> +	if (state->polarity != PWM_POLARITY_INVERSED)
> >> +		return -EINVAL;
> >> +
> >> +	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,
> >> +					    NSEC_PER_SEC);
> >> +	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
> >> +					  NSEC_PER_SEC);
> >> +
> >> +	writel(period_data, REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
> >> +	writel(duty_data, REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
> >> +	writel(0,  REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm));
> >> +
> >> +	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
> >> +	if (state->enabled)
> >> +		writel(ctrl_data | PTC_EN | PTC_OE, REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
> >> +	else
> >> +		writel(ctrl_data & ~(PTC_EN | PTC_OE), REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct pwm_ops starfive_pwm_ptc_ops = {
> >> +	.get_state	= starfive_pwm_ptc_get_state,
> >> +	.apply		= starfive_pwm_ptc_apply,
> >> +	.owner		= THIS_MODULE,
> >> +};
> >> +
> >> +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;
> >> +	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 = 3;
> >> +
> >> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
> >> +	if (IS_ERR(pwm->regs))
> >> +		return dev_err_probe(dev, PTR_ERR(pwm->regs),
> >> +				     "Unable to map IO resources\n");
> >> +
> >> +	pwm->clk = devm_clk_get_enabled(dev, NULL);
> >> +	if (IS_ERR(pwm->clk))
> >> +		return dev_err_probe(dev, PTR_ERR(pwm->clk),
> >> +				     "Unable to get pwm's clock\n");
> >> +
> >> +	pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
> >> +	if (IS_ERR(pwm->rst))
> >> +		return dev_err_probe(dev, PTR_ERR(pwm->rst),
> >> +				     "Unable to get pwm's reset\n");
> >> +
> >> +	ret = reset_control_deassert(pwm->rst);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to enable clock for pwm: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	pwm->clk_rate = clk_get_rate(pwm->clk);
> >> +	if (pwm->clk_rate <= 0) {
> >> +		dev_warn(dev, "Failed to get APB clock rate\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	ret = devm_pwmchip_add(dev, chip);
> >> +	if (ret < 0) {
> >> +		dev_err(dev, "Cannot register PTC: %d\n", ret);
> >> +		clk_disable_unprepare(pwm->clk);
> >> +		reset_control_assert(pwm->rst);
> >> +		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);
> >> +
> >> +	reset_control_assert(pwm->rst);
> >> +	clk_disable_unprepare(pwm->clk);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct of_device_id starfive_pwm_ptc_of_match[] = {
> >> +	{ .compatible = "starfive,jh7100-pwm" },
> >> +	{ .compatible = "starfive,jh7110-pwm" },
> >> +	{ /* sentinel */ }
> >> +};
> >> +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",
> >
> > Here
> >
> >> +		.of_match_table = starfive_pwm_ptc_of_match,
> >> +	},
> >> +};
> >> +module_platform_driver(starfive_pwm_ptc_driver);
> >> +
> >> +MODULE_AUTHOR("Jieqin Chen");
> >> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
> >> +MODULE_DESCRIPTION("StarFive PWM PTC driver");
> >
> > ..and here you're also still calling the driver PTC without explaining why.
> >
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.34.1
> >>
William Qiu Sept. 25, 2023, 10:48 a.m. UTC | #4 
On 2023/9/25 18:31, Emil Renner Berthing wrote:
> William Qiu wrote:
>>
>>
>> On 2023/9/23 20:08, Emil Renner Berthing wrote:
>> > William Qiu wrote:
>> >> Add Pulse Width Modulation driver support for StarFive
>> >> JH7100 and JH7110 SoC.
>> >>
>> >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> >> ---
>> >>  MAINTAINERS                |   7 ++
>> >>  drivers/pwm/Kconfig        |   9 ++
>> >>  drivers/pwm/Makefile       |   1 +
>> >>  drivers/pwm/pwm-starfive.c | 190 +++++++++++++++++++++++++++++++++++++
>> >>  4 files changed, 207 insertions(+)
>> >>  create mode 100644 drivers/pwm/pwm-starfive.c
>> >>
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index bf0f54c24f81..bc2155bd2712 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -20495,6 +20495,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@starfivetech.com>
>> >> +M:	Hal Feng <hal.feng@starfivetech.com>
>> >> +S:	Supported
>> >> +F:	Documentation/devicetree/bindings/pwm/starfive,jh7100-pwm.yaml
>> >> +F:	drivers/pwm/pwm-starfive-ptc.c
>> >> +
>> >>  STARFIVE JH71X0 RESET CONTROLLER DRIVERS
>> >>  M:	Emil Renner Berthing <kernel@esmil.dk>
>> >>  M:	Hal Feng <hal.feng@starfivetech.com>
>> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> >> index 8ebcddf91f7b..e2ee0169f6e4 100644
>> >> --- a/drivers/pwm/Kconfig
>> >> +++ b/drivers/pwm/Kconfig
>> >> @@ -569,6 +569,15 @@ config PWM_SPRD
>> >>  	  To compile this driver as a module, choose M here: the module
>> >>  	  will be called pwm-sprd.
>> >>
>> >> +config PWM_STARFIVE
>> >> +	tristate "StarFive PWM support"
>> >> +	depends on ARCH_STARFIVE || COMPILE_TEST
>> >> +	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.
>> >> +
>> >>  config PWM_STI
>> >>  	tristate "STiH4xx PWM support"
>> >>  	depends on ARCH_STI || COMPILE_TEST
>> >> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> >> index c822389c2a24..93b954376873 100644
>> >> --- a/drivers/pwm/Makefile
>> >> +++ b/drivers/pwm/Makefile
>> >> @@ -52,6 +52,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)	+= pwm-starfive.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.c b/drivers/pwm/pwm-starfive.c
>> >
>> > Hi William,
>> >
>> > You never answered my questions about what PTC is short for and if there are
>> > other PWMs on the JH7110. You just removed -ptc from the name of this file..
>> >
>> Hi Emil,
>>
>> The PTC, short for PWM/TIMER/CONUTER, comes from OpenCore's ip, but only PWM
>> mode is used in the JH7110. So the register still has the word "PTC".
>> s the best way to change all the prefix to STARFIVE?
> 
> I see. Yeah, since you're only using the P from PTC the PTC name doesn't make a
> lot of sense anymore. I'd just call this whole driver
> STARFIVE_PWM_/starfive_pwm_ consistently.
> 

I see, I'll update it in next version.
>>
>> Best regards,
>> William
>> >> new file mode 100644
>> >> index 000000000000..d390349fc95d
>> >> --- /dev/null
>> >> +++ b/drivers/pwm/pwm-starfive.c
>> >> @@ -0,0 +1,190 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +/*
>> >> + * PWM driver for the StarFive JH71x0 SoC
>> >> + *
>> >> + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
>> >> + */
>> >> +
>> >> +#include <linux/clk.h>
>> >> +#include <linux/io.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/platform_device.h>
>> >> +#include <linux/pwm.h>
>> >> +#include <linux/reset.h>
>> >> +#include <linux/slab.h>
>> >> +
>> >> +/* Access PTC register (CNTR, HRC, LRC and CTRL) */
>> >> +#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)
>> >
>> > ..but these defines
>> >
>> >> +
>> >> +/* PTC_RPTC_CTRL register bits*/
>> >> +#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)
>> >
>> > ..and these defines are still prefixed with *PTC where I'd expect something like
>> > STARFIVE_PWM_, and below structs and function names are also still
>> > using starfive_pwm_ptc_
>> > where I'd expect starfive_pwm_. Please be consistant in your naming.
>> >
>> >> +struct starfive_pwm_ptc_device {
>> >> +	struct pwm_chip chip;
>> >> +	struct clk *clk;
>> >> +	struct reset_control *rst;
>> >> +	void __iomem *regs;
>> >> +	u32 clk_rate; /* PWM APB clock frequency */
>> >> +};
>> >> +
>> >> +static inline struct starfive_pwm_ptc_device *
>> >> +chip_to_starfive_ptc(struct pwm_chip *chip)
>> >> +
>> >> +{
>> >> +	return container_of(chip, struct starfive_pwm_ptc_device, chip);
>> >> +}
>> >> +
>> >> +static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
>> >> +				      struct pwm_device *dev,
>> >> +				      struct pwm_state *state)
>> >> +{
>> >> +	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
>> >> +	u32 period_data, duty_data, ctrl_data;
>> >> +
>> >> +	period_data = readl(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
>> >> +	duty_data = readl(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
>> >> +	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
>> >> +
>> >> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
>> >> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
>> >> +	state->polarity = PWM_POLARITY_INVERSED;
>> >> +	state->enabled = (ctrl_data & PTC_EN) ? true : false;
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
>> >> +				  struct pwm_device *dev,
>> >> +				  const struct pwm_state *state)
>> >> +{
>> >> +	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
>> >> +	u32 period_data, duty_data, ctrl_data = 0;
>> >> +
>> >> +	if (state->polarity != PWM_POLARITY_INVERSED)
>> >> +		return -EINVAL;
>> >> +
>> >> +	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,
>> >> +					    NSEC_PER_SEC);
>> >> +	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
>> >> +					  NSEC_PER_SEC);
>> >> +
>> >> +	writel(period_data, REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
>> >> +	writel(duty_data, REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
>> >> +	writel(0,  REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm));
>> >> +
>> >> +	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
>> >> +	if (state->enabled)
>> >> +		writel(ctrl_data | PTC_EN | PTC_OE, REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
>> >> +	else
>> >> +		writel(ctrl_data & ~(PTC_EN | PTC_OE), REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static const struct pwm_ops starfive_pwm_ptc_ops = {
>> >> +	.get_state	= starfive_pwm_ptc_get_state,
>> >> +	.apply		= starfive_pwm_ptc_apply,
>> >> +	.owner		= THIS_MODULE,
>> >> +};
>> >> +
>> >> +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;
>> >> +	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 = 3;
>> >> +
>> >> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
>> >> +	if (IS_ERR(pwm->regs))
>> >> +		return dev_err_probe(dev, PTR_ERR(pwm->regs),
>> >> +				     "Unable to map IO resources\n");
>> >> +
>> >> +	pwm->clk = devm_clk_get_enabled(dev, NULL);
>> >> +	if (IS_ERR(pwm->clk))
>> >> +		return dev_err_probe(dev, PTR_ERR(pwm->clk),
>> >> +				     "Unable to get pwm's clock\n");
>> >> +
>> >> +	pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
>> >> +	if (IS_ERR(pwm->rst))
>> >> +		return dev_err_probe(dev, PTR_ERR(pwm->rst),
>> >> +				     "Unable to get pwm's reset\n");
>> >> +
>> >> +	ret = reset_control_deassert(pwm->rst);
>> >> +	if (ret) {
>> >> +		dev_err(dev, "Failed to enable clock for pwm: %d\n", ret);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	pwm->clk_rate = clk_get_rate(pwm->clk);
>> >> +	if (pwm->clk_rate <= 0) {
>> >> +		dev_warn(dev, "Failed to get APB clock rate\n");
>> >> +		return -EINVAL;
>> >> +	}
>> >> +
>> >> +	ret = devm_pwmchip_add(dev, chip);
>> >> +	if (ret < 0) {
>> >> +		dev_err(dev, "Cannot register PTC: %d\n", ret);
>> >> +		clk_disable_unprepare(pwm->clk);
>> >> +		reset_control_assert(pwm->rst);
>> >> +		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);
>> >> +
>> >> +	reset_control_assert(pwm->rst);
>> >> +	clk_disable_unprepare(pwm->clk);
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static const struct of_device_id starfive_pwm_ptc_of_match[] = {
>> >> +	{ .compatible = "starfive,jh7100-pwm" },
>> >> +	{ .compatible = "starfive,jh7110-pwm" },
>> >> +	{ /* sentinel */ }
>> >> +};
>> >> +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",
>> >
>> > Here
>> >
>> >> +		.of_match_table = starfive_pwm_ptc_of_match,
>> >> +	},
>> >> +};
>> >> +module_platform_driver(starfive_pwm_ptc_driver);
>> >> +
>> >> +MODULE_AUTHOR("Jieqin Chen");
>> >> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
>> >> +MODULE_DESCRIPTION("StarFive PWM PTC driver");
>> >
>> > ..and here you're also still calling the driver PTC without explaining why.
>> >
>> >> +MODULE_LICENSE("GPL");
>> >> --
>> >> 2.34.1
>> >>
Uwe Kleine-König Sept. 25, 2023, 12:39 p.m. UTC | #5 
Hello,

On Mon, Sep 25, 2023 at 10:31:49AM +0000, Emil Renner Berthing wrote:
> William Qiu wrote:
> > The PTC, short for PWM/TIMER/CONUTER, comes from OpenCore's ip, but only PWM
> > mode is used in the JH7110. So the register still has the word "PTC".
> > s the best way to change all the prefix to STARFIVE?
> 
> I see. Yeah, since you're only using the P from PTC the PTC name doesn't make a
> lot of sense anymore. I'd just call this whole driver
> STARFIVE_PWM_/starfive_pwm_ consistently.

I don't care much how the driver is named iff there is only a single
type of hardware unit on this platform that can be used as a PWM.
However if the hardware manual calls this unit PTC I'd at least mention
that in a comment at the top of the driver.

Thanks
Uwe
Thierry Reding Oct. 6, 2023, 9:02 a.m. UTC | #6 
On Mon, Sep 25, 2023 at 06:27:16PM +0800, William Qiu wrote:
> 
> 
> On 2023/9/23 20:08, Emil Renner Berthing wrote:
> > William Qiu wrote:
> >> Add Pulse Width Modulation driver support for StarFive
> >> JH7100 and JH7110 SoC.
> >>
> >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> >> ---
> >>  MAINTAINERS                |   7 ++
> >>  drivers/pwm/Kconfig        |   9 ++
> >>  drivers/pwm/Makefile       |   1 +
> >>  drivers/pwm/pwm-starfive.c | 190 +++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 207 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-starfive.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index bf0f54c24f81..bc2155bd2712 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -20495,6 +20495,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@starfivetech.com>
> >> +M:	Hal Feng <hal.feng@starfivetech.com>
> >> +S:	Supported
> >> +F:	Documentation/devicetree/bindings/pwm/starfive,jh7100-pwm.yaml
> >> +F:	drivers/pwm/pwm-starfive-ptc.c
> >> +
> >>  STARFIVE JH71X0 RESET CONTROLLER DRIVERS
> >>  M:	Emil Renner Berthing <kernel@esmil.dk>
> >>  M:	Hal Feng <hal.feng@starfivetech.com>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 8ebcddf91f7b..e2ee0169f6e4 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -569,6 +569,15 @@ config PWM_SPRD
> >>  	  To compile this driver as a module, choose M here: the module
> >>  	  will be called pwm-sprd.
> >>
> >> +config PWM_STARFIVE
> >> +	tristate "StarFive PWM support"
> >> +	depends on ARCH_STARFIVE || COMPILE_TEST
> >> +	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.
> >> +
> >>  config PWM_STI
> >>  	tristate "STiH4xx PWM support"
> >>  	depends on ARCH_STI || COMPILE_TEST
> >> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> >> index c822389c2a24..93b954376873 100644
> >> --- a/drivers/pwm/Makefile
> >> +++ b/drivers/pwm/Makefile
> >> @@ -52,6 +52,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)	+= pwm-starfive.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.c b/drivers/pwm/pwm-starfive.c
> > 
> > Hi William,
> > 
> > You never answered my questions about what PTC is short for and if there are
> > other PWMs on the JH7110. You just removed -ptc from the name of this file..
> > 
> Hi Emil,
> 
> The PTC, short for PWM/TIMER/CONUTER, comes from OpenCore's ip, but only PWM
> mode is used in the JH7110. So the register still has the word "PTC".
> s the best way to change all the prefix to STARFIVE?

This is the first time I see mentioned that this is based on an Open-
Cores IP. It's definitely something you want to note somewhere so that
others can reuse this driver if they've incorporated the same IP into
their device.

Given the above it might be better to name this something different
entirely. The original OpenCores PTC IP seems to be single-instance,
but that's about the only difference here (i.e. the OpenCores IP lists
one clock and one reset, which this driver supports).

So it'd be easy to turn this into a generic OpenCores driver and then
use the starfive compatible string(s) to parameterize (number of
instances, register stride, etc.).

Thierry
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index bf0f54c24f81..bc2155bd2712 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20495,6 +20495,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@starfivetech.com>
+M:	Hal Feng <hal.feng@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/pwm/starfive,jh7100-pwm.yaml
+F:	drivers/pwm/pwm-starfive-ptc.c
+
 STARFIVE JH71X0 RESET CONTROLLER DRIVERS
 M:	Emil Renner Berthing <kernel@esmil.dk>
 M:	Hal Feng <hal.feng@starfivetech.com>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8ebcddf91f7b..e2ee0169f6e4 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -569,6 +569,15 @@  config PWM_SPRD
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sprd.
 
+config PWM_STARFIVE
+	tristate "StarFive PWM support"
+	depends on ARCH_STARFIVE || COMPILE_TEST
+	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.
+
 config PWM_STI
 	tristate "STiH4xx PWM support"
 	depends on ARCH_STI || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c822389c2a24..93b954376873 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -52,6 +52,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)	+= pwm-starfive.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.c b/drivers/pwm/pwm-starfive.c
new file mode 100644
index 000000000000..d390349fc95d
--- /dev/null
+++ b/drivers/pwm/pwm-starfive.c
@@ -0,0 +1,190 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM driver for the StarFive JH71x0 SoC
+ *
+ * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+/* Access PTC register (CNTR, HRC, LRC and CTRL) */
+#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 register bits*/
+#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;
+	u32 clk_rate; /* PWM APB clock frequency */
+};
+
+static inline struct starfive_pwm_ptc_device *
+chip_to_starfive_ptc(struct pwm_chip *chip)
+
+{
+	return container_of(chip, struct starfive_pwm_ptc_device, chip);
+}
+
+static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
+				      struct pwm_device *dev,
+				      struct pwm_state *state)
+{
+	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
+	u32 period_data, duty_data, ctrl_data;
+
+	period_data = readl(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
+	duty_data = readl(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
+	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
+
+	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
+	state->polarity = PWM_POLARITY_INVERSED;
+	state->enabled = (ctrl_data & PTC_EN) ? true : false;
+
+	return 0;
+}
+
+static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
+				  struct pwm_device *dev,
+				  const struct pwm_state *state)
+{
+	struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
+	u32 period_data, duty_data, ctrl_data = 0;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,
+					    NSEC_PER_SEC);
+	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
+					  NSEC_PER_SEC);
+
+	writel(period_data, REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
+	writel(duty_data, REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
+	writel(0,  REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm));
+
+	ctrl_data = readl(REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
+	if (state->enabled)
+		writel(ctrl_data | PTC_EN | PTC_OE, REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
+	else
+		writel(ctrl_data & ~(PTC_EN | PTC_OE), REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm));
+
+	return 0;
+}
+
+static const struct pwm_ops starfive_pwm_ptc_ops = {
+	.get_state	= starfive_pwm_ptc_get_state,
+	.apply		= starfive_pwm_ptc_apply,
+	.owner		= THIS_MODULE,
+};
+
+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;
+	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 = 3;
+
+	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pwm->regs))
+		return dev_err_probe(dev, PTR_ERR(pwm->regs),
+				     "Unable to map IO resources\n");
+
+	pwm->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk),
+				     "Unable to get pwm's clock\n");
+
+	pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(pwm->rst))
+		return dev_err_probe(dev, PTR_ERR(pwm->rst),
+				     "Unable to get pwm's reset\n");
+
+	ret = reset_control_deassert(pwm->rst);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock for pwm: %d\n", ret);
+		return ret;
+	}
+
+	pwm->clk_rate = clk_get_rate(pwm->clk);
+	if (pwm->clk_rate <= 0) {
+		dev_warn(dev, "Failed to get APB clock rate\n");
+		return -EINVAL;
+	}
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret < 0) {
+		dev_err(dev, "Cannot register PTC: %d\n", ret);
+		clk_disable_unprepare(pwm->clk);
+		reset_control_assert(pwm->rst);
+		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);
+
+	reset_control_assert(pwm->rst);
+	clk_disable_unprepare(pwm->clk);
+
+	return 0;
+}
+
+static const struct of_device_id starfive_pwm_ptc_of_match[] = {
+	{ .compatible = "starfive,jh7100-pwm" },
+	{ .compatible = "starfive,jh7110-pwm" },
+	{ /* sentinel */ }
+};
+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 = starfive_pwm_ptc_of_match,
+	},
+};
+module_platform_driver(starfive_pwm_ptc_driver);
+
+MODULE_AUTHOR("Jieqin Chen");
+MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
+MODULE_DESCRIPTION("StarFive PWM PTC driver");
+MODULE_LICENSE("GPL");