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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b47e7168a58e840f65c1ef150c914c077905fabf.camel@gmail.com>
Date: Fri, 25 Oct 2024 14:07:33 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Mark Brown <broonie@...nel.org>, 
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Nuno Sá <nuno.sa@...log.com>, Uwe
 Kleine-König <ukleinek@...nel.org>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>, Lars-Peter Clausen
	 <lars@...afoo.de>, David Jander <david@...tonic.nl>, Martin Sperl
	 <kernel@...tin.sperl.org>, linux-spi@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH RFC v4 06/15] spi: offload-trigger: add PWM trigger
 driver

Hi David,

Looks mostly good... Just one minor comments from me.

On Wed, 2024-10-23 at 15:59 -0500, David Lechner wrote:
> Add a new driver for a generic PWM trigger for SPI offloads.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> 
> v4 changes: new patch in v4
> ---
>  drivers/spi/Kconfig                   |  12 +++
>  drivers/spi/Makefile                  |   3 +
>  drivers/spi/spi-offload-trigger-pwm.c | 169 ++++++++++++++++++++++++++++++++++
>  3 files changed, 184 insertions(+)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index d65074b85f62..50d04fa317b7 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -1286,4 +1286,16 @@ endif # SPI_SLAVE
>  config SPI_DYNAMIC
>  	def_bool ACPI || OF_DYNAMIC || SPI_SLAVE
>  
> +if SPI_OFFLOAD
> +
> +comment "SPI Offload triggers"
> +
> +config SPI_OFFLOAD_TRIGGER_PWM
> +	tristate "SPI offload trigger using PWM"
> +	depends on PWM
> +	help
> +	  Generic SPI offload trigger implemented using PWM output.
> +
> +endif # SPI_OFFLOAD
> +
>  endif # SPI
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 6a470eb475a2..3a76b9c61486 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -161,3 +161,6 @@ obj-$(CONFIG_SPI_AMD)			+= spi-amd.o
>  # SPI slave protocol handlers
>  obj-$(CONFIG_SPI_SLAVE_TIME)		+= spi-slave-time.o
>  obj-$(CONFIG_SPI_SLAVE_SYSTEM_CONTROL)	+= spi-slave-system-control.o
> +
> +# SPI offload triggers
> +obj-$(CONFIG_SPI_OFFLOAD_TRIGGER_PWM)	+= spi-offload-trigger-pwm.o
> diff --git a/drivers/spi/spi-offload-trigger-pwm.c b/drivers/spi/spi-offload-
> trigger-pwm.c
> new file mode 100644
> index 000000000000..ffb0bf75cace
> --- /dev/null
> +++ b/drivers/spi/spi-offload-trigger-pwm.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Analog Devices Inc.
> + * Copyright (C) 2024 BayLibre, SAS
> + *
> + * Generic PWM trigger for SPI offload.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/spi/spi-offload.h>
> +#include <linux/types.h>
> +
> +struct spi_offload_trigger_pwm_state {
> +	struct device *dev;
> +	struct pwm_device *pwm;
> +};
> +
> +static bool spi_offload_trigger_pwm_match(void *priv,
> +					  enum spi_offload_trigger_type type,
> +					  u64 *args, u32 nargs)
> +{
> +	if (nargs)
> +		return false;
> +
> +	return type == SPI_OFFLOAD_TRIGGER_PERIODIC;

Hmm will we ever be in a place where a trigger provide might have multiple types? If
so, then I'm mostly fine with this match() callback. But we could still avoid it if
we use a bitmask for trigger types and having any trigger provider to give the
supported types. Then the core could pretty much do the match between the requested
trigger type and what the provider supports.

> +}
> +
> +static int spi_offload_trigger_pwm_validate(void *priv,
> +					    struct spi_offload_trigger_config
> *config)
> +{
> +	struct spi_offload_trigger_pwm_state *st = priv;
> +	struct spi_offload_trigger_periodic *periodic = &config->periodic;
> +	struct pwm_waveform wf = { };
> +	int ret;
> +
> +	if (config->type != SPI_OFFLOAD_TRIGGER_PERIODIC)
> +		return -EINVAL;

Checking the above every time seems redundant to me. We should match it once during
the trigger request and then just use that trigger type. Otherwise I'm not seeing the
point of the match() callback.

> +
> +	if (!periodic->frequency_hz)
> +		return -EINVAL;
> +
> +	wf.period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC, periodic-
> >frequency_hz);
> +	/* REVISIT: 50% duty-cycle for now - may add config parameter later */
> +	wf.duty_length_ns = wf.period_length_ns / 2;
> +
> +	ret = pwm_round_waveform_might_sleep(st->pwm, &wf);
> +	if (ret < 0)
> +		return ret;
> +
> +	periodic->frequency_hz = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> wf.period_length_ns);
> +
> +	return 0;
> +}
> +
> +static int spi_offload_trigger_pwm_enable(void *priv,
> +					  struct spi_offload_trigger_config
> *config)
> +{
> +	struct spi_offload_trigger_pwm_state *st = priv;
> +	struct spi_offload_trigger_periodic *periodic = &config->periodic;
> +	struct pwm_waveform wf = { };
> +
> +	if (config->type != SPI_OFFLOAD_TRIGGER_PERIODIC)
> +		return -EINVAL;
> +
> +	if (!periodic->frequency_hz)
> +		return -EINVAL;
> +
> +	wf.period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC, periodic-
> >frequency_hz);
> +	/* REVISIT: 50% duty-cycle for now - may add config parameter later */
> +	wf.duty_length_ns = wf.period_length_ns / 2;
> +
> +	return pwm_set_waveform_might_sleep(st->pwm, &wf, false);
> +}
> +
> +static void spi_offload_trigger_pwm_disable(void *priv)
> +{
> +	struct spi_offload_trigger_pwm_state *st = priv;
> +	struct pwm_waveform wf;
> +	int ret;
> +
> +	ret = pwm_get_waveform_might_sleep(st->pwm, &wf);
> +	if (ret < 0) {
> +		dev_err(st->dev, "failed to get waveform: %d\n", ret);
> +		return;
> +	}
> +
> +	wf.duty_length_ns = 0;
> +
> +	ret = pwm_set_waveform_might_sleep(st->pwm, &wf, false);
> +	if (ret < 0)
> +		dev_err(st->dev, "failed to disable PWM: %d\n", ret);
> +}
> +
> +static const struct spi_offload_trigger_ops spi_offload_trigger_pwm_ops = {
> +	.match = spi_offload_trigger_pwm_match,
> +	.validate = spi_offload_trigger_pwm_validate,
> +	.enable = spi_offload_trigger_pwm_enable,
> +	.disable = spi_offload_trigger_pwm_disable,
> +};
> +
> +static void spi_offload_trigger_pwm_release(void *data)
> +{
> +	pwm_disable(data);
> +}
> +
> +static int spi_offload_trigger_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct spi_offload_trigger_info info = {
> +		.name = "trigger",

pwm-trigger or trigger-pwm?

> +		.id = 0,

nit: Not really needed

- Nuno Sá



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