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

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

[Ban Tao]

From: Ban Tao <fengzheng923@gmail.com>

The Allwinner R818, A133, R329, V536 and V833 has a new PWM controller
IP compared to the older Allwinner SoCs.

Signed-off-by: Ban Tao <fengzheng923@gmail.com>

---
v1->v2:
1.delete unnecessary code.
2.using a named define for some constants.
3.Add comment in sun50i_pwm_config function.
4.using dev_err_probe() for error handling.
5.disable the clock after pwmchip_remove().
---
 MAINTAINERS              |   6 +
 drivers/pwm/Kconfig      |  11 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-sun50i.c | 348 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 366 insertions(+)
 create mode 100644 drivers/pwm/pwm-sun50i.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e73636b75f29..d33cf1b69b43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -737,6 +737,12 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/staging/media/sunxi/cedrus/
 
+ALLWINNER PWM DRIVER
+M:	Ban Tao <fengzheng923@gmail.com>
+L:	linux-pwm@vger.kernel.org
+S:	Maintained
+F:	drivers/pwm/pwm-sun50i.c
+
 ALPHA PORT
 M:	Richard Henderson <rth@twiddle.net>
 M:	Ivan Kokshaysky <ink@jurassic.park.msu.ru>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 9a4f66ae8070..17635a8f2ed3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -552,6 +552,17 @@ config PWM_SUN4I
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sun4i.
 
+config PWM_SUN50I
+	tristate "Allwinner enhanced PWM support"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	depends on HAS_IOMEM && COMMON_CLK
+	help
+	  Enhanced PWM framework driver for Allwinner R818, A133, R329,
+	  V536 and V833 SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sun50i.
+
 config PWM_TEGRA
 	tristate "NVIDIA Tegra PWM support"
 	depends on ARCH_TEGRA || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 6374d3b1d6f3..b4754927fd8f 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
 obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
 obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
+obj-$(CONFIG_PWM_SUN50I)	+= pwm-sun50i.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
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>
+ */
+
+#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
+
+#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)))
+
+#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;
+}
+
+static int sun50i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+		int duty_ns, int period_ns)
+{
+	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);
+
+	/*
+	 * (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++) {
+			/*
+			 * actual prescaler = prescaler + 1.
+			 * actual div_m = 0x1 << div_m.
+			*/
+			entire_cycles = ((unsigned long)c/(0x1 << div_m))/(prescaler + 1);
+			if (entire_cycles <= PWM_ENTIRE_CYCLE_MAX) {
+				goto calc_end;
+			}
+		}
+	}
+
+calc_end:
+	/*
+	 * duty_ns / period_ns = active_cycles / entire_cycles.
+	 * So, active_cycles = entire_cycles * duty_ns / period_ns.
+	 */
+	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 */
+	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);
+
+	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;
+	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)
+		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");
+
+	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;
+	}
+
+	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;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+		goto err_pwm_add;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+
+err_pwm_add:
+	clk_disable_unprepare(pwm->clk);
+err_clk:
+	reset_control_assert(pwm->rst_clk);
+
+	return ret;
+}
+
+static int sun50i_pwm_remove(struct platform_device *pdev)
+{
+	struct sun50i_pwm_chip *pwm = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&pwm->chip);
+	if (ret)
+		return ret;
+
+	clk_disable_unprepare(pwm->clk);
+	reset_control_assert(pwm->rst_clk);
+
+	return 0;
+}
+
+static struct platform_driver sun50i_pwm_driver = {
+	.driver = {
+		.name = "sun50i-pwm",
+		.of_match_table = sun50i_pwm_dt_ids,
+	},
+	.probe = sun50i_pwm_probe,
+	.remove = sun50i_pwm_remove,
+};
+module_platform_driver(sun50i_pwm_driver);
+
+MODULE_ALIAS("platform:sun50i-pwm");
+MODULE_AUTHOR("Ban Tao <fengzheng923@gmail.com>");
+MODULE_DESCRIPTION("Allwinner sun50i PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.22.0.windows.1

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

[Uwe Kleine-König]

[-- 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 --]

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

[Thierry Reding]

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

On Wed, Feb 03, 2021 at 04:12:00PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
[...]
> > +#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.

These are nice and to the point, so I think it'd be fine to leave these
as-is. Unless of course if they conflict with something from the PWM
core, which I don't think any of these do.

> > +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);
> > +}

I wanted to reply to Uwe's comment on v1 but you sent this before I got
around to it, so I'll mention it here. I recognize the usefulness of
having a common prefix for function names, though admittedly it's not
totally necessary in these drivers because these are all local symbols.
But it does makes things a bit consistent and helps when looking at
backtraces and such, so that's all good.

That said, I consider these casting helpers a bit of a special case
because they usually won't show up in backtraces, or anywhere else for
that matter. Traditionally these have been named to_*(), so it'd be
entirely consistent to keep doing that.

But I don't have any strong objections to this variant either, so pick
whichever you want. Although, please, nobody take that as a hint that
any existing helpers should be converted.

[...]
> > +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".

This driver already declares sun50i_pwm_data for per-SoC data, so having
a struct sun50i_pwm_ddata would just be confusing. sun50i_pwm_chip is
consistent with what other drivers name this, so I think that's fine.

Agreed on "pwm" being a bad choice here, though.

Thierry

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

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

[Uwe Kleine-König]

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

Hello Thierry, hello Ban,

On Wed, Feb 03, 2021 at 05:33:16PM +0100, Thierry Reding wrote:
> On Wed, Feb 03, 2021 at 04:12:00PM +0100, Uwe Kleine-König wrote:
> > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> [...]
> > > +#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.
> 
> These are nice and to the point, so I think it'd be fine to leave these
> as-is. Unless of course if they conflict with something from the PWM
> core, which I don't think any of these do.

In my eyes PWM_CLK_REG, PWM_CLK_GATING, PWM_ENABLE_REG and PWM_EN are
not so nice. pwm-sun4i.c has already PWM_EN, pwm-mtk-disp.c has
DISP_PWM_EN, pwm-zx.c has ZX_PWM_EN, pwm-berlin.c has BERLIN_PWM_EN.
Luckily most of them already use a prefix. So it doesn't conflict with
the core, but might with other drivers. And also if you only care about
conflicts with the core, using a prefix is the future-proof strategy.
For tools like ctags and cscope a unique name is great, too.

Also comparing

	tmp |= BIT_CH(PWM_EN, pwm->hwpwm);

with (say)

	tmp |= BIT_CH(SUN5I_PWM_PWM_EN, pwm->hwpwm);

I prefer the latter as it is obvious that this is a driver specific
constant.

Having said that, I'm also a fan of having the register name in the bit
field define to simplify spotting errors like 4b75f8000361 ("serial:
imx: Fix DCD reading").

So looking at:

+       /* 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);

my preferred version would be instead:

+       /* disable pwm controller */
+       enable = sun50i_pwm_readl(sun50i_pwm, SUN50I_REG_PWM_ENABLE);
+       enable &= ~SUN50I_REG_PWM_ENABLE_EN(pwm->hwpwm);
+       sun50i_pwm_writel(sun50i_pwm, enable, SUN50I_REG_PWM_ENABLE);

where variable name, register name and bitfield name are obviously
matching.

> > > +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);
> > > +}
> 
> I wanted to reply to Uwe's comment on v1 but you sent this before I got
> around to it, so I'll mention it here. I recognize the usefulness of
> having a common prefix for function names, though admittedly it's not
> totally necessary in these drivers because these are all local symbols.
> But it does makes things a bit consistent and helps when looking at
> backtraces and such, so that's all good.
> 
> That said, I consider these casting helpers a bit of a special case
> because they usually won't show up in backtraces, or anywhere else for
> that matter. Traditionally these have been named to_*(), so it'd be
> entirely consistent to keep doing that.
> 
> But I don't have any strong objections to this variant either, so pick
> whichever you want. Although, please, nobody take that as a hint that
> any existing helpers should be converted.

@Thierry: I don't understand your motivation to write this. For me
consistence is a quite important aspect. Obviously for you it's less so
and the code presented here over-fulfils your standards. So everything
is fine as is, isn't it?

I think using a rule like "A common prefix for driver specific functions"
consistently is easier than allowing some exceptions. There is no gain
in not using the prefix, so IMHO keep the rules simple without
exceptions.

> [...]
> > > +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".
> 
> This driver already declares sun50i_pwm_data for per-SoC data, so having
> a struct sun50i_pwm_ddata would just be confusing.

Agreed. So maybe pick a better name for sun50i_pwm_data; my suggestion
would be sun50i_pwm_soc_info.

> sun50i_pwm_chip is consistent with what other drivers name this, so I
> think that's fine.

Either the name is good, then this alone justifies to use it. If however
the name is bad, it IMHO doesn't matter if others use the same bad
naming scheme and you better don't use it. So please don't let us
consider it a good name *only* because it's used in several other
places.

@Ban: You see Thierry and I don't agree in every aspect. So please take
the feedback you get from us as cause for thought and then try to come
up with a v3 with choices you can justify.

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 --]

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

[Thierry Reding]

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

On Wed, Feb 03, 2021 at 09:59:13PM +0100, Uwe Kleine-König wrote:
> Hello Thierry, hello Ban,
> 
> On Wed, Feb 03, 2021 at 05:33:16PM +0100, Thierry Reding wrote:
> > On Wed, Feb 03, 2021 at 04:12:00PM +0100, Uwe Kleine-König wrote:
> > > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> > [...]
> > > > +#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.
> > 
> > These are nice and to the point, so I think it'd be fine to leave these
> > as-is. Unless of course if they conflict with something from the PWM
> > core, which I don't think any of these do.
> 
> In my eyes PWM_CLK_REG, PWM_CLK_GATING, PWM_ENABLE_REG and PWM_EN are
> not so nice. pwm-sun4i.c has already PWM_EN, pwm-mtk-disp.c has
> DISP_PWM_EN, pwm-zx.c has ZX_PWM_EN, pwm-berlin.c has BERLIN_PWM_EN.
> Luckily most of them already use a prefix. So it doesn't conflict with
> the core, but might with other drivers.

"Conflicts with another driver" is not something that makes sense. By
definition drivers should be specific to a given device, so you better
make sure there's no namespace sharing between them.

I'm fine if somebody wants to use a prefix, but I'm also fine if they
don't use a prefix, provided that there are no conflicts, which should
be immediately obvious because it typically causes build errors.

All I'm saying is that I don't think we need to require everyone to
adopt a prefix, especially if this hasn't been followed consistently,
because then we don't actually gain anything.

>                                         And also if you only care about
> conflicts with the core, using a prefix is the future-proof strategy.

It's not like these symbol names are carved in stone. If we ever
introduce a symbol in the PWM core that happens to conflicts with one or
more drivers, we can always fix the drivers at that point.

> For tools like ctags and cscope a unique name is great, too.
> 
> Also comparing
> 
> 	tmp |= BIT_CH(PWM_EN, pwm->hwpwm);
> 
> with (say)
> 
> 	tmp |= BIT_CH(SUN5I_PWM_PWM_EN, pwm->hwpwm);
> 
> I prefer the latter as it is obvious that this is a driver specific
> constant.

I think the context makes it plenty clear that this is driver specific,
so the prefix is a bit redundant.

> Having said that, I'm also a fan of having the register name in the bit
> field define to simplify spotting errors like 4b75f8000361 ("serial:
> imx: Fix DCD reading").
> 
> So looking at:
> 
> +       /* 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);
> 
> my preferred version would be instead:
> 
> +       /* disable pwm controller */
> +       enable = sun50i_pwm_readl(sun50i_pwm, SUN50I_REG_PWM_ENABLE);
> +       enable &= ~SUN50I_REG_PWM_ENABLE_EN(pwm->hwpwm);
> +       sun50i_pwm_writel(sun50i_pwm, enable, SUN50I_REG_PWM_ENABLE);
> 
> where variable name, register name and bitfield name are obviously
> matching.

I don't have any particular objection to the above, but I've also seen
this kind of naming result in ridiculously long (as in almost impossible
to read) lines, so I think we need to strike the right balance. Given
that there aren't any rigorous rules for this, I don't think it's worth
requiring everyone to adopt some style that's not consistently followed
anyway.

> > > > +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);
> > > > +}
> > 
> > I wanted to reply to Uwe's comment on v1 but you sent this before I got
> > around to it, so I'll mention it here. I recognize the usefulness of
> > having a common prefix for function names, though admittedly it's not
> > totally necessary in these drivers because these are all local symbols.
> > But it does makes things a bit consistent and helps when looking at
> > backtraces and such, so that's all good.
> > 
> > That said, I consider these casting helpers a bit of a special case
> > because they usually won't show up in backtraces, or anywhere else for
> > that matter. Traditionally these have been named to_*(), so it'd be
> > entirely consistent to keep doing that.
> > 
> > But I don't have any strong objections to this variant either, so pick
> > whichever you want. Although, please, nobody take that as a hint that
> > any existing helpers should be converted.
> 
> @Thierry: I don't understand your motivation to write this.

