This series adds support for the AXI PWM GEN subsystem found on FPGA IP
cores. It can be used to generate up to four configurable PWM outputs,
and includes options for external synchronization and clock signals.
The work is being done on behalf of, and therefore lists maintainers
from Analog Devices, Inc.
The series has been tested on actual hardware using an EVAL-AD7985FMCZ
evaluation board. An oscilloscope was used to validate that the
generated PWM signal matched the requested one.
Drew Fustini (2):
dt-bindings: pwm: Add bindings for AXI PWM generator
pwm: Add driver for AXI PWM generator
.../bindings/pwm/adi,axi-pwmgen.yaml | 48 ++++
MAINTAINERS | 9 +
drivers/pwm/Kconfig | 12 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-axi-pwmgen.c | 229 ++++++++++++++++++
5 files changed, 299 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
create mode 100644 drivers/pwm/pwm-axi-pwmgen.c
--
2.43.0
From: Drew Fustini <[email protected]>
Add support for the Analog Devices AXI PWM Generator. This device is an
FPGA-implemented peripheral used as PWM signal generator and can be
interfaced with AXI4. The register map of this peripheral makes it
possible to configure the period and duty cycle of the output signal.
Link: https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen
Co-developed-by: Sergiu Cuciurean <[email protected]>
Signed-off-by: Sergiu Cuciurean <[email protected]>
Co-developed-by: David Lechner <[email protected]>
Signed-off-by: David Lechner <[email protected]>
Signed-off-by: Drew Fustini <[email protected]>
Co-developed-by: Trevor Gamblin <[email protected]>
Signed-off-by: Trevor Gamblin <[email protected]>
---
MAINTAINERS | 1 +
drivers/pwm/Kconfig | 12 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-axi-pwmgen.c | 229 +++++++++++++++++++++++++++++++++++
4 files changed, 243 insertions(+)
create mode 100644 drivers/pwm/pwm-axi-pwmgen.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7b0f3aec5381..3abe90dec82e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3422,6 +3422,7 @@ L: [email protected]
S: Supported
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
+F: drivers/pwm/pwm-axi-pwmgen.c
AXXIA I2C CONTROLLER
M: Krzysztof Adamski <[email protected]>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..b105c0db4936 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -98,6 +98,18 @@ config PWM_ATMEL_TCB
To compile this driver as a module, choose M here: the module
will be called pwm-atmel-tcb.
+config PWM_AXI_PWMGEN
+ tristate "Analog Devices AXI PWM generator"
+ select REGMAP_MMIO
+ help
+ This enables support for the Analog Devices AXI PWM generator.
+
+ This is a configurable PWM generator with variable pulse width and
+ period.
+
+ To compile this driver as a module, choose M here: the module will be
+ called pwm-axi-pwmgen.
+
config PWM_BCM_IPROC
tristate "iProc PWM support"
depends on ARCH_BCM_IPROC || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..8322089954e9 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_PWM_APPLE) += pwm-apple.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
+obj-$(CONFIG_PWM_AXI_PWMGEN) += pwm-axi-pwmgen.o
obj-$(CONFIG_PWM_BCM_IPROC) += pwm-bcm-iproc.o
obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o
obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o
diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
new file mode 100644
index 000000000000..5e91636b88b4
--- /dev/null
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Analog Devices AXI PWM generator
+ *
+ * Copyright 2024 Analog Devices Inc.
+ * Copyright 2024 Baylibre SAS
+ */
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define AXI_PWMGEN_NPWM 4
+#define AXI_PWMGEN_REG_CORE_VERSION 0x00
+#define AXI_PWMGEN_REG_ID 0x04
+#define AXI_PWMGEN_REG_SCRATCHPAD 0x08
+#define AXI_PWMGEN_REG_CORE_MAGIC 0x0C
+#define AXI_PWMGEN_REG_CONFIG 0x10
+#define AXI_PWMGEN_REG_NPWM 0x14
+#define AXI_PWMGEN_CH_PERIOD_BASE 0x40
+#define AXI_PWMGEN_CH_DUTY_BASE 0x44
+#define AXI_PWMGEN_CH_OFFSET_BASE 0x48
+#define AXI_PWMGEN_CHX_PERIOD(ch) (AXI_PWMGEN_CH_PERIOD_BASE + (12 * (ch)))
+#define AXI_PWMGEN_CHX_DUTY(ch) (AXI_PWMGEN_CH_DUTY_BASE + (12 * (ch)))
+#define AXI_PWMGEN_CHX_OFFSET(ch) (AXI_PWMGEN_CH_OFFSET_BASE + (12 * (ch)))
+#define AXI_PWMGEN_TEST_DATA 0x5A0F0081
+#define AXI_PWMGEN_LOAD_CONFIG BIT(1)
+#define AXI_PWMGEN_RESET BIT(0)
+#define AXI_PWMGEN_MAX_REGISTER 0x6C
+
+struct axi_pwmgen {
+ struct pwm_chip chip;
+ struct clk *clk;
+ struct regmap *regmap;
+
+ /* Used to store the period when the channel is disabled */
+ unsigned int ch_period[AXI_PWMGEN_NPWM];
+ bool ch_enabled[AXI_PWMGEN_NPWM];
+};
+
+static const struct regmap_config axi_pwm_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .max_register = AXI_PWMGEN_MAX_REGISTER,
+};
+
+static struct axi_pwmgen *to_axi_pwmgen(struct pwm_chip *chip)
+{
+ return container_of(chip, struct axi_pwmgen, chip);
+}
+
+static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *device,
+ const struct pwm_state *state)
+{
+ struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
+ unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
+ unsigned int ch = device->hwpwm;
+ struct regmap *regmap = pwm->regmap;
+ u64 period_cnt, duty_cnt;
+ int ret;
+
+ if (!clk_rate_hz)
+ return -EINVAL;
+
+ period_cnt = DIV_ROUND_UP_ULL(state->period * clk_rate_hz, NSEC_PER_SEC);
+ if (period_cnt > UINT_MAX)
+ return -EINVAL;
+
+ pwm->ch_period[ch] = period_cnt;
+ pwm->ch_enabled[ch] = state->enabled;
+ ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), state->enabled ? period_cnt : 0);
+ if (ret)
+ return ret;
+
+ duty_cnt = DIV_ROUND_UP_ULL(state->duty_cycle * clk_rate_hz, NSEC_PER_SEC);
+ ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
+ if (ret)
+ return ret;
+
+ return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
+}
+
+static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *device,
+ struct pwm_state *state)
+{
+ struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
+ unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
+ struct regmap *regmap = pwm->regmap;
+ unsigned int ch = device->hwpwm;
+ u32 cnt;
+ int ret;
+
+ if (!clk_rate_hz) {
+ dev_err(device->chip->dev, "axi pwm clock has no frequency\n");
+ return -EINVAL;
+ }
+
+ state->enabled = pwm->ch_enabled[ch];
+
+ if (state->enabled) {
+ ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
+ if (ret)
+ return ret;
+ } else {
+ cnt = pwm->ch_period[ch];
+ }
+
+ state->period = DIV_ROUND_CLOSEST_ULL((u64)cnt * NSEC_PER_SEC, clk_rate_hz);
+
+ ret = regmap_read(regmap, AXI_PWMGEN_CHX_DUTY(ch), &cnt);
+ if (ret)
+ return ret;
+
+ state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)cnt * NSEC_PER_SEC, clk_rate_hz);
+
+ return 0;
+}
+
+static const struct pwm_ops axi_pwmgen_pwm_ops = {
+ .apply = axi_pwmgen_apply,
+ .get_state = axi_pwmgen_get_state,
+};
+
+static int axi_pwmgen_setup(struct axi_pwmgen *pwm, struct device *dev)
+{
+ struct regmap *regmap = pwm->regmap;
+ int idx;
+ int ret;
+ u32 val;
+
+ ret = regmap_write(regmap, AXI_PWMGEN_REG_SCRATCHPAD, AXI_PWMGEN_TEST_DATA);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(regmap, AXI_PWMGEN_REG_SCRATCHPAD, &val);
+ if (ret)
+ return ret;
+
+ if (val != AXI_PWMGEN_TEST_DATA)
+ return dev_err_probe(dev, -EIO, "failed to access the device registers\n");
+
+ ret = regmap_read(regmap, AXI_PWMGEN_REG_NPWM, &pwm->chip.npwm);
+ if (ret)
+ return ret;
+
+ if (pwm->chip.npwm > AXI_PWMGEN_NPWM) {
+ dev_warn(dev, "driver is limited to %d channels but hardware reported %u\n",
+ AXI_PWMGEN_NPWM, pwm->chip.npwm);
+ pwm->chip.npwm = AXI_PWMGEN_NPWM;
+ }
+
+ /* Disable all the outputs */
+ for (idx = 0; idx < pwm->chip.npwm; idx++) {
+ ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(idx), 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(idx), 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(regmap, AXI_PWMGEN_CHX_OFFSET(idx), 0);
+ if (ret)
+ return ret;
+ }
+
+ /* Enable the core */
+ return regmap_update_bits(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_RESET, 0);
+}
+
+static int axi_pwmgen_probe(struct platform_device *pdev)
+{
+ struct axi_pwmgen *pwm;
+ void __iomem *io_base;
+ int ret;
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ io_base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(io_base))
+ return PTR_ERR(io_base);
+
+ pwm->regmap = devm_regmap_init_mmio(&pdev->dev, io_base, &axi_pwm_regmap_config);
+ if (IS_ERR(pwm->regmap))
+ return dev_err_probe(&pdev->dev, PTR_ERR(pwm->regmap),
+ "failed to init register map\n");
+
+ pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+ if (IS_ERR(pwm->clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk), "failed to get clock\n");
+
+ pwm->chip.dev = &pdev->dev;
+ pwm->chip.ops = &axi_pwmgen_pwm_ops;
+ pwm->chip.base = -1;
+
+ ret = axi_pwmgen_setup(pwm, &pdev->dev);
+ if (ret < 0)
+ return ret;
+
+ return devm_pwmchip_add(&pdev->dev, &pwm->chip);
+}
+
+static const struct of_device_id axi_pwmgen_ids[] = {
+ { .compatible = "adi,axi-pwmgen-1.00.a" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, axi_pwmgen_ids);
+
+static struct platform_driver axi_pwmgen_driver = {
+ .driver = {
+ .name = "axi-pwmgen",
+ .of_match_table = axi_pwmgen_ids,
+ },
+ .probe = axi_pwmgen_probe,
+};
+
+module_platform_driver(axi_pwmgen_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sergiu Cuciurean <[email protected]>");
+MODULE_DESCRIPTION("Driver for the Analog Devices AXI PWM generator");
--
2.43.0
Hello,
On Mon, Jan 15, 2024 at 03:12:21PM -0500, Trevor Gamblin wrote:
> From: Drew Fustini <[email protected]>
>
> Add support for the Analog Devices AXI PWM Generator. This device is an
> FPGA-implemented peripheral used as PWM signal generator and can be
> interfaced with AXI4. The register map of this peripheral makes it
> possible to configure the period and duty cycle of the output signal.
>
> Link: https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen
> Co-developed-by: Sergiu Cuciurean <[email protected]>
> Signed-off-by: Sergiu Cuciurean <[email protected]>
> Co-developed-by: David Lechner <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
> Signed-off-by: Drew Fustini <[email protected]>
> Co-developed-by: Trevor Gamblin <[email protected]>
> Signed-off-by: Trevor Gamblin <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/pwm/Kconfig | 12 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-axi-pwmgen.c | 229 +++++++++++++++++++++++++++++++++++
> 4 files changed, 243 insertions(+)
> create mode 100644 drivers/pwm/pwm-axi-pwmgen.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b0f3aec5381..3abe90dec82e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3422,6 +3422,7 @@ L: [email protected]
> S: Supported
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
> +F: drivers/pwm/pwm-axi-pwmgen.c
>
> AXXIA I2C CONTROLLER
> M: Krzysztof Adamski <[email protected]>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..b105c0db4936 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -98,6 +98,18 @@ config PWM_ATMEL_TCB
> To compile this driver as a module, choose M here: the module
> will be called pwm-atmel-tcb.
>
> +config PWM_AXI_PWMGEN
> + tristate "Analog Devices AXI PWM generator"
> + select REGMAP_MMIO
Assuming you won't find the device on all machines, can you please add a
reasonable dependency to not annoy users?
> + help
> + This enables support for the Analog Devices AXI PWM generator.
> +
> + This is a configurable PWM generator with variable pulse width and
> + period.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called pwm-axi-pwmgen.
> +
> config PWM_BCM_IPROC
> tristate "iProc PWM support"
> depends on ARCH_BCM_IPROC || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c5ec9e168ee7..8322089954e9 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_PWM_APPLE) += pwm-apple.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
> +obj-$(CONFIG_PWM_AXI_PWMGEN) += pwm-axi-pwmgen.o
> obj-$(CONFIG_PWM_BCM_IPROC) += pwm-bcm-iproc.o
> obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o
> obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o
> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
> new file mode 100644
> index 000000000000..5e91636b88b4
> --- /dev/null
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices AXI PWM generator
> + *
> + * Copyright 2024 Analog Devices Inc.
> + * Copyright 2024 Baylibre SAS
> + */
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define AXI_PWMGEN_NPWM 4
> +#define AXI_PWMGEN_REG_CORE_VERSION 0x00
> +#define AXI_PWMGEN_REG_ID 0x04
> +#define AXI_PWMGEN_REG_SCRATCHPAD 0x08
> +#define AXI_PWMGEN_REG_CORE_MAGIC 0x0C
> +#define AXI_PWMGEN_REG_CONFIG 0x10
> +#define AXI_PWMGEN_REG_NPWM 0x14
> +#define AXI_PWMGEN_CH_PERIOD_BASE 0x40
> +#define AXI_PWMGEN_CH_DUTY_BASE 0x44
> +#define AXI_PWMGEN_CH_OFFSET_BASE 0x48
> +#define AXI_PWMGEN_CHX_PERIOD(ch) (AXI_PWMGEN_CH_PERIOD_BASE + (12 * (ch)))
> +#define AXI_PWMGEN_CHX_DUTY(ch) (AXI_PWMGEN_CH_DUTY_BASE + (12 * (ch)))
> +#define AXI_PWMGEN_CHX_OFFSET(ch) (AXI_PWMGEN_CH_OFFSET_BASE + (12 * (ch)))
I'd drop the name AXI_PWMGEN_CH_PERIOD_BASE and just hard code it in the
definition of AXI_PWMGEN_CHX_PERIOD. Ditto for the two other macros.
> +#define AXI_PWMGEN_TEST_DATA 0x5A0F0081
Is this a documented constant, or just a random (as in xkcd#221) value?
> +#define AXI_PWMGEN_LOAD_CONFIG BIT(1)
> +#define AXI_PWMGEN_RESET BIT(0)
> +#define AXI_PWMGEN_MAX_REGISTER 0x6C
> +
> +struct axi_pwmgen {
> + struct pwm_chip chip;
> + struct clk *clk;
> + struct regmap *regmap;
> +
> + /* Used to store the period when the channel is disabled */
> + unsigned int ch_period[AXI_PWMGEN_NPWM];
> + bool ch_enabled[AXI_PWMGEN_NPWM];
> +};
I'm not a fan of this alignment. If you don't have a strong opinion here
please just use a single space here. (I would expect you to not have a
strong opinion as other structs in this driver are not aligned in this
way.)
> +
> +static const struct regmap_config axi_pwm_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .max_register = AXI_PWMGEN_MAX_REGISTER,
> +};
> +
> +static struct axi_pwmgen *to_axi_pwmgen(struct pwm_chip *chip)
I'm a big fan of common symbol prefixes. I suggest to rename
axi_pwm_regmap_config to axi_pwmgen_regmap_config and to_axi_pwmgen to
axi_pwmgen_from_chip.
> +{
> + return container_of(chip, struct axi_pwmgen, chip);
> +}
> +
> +static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *device,
> + const struct pwm_state *state)
> +{
> + struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
Please pick a different name. "pwm" is reserved for pwm_device
variables. (And please use it for these.) A typical name would be
"ddata", or "pc" or "apg". (Maybe also rename axi_pwmgen to
axi_pwmgen_ddata?)
> + unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
> + unsigned int ch = device->hwpwm;
> + struct regmap *regmap = pwm->regmap;
> + u64 period_cnt, duty_cnt;
> + int ret;
You didn't check for state->polarity. You should however. Also
.get_state() needs to assign that one.
> + if (!clk_rate_hz)
> + return -EINVAL;
> +
> + period_cnt = DIV_ROUND_UP_ULL(state->period * clk_rate_hz, NSEC_PER_SEC);
The multiplication might overflow. Please use mul_u64_u64_div_u64() (or
one of its variant) and error out on clk_rate_hz > NSEC_PER_SEC.
Also round-up is wrong. I would expect that enabling PWM_DEBUG and
enough testing should tell you that. .apply is supposed to implement the
biggest period not bigger than the requested one. So you have to round
down here.
> + if (period_cnt > UINT_MAX)
> + return -EINVAL;
That's wrong. Please continue with period_cnd = UINT_MAX here.
Instead you should probably error out on period_cnt == 0.
> + pwm->ch_period[ch] = period_cnt;
> + pwm->ch_enabled[ch] = state->enabled;
> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), state->enabled ? period_cnt : 0);
> + if (ret)
> + return ret;
> +
> + duty_cnt = DIV_ROUND_UP_ULL(state->duty_cycle * clk_rate_hz, NSEC_PER_SEC);
> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
> + if (ret)
> + return ret;
> +
> + return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
I assume this means that the writes above are to shadow registers and on
this write they are latched into the hardware. So there is no glitch?!
Does this wait for the currently running period to complete before
switching to the new configuration?
Please document these two hardware properties in a "Limitations"
paragraph at the top of the driver. See other drivers for the format.
> +}
> +
> +static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *device,
> + struct pwm_state *state)
> +{
> + struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
> + unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
> + struct regmap *regmap = pwm->regmap;
> + unsigned int ch = device->hwpwm;
> + u32 cnt;
> + int ret;
> +
> + if (!clk_rate_hz) {
> + dev_err(device->chip->dev, "axi pwm clock has no frequency\n");
> + return -EINVAL;
> + }
> +
> + state->enabled = pwm->ch_enabled[ch];
> +
> + if (state->enabled) {
> + ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
> + if (ret)
> + return ret;
> + } else {
> + cnt = pwm->ch_period[ch];
> + }
If state->enabled is false, state->period is (or should) be ignored by
the caller, so there shouldn't be a need to track ch_period.
Also ch_enabled shouldn't be needed. Just reporting
AXI_PWMGEN_CHX_PERIOD(ch) == 0 as disabled should work fine?!
I think then you also don't need to artificially limit npwm to four.
> + state->period = DIV_ROUND_CLOSEST_ULL((u64)cnt * NSEC_PER_SEC, clk_rate_hz);
As feeding the result for .get_state() to .apply() should not modify the
hardware state, you have to round up here.
> + ret = regmap_read(regmap, AXI_PWMGEN_CHX_DUTY(ch), &cnt);
> + if (ret)
> + return ret;
> +
> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)cnt * NSEC_PER_SEC, clk_rate_hz);
ditto.
> + return 0;
> +}
> +
> +static const struct pwm_ops axi_pwmgen_pwm_ops = {
> + .apply = axi_pwmgen_apply,
> + .get_state = axi_pwmgen_get_state,
> +};
> +
> +static int axi_pwmgen_setup(struct axi_pwmgen *pwm, struct device *dev)
> +{
> + struct regmap *regmap = pwm->regmap;
> + int idx;
> + int ret;
> + u32 val;
> +
> + ret = regmap_write(regmap, AXI_PWMGEN_REG_SCRATCHPAD, AXI_PWMGEN_TEST_DATA);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(regmap, AXI_PWMGEN_REG_SCRATCHPAD, &val);
> + if (ret)
> + return ret;
> +
> + if (val != AXI_PWMGEN_TEST_DATA)
> + return dev_err_probe(dev, -EIO, "failed to access the device registers\n");
> +
> + ret = regmap_read(regmap, AXI_PWMGEN_REG_NPWM, &pwm->chip.npwm);
> + if (ret)
> + return ret;
> +
> + if (pwm->chip.npwm > AXI_PWMGEN_NPWM) {
> + dev_warn(dev, "driver is limited to %d channels but hardware reported %u\n",
> + AXI_PWMGEN_NPWM, pwm->chip.npwm);
> + pwm->chip.npwm = AXI_PWMGEN_NPWM;
> + }
> +
> + /* Disable all the outputs */
Please don't. If the bootloader setup a splash screen, loading the pwm
driver shouldn't disable the backlight. So please don't touch the
configuration in .probe().
> + for (idx = 0; idx < pwm->chip.npwm; idx++) {
> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(idx), 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(idx), 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_OFFSET(idx), 0);
> + if (ret)
> + return ret;
> + }
> +
> + /* Enable the core */
> + return regmap_update_bits(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_RESET, 0);
> +}
> +
> +static int axi_pwmgen_probe(struct platform_device *pdev)
> +{
> + struct axi_pwmgen *pwm;
> + void __iomem *io_base;
> + int ret;
> +
> + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + io_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(io_base))
> + return PTR_ERR(io_base);
> +
> + pwm->regmap = devm_regmap_init_mmio(&pdev->dev, io_base, &axi_pwm_regmap_config);
> + if (IS_ERR(pwm->regmap))
> + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->regmap),
> + "failed to init register map\n");
> +
> + pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> + if (IS_ERR(pwm->clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk), "failed to get clock\n");
Please call clk_rate_exclusive_get() on pwm->clk and cache the rate in
struct axi_pwmgen.
> + pwm->chip.dev = &pdev->dev;
> + pwm->chip.ops = &axi_pwmgen_pwm_ops;
> + pwm->chip.base = -1;
Don't assign .base.
> +
> + ret = axi_pwmgen_setup(pwm, &pdev->dev);
> + if (ret < 0)
> + return ret;
> +
> + return devm_pwmchip_add(&pdev->dev, &pwm->chip);
> +}
> +
> +static const struct of_device_id axi_pwmgen_ids[] = {
> + { .compatible = "adi,axi-pwmgen-1.00.a" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, axi_pwmgen_ids);
> +
> +static struct platform_driver axi_pwmgen_driver = {
> + .driver = {
> + .name = "axi-pwmgen",
> + .of_match_table = axi_pwmgen_ids,
> + },
> + .probe = axi_pwmgen_probe,
> +};
> +
Drop this empty line.
> +module_platform_driver(axi_pwmgen_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sergiu Cuciurean <[email protected]>");
> +MODULE_DESCRIPTION("Driver for the Analog Devices AXI PWM generator");
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Jan 15, 2024 at 10:18:04PM +0100, Uwe Kleine-K?nig wrote:
> On Mon, Jan 15, 2024 at 03:12:21PM -0500, Trevor Gamblin wrote:
> > +static int axi_pwmgen_probe(struct platform_device *pdev)
> > +{
> > + struct axi_pwmgen *pwm;
> > + void __iomem *io_base;
> > + int ret;
> > +
> > + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> > + if (!pwm)
> > + return -ENOMEM;
> > +
> > + io_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(io_base))
> > + return PTR_ERR(io_base);
> > +
> > + pwm->regmap = devm_regmap_init_mmio(&pdev->dev, io_base, &axi_pwm_regmap_config);
> > + if (IS_ERR(pwm->regmap))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->regmap),
> > + "failed to init register map\n");
> > +
> > + pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > + if (IS_ERR(pwm->clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk), "failed to get clock\n");
>
> Please call clk_rate_exclusive_get() on pwm->clk and cache the rate in
> struct axi_pwmgen.
>
> > + pwm->chip.dev = &pdev->dev;
> > + pwm->chip.ops = &axi_pwmgen_pwm_ops;
In that case pwm->chip.atomic = true; can be set too (although this should
be tested with CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PWM_DEBUG).
Thanks,
Sean
Hello,
On Tue, Jan 16, 2024 at 08:28:06AM +0000, Sean Young wrote:
> On Mon, Jan 15, 2024 at 10:18:04PM +0100, Uwe Kleine-K?nig wrote:
> > On Mon, Jan 15, 2024 at 03:12:21PM -0500, Trevor Gamblin wrote:
> > > + pwm->chip.dev = &pdev->dev;
> > > + pwm->chip.ops = &axi_pwmgen_pwm_ops;
>
> In that case pwm->chip.atomic = true; can be set too (although this should
> be tested with CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PWM_DEBUG).
Oh indeed. Good catch.
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello,
Responses inline - I'll have another series with all fixes submitted soon.
On 2024-01-15 16:18, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, Jan 15, 2024 at 03:12:21PM -0500, Trevor Gamblin wrote:
>> From: Drew Fustini <[email protected]>
>>
>> Add support for the Analog Devices AXI PWM Generator. This device is an
>> FPGA-implemented peripheral used as PWM signal generator and can be
>> interfaced with AXI4. The register map of this peripheral makes it
>> possible to configure the period and duty cycle of the output signal.
>>
>> Link: https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen
>> Co-developed-by: Sergiu Cuciurean <[email protected]>
>> Signed-off-by: Sergiu Cuciurean <[email protected]>
>> Co-developed-by: David Lechner <[email protected]>
>> Signed-off-by: David Lechner <[email protected]>
>> Signed-off-by: Drew Fustini <[email protected]>
>> Co-developed-by: Trevor Gamblin <[email protected]>
>> Signed-off-by: Trevor Gamblin <[email protected]>
>> ---
>> MAINTAINERS | 1 +
>> drivers/pwm/Kconfig | 12 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-axi-pwmgen.c | 229 +++++++++++++++++++++++++++++++++++
>> 4 files changed, 243 insertions(+)
>> create mode 100644 drivers/pwm/pwm-axi-pwmgen.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7b0f3aec5381..3abe90dec82e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3422,6 +3422,7 @@ L: [email protected]
>> S: Supported
>> W: https://ez.analog.com/linux-software-drivers
>> F: Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
>> +F: drivers/pwm/pwm-axi-pwmgen.c
>>
>> AXXIA I2C CONTROLLER
>> M: Krzysztof Adamski <[email protected]>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 4b956d661755..b105c0db4936 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -98,6 +98,18 @@ config PWM_ATMEL_TCB
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-atmel-tcb.
>>
>> +config PWM_AXI_PWMGEN
>> + tristate "Analog Devices AXI PWM generator"
>> + select REGMAP_MMIO
> Assuming you won't find the device on all machines, can you please add a
> reasonable dependency to not annoy users?
Will do.
>
>> + help
>> + This enables support for the Analog Devices AXI PWM generator.
>> +
>> + This is a configurable PWM generator with variable pulse width and
>> + period.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called pwm-axi-pwmgen.
>> +
>> config PWM_BCM_IPROC
>> tristate "iProc PWM support"
>> depends on ARCH_BCM_IPROC || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index c5ec9e168ee7..8322089954e9 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_PWM_APPLE) += pwm-apple.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
>> +obj-$(CONFIG_PWM_AXI_PWMGEN) += pwm-axi-pwmgen.o
>> obj-$(CONFIG_PWM_BCM_IPROC) += pwm-bcm-iproc.o
>> obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o
>> obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o
>> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
>> new file mode 100644
>> index 000000000000..5e91636b88b4
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-axi-pwmgen.c
>> @@ -0,0 +1,229 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Analog Devices AXI PWM generator
>> + *
>> + * Copyright 2024 Analog Devices Inc.
>> + * Copyright 2024 Baylibre SAS
>> + */
>> +#include <linux/bits.h>
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#define AXI_PWMGEN_NPWM 4
>> +#define AXI_PWMGEN_REG_CORE_VERSION 0x00
>> +#define AXI_PWMGEN_REG_ID 0x04
>> +#define AXI_PWMGEN_REG_SCRATCHPAD 0x08
>> +#define AXI_PWMGEN_REG_CORE_MAGIC 0x0C
>> +#define AXI_PWMGEN_REG_CONFIG 0x10
>> +#define AXI_PWMGEN_REG_NPWM 0x14
>> +#define AXI_PWMGEN_CH_PERIOD_BASE 0x40
>> +#define AXI_PWMGEN_CH_DUTY_BASE 0x44
>> +#define AXI_PWMGEN_CH_OFFSET_BASE 0x48
>> +#define AXI_PWMGEN_CHX_PERIOD(ch) (AXI_PWMGEN_CH_PERIOD_BASE + (12 * (ch)))
>> +#define AXI_PWMGEN_CHX_DUTY(ch) (AXI_PWMGEN_CH_DUTY_BASE + (12 * (ch)))
>> +#define AXI_PWMGEN_CHX_OFFSET(ch) (AXI_PWMGEN_CH_OFFSET_BASE + (12 * (ch)))
> I'd drop the name AXI_PWMGEN_CH_PERIOD_BASE and just hard code it in the
> definition of AXI_PWMGEN_CHX_PERIOD. Ditto for the two other macros.
>
>> +#define AXI_PWMGEN_TEST_DATA 0x5A0F0081
> Is this a documented constant, or just a random (as in xkcd#221) value?
This is just a random (as in xkcd#221) value to write to the scratch
register.
>
>> +#define AXI_PWMGEN_LOAD_CONFIG BIT(1)
>> +#define AXI_PWMGEN_RESET BIT(0)
>> +#define AXI_PWMGEN_MAX_REGISTER 0x6C
>> +
>> +struct axi_pwmgen {
>> + struct pwm_chip chip;
>> + struct clk *clk;
>> + struct regmap *regmap;
>> +
>> + /* Used to store the period when the channel is disabled */
>> + unsigned int ch_period[AXI_PWMGEN_NPWM];
>> + bool ch_enabled[AXI_PWMGEN_NPWM];
>> +};
> I'm not a fan of this alignment. If you don't have a strong opinion here
> please just use a single space here. (I would expect you to not have a
> strong opinion as other structs in this driver are not aligned in this
> way.)
>
>> +
>> +static const struct regmap_config axi_pwm_regmap_config = {
>> + .reg_bits = 32,
>> + .reg_stride = 4,
>> + .val_bits = 32,
>> + .max_register = AXI_PWMGEN_MAX_REGISTER,
>> +};
>> +
>> +static struct axi_pwmgen *to_axi_pwmgen(struct pwm_chip *chip)
> I'm a big fan of common symbol prefixes. I suggest to rename
> axi_pwm_regmap_config to axi_pwmgen_regmap_config and to_axi_pwmgen to
> axi_pwmgen_from_chip.
>
>> +{
>> + return container_of(chip, struct axi_pwmgen, chip);
>> +}
>> +
>> +static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *device,
>> + const struct pwm_state *state)
>> +{
>> + struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
> Please pick a different name. "pwm" is reserved for pwm_device
> variables. (And please use it for these.) A typical name would be
> "ddata", or "pc" or "apg". (Maybe also rename axi_pwmgen to
> axi_pwmgen_ddata?)
>
>> + unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
>> + unsigned int ch = device->hwpwm;
>> + struct regmap *regmap = pwm->regmap;
>> + u64 period_cnt, duty_cnt;
>> + int ret;
> You didn't check for state->polarity. You should however. Also
> .get_state() needs to assign that one.
>
>> + if (!clk_rate_hz)
>> + return -EINVAL;
>> +
>> + period_cnt = DIV_ROUND_UP_ULL(state->period * clk_rate_hz, NSEC_PER_SEC);
> The multiplication might overflow. Please use mul_u64_u64_div_u64() (or
> one of its variant) and error out on clk_rate_hz > NSEC_PER_SEC.
>
> Also round-up is wrong. I would expect that enabling PWM_DEBUG and
> enough testing should tell you that. .apply is supposed to implement the
> biggest period not bigger than the requested one. So you have to round
> down here.
To be clear, I should use mul_u64_u64_div_u64 only, or should I round
down afterwards with another function as well?
>
>> + if (period_cnt > UINT_MAX)
>> + return -EINVAL;
> That's wrong. Please continue with period_cnd = UINT_MAX here.
>
> Instead you should probably error out on period_cnt == 0.
>
>> + pwm->ch_period[ch] = period_cnt;
>> + pwm->ch_enabled[ch] = state->enabled;
>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), state->enabled ? period_cnt : 0);
>> + if (ret)
>> + return ret;
>> +
>> + duty_cnt = DIV_ROUND_UP_ULL(state->duty_cycle * clk_rate_hz, NSEC_PER_SEC);
>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
>> + if (ret)
>> + return ret;
>> +
>> + return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
> I assume this means that the writes above are to shadow registers and on
> this write they are latched into the hardware. So there is no glitch?!
>
> Does this wait for the currently running period to complete before
> switching to the new configuration?
>
> Please document these two hardware properties in a "Limitations"
> paragraph at the top of the driver. See other drivers for the format.
The registers are shadow registers and changes don't take effect right
away. It also keeps the phase offset in sync between outputs.
>
>> +}
>> +
>> +static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *device,
>> + struct pwm_state *state)
>> +{
>> + struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
>> + unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
>> + struct regmap *regmap = pwm->regmap;
>> + unsigned int ch = device->hwpwm;
>> + u32 cnt;
>> + int ret;
>> +
>> + if (!clk_rate_hz) {
>> + dev_err(device->chip->dev, "axi pwm clock has no frequency\n");
>> + return -EINVAL;
>> + }
>> +
>> + state->enabled = pwm->ch_enabled[ch];
>> +
>> + if (state->enabled) {
>> + ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
>> + if (ret)
>> + return ret;
>> + } else {
>> + cnt = pwm->ch_period[ch];
>> + }
> If state->enabled is false, state->period is (or should) be ignored by
> the caller, so there shouldn't be a need to track ch_period.
>
> Also ch_enabled shouldn't be needed. Just reporting
> AXI_PWMGEN_CHX_PERIOD(ch) == 0 as disabled should work fine?!
>
> I think then you also don't need to artificially limit npwm to four.
The only concern I have with not tracking ch_period is that it might not
be clear what period to restore after disabling and re-enabling the
device, unless I'm missing something.
>
>> + state->period = DIV_ROUND_CLOSEST_ULL((u64)cnt * NSEC_PER_SEC, clk_rate_hz);
> As feeding the result for .get_state() to .apply() should not modify the
> hardware state, you have to round up here.
>
>> + ret = regmap_read(regmap, AXI_PWMGEN_CHX_DUTY(ch), &cnt);
>> + if (ret)
>> + return ret;
>> +
>> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)cnt * NSEC_PER_SEC, clk_rate_hz);
> ditto.
>
>> + return 0;
>> +}
>> +
>> +static const struct pwm_ops axi_pwmgen_pwm_ops = {
>> + .apply = axi_pwmgen_apply,
>> + .get_state = axi_pwmgen_get_state,
>> +};
>> +
>> +static int axi_pwmgen_setup(struct axi_pwmgen *pwm, struct device *dev)
>> +{
>> + struct regmap *regmap = pwm->regmap;
>> + int idx;
>> + int ret;
>> + u32 val;
>> +
>> + ret = regmap_write(regmap, AXI_PWMGEN_REG_SCRATCHPAD, AXI_PWMGEN_TEST_DATA);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_read(regmap, AXI_PWMGEN_REG_SCRATCHPAD, &val);
>> + if (ret)
>> + return ret;
>> +
>> + if (val != AXI_PWMGEN_TEST_DATA)
>> + return dev_err_probe(dev, -EIO, "failed to access the device registers\n");
>> +
>> + ret = regmap_read(regmap, AXI_PWMGEN_REG_NPWM, &pwm->chip.npwm);
>> + if (ret)
>> + return ret;
>> +
>> + if (pwm->chip.npwm > AXI_PWMGEN_NPWM) {
>> + dev_warn(dev, "driver is limited to %d channels but hardware reported %u\n",
>> + AXI_PWMGEN_NPWM, pwm->chip.npwm);
>> + pwm->chip.npwm = AXI_PWMGEN_NPWM;
>> + }
>> +
>> + /* Disable all the outputs */
> Please don't. If the bootloader setup a splash screen, loading the pwm
> driver shouldn't disable the backlight. So please don't touch the
> configuration in .probe().
Thank you for the catch.
>
>> + for (idx = 0; idx < pwm->chip.npwm; idx++) {
>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(idx), 0);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(idx), 0);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_write(regmap, AXI_PWMGEN_CHX_OFFSET(idx), 0);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + /* Enable the core */
>> + return regmap_update_bits(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_RESET, 0);
>> +}
>> +
>> +static int axi_pwmgen_probe(struct platform_device *pdev)
>> +{
>> + struct axi_pwmgen *pwm;
>> + void __iomem *io_base;
>> + int ret;
>> +
>> + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + io_base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(io_base))
>> + return PTR_ERR(io_base);
>> +
>> + pwm->regmap = devm_regmap_init_mmio(&pdev->dev, io_base, &axi_pwm_regmap_config);
>> + if (IS_ERR(pwm->regmap))
>> + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->regmap),
>> + "failed to init register map\n");
>> +
>> + pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>> + if (IS_ERR(pwm->clk))
>> + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk), "failed to get clock\n");
> Please call clk_rate_exclusive_get() on pwm->clk and cache the rate in
> struct axi_pwmgen.
>
>> + pwm->chip.dev = &pdev->dev;
>> + pwm->chip.ops = &axi_pwmgen_pwm_ops;
>> + pwm->chip.base = -1;
> Don't assign .base.
Alright. I will set pwm->chip.atomic as per Sean's comment as well.
>
>> +
>> + ret = axi_pwmgen_setup(pwm, &pdev->dev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return devm_pwmchip_add(&pdev->dev, &pwm->chip);
>> +}
>> +
>> +static const struct of_device_id axi_pwmgen_ids[] = {
>> + { .compatible = "adi,axi-pwmgen-1.00.a" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, axi_pwmgen_ids);
>> +
>> +static struct platform_driver axi_pwmgen_driver = {
>> + .driver = {
>> + .name = "axi-pwmgen",
>> + .of_match_table = axi_pwmgen_ids,
>> + },
>> + .probe = axi_pwmgen_probe,
>> +};
>> +
> Drop this empty line.
>
>> +module_platform_driver(axi_pwmgen_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Sergiu Cuciurean <[email protected]>");
>> +MODULE_DESCRIPTION("Driver for the Analog Devices AXI PWM generator");
> Best regards
> Uwe
>
Hello Trevor,
On Wed, Jan 17, 2024 at 07:51:26AM -0500, Trevor Gamblin wrote:
> On 2024-01-15 16:18, Uwe Kleine-K?nig wrote:
> > On Mon, Jan 15, 2024 at 03:12:21PM -0500, Trevor Gamblin wrote:
> > > +#define AXI_PWMGEN_TEST_DATA 0x5A0F0081
> > Is this a documented constant, or just a random (as in xkcd#221) value?
>
> This is just a random (as in xkcd#221) value to write to the scratch
> register.
Then I suggest to add a comment telling that.
> > > + period_cnt = DIV_ROUND_UP_ULL(state->period * clk_rate_hz, NSEC_PER_SEC);
> > The multiplication might overflow. Please use mul_u64_u64_div_u64() (or
> > one of its variant) and error out on clk_rate_hz > NSEC_PER_SEC.
> >
> > Also round-up is wrong. I would expect that enabling PWM_DEBUG and
> > enough testing should tell you that. .apply is supposed to implement the
> > biggest period not bigger than the requested one. So you have to round
> > down here.
> To be clear, I should use mul_u64_u64_div_u64 only, or should I round down
> afterwards with another function as well?
mul_u64_u64_div_u64() already rounds down.
> > > + if (period_cnt > UINT_MAX)
> > > + return -EINVAL;
> > That's wrong. Please continue with period_cnd = UINT_MAX here.
> >
> > Instead you should probably error out on period_cnt == 0.
> >
> > > + pwm->ch_period[ch] = period_cnt;
> > > + pwm->ch_enabled[ch] = state->enabled;
> > > + ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), state->enabled ? period_cnt : 0);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + duty_cnt = DIV_ROUND_UP_ULL(state->duty_cycle * clk_rate_hz, NSEC_PER_SEC);
> > > + ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
> > I assume this means that the writes above are to shadow registers and on
> > this write they are latched into the hardware. So there is no glitch?!
> >
> > Does this wait for the currently running period to complete before
> > switching to the new configuration?
> >
> > Please document these two hardware properties in a "Limitations"
> > paragraph at the top of the driver. See other drivers for the format.
>
> The registers are shadow registers and changes don't take effect right away.
> It also keeps the phase offset in sync between outputs.
I don't understand that. This only makes sense if the different PWMs
share the same period length, doesn't it?
Please add this to the Limitations paragraph, too.
> > > +static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *device,
> > > + struct pwm_state *state)
> > > +{
> > > + struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
> > > + unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
> > > + struct regmap *regmap = pwm->regmap;
> > > + unsigned int ch = device->hwpwm;
> > > + u32 cnt;
> > > + int ret;
> > > +
> > > + if (!clk_rate_hz) {
> > > + dev_err(device->chip->dev, "axi pwm clock has no frequency\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + state->enabled = pwm->ch_enabled[ch];
> > > +
> > > + if (state->enabled) {
> > > + ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
> > > + if (ret)
> > > + return ret;
> > > + } else {
> > > + cnt = pwm->ch_period[ch];
> > > + }
> > If state->enabled is false, state->period is (or should) be ignored by
> > the caller, so there shouldn't be a need to track ch_period.
> >
> > Also ch_enabled shouldn't be needed. Just reporting
> > AXI_PWMGEN_CHX_PERIOD(ch) == 0 as disabled should work fine?!
> >
> > I think then you also don't need to artificially limit npwm to four.
> The only concern I have with not tracking ch_period is that it might not be
> clear what period to restore after disabling and re-enabling the device,
> unless I'm missing something.
A consumer that reenables the device should use pwm_apply_* which gets
the complete state that should be configured. (And for the few other
cases, this is solved in the core.)
> > > + pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > > + if (IS_ERR(pwm->clk))
> > > + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk), "failed to get clock\n");
> > Please call clk_rate_exclusive_get() on pwm->clk and cache the rate in
> > struct axi_pwmgen.
> >
> > > + pwm->chip.dev = &pdev->dev;
> > > + pwm->chip.ops = &axi_pwmgen_pwm_ops;
> > > + pwm->chip.base = -1;
> > Don't assign .base.
> Alright. I will set pwm->chip.atomic as per Sean's comment as well.
Sounds good.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |