Re: [PATCH v4 05/12] spi: spi-mem: Export the spi_mem_generic_supports_op() helper

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

boris.brezillon@collabora.com wrote on Fri, 10 Dec 2021 17:27:17 +0100:

> On Thu,  9 Dec 2021 18:40:39 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > The combination of checks against the number of supported operations is
> > going to increase exponentially each time we add a new parameter. So far
> > we only had a dtr parameter. Now we are introducing an ECC parameter. We
> > need to make this helper available for drivers with specific needs,
> > instead of creating another set of helpers each time we want to check
> > something new. In the future if we see that many different drivers use
> > the same parameter values, we might be tempted to create a specific
> > helper for that. But for now, let's just make the generic one available.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/spi/spi-mem.c       |  7 ++++---
> >  include/linux/spi/spi-mem.h | 13 +++++++++++++
> >  2 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index 9e06cd918273..48b55395178f 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -160,9 +160,9 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
> >  	return true;
> >  }
> >  
> > -static bool spi_mem_generic_supports_op(struct spi_mem *mem,
> > -					const struct spi_mem_op *op,
> > -					bool dtr, bool ecc)
> > +bool spi_mem_generic_supports_op(struct spi_mem *mem,
> > +				 const struct spi_mem_op *op,
> > +				 bool dtr, bool ecc)  
> 
> Looks like you're replacing a maintenance burden by another. Every time
> you'll add a new capability, you'll have to patch all the wrappers and
> the drivers using this generic function to pass the extra parameter.
> How about passing a
> 
> 	struct spi_mem_controller_caps {
> 		bool dtr;
> 		bool ecc;
> 	};

Oh yeah good idea, I like the structure more than the bitfield.

> or making the caps a bitmask, such that next time you add a new cap, you
> can just assume all drivers have it set to 0 by default.

Thanks,
Miquèl