I'm writing this because I think your recommendation was the wrong thing
to do here. Ban was obviously doing what *all* other drivers are doing
for this casting helper, so why should this one be different?

>                                                             For me
> consistence is a quite important aspect. Obviously for you it's less so
> and the code presented here over-fulfils your standards. So everything
> is fine as is, isn't it?

Like I said, 100% of the PWM drivers use to_*() for these, so how is it
consistent if we now start to change that?

What I was trying to say is that I would've been fine with this if the
original patch had named this sun50i_pwm_from_chip(). It wouldn't have
been consistent, but I would probably not have objected. However, the
original patch was making this consistent and then you suggested to
change it so that it became inconsistent.

I want to avoid a situation where there's a quasi-standard rule, even if
it's not written down anywhere, and then out of nowhere we change the
rule, for no obvious reason.

> I think using a rule like "A common prefix for driver specific functions"
> consistently is easier than allowing some exceptions. There is no gain
> in not using the prefix, so IMHO keep the rules simple without
> exceptions.

That's a matter of perspective. The way I see it, the simple rule here
is that these casting helpers should be named to_*(). Like I said, that
is what every single one of the existing drivers in drivers/pwm does, so
using the driver prefix breaks the rule and make things inconsistent.

> > [...]
> > > > +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".
> > 
> > This driver already declares sun50i_pwm_data for per-SoC data, so having
> > a struct sun50i_pwm_ddata would just be confusing.
> 
> Agreed. So maybe pick a better name for sun50i_pwm_data; my suggestion
> would be sun50i_pwm_soc_info.

Yeah, I think that'd make it much clearer what this is used for.

> > sun50i_pwm_chip is consistent with what other drivers name this, so I
> > think that's fine.
> 
> Either the name is good, then this alone justifies to use it. If however
> the name is bad, it IMHO doesn't matter if others use the same bad
> naming scheme and you better don't use it. So please don't let us
> consider it a good name *only* because it's used in several other
> places.

I don't consider this naming scheme good just because it's consistent.
The reason why it is used fairly consistently is because I think the
name is good and I've recommended it to be used when people chose what
I thought were bad names.

Now there are other cases that don't follow this scheme and that's fine
too. I think it's okay to let people be creative every once in a while.
As long as everybody understands what the purpose is and as long as it
doesn't conflict with anything or confuses anyone, that's fine with me.

> @Ban: You see Thierry and I don't agree in every aspect. So please take
> the feedback you get from us as cause for thought and then try to come
> up with a v3 with choices you can justify.

That's a bit unfair. The purpose of review is to identify things that
need to be changed. If we go around giving people a bunch of options and
tell them to pick one of them and try again isn't terribly helpful.

So I think either we have to write down the rules to make sure everybody
is on the same page, or we let people look at existing code and pick up
the rules. In the latter case we may have to accept that people have
different preferences, and not everything may be 100% consistent. I
think that's totally fine. Achieving 100% consistency is wishful
thinking anyway. Even if you manage to achieve it in the PWM subsystem,
you then move to the next subsystem and things are likely to be done
differently there. We might as well just get used to having a bit of
inconsistency.

Thierry

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

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

[Uwe Kleine-König]

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

Hello Thierry,

On Thu, Feb 04, 2021 at 02:38:40PM +0100, Thierry Reding wrote:
> All I'm saying is that I don't think we need to require everyone to
> adopt a prefix, especially if this hasn't been followed consistently,
> because then we don't actually gain anything.

Is "we didn't do this consistently in the past" a reason to never
improve here? I hope it's not ...

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 --]

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

[Thierry Reding]

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

On Thu, Feb 04, 2021 at 02:54:36PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Feb 04, 2021 at 02:38:40PM +0100, Thierry Reding wrote:
> > All I'm saying is that I don't think we need to require everyone to
> > adopt a prefix, especially if this hasn't been followed consistently,
> > because then we don't actually gain anything.
> 
> Is "we didn't do this consistently in the past" a reason to never
> improve here? I hope it's not ...

You're implying that consistently using a prefix is an improvement. I
don't agree, so I don't see a need to introduce any new rules for this.

Thierry

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

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

[Uwe Kleine-König]

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

