| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <39c9bff3b97408b2f2b80f3290e517b5af536fff.camel@gmail.com>
Date: Wed, 24 Jan 2024 10:58:16 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Trevor Gamblin <tgamblin@...libre.com>, linux-pwm@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, u.kleine-koenig@...gutronix.de,
michael.hennerich@...log.com, nuno.sa@...log.com,
devicetree@...r.kernel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, Drew Fustini
<dfustini@...libre.com>, Sergiu Cuciurean <sergiu.cuciurean@...log.com>,
David Lechner <dlechner@...libre.com>
Subject: Re: [PATCH 2/2 v2] pwm: Add driver for AXI PWM generator
On Tue, 2024-01-23 at 17:05 -0500, Trevor Gamblin wrote:
> From: Drew Fustini <dfustini@...libre.com>
>
> 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@...log.com>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@...log.com>
> Co-developed-by: David Lechner <dlechner@...libre.com>
> Signed-off-by: David Lechner <dlechner@...libre.com>
> Signed-off-by: Drew Fustini <dfustini@...libre.com>
> Co-developed-by: Trevor Gamblin <tgamblin@...libre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@...libre.com>
>
> ---
One minor nitpick below (but no need to re-spin just for that).
Anyways, from a code point of view, LGTM:
Acked-by: Nuno Sa <nuno.sa@...log.com>
> 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 | 246 +++++++++++++++++++++++++++++++++++
> 4 files changed, 261 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@...r.kernel.org
> 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@...ia.com>
> 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..39d2c7be0cb4
> --- /dev/null
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -0,0 +1,246 @@
> +// 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.
> + * - Supports normal polarity. Does not support changing polarity.
> + */
> +#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_VERSION_MAJOR(x) (((x) >> 16) & 0xff)
> +#define AXI_PWMGEN_VERSION_MINOR(x) (((x) >> 8) & 0xff)
> +#define AXI_PWMGEN_VERSION_PATCH(x) ((x) & 0xff)
> +
> +#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_CHX_PERIOD(ch) (0x40 + (12 * (ch)))
> +#define AXI_PWMGEN_CHX_DUTY(ch) (0x44 + (12 * (ch)))
> +#define AXI_PWMGEN_CHX_OFFSET(ch) (0x48 + (12 * (ch)))
> +#define AXI_PWMGEN_REG_CORE_MAGIC_VAL 0x601A3471 /* Identification number
> to test during setup */
> +#define AXI_PWMGEN_LOAD_CONFIG BIT(1)
> +#define AXI_PWMGEN_RESET BIT(0)
> +
> +struct axi_pwmgen_ddata {
> + struct pwm_chip chip;
> + struct regmap *regmap;
> + unsigned long clk_rate_hz;
> +};
> +
> +static const struct regmap_config axi_pwmgen_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> +};
> +
> +static struct axi_pwmgen_ddata *axi_pwmgen_from_chip(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct axi_pwmgen_ddata, chip);
> +}
> +
> +static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct axi_pwmgen_ddata *ddata = axi_pwmgen_from_chip(chip);
> + unsigned int ch = pwm->hwpwm;
> + struct regmap *regmap = ddata->regmap;
> + u64 period_cnt, duty_cnt;
> + int ret;
> +
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + if (state->enabled) {
> +
no need for extra line in here... at the very least, you should be coherent in
both your code paths...
- Nuno Sá
Powered by blists - more mailing lists