Re: [PATCH RFC v4 06/15] spi: offload-trigger: add PWM trigger driver

["Nuno Sá" <noname.nuno@gmail.com>]

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@baylibre.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á