| Message ID | 20241023-dlech-mainline-spi-engine-offload-2-v4-6-f8125b99f5a1@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
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á
On 10/25/24 7:07 AM, Nuno Sá wrote: > 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> >> --- >> ... >> +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. We will still need some callback though to handle drivers that use phandle args. > >> +} >> + >> +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. > Here it is validating struct spi_offload_trigger_config has the right type, which is needed before we can safely trust that the correct union member was used in that struct. So it has a different purpose from the match check.
On Fri, 2024-10-25 at 11:28 -0500, David Lechner wrote: > On 10/25/24 7:07 AM, Nuno Sá wrote: > > 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> > > > --- > > > > > ... > > > > +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. > > We will still need some callback though to handle drivers that use > phandle args. Hmmm true. > > > > > > +} > > > + > > > +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. > > > > Here it is validating struct spi_offload_trigger_config has the right > type, which is needed before we can safely trust that the correct > union member was used in that struct. So it has a different purpose from > the match check. > I'm still not convinced tbh. We already pass in the type for the match() callback which is done when we get the trigger. I don't expect (at least at this point) for a given offload trigger to dynamically change. And if we do allow that, we should still need a new API to change between triggers (which could then be another validation point). But key point is that at any given time, only one trigger should be "in use". For this first simple case It really feels that passing around the trigger type (and validating on every API) is redundant. We could do it once in the match() callback and the pwm driver could then assume the periodic trigger is the one being use. Having said the above, this really does not matter that much so I'm not arguing more. If you prefer to be extra cautious, fair enough :) - Nuno Sá
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; +} + +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; + + 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", + .id = 0, + .parent = dev, + .fwnode = dev_fwnode(dev), + .ops = &spi_offload_trigger_pwm_ops, + }; + struct spi_offload_trigger_pwm_state *st; + struct spi_offload_trigger *trigger; + struct pwm_state state; + int ret; + + st = devm_kzalloc(&pdev->dev, sizeof(*st), GFP_KERNEL); + if (!st) + return -ENOMEM; + + st->dev = dev; + + st->pwm = devm_pwm_get(&pdev->dev, NULL); + if (IS_ERR(st->pwm)) + return dev_err_probe(dev, PTR_ERR(st->pwm), "failed to get PWM\n"); + + /* init with duty_cycle = 0, output enabled to ensure trigger off */ + pwm_init_state(st->pwm, &state); + state.enabled = true; + + ret = pwm_apply_might_sleep(st->pwm, &state); + if (ret < 0) + return dev_err_probe(dev, ret, "failed to apply PWM state\n"); + + ret = devm_add_action_or_reset(dev, spi_offload_trigger_pwm_release, st->pwm); + if (ret) + return ret; + + trigger = devm_spi_offload_trigger_alloc(dev, &info); + if (IS_ERR(trigger)) + return dev_err_probe(dev, PTR_ERR(trigger), "failed to allocate trigger\n"); + + return devm_spi_offload_trigger_register(dev, trigger, st); +} + +static const struct of_device_id spi_offload_trigger_pwm_of_match_table[] = { + { .compatible = "trigger-pwm" }, + { } +}; +MODULE_DEVICE_TABLE(of, spi_offload_trigger_pwm_of_match_table); + +static struct platform_driver spi_offload_trigger_pwm_driver = { + .driver = { + .name = "trigger-pwm", + .of_match_table = spi_offload_trigger_pwm_of_match_table, + }, + .probe = spi_offload_trigger_pwm_probe, +}; +module_platform_driver(spi_offload_trigger_pwm_driver); + +MODULE_AUTHOR("David Lechner <dlechner@baylibre.com>"); +MODULE_DESCRIPTION("Generic PWM trigger"); +MODULE_LICENSE("GPL");
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(+)