On Sat, Feb 20, 2021 at 10:18:27AM +0800, 班涛 wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 于2021年2月3日周三 下午11:12写道：
> 
> > 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.
> >
> 
> Sorry, I understand. So is the next patch v3? Or v2?

Using v3 is fine. Please don't send another series that is called v2.

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 --]

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

[Maxime Ripard]

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

Hi,

On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> From: Ban Tao <fengzheng923@gmail.com>
> 
> The Allwinner R818, A133, R329, V536 and V833 has a new PWM controller
> IP compared to the older Allwinner SoCs.
> 
> Signed-off-by: Ban Tao <fengzheng923@gmail.com>

Thanks for your patch. There's a bunch of warnings reported by
checkpatch --strict, they should be addressed.

> ---
> v1->v2:
> 1.delete unnecessary code.
> 2.using a named define for some constants.
> 3.Add comment in sun50i_pwm_config function.
> 4.using dev_err_probe() for error handling.
> 5.disable the clock after pwmchip_remove().
> ---
>  MAINTAINERS              |   6 +
>  drivers/pwm/Kconfig      |  11 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-sun50i.c | 348 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 366 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sun50i.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e73636b75f29..d33cf1b69b43 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -737,6 +737,12 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	drivers/staging/media/sunxi/cedrus/
>  
> +ALLWINNER PWM DRIVER
> +M:	Ban Tao <fengzheng923@gmail.com>
> +L:	linux-pwm@vger.kernel.org
> +S:	Maintained
> +F:	drivers/pwm/pwm-sun50i.c
> +
>  ALPHA PORT
>  M:	Richard Henderson <rth@twiddle.net>
>  M:	Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 9a4f66ae8070..17635a8f2ed3 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -552,6 +552,17 @@ config PWM_SUN4I
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sun4i.
>  
> +config PWM_SUN50I
> +	tristate "Allwinner enhanced PWM support"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	depends on HAS_IOMEM && COMMON_CLK
> +	help
> +	  Enhanced PWM framework driver for Allwinner R818, A133, R329,
> +	  V536 and V833 SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sun50i.
> +

Even though it's unfortunate, there's a bunch of other SoCs part of the
sun50i family that are supported by the sun4i driver.

Which SoC introduced that new design? It's usually the name we pick up
then.

>  config PWM_TEGRA
>  	tristate "NVIDIA Tegra PWM support"
>  	depends on ARCH_TEGRA || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 6374d3b1d6f3..b4754927fd8f 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
>  obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
>  obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
> +obj-$(CONFIG_PWM_SUN50I)	+= pwm-sun50i.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
> 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>
> + */
> +
> +#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
> +
> +#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)))
> +
> +#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;
> +}
> +
> +static int sun50i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +		int duty_ns, int period_ns)
> +{
> +	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);
> +
> +	/*
> +	 * (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++) {
> +			/*
> +			 * actual prescaler = prescaler + 1.
> +			 * actual div_m = 0x1 << div_m.
> +			*/
> +			entire_cycles = ((unsigned long)c/(0x1 << div_m))/(prescaler + 1);
> +			if (entire_cycles <= PWM_ENTIRE_CYCLE_MAX) {
> +				goto calc_end;
> +			}
> +		}
> +	}
> +
> +calc_end:
> +	/*
> +	 * duty_ns / period_ns = active_cycles / entire_cycles.
> +	 * So, active_cycles = entire_cycles * duty_ns / period_ns.
> +	 */
> +	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 */
> +	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);
> +
> +	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);

What are the differences between all these SoCs? If there's none between
the v833, v536 and R329, and between the r818 and the A133, you should
use the same compatible.

The device tree binding must be documented too

Maxime

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

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

[Maxime Ripard]

Hi,

