On Mon, Oct 28, 2024 at 06:27:21PM +0800, Ben Zong-You Xie wrote:
> Add PWM driver suuport for Andes atcpit100.
s/uup/upp/
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..ad6e803f12d0 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_PWM) += core.o
> obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
> obj-$(CONFIG_PWM_ADP5585) += pwm-adp5585.o
> obj-$(CONFIG_PWM_APPLE) += pwm-apple.o
> +obj-$(CONFIG_PWM_ATCPIT100) += pwm-atcpit100.o
> obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
> obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o
> obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o
> diff --git a/drivers/pwm/pwm-atcpit100.c b/drivers/pwm/pwm-atcpit100.c
> new file mode 100644
> index 000000000000..cf83e8702d60
> --- /dev/null
> +++ b/drivers/pwm/pwm-atcpit100.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Please add a comment here to list hardware limitations (such that the
gist is available from the output in
sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
. Check how other newer driver do it.) Questions to answer there (at
least) include:
- How does the hardware behave when disabled? (Typical: HighZ, constant
0, constant inactive)
- Are there glitches during reconfiguration?
> +#include <linux/err.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +
> +#define ATCPIT100_CHANNEL_MAX 4
> +#define ATCPIT100_CHANNEL_ENABLE 0x1C
> +#define ATCPIT100_CHANNEL_ENABLE_PWM(ch) BIT(3 + (4 * ch))
> +#define ATCPIT100_CHANNEL_CTRL(ch) (0x20 + (0x10 * ch))
> +#define ATCPIT100_CHANNEL_CTRL_MODE_PWM 0x04
> +#define ATCPIT100_CHANNEL_CTRL_CLK BIT(3)
> +#define ATCPIT100_CHANNEL_CTRL_MASK GENMASK(4, 0)
> +#define ATCPIT100_CHANNEL_RELOAD(ch) (0x24 + (0x10 * ch))
> +#define CLK_EXTERNAL 32768
> +#define CLK_APB 60000000
> +#define CYCLE_MIN 0x01
> +#define CYCLE_MAX 0x010000
Please give these last 4 constants a name that makes it clear that they
belong to this driver. I.e. use "ATCPIT100_" as prefix.
CYCLE_MIN is one because you have to write numcycles - 1 to the
respective register, right? I would have use a plain 1 in the
implementation then.
> +struct atcpit100_pwm {
> + struct regmap *regmap;
> + u32 clk_src[ATCPIT100_CHANNEL_MAX];
> +};
> +
> +static const struct regmap_config atcpit100_pwm_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> +};
> +
> +static inline struct atcpit100_pwm *to_atcpit100_pwm(struct pwm_chip *chip)
> +{
> + return pwmchip_get_drvdata(chip);
> +}
> +
> +static int of_atcpit100_pwm_set_clk_src(struct atcpit100_pwm *ap,
> + struct device_node *np)
> +{
> + int ret;
> +
> + for (int i = 0; i < ATCPIT100_CHANNEL_MAX; i++) {
> + ret = of_property_read_u32_index(np, "andestech,clock-source",
> + i, &(ap->clk_src[i]));
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
Instead of having that statically configured in the dtb, it would be
beneficial to switch the parent depending on the requested setting.
> +static int atcpit100_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> + bool enable)
> +{
> + unsigned int channel = pwm->hwpwm;
> + unsigned int enable_bit = ATCPIT100_CHANNEL_ENABLE_PWM(channel);
> + struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
> +
> + pwm->state.enabled = enable;
The pwm core cares for that. Please drop that assignment.
> + return regmap_update_bits(ap->regmap, ATCPIT100_CHANNEL_ENABLE,
> + enable_bit, enable ? enable_bit : 0);
> +}
> +
> +static int atcpit100_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + int ret;
> + u64 period_cycle;
> + u64 duty_cycle;
> + u16 pwm_high;
> + u16 pwm_low;
> + struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
> + unsigned int ctrl_val = 0;
> + unsigned int channel = pwm->hwpwm;
> + u64 rate = ap->clk_src[channel] ? CLK_APB : CLK_EXTERNAL;
Huh, I would have expected a call to clk_get_rate() here.
> + /* cycle count = clock rate * time */
> + period_cycle = mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC);
> + duty_cycle = mul_u64_u64_div_u64(rate, state->duty_cycle,
> + NSEC_PER_SEC);
> + if (period_cycle < CYCLE_MIN || period_cycle > CYCLE_MAX ||
> + duty_cycle < CYCLE_MIN || duty_cycle > CYCLE_MAX) {
> + dev_err(pwmchip_parent(chip),
> + "channel%d: period cycles = 0x%llx, duty cycles = 0x%llx\n",
> + channel, period_cycle, duty_cycle);
> + return -EINVAL;
> + }
Don't error out on period_cycle > CYCLE_MAX or duty_cycle > CYCLE_MAX.
Just continue with period_cycle = CYCLE_MAX (and duty_cycle = CYCLE_MAX
resp.).
> + /*
> + * In the PWM mode, the high period is (PWM16_Hi + 1) cycles, and the
> + * low period is (PWM16_Lo + 1) cycles.
> + * For example, if period is 30 cycles and duty_cycle is 10 cycles,
> + * PWM16_Hi = 10 - 1 = 9, PWM16_Lo = 30 - 10 - 1 = 19.
> + */
> + pwm_high = duty_cycle - 1;
> + pwm_low = period_cycle - duty_cycle - 1;
If period_cycle == duty_cycle surprising things happen?
I guess the hardware can neither do a 0% nor a 100% relative duty cycle?
That's something to document in the above mentioned Limitations
paragraph.
> +
> + /* Set control register. */
> + ctrl_val |= ATCPIT100_CHANNEL_CTRL_MODE_PWM;
> + ctrl_val |= ap->clk_src[channel] ? ATCPIT100_CHANNEL_CTRL_CLK : 0;
> + ret = regmap_update_bits(ap->regmap, ATCPIT100_CHANNEL_CTRL(channel),
> + ATCPIT100_CHANNEL_CTRL_MASK, ctrl_val);
> + if (ret)
> + return ret;
What happens to the output here? I guess it might already change and so
it might emit a wave form that is neither the old nor the new one?
(=> Limitations)
> +
> + /* Set reload register. */
> + ret = regmap_write(ap->regmap, ATCPIT100_CHANNEL_RELOAD(channel),
> + (pwm_high << 16) | pwm_low);
Please define proper bitfield accessors. E.g.
ATCPIT100_CHANNEL_RELOAD_HIGH GENMASK(31, 16)
...
and then use FIELD_PREP to assign.
> + if (ret)
> + return ret;
> +
> + return 0;
This can be abbreviated to
return regmap_write(....);
> +}
> +
> +static int atcpit100_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + int ret;
> +
> + /* ATCPIT100 PWM driver now only supports normal polarity. */
> + if (state->polarity != PWM_POLARITY_NORMAL) {
> + dev_err(pwmchip_parent(chip),
> + "only supports normal polarity now\n");
No error message in .apply() please. Returning an error code is enough.
> + return -EINVAL;
> + }
> +
> + if (!state->enabled) {
> + if (pwm->state.enabled)
> + return atcpit100_pwm_enable(chip, pwm, 0);
> +
> + return 0;
> + }
> +
> + ret = atcpit100_pwm_config(chip, pwm, state);
> + if (ret)
> + return ret;
> +
> + return atcpit100_pwm_enable(chip, pwm, 1);
> +}
> +
> +static int atcpit100_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + int ret;
> + unsigned int reload_val;
> + u16 pwm_high;
> + u16 pwm_low;
> + unsigned int channel = pwm->hwpwm;
> + struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
> + u64 rate = ap->clk_src[channel] ? CLK_APB : CLK_EXTERNAL;
> +
> + state->enabled =
> + regmap_test_bits(ap->regmap, ATCPIT100_CHANNEL_ENABLE,
> + ATCPIT100_CHANNEL_ENABLE_PWM(channel));
> + state->polarity = PWM_POLARITY_NORMAL;
> + ret = regmap_read(ap->regmap, ATCPIT100_CHANNEL_RELOAD(channel),
> + &reload_val);
> + if (ret)
> + return ret;
> +
> + pwm_high = reload_val >> 16;
> + pwm_low = reload_val & 0xFFFF;
> + state->duty_cycle = mul_u64_u64_div_u64(pwm_high + 1, NSEC_PER_SEC,
> + rate);
> + state->period = mul_u64_u64_div_u64(pwm_low + pwm_high + 1,
> + NSEC_PER_SEC, rate);
> +
Please enable PWM_DEBUG and test extensively. Hint: You have to round up
here.
> + return 0;
> +}
> +
> +static const struct pwm_ops atcpit_pwm_ops = {
> + .apply = atcpit100_pwm_apply,
> + .get_state = atcpit100_pwm_get_state,
> +};
> +
> +static int atcpit100_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct atcpit100_pwm *ap;
> + struct pwm_chip *chip;
> + void __iomem *base;
> + int ret;
> +
> + chip = devm_pwmchip_alloc(dev, ATCPIT100_CHANNEL_MAX, sizeof(*ap));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + ap = to_atcpit100_pwm(chip);
> +
> + /*
> + * Each channel can select two different clock sources by toggling the
> + * third bit in its control register. 0 means using an external clock,
> + * and 1 means using APB clock from APB bus. Select the clock source for
> + * each channel by DTS.
> + */
> + ret = of_atcpit100_pwm_set_clk_src(ap, np);
> + if (ret)
> + return ret;
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + ap->regmap = devm_regmap_init_mmio(dev, base,
> + &atcpit100_pwm_regmap_config);
> + if (IS_ERR(ap->regmap))
> + return dev_err_probe(dev, PTR_ERR(ap->regmap),
> + "failed to init register map\n");
> +
> + chip->ops = &atcpit_pwm_ops;
> + ret = devm_pwmchip_add(dev, chip);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to add PWM chip\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id atcpit100_pwm_dt[] = {
> + { .compatible = "andestech,atcpit100-pwm" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, atcpit100_pwm_dt);
> +
> +static struct platform_driver atcpit100_pwm_driver = {
> + .driver = {
> + .name = "atcpit100-pwm",
> + .of_match_table = atcpit100_pwm_dt,
> + },
> + .probe = atcpit100_pwm_probe,
> +};
> +module_platform_driver(atcpit100_pwm_driver);
empty new line here
> +MODULE_AUTHOR("Andes Technology Corporation <ben717@xxxxxxxxxxxxx>");
I would have expected your name here given that's also your email
address.
> +MODULE_DESCRIPTION("Andes ATCPIT100 PWM driver");
> +MODULE_LICENSE("GPL");
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
