Hello,
On Fri, Dec 06, 2024 at 04:55:01PM +0800, Chi-Wen Weng wrote:
> [...]
> diff --git a/drivers/pwm/pwm-ma35d1.c b/drivers/pwm/pwm-ma35d1.c
> new file mode 100644
> index 000000000000..380f17b20a3d
> --- /dev/null
> +++ b/drivers/pwm/pwm-ma35d1.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the Nuvoton MA35D1 PWM controller
> + *
> + * Copyright (C) 2024 Nuvoton Corporation
> + * Chi-Wen Weng <cwweng@xxxxxxxxxxx>
Please add some information here about the hardware. The idea is to get
some info about the device's capabilities. Please stick to the format
that many other drivers are using such that
sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-ma35d1.c
emits the info for your driver. Things to mention are: possible glitches
during .apply(); behaviour of the pin when the PWM is disabled (constant
signal? High-Z?)
Also a link to a reference manual would be awesome.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +/* The following are registers address offset for MA35D1 PWM controller */
> +#define MA35D1_REG_PWM_CTL0 (0x00)
> +#define MA35D1_REG_PWM_CNTEN (0x20)
> +#define MA35D1_REG_PWM_PERIOD0 (0x30)
> +#define MA35D1_REG_PWM_CMPDAT0 (0x50)
> +#define MA35D1_REG_PWM_WGCTL0 (0xB0)
> +#define MA35D1_REG_PWM_POLCTL (0xD4)
> +#define MA35D1_REG_PWM_POEN (0xD8)
> +
> +/* The following are register address of MA35D1 PWM controller */
> +#define MA35D1_PWM_CH_REG_SIZE 4
> +#define MA35D1_PWM_CMPDAT0_ADDR(base, ch) ((base) + MA35D1_REG_PWM_CMPDAT0 + \
> + ((ch) * MA35D1_PWM_CH_REG_SIZE))
> +#define MA35D1_PWM_CNTEN_ADDR(base) ((base) + MA35D1_REG_PWM_CNTEN)
> +#define MA35D1_PWM_PERIOD0_ADDR(base, ch) ((base) + MA35D1_REG_PWM_PERIOD0 + \
> + ((ch) * MA35D1_PWM_CH_REG_SIZE))
> +#define MA35D1_PWM_POEN_ADDR(base) ((base) + MA35D1_REG_PWM_POEN)
> +#define MA35D1_PWM_POLCTL_ADDR(base) ((base) + MA35D1_REG_PWM_POLCTL)
I would drop the base part in these defines and add them to the above
list sorted by address.
So something like:
#define MA35D1_REG_PWM_CTL0 0x00
#define MA35D1_REG_PWM_CNTEN 0x20
#define MA35D1_REG_PWM_PERIOD(ch) (0x30 + 4 * (ch))
#define MA35D1_REG_PWM_CMPDAT(ch) (0x50 + 4 * (ch))
#define MA35D1_REG_PWM_WGCTL0 0xB0
#define MA35D1_REG_PWM_POLCTL 0xD4
#define MA35D1_REG_PWM_POEN 0xD8
To make up for the missing base, I'd create wrappers for readl and
writel à la:
static u32 nuvoton_pwm_readl(struct nuvoton_pwm *nvtpwm, unsigned int offset);
static void nuvoton_pwm_writel(struct nuvoton_pwm *nvtpwm, unsigned int offset, u32 value);
As you see I wouldn't use a define for 4, because IMHO that hurts
readability. But I don't feel strong here.
> +#define MA35D1_PWM_MAX_COUNT 0xFFFF
> +#define MA35D1_PWM_TOTAL_CHANNELS 6
s/TOTAL/NUM/
> +
> +struct nuvoton_pwm {
> + void __iomem *base;
> + u64 clkrate;
clk_get_rate() returns an unsigned long value. Please stick to that for
.clkrate as there is no use to double the size of this struct on 32-bit
platforms just to store zeros and padding.
> +};
> +
> +static inline struct nuvoton_pwm *to_nuvoton_pwm(struct pwm_chip *chip)
> +{
> + return pwmchip_get_drvdata(chip);
> +}
I slightly prefer nuvoton_pwm_from_chip() for this function's name to
have the same function prefix for all functions here.
> +
> +static int nuvoton_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct nuvoton_pwm *nvtpwm;
> + u32 ch = pwm->hwpwm;
> +
> + nvtpwm = to_nuvoton_pwm(chip);
> + if (state->enabled) {
> + u64 duty_cycles, period_cycles;
> +
> + /* Calculate the duty and period cycles */
> + duty_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> + state->duty_cycle, NSEC_PER_SEC);
> + if (duty_cycles > MA35D1_PWM_MAX_COUNT)
> + duty_cycles = MA35D1_PWM_MAX_COUNT;
> +
> + period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> + state->period, NSEC_PER_SEC);
> + if (period_cycles > MA35D1_PWM_MAX_COUNT)
> + period_cycles = MA35D1_PWM_MAX_COUNT;
> +
> + /* Write the duty and period cycles to registers */
> + writel(duty_cycles, MA35D1_PWM_CMPDAT0_ADDR(nvtpwm->base, ch));
> + writel(period_cycles, MA35D1_PWM_PERIOD0_ADDR(nvtpwm->base, ch));
> + /* Enable counter */
> + writel(readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base)) | BIT(ch),
> + MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
> + /* Enable output */
> + writel(readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base)) | BIT(ch),
> + MA35D1_PWM_POEN_ADDR(nvtpwm->base));
Can this result in glitches? E.g. is it possible to see a waveform with
the old period_cycles but the new duty_cycles output? If you switch
polarity, do you see the new settings with the wrong polarity for some
time? Setup polarity before enabling the counter and output? Maybe only
enable the counter when the output is enabled to be sure to emit the
first edge with the correct timing?
> + } else {
> + /* Disable counter */
> + writel(readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base)) & ~BIT(ch),
> + MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
> + /* Disable output */
> + writel(readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base)) & ~BIT(ch),
> + MA35D1_PWM_POEN_ADDR(nvtpwm->base));
Maybe add another wrapper for this kind of rmw operation. Also I suggest
to introduce a name for BIT(ch) here such that this can become:
nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POEN, MA35D1_REG_PWM_POEN_EN(ch), 0);
(Assuming "EN0" is the name of the bit for channel 0 in
MA35D1_REG_PWM_POEN.)
> + }
> +
> + /* Set polarity state to register */
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + writel(readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) & ~BIT(ch),
> + MA35D1_PWM_POLCTL_ADDR(nvtpwm->base));
> + else
> + writel(readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) | BIT(ch),
> + MA35D1_PWM_POLCTL_ADDR(nvtpwm->base));
You can skip setting up period if !state->enabled.
> + return 0;
> +}
> +
> [...]
> +
> +static int nuvoton_pwm_probe(struct platform_device *pdev)
> +{
> + struct pwm_chip *chip;
> + struct nuvoton_pwm *nvtpwm;
> + struct clk *clk;
> + int ret;
> +
> + chip = devm_pwmchip_alloc(&pdev->dev, MA35D1_PWM_TOTAL_CHANNELS, sizeof(*nvtpwm));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + nvtpwm = to_nuvoton_pwm(chip);
> +
> + nvtpwm->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(nvtpwm->base))
> + return PTR_ERR(nvtpwm->base);
> +
> + clk = devm_clk_get_enabled(&pdev->dev, NULL);
> + if (IS_ERR(clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(clk), "unable to get the clock");
Please start all error messages with a capital letter and end them with
\n.
devm_clk_rate_exclusive_get(&pdev->dev, clk) here please.
> +
> + nvtpwm->clkrate = clk_get_rate(clk);
> + if (nvtpwm->clkrate > NSEC_PER_SEC)
> + return dev_err_probe(&pdev->dev, -EINVAL, "pwm clock out of range");
return dev_err_probe(&pdev->dev, -EINVAL, "PWM clock out of range (%lu)\n", nvtpwm->clkrate);
> +
> + chip->ops = &nuvoton_pwm_ops;
> + chip->atomic = true;
> +
> + ret = devm_pwmchip_add(&pdev->dev, chip);
> + if (ret < 0)
> + return dev_err_probe(&pdev->dev, ret, "unable to add pwm chip");
> +
> + return 0;
> +}
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
