Hello,
On Thu, Sep 05, 2024 at 08:10:42PM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@xxxxxxxxxxx>
>
> Add a PWM driver for PWM controller in Sophgo SG2042 SoC.
>
> Signed-off-by: Chen Wang <unicorn_wang@xxxxxxxxxxx>
> ---
> 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
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 3e53838990f5..6287d63a84fd 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -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
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 0be4f3e6dd43..ef2555e83183 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -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
> diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c
> new file mode 100644
> index 000000000000..cf11ad54b4de
> --- /dev/null
> +++ b/drivers/pwm/pwm-sophgo-sg2042.c
> @@ -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 <unicorn_wang@xxxxxxxxxxx>
> + */
> +
> +#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
> +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
)
> +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;
> + /*
> + * 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.
> + 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.
> + 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.
> + 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 =.
> +};
> +
> +static const struct of_device_id sg2042_pwm_match[] = {
> + { .compatible = "sophgo,sg2042-pwm" },
> + { },
Please drop the , after the sentinel entry.
> +};
> +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),
> + "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.
> +
> + 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 =.
> +MODULE_AUTHOR("Chen Wang");
> +MODULE_DESCRIPTION("Sophgo SG2042 PWM driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
