Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 12475 bytes --]

Hello Ban,

On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> v1->v2:

FTR: v1 wasn't sent to any list, so don't try to find it in some
archive.

> diff --git a/drivers/pwm/pwm-sun50i.c b/drivers/pwm/pwm-sun50i.c
> new file mode 100644
> index 000000000000..37285d771924
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun50i.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for Allwinner sun50i Pulse Width Modulation Controller
> + *
> + * Copyright (C) 2020 Ban Tao <fengzheng923@gmail.com>
> + */

Please add a section here about how this PWM behaves. Things to cover:

 - Mention if the hardware cannot do 0% or 100%
 - Describe if changing the settings is racy, e.g. if it can happen when
   switching from duty_cycle=100 + period=1000 to duty_cycle=200 +
   period=2000 that we see a period with duty_cycle=100 and period=2000
   or similar
 - Tell if on a reconfiguration the currently running period is
   completed.

Please stick to the format used in other drivers to simplify grepping,
see the Limitations section in drivers/pwm/pwm-sl28cpld.c for an
example.

> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +
> +#define PWM_GET_CLK_OFFSET(chan)	(0x20 + ((chan >> 1) * 0x4))
> +#define PWM_CLK_APB_SCR			BIT(7)
> +#define PWM_DIV_M			0
> +#define PWM_DIV_M_WIDTH			0x4
> +
> +#define PWM_CLK_REG			0x40
> +#define PWM_CLK_GATING			BIT(0)
> +
> +#define PWM_ENABLE_REG			0x80
> +#define PWM_EN				BIT(0)
> +
> +#define PWM_CTL_REG(chan)		(0x100 + 0x20 * chan)
> +#define PWM_ACT_STA			BIT(8)
> +#define PWM_PRESCAL_K			0
> +#define PWM_PRESCAL_K_WIDTH		0x8
> +
> +#define PWM_PERIOD_REG(chan)		(0x104 + 0x20 * chan)
> +#define PWM_ENTIRE_CYCLE		16
> +#define PWM_ENTIRE_CYCLE_WIDTH		0x10
> +#define PWM_ACT_CYCLE			0
> +#define PWM_ACT_CYCLE_WIDTH		0x10

Please use a driver specific prefix for these defines.

> +
> +#define BIT_CH(bit, chan)		((bit) << (chan))
> +
> +#define SETMASK(width, shift)		((width?((-1U) >> (32-width)):0)  << (shift))
> +#define CLRMASK(width, shift)		(~(SETMASK(width, shift)))
> +#define GET_BITS(shift, width, reg)     \
> +	    (((reg) & SETMASK(width, shift)) >> (shift))
> +#define SET_BITS(shift, width, reg, val) \
> +	    (((reg) & CLRMASK(width, shift)) | (val << (shift)))

You're reinventing the stuff from <linux/bitfield.h> here. Please don't.

> +
> +#define PWM_OSC_CLK			24000000
> +#define PWM_PRESCALER_MAX		256
> +#define PWM_CLK_DIV_M__MAX		9
> +#define PWM_ENTIRE_CYCLE_MAX		65536
> +
> +struct sun50i_pwm_data {
> +	unsigned int npwm;
> +};
> +
> +struct sun50i_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	struct reset_control *rst_clk;
> +	void __iomem *base;
> +	const struct sun50i_pwm_data *data;
> +};
> +
> +static inline struct sun50i_pwm_chip *sun50i_pwm_from_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct sun50i_pwm_chip, chip);
> +}
> +
> +static inline u32 sun50i_pwm_readl(struct sun50i_pwm_chip *chip,
> +				  unsigned long offset)
> +{
> +	return readl(chip->base + offset);
> +}
> +
> +static inline void sun50i_pwm_writel(struct sun50i_pwm_chip *chip,
> +				    u32 val, unsigned long offset)
> +{
> +	writel(val, chip->base + offset);
> +}
> +
> +static int sun50i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				enum pwm_polarity polarity)
> +{
> +	struct sun50i_pwm_chip *sun50i_pwm = sun50i_pwm_from_chip(chip);
> +	u32 temp;
> +
> +	temp = sun50i_pwm_readl(sun50i_pwm, PWM_CTL_REG(pwm->hwpwm));
> +
> +	if (polarity == PWM_POLARITY_NORMAL)
> +		temp |= PWM_ACT_STA;
> +	else
> +		temp &= ~PWM_ACT_STA;
> +
> +	sun50i_pwm_writel(sun50i_pwm, temp, PWM_CTL_REG(pwm->hwpwm));
> +
> +	return 0;
> +}

For v1 I asked to test this driver with PWM_DEBUG enabled and to fix all
emitted warnings. This didn't happen :-\

> +static int sun50i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +		int duty_ns, int period_ns)

Please align to the opening brace in the line above.