On Thu, Feb 04, 2021 at 11:47:34AM +0800, 班涛 wrote:
> Maxime Ripard <maxime@cerno.tech> 于2021年2月3日周三 下午11:46写道：
> 
> > Hi,
> >
> > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> > > From: Ban Tao <fengzheng923@gmail.com>
> > >
> > > The Allwinner R818, A133, R329, V536 and V833 has a new PWM controller
> > > IP compared to the older Allwinner SoCs.
> > >
> > > Signed-off-by: Ban Tao <fengzheng923@gmail.com>
> >
> > Thanks for your patch. There's a bunch of warnings reported by
> > checkpatch --strict, they should be addressed.
> >
> > > ---
> > > v1->v2:
> > > 1.delete unnecessary code.
> > > 2.using a named define for some constants.
> > > 3.Add comment in sun50i_pwm_config function.
> > > 4.using dev_err_probe() for error handling.
> > > 5.disable the clock after pwmchip_remove().
> > > ---
> > >  MAINTAINERS              |   6 +
> > >  drivers/pwm/Kconfig      |  11 ++
> > >  drivers/pwm/Makefile     |   1 +
> > >  drivers/pwm/pwm-sun50i.c | 348 +++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 366 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sun50i.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e73636b75f29..d33cf1b69b43 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -737,6 +737,12 @@ L:       linux-media@vger.kernel.org
> > >  S:   Maintained
> > >  F:   drivers/staging/media/sunxi/cedrus/
> > >
> > > +ALLWINNER PWM DRIVER
> > > +M:   Ban Tao <fengzheng923@gmail.com>
> > > +L:   linux-pwm@vger.kernel.org
> > > +S:   Maintained
> > > +F:   drivers/pwm/pwm-sun50i.c
> > > +
> > >  ALPHA PORT
> > >  M:   Richard Henderson <rth@twiddle.net>
> > >  M:   Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 9a4f66ae8070..17635a8f2ed3 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -552,6 +552,17 @@ config PWM_SUN4I
> > >         To compile this driver as a module, choose M here: the module
> > >         will be called pwm-sun4i.
> > >
> > > +config PWM_SUN50I
> > > +     tristate "Allwinner enhanced PWM support"
> > > +     depends on ARCH_SUNXI || COMPILE_TEST
> > > +     depends on HAS_IOMEM && COMMON_CLK
> > > +     help
> > > +       Enhanced PWM framework driver for Allwinner R818, A133, R329,
> > > +       V536 and V833 SoCs.
> > > +
> > > +       To compile this driver as a module, choose M here: the module
> > > +       will be called pwm-sun50i.
> > > +
> >
> > Even though it's unfortunate, there's a bunch of other SoCs part of the
> > sun50i family that are supported by the sun4i driver.
> >
> > Which SoC introduced that new design? It's usually the name we pick up
> > then.
> >
> 
> In fact, some SoCs has not been supported by the sun4i driver, such as v833,
> v536, r818, a133 and r329.
> v833 and v536 are part of the sun8i family, but r818, a133 and r329 are
> part of the sun50i family.

Indeed, I missed that sorry. 

> So,I'm confused, how do choose the name of this driver?

Just go for the earliest SoC that introduced that design. What was the
first SoC to use it?

> > > +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);
> >
> > What are the differences between all these SoCs? If there's none between
> > the v833, v536 and R329, and between the r818 and the A133, you should
> > use the same compatible.
> 
> Compared with the sun4i driver, this patch is a completely different PWM IP
> controller.

Sure, I didn't mean to compare it to sun4i. What I meant was that as far
as these struct goes, the A133 and the R818 look to have the same PWM
controller. Just like the v833, v536 and R329.

If the PWM controllers are indeed identical across those SoCs, you can
just use two compatibles, one for the PWM with 9 channels (again, the
earliest SoC among the V833, v536 and r329), and one for the PWM with 16
channels.

None of those SoCs are supported at the moment in Linux though, so it
would make more sense to support them first before adding a new driver
for those SoCs, it's gonna be dead code otherwise.

Maxime

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

[Maxime Ripard]

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

