Re: [PATCH 2/2 v3] pwm: Add driver for AXI PWM generator

[Uwe Kleine-König]

On Wed, Jan 31, 2024 at 04:40:41PM -0500, Trevor Gamblin wrote:
> From: Drew Fustini <dfustini@xxxxxxxxxxxx>
> 
> 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 <sergiu.cuciurean@xxxxxxxxxx>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@xxxxxxxxxx>
> Co-developed-by: David Lechner <dlechner@xxxxxxxxxxxx>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> Signed-off-by: Drew Fustini <dfustini@xxxxxxxxxxxx>
> Co-developed-by: Trevor Gamblin <tgamblin@xxxxxxxxxxxx>
> Acked-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> Signed-off-by: Trevor Gamblin <tgamblin@xxxxxxxxxxxx>
> ---
> v3 changes:
> * Address feedback for driver in v2:
>   * Remove unnecessary blank line in axi_pwmgen_apply
>   * Use macros already defined in <linux/fpga/adi-axi-common.h> for
>     version checking
> 
> v2 changes:
> * Address feedback for driver and device tree in v1:
>   * Use more reasonable Kconfig approach
>   * Use common prefixes for all functions
>   * Rename axi_pwmgen struct to axi_pwmgen_ddata
>   * Change use of "pwm" to "ddata"
>   * Set and check state->polarity
>   * Multiply safely with mul_u64_u64_div_u64()
>   * Improve handling of max and zero periods
>   * Error if clk_rate_hz > NSEC_PER_SEC
>   * Add "Limitations" section at top of pwm-axi-pwmgen.c
>   * Don't disable outputs by default
>   * Remove unnecessary macros for period, duty, offset
>   * Fix axi_pwmgen_ddata alignment
>   * Don't artificially limit npwm to four
>   * Use clk_rate_exclusive_get(), balance with clk_rate_exclusive_put()
>   * Cache clk rate in axi_pwmgen_ddata
>   * Don't assign pwm->chip.base, do assign pwm->chip.atomic
> * Remove redundant calls to clk_get_rate
> * Test contents of AXI_PWMGEN_REG_CORE_MAGIC instead of
>   arbitrary AXI_PWMGEN_TEST_DATA in AXI_PWMGEN_REG_SCRATCHPAD
> * Remove redundant clk struct from axi_pwmgen_ddata
> * Add self as module author
> * Add major version check for IP core
> 
> ---
>  MAINTAINERS                  |   1 +
>  drivers/pwm/Kconfig          |  13 ++
>  drivers/pwm/Makefile         |   1 +
>  drivers/pwm/pwm-axi-pwmgen.c | 242 +++++++++++++++++++++++++++++++++++
>  4 files changed, 257 insertions(+)
>  create mode 100644 drivers/pwm/pwm-axi-pwmgen.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8a4ed5545680..2baa7a0a1c8c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3438,6 +3438,7 @@ L:	linux-pwm@xxxxxxxxxxxxxxx
>  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 <krzysztof.adamski@xxxxxxxxx>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..d44b0e86adee 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -98,6 +98,19 @@ 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"
> +	depends on MICROBLAZE || NIOS2 || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_INTEL_SOCFPGA || COMPILE_TEST
> +	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..44e62e90b227
> --- /dev/null
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices AXI PWM generator
> + *
> + * Copyright 2024 Analog Devices Inc.
> + * Copyright 2024 Baylibre SAS
> + *
> + * Limitations:
> + * - The writes to registers for period and duty are shadowed until
> + *   LOAD_CONFIG is written to AXI_PWMGEN_REG_CONFIG at the end of the
> + *   current period.
> + * - Writing LOAD_CONFIG also has the effect of re-synchronizing all
> + *   enabled channels, which could cause glitching on other channels. It
> + *   is therefore expected that channels are assigned harmonic periods
> + *   and all have a single user coordinating this.

That means that when I reconfigure pwm2 the currently running period of
pwm2 is completed and then all pwmX's counters are reset to zero?

The patch looks good to me now. It needs a rework to use
pwmchip_alloc(). If you could care for that, that would be great.
Otherwise I'd pick up your patch later and add the conversion myself.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature