Re: [PATCH v5 04/13] spi: spi-mem: Create a helper to gather all the supports_op checks

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Pratyush,

p.yadav@xxxxxx wrote on Wed, 15 Dec 2021 01:23:17 +0530:

> Hi Miquel,
> 
> On 14/12/21 12:41PM, Miquel Raynal wrote:
> > So far we check the support for:
> > - regular operations
> > - dtr operations
> > Soon, we will also need to check the support for ECC operations.
> > 
> > As the combinatorial will increase exponentially, let's gather all the
> > checks in a single generic function which takes a capabilities structure
> > as input. This new helper is supposed to be called by the currently
> > exported functions instead of repeating a similar implementation.  
> 
> Nice! I think this is a very good idea.

Thanks!

> 
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > ---
> >  drivers/spi/spi-mem.c       | 34 +++++++++++++++++++++++++---------
> >  include/linux/spi/spi-mem.h |  8 ++++++++
> >  2 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index 37f4443ce9a0..4c6944d7b174 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -160,26 +160,42 @@ 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,
> > +					struct spi_mem_controller_caps *caps)
> > +{
> > +	if (!caps->dtr) {  
> 
> Nitpick: Please turn the order of the if-else around:
> 
>   if (caps->dtr)
>     ...
>   else
>     ...

Yup

> 
> > +		if (op->cmd.dtr || op->addr.dtr ||
> > +		    op->dummy.dtr || op->data.dtr)
> > +			return false;
> > +
> > +		if (op->cmd.nbytes != 1)
> > +			return false;
> > +	} else {
> > +		if (op->cmd.nbytes != 2)
> > +			return false;
> > +	}
> > +
> > +	return spi_mem_check_buswidth(mem, op);
> > +}
> > +
> >  bool spi_mem_dtr_supports_op(struct spi_mem *mem,
> >  			     const struct spi_mem_op *op)
> >  {
> > -	if (op->cmd.nbytes != 2)
> > -		return false;
> > +	struct spi_mem_controller_caps caps = {
> > +		.dtr = true,
> > +	};
> >  
> > -	return spi_mem_check_buswidth(mem, op);
> > +	return spi_mem_generic_supports_op(mem, op, &caps);
> >  }
> >  EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op);
> >  
> >  bool spi_mem_default_supports_op(struct spi_mem *mem,
> >  				 const struct spi_mem_op *op)  
> 
> I know this would require a little bit more work from you, but can we 
> make spi_mem_default_supports_op() accept the caps as a parameter? And 
> drop spi_mem_dtr_supports_op() while we are at it? This would be a lot 
> neater I think since we won't have lots of wrapper functions lying 
> around.

Following Boris' review I planned to kill spi_mem_dtr_supports_op(). I
might as well update spi_mem_default_supports_op() so that controllers
can provide their static capacities if any.

> >  {
> > -	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
> > -		return false;
> > +	struct spi_mem_controller_caps caps = {};
> >  
> > -	if (op->cmd.nbytes != 1)
> > -		return false;
> > -
> > -	return spi_mem_check_buswidth(mem, op);
> > +	return spi_mem_generic_supports_op(mem, op, &caps);
> >  }
> >  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
> >  
> > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> > index 85e2ff7b840d..f365efcfb719 100644
> > --- a/include/linux/spi/spi-mem.h
> > +++ b/include/linux/spi/spi-mem.h
> > @@ -220,6 +220,14 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
> >  	return mem->drvpriv;
> >  }
> >  
> > +/**
> > + * struct spi_mem_controller_caps - SPI memory controller capabilities
> > + * @dtr: Supports DTR operations
> > + */
> > +struct spi_mem_controller_caps {
> > +	bool dtr;
> > +};
> > +
> >  /**
> >   * struct spi_controller_mem_ops - SPI memory operations
> >   * @adjust_op_size: shrink the data xfer of an operation to match controller's
> > -- 
> > 2.27.0
> >   
> 


Thanks,
Miquèl