> +{
> +	struct sun50i_pwm_chip *sun50i_pwm = sun50i_pwm_from_chip(chip);
> +	unsigned long long c;
> +	unsigned long entire_cycles, active_cycles;
> +	unsigned int div_m, prescaler;
> +	u32 tmp;
> +
> +	if (period_ns > 334) {
> +		/* if freq < 3M, then select 24M clock */
> +		c = PWM_OSC_CLK;
> +		tmp = sun50i_pwm_readl(sun50i_pwm, PWM_GET_CLK_OFFSET(pwm->hwpwm));
> +		tmp &= ~PWM_CLK_APB_SCR;
> +		sun50i_pwm_writel(sun50i_pwm, tmp, PWM_GET_CLK_OFFSET(pwm->hwpwm));
> +	} else {
> +		/* if freq > 3M, then select APB as clock */
> +		c = clk_get_rate(sun50i_pwm->clk);
> +		tmp = sun50i_pwm_readl(sun50i_pwm, PWM_GET_CLK_OFFSET(pwm->hwpwm));
> +		tmp |= PWM_CLK_APB_SCR;
> +		sun50i_pwm_writel(sun50i_pwm, tmp, PWM_GET_CLK_OFFSET(pwm->hwpwm));
> +	}
> +
> +	dev_dbg(chip->dev, "duty_ns=%d period_ns=%d c =%llu.\n",
> +			duty_ns, period_ns, c);

Inconsistent spacing in the message.

> +
> +	/*
> +	 * (clk / div_m / prescaler) / entire_cycles = NSEC_PER_SEC / period_ns.
> +	 * So, entire_cycles = clk * period_ns / NSEC_PER_SEC / div_m / prescaler.
> +	 */
> +	c = c * period_ns;
> +	do_div(c, NSEC_PER_SEC);
> +	for (div_m = 0; div_m < PWM_CLK_DIV_M__MAX; div_m++) {
> +		for (prescaler = 0; prescaler < PWM_PRESCALER_MAX; prescaler++) {

The calculation could be done without this double-for loop. Something
like:

	div_m = order_base_2(PWM_ENTIRE_CYCLE_MAX * PWM_PRESCALER_MAX / c);
	if (div_m >= PWM_CLK_DIV_M__MAX)
		bail out;

	prescaler = DIV_ROUND_UP(c << div_m, PWM_ENTIRE_CYCLE_MAX) - 1;

(but please double check, I didn't.) Also note that this doesn't yield
the best approximation for period in general. (But that's ok for now,
maybe just mention it in a comment.)

> +			/*
> +			 * actual prescaler = prescaler + 1.
> +			 * actual div_m = 0x1 << div_m.
> +			*/
> +			entire_cycles = ((unsigned long)c/(0x1 << div_m))/(prescaler + 1);

c / (1 << div_m) can be written as c >> div_m which reads better and
might result in better code.

> +			if (entire_cycles <= PWM_ENTIRE_CYCLE_MAX) {
> +				goto calc_end;
> +			}

No { } for one line bodys please.

> +		}
> +	}

Missing error handling for the case that

	c >> (PWM_CLK_DIV_M__MAX - 1) / PWM_PRESCALER_MAX > PWM_ENTIRE_CYCLE_MAX

> +
> +calc_end:
> +	/*
> +	 * duty_ns / period_ns = active_cycles / entire_cycles.
> +	 * So, active_cycles = entire_cycles * duty_ns / period_ns.

active_cyles is the number of clock steps for duty_cycle, right?

> +	 */
> +	c = (unsigned long long)entire_cycles * duty_ns;
> +	do_div(c, period_ns);
> +	active_cycles = c;
> +	if (entire_cycles == 0)
> +		entire_cycles++;
> +
> +	/* enable clk gating */
> +	tmp = sun50i_pwm_readl(sun50i_pwm, PWM_CLK_REG);
> +	tmp |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +	sun50i_pwm_writel(sun50i_pwm, tmp, PWM_CLK_REG);
> +
> +	/* config  clk div_m*/
> +	tmp = sun50i_pwm_readl(sun50i_pwm, PWM_GET_CLK_OFFSET(pwm->hwpwm));
> +	tmp = SET_BITS(PWM_DIV_M, PWM_DIV_M_WIDTH, tmp, div_m);
> +	sun50i_pwm_writel(sun50i_pwm, tmp, PWM_GET_CLK_OFFSET(pwm->hwpwm));
> +
> +	/* config prescal */

prescale

> +	tmp = sun50i_pwm_readl(sun50i_pwm, PWM_CTL_REG(pwm->hwpwm));
> +	tmp = SET_BITS(PWM_PRESCAL_K, PWM_PRESCAL_K_WIDTH, tmp, prescaler);
> +	sun50i_pwm_writel(sun50i_pwm, tmp, PWM_CTL_REG(pwm->hwpwm));
> +
> +	/* config active and period cycles */
> +	tmp = sun50i_pwm_readl(sun50i_pwm, PWM_PERIOD_REG(pwm->hwpwm));
> +	tmp = SET_BITS(PWM_ACT_CYCLE, PWM_ACT_CYCLE_WIDTH, tmp, active_cycles);
> +	tmp = SET_BITS(PWM_ENTIRE_CYCLE, PWM_ENTIRE_CYCLE_WIDTH, tmp, (entire_cycles - 1));
> +	sun50i_pwm_writel(sun50i_pwm, tmp, PWM_PERIOD_REG(pwm->hwpwm));
> +
> +	dev_dbg(chip->dev, "active_cycles=%lu entire_cycles=%lu prescaler=%u div_m=%u\n",
> +			active_cycles, entire_cycles, prescaler, div_m);

align to opening brace.

> +
> +	return 0;
> +}
> +
> +static int sun50i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct sun50i_pwm_chip *sun50i_pwm = sun50i_pwm_from_chip(chip);
> +	u32 tmp;
> +
> +	/* enable pwm controller */
> +	tmp = sun50i_pwm_readl(sun50i_pwm, PWM_ENABLE_REG);
> +	tmp |= BIT_CH(PWM_EN, pwm->hwpwm);
> +	sun50i_pwm_writel(sun50i_pwm, tmp, PWM_ENABLE_REG);
> +
> +	tmp = sun50i_pwm_readl(sun50i_pwm, PWM_CLK_REG);
> +	tmp |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +	sun50i_pwm_writel(sun50i_pwm, tmp, PWM_CLK_REG);
> +
> +	return 0;
> +}
> +
> +static void sun50i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct sun50i_pwm_chip *sun50i_pwm = sun50i_pwm_from_chip(chip);
> +	u32 tmp;
> +
> +	/* disable pwm controller */
> +	tmp = sun50i_pwm_readl(sun50i_pwm, PWM_ENABLE_REG);
> +	tmp &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> +	sun50i_pwm_writel(sun50i_pwm, tmp, PWM_ENABLE_REG);
> +
> +	tmp = sun50i_pwm_readl(sun50i_pwm, PWM_CLK_REG);
> +	tmp &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +	sun50i_pwm_writel(sun50i_pwm, tmp, PWM_CLK_REG);
> +}
> +
> +static const struct pwm_ops sun50i_pwm_ops = {
> +	.config = sun50i_pwm_config,
> +	.enable = sun50i_pwm_enable,
> +	.disable = sun50i_pwm_disable,
> +	.set_polarity = sun50i_pwm_set_polarity,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct sun50i_pwm_data sun8i_pwm_data_c9 = {
> +	.npwm = 9,
> +};
> +
> +static const struct sun50i_pwm_data sun50i_pwm_data_c16 = {
> +	.npwm = 16,
> +};
> +
> +static const struct of_device_id sun50i_pwm_dt_ids[] = {
> +	{
> +		.compatible = "allwinner,sun8i-v833-pwm",
> +		.data = &sun8i_pwm_data_c9,
> +	}, {
> +		.compatible = "allwinner,sun8i-v536-pwm",
> +		.data = &sun8i_pwm_data_c9,
> +	}, {
> +		.compatible = "allwinner,sun50i-r818-pwm",
> +		.data = &sun50i_pwm_data_c16,
> +	}, {
> +		.compatible = "allwinner,sun50i-a133-pwm",
> +		.data = &sun50i_pwm_data_c16,
> +	}, {
> +		.compatible = "allwinner,sun50i-r329-pwm",
> +		.data = &sun8i_pwm_data_c9,
> +	}, {
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, sun50i_pwm_dt_ids);
> +
> +static int sun50i_pwm_probe(struct platform_device *pdev)
> +{
> +	struct sun50i_pwm_chip *pwm;

"pwm" isn't a good name, in PWM code this name is usually used for
struct pwm_device pointers (and sometimes the global pwm id). I usually
use "ddata" for driver data (and would have called "sun50i_pwm_chip"
"sun50i_pwm_ddata" instead). Another common name is "priv".

> +	int ret;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->data = of_device_get_match_data(&pdev->dev);
> +	if (!pwm->data)

Error message here

> +		return -ENODEV;
> +
> +	pwm->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pwm->base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pwm->base),
> +				     "can't remap pwm resource\n");
> +
> +	pwm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk),
> +				     "get unnamed clock failed\n");

s/unnamed/PWM/ ?

> +
> +	pwm->rst_clk = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(pwm->rst_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pwm->rst_clk),
> +				     "get reset failed\n");
> +
> +	/* Deassert reset */
> +	ret = reset_control_deassert(pwm->rst_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot deassert reset control: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;

Even though reset_control_deassert probably will never return
-EPROBE_DEFER, you should use dev_err_probe here to for consistency.

> +	}
> +
> +	ret = clk_prepare_enable(pwm->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot prepare and enable clk %pe\n",
> +			ERR_PTR(ret));
> +		goto err_clk;
> +	}
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &sun50i_pwm_ops;
> +	pwm->chip.npwm = pwm->data->npwm;
> +	pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> +	pwm->chip.base = -1;
> +	pwm->chip.of_pwm_n_cells = 3;
> +
> [...]

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]