On Wed, Dec 04, 2024 at 11:17:18AM +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>
> Signed-off-by: Sean Young <sean@xxxxxxxx>
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sophgo-sg2042.c | 194 ++++++++++++++++++++++++++++++++
> 3 files changed, 205 insertions(+)
> create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..ec85f3895936 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -584,6 +584,16 @@ 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 the PWM controller on Sophgo SG2042 SoC. The PWM
> + controller supports outputing 4 channels of PWM waveforms.
> +
> + 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 9081e0c0e9e0..539e0def3f82 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -53,6 +53,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..a3d12505e4aa
> --- /dev/null
> +++ b/drivers/pwm/pwm-sophgo-sg2042.c
> @@ -0,0 +1,194 @@
> +// 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>
> + *
> + * Limitations:
> + * - After reset, the output of the PWM channel is always high.
> + * The value of HLPERIOD/PERIOD is 0.
> + * - When HLPERIOD or PERIOD is reconfigured, PWM will start to
> + * output waveforms with the new configuration after completing
> + * the running period.
> + * - When PERIOD and HLPERIOD is set to 0, the PWM wave output will
> + * be stopped and the output is pulled to high.
Maybe I already asked: If there is a public manual for this chip, please
add a link to it here.
> + */
> +
> +#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 <linux/reset.h>
> +
> +#include <asm/div64.h>
The canonical include for that is <linux/math64.h>. This is also the
header that defines mul_u64_u64_div_u64(). Your driver seems to compile
only because clk.h includes math64.h via <linux/notifier.h> ->
<linux/srcu.h> -> <linux/workqueue.h> -> <linux/timer.h> ->
<linux/ktime.h> -> <linux/jiffies.h> -> <linux/math64.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 SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
> +#define SG2042_PERIOD(chan) ((chan) * 8 + 4)
s/SG2042_/SG2042_PWM_/ to match the function prefix and driver name?
> +
> +#define SG2042_PWM_CHANNELNUM 4
> +
> +/**
> + * struct sg2042_pwm_ddata - private driver data
> + * @base: base address of mapped PWM registers
> + * @clk_rate_hz: rate of base clock in HZ
> + */
> +struct sg2042_pwm_ddata {
> + void __iomem *base;
> + unsigned long clk_rate_hz;
> +};
> +
> +static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod)
Maybe pass ddata here instead of base and add
void __iomem *base = ddata->base;
to the function body. Then the calls simplify from
pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);
to
pwm_sg2042_config(ddata, pwm->hwpwm, period, hlperiod);
.
> +{
> + writel(period, base + SG2042_PERIOD(chan));
> + writel(hlperiod, base + SG2042_HLPERIOD(chan));
> +}
> +
> +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
> + u32 hlperiod;
> + u32 period;
state->period is measured in ns, the local variable period however
holds a value measured in clock ticks. To make this still clearer than
it already is, I suggest to rename the variable to period_ticks. Ditto
for hlperiod.
> + if (state->polarity == PWM_POLARITY_INVERSED)
> + return -EINVAL;
> +
> + if (!state->enabled) {
> + pwm_sg2042_config(ddata->base, pwm->hwpwm, 0, 0);
> + return 0;
> + }
> +
> + /*
> + * Period of High level (duty_cycle) = HLPERIOD x Period_clk
> + * Period of One Cycle (period) = PERIOD x Period_clk
s/Period/Duration/ ? What is Period_clk?
> + */
> + period = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period, NSEC_PER_SEC), U32_MAX);
> + hlperiod = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
> +
> + if (hlperiod > period) {
> + dev_err(pwmchip_parent(chip), "period < hlperiod, failed to apply current setting\n");
> + return -EINVAL;
No need to check for that, .apply() is only called with
state->duty_cycle <= state->period.
> + }
> +
> + dev_dbg(pwmchip_parent(chip), "chan[%u]: period=%u, hlperiod=%u\n",
> + pwm->hwpwm, period, hlperiod);
> +
> + pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);
> +
> + return 0;
> +}
> +
> +static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
> + unsigned int chan = pwm->hwpwm;
> + u32 hlperiod;
> + u32 period;
> +
> + period = readl(ddata->base + SG2042_PERIOD(chan));
> + hlperiod = readl(ddata->base + SG2042_HLPERIOD(chan));
> +
> + if (!period && !hlperiod)
> + state->enabled = false;
> + else
> + state->enabled = true;
What happens if hlperiod > period? Isn't period==0 enough for
state->enabled = false? Also if period==0 there is no use in determining
state->period and state->duty_cycle.
So I would expect here:
period_ticks = readl(ddata->base + SG2042_PERIOD(chan));
hlperiod_ticks = readl(ddata->base + SG2042_HLPERIOD(chan));
if (!period_ticks) {
state->enabled = false;
return 0;
}
if (hlperiod_ticks > period_ticks)
hlperiod_ticks = period_ticks;
state->enabled = true;
state->period = DIV_ROUND_UP_ULL((u64)period_ticks * NSEC_PER_SEC, ddata->clk_rate_hz);
state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod_ticks * NSEC_PER_SEC, ddata->clk_rate_hz);
state->polarity = PWM_POLARITY_NORMAL;
> + state->period = DIV_ROUND_UP_ULL((u64)period * NSEC_PER_SEC, ddata->clk_rate_hz);
> + state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod * NSEC_PER_SEC, ddata->clk_rate_hz);
> +
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops pwm_sg2042_ops = {
> + .apply = pwm_sg2042_apply,
> + .get_state = pwm_sg2042_get_state,
> +};
> +
> +static const struct of_device_id sg2042_pwm_ids[] = {
> + { .compatible = "sophgo,sg2042-pwm" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);
> +
> +static int pwm_sg2042_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sg2042_pwm_ddata *ddata;
> + struct reset_control *rst;
> + struct pwm_chip *chip;
> + struct clk *clk;
> + int ret;
> +
> + chip = devm_pwmchip_alloc(dev, SG2042_PWM_CHANNELNUM, sizeof(*ddata));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> + ddata = pwmchip_get_drvdata(chip);
> +
> + ddata->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(ddata->base))
> + return PTR_ERR(ddata->base);
> +
> + clk = devm_clk_get_enabled(dev, "apb");
> + if (IS_ERR(clk))
> + return dev_err_probe(dev, PTR_ERR(clk), "failed to get base clk\n");
I like error messages to start consistently with a capital letter.
> + ret = devm_clk_rate_exclusive_get(dev, clk);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to get exclusive rate\n");
> +
> + ddata->clk_rate_hz = clk_get_rate(clk);
> + if (!ddata->clk_rate_hz || ddata->clk_rate_hz > NSEC_PER_SEC)
Please add a comment about why you check for > NSEC_PER_SEC.
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid clock rate: %lu\n", ddata->clk_rate_hz);
> +
> + rst = devm_reset_control_get_optional_shared(dev, NULL);
> + if (IS_ERR(rst))
> + return dev_err_probe(dev, PTR_ERR(rst), "failed to get reset\n");
> +
> + /* Deassert reset */
> + ret = reset_control_deassert(rst);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to deassert\n");
There is devm_reset_control_get_optional_shared_deasserted() that does
the two calls devm_reset_control_get_optional_shared() and
reset_control_deassert() together and also cares for reasserting the
reset at remove time.
> + chip->ops = &pwm_sg2042_ops;
> + chip->atomic = true;
> +
> + ret = devm_pwmchip_add(dev, chip);
> + if (ret < 0) {
> + reset_control_assert(rst);
> + return dev_err_probe(dev, ret, "failed to register PWM chip\n");
> + }
> +
> + return 0;
> +}
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