On Sat, Feb 06, 2021 at 09:27:30PM +0800, 班涛 wrote:
> Maxime Ripard <maxime@cerno.tech> 于2021年2月6日周六 上午12:12写道：
> 
> > Hi,
> >
> > On Thu, Feb 04, 2021 at 11:47:34AM +0800, 班涛 wrote:
> > > Maxime Ripard <maxime@cerno.tech> 于2021年2月3日周三 下午11:46写道：
> > >
> > > > Hi,
> > > >
> > > > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> > > > > From: Ban Tao <fengzheng923@gmail.com>
> > > > >
> > > > > The Allwinner R818, A133, R329, V536 and V833 has a new PWM
> > controller
> > > > > IP compared to the older Allwinner SoCs.
> > > > >
> > > > > Signed-off-by: Ban Tao <fengzheng923@gmail.com>
> > > >
> > > > Thanks for your patch. There's a bunch of warnings reported by
> > > > checkpatch --strict, they should be addressed.
> > > >
> > > > > ---
> > > > > v1->v2:
> > > > > 1.delete unnecessary code.
> > > > > 2.using a named define for some constants.
> > > > > 3.Add comment in sun50i_pwm_config function.
> > > > > 4.using dev_err_probe() for error handling.
> > > > > 5.disable the clock after pwmchip_remove().
> > > > > ---
> > > > >  MAINTAINERS              |   6 +
> > > > >  drivers/pwm/Kconfig      |  11 ++
> > > > >  drivers/pwm/Makefile     |   1 +
> > > > >  drivers/pwm/pwm-sun50i.c | 348
> > +++++++++++++++++++++++++++++++++++++++
> > > > >  4 files changed, 366 insertions(+)
> > > > >  create mode 100644 drivers/pwm/pwm-sun50i.c
> > > > >
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > index e73636b75f29..d33cf1b69b43 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -737,6 +737,12 @@ L:       linux-media@vger.kernel.org
> > > > >  S:   Maintained
> > > > >  F:   drivers/staging/media/sunxi/cedrus/
> > > > >
> > > > > +ALLWINNER PWM DRIVER
> > > > > +M:   Ban Tao <fengzheng923@gmail.com>
> > > > > +L:   linux-pwm@vger.kernel.org
> > > > > +S:   Maintained
> > > > > +F:   drivers/pwm/pwm-sun50i.c
> > > > > +
> > > > >  ALPHA PORT
> > > > >  M:   Richard Henderson <rth@twiddle.net>
> > > > >  M:   Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index 9a4f66ae8070..17635a8f2ed3 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -552,6 +552,17 @@ config PWM_SUN4I
> > > > >         To compile this driver as a module, choose M here: the module
> > > > >         will be called pwm-sun4i.
> > > > >
> > > > > +config PWM_SUN50I
> > > > > +     tristate "Allwinner enhanced PWM support"
> > > > > +     depends on ARCH_SUNXI || COMPILE_TEST
> > > > > +     depends on HAS_IOMEM && COMMON_CLK
> > > > > +     help
> > > > > +       Enhanced PWM framework driver for Allwinner R818, A133, R329,
> > > > > +       V536 and V833 SoCs.
> > > > > +
> > > > > +       To compile this driver as a module, choose M here: the module
> > > > > +       will be called pwm-sun50i.
> > > > > +
> > > >
> > > > Even though it's unfortunate, there's a bunch of other SoCs part of the
> > > > sun50i family that are supported by the sun4i driver.
> > > >
> > > > Which SoC introduced that new design? It's usually the name we pick up
> > > > then.
> > > >
> > >
> > > In fact, some SoCs has not been supported by the sun4i driver, such as
> > v833,
> > > v536, r818, a133 and r329.
> > > v833 and v536 are part of the sun8i family, but r818, a133 and r329 are
> > > part of the sun50i family.
> >
> > Indeed, I missed that sorry.
> >
> > > So,I'm confused, how do choose the name of this driver?
> >
> > Just go for the earliest SoC that introduced that design. What was the
> > first SoC to use it?
> >
>
> The V536 SOC first used this design, so, we should use the name
> "sun8i-pwm.c"?

sun8i-pwm would be too generic, but sun8i-v536-pwm would be great then

> 
> > > > > +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);
> > > >
> > > > What are the differences between all these SoCs? If there's none
> > between
> > > > the v833, v536 and R329, and between the r818 and the A133, you should
> > > > use the same compatible.
> > >
> > > Compared with the sun4i driver, this patch is a completely different PWM
> > IP
> > > controller.
> >
> > Sure, I didn't mean to compare it to sun4i. What I meant was that as far
> > as these struct goes, the A133 and the R818 look to have the same PWM
> > controller. Just like the v833, v536 and R329.
> >
> > If the PWM controllers are indeed identical across those SoCs, you can
> > just use two compatibles, one for the PWM with 9 channels (again, the
> > earliest SoC among the V833, v536 and r329), and one for the PWM with 16
> > channels.
> >
> Ok i see what you mean.
>  static const struct of_device_id sun50i_pwm_dt_ids[] = {
>         {
>                 .compatible = "allwinner,sun8i-v536-pwm",
>                 .data = &sun8i_pwm_data_c9,
>         }, {
>                 .compatible = "allwinner,sun50i-r818-pwm",
>                 .data = &sun50i_pwm_data_c16,
>         }, {
>                 /* sentinel */
>         },
> };
> is this OK? Just write two SoC for the " compatible "？

Yes, this is perfect :)

Maxime

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