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

[Nuno Sá <noname.nuno@xxxxxxxxx>]

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@xxxxxxxxxxxx>
> > > ---
> > > 
> 
> ...
> 
> > > +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á