Message-ID: <3sd27d74rp4nb7h23dquynuytzsipceeue4b46dm6ti4yyuu57@txg5n3u5ztn6>
Date: Mon, 28 Oct 2024 14:38:38 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Ben Zong-You Xie <ben717@...estech.com>
Cc: linux-pwm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Subject: Re: [PATCH 2/2] pwm: atcpit100: add Andes PWM driver support

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@...estech.com>");

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists