diff mbox series

[v5,12/13] spi: mxic: Use spi_mem_generic_supports_op()

Message ID 20211214114140.54629-13-miquel.raynal@bootlin.com (mailing list archive)
State New, archived
Headers show
Series Pipelined ECC engines & Macronix support | expand

Commit Message

Miquel Raynal Dec. 14, 2021, 11:41 a.m. UTC
This driver can be simplified a little bit by using
spi_mem_generic_supports_op() instead of the
spi_mem_default/dtr_supports_op() couple. The all_false boolean is
inverted to become a dtr boolean, which checks if at least one of the
operation member uses dtr mode. The idea behind this change is to
simplify the introduction of the pipelined ECC engine.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mxic.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Comments

Boris Brezillon Dec. 14, 2021, 4:24 p.m. UTC | #1
On Tue, 14 Dec 2021 12:41:39 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> This driver can be simplified a little bit by using
> spi_mem_generic_supports_op() instead of the
> spi_mem_default/dtr_supports_op() couple. The all_false boolean is
> inverted to become a dtr boolean, which checks if at least one of the
> operation member uses dtr mode. The idea behind this change is to
> simplify the introduction of the pipelined ECC engine.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-mxic.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index 485a7f2afb44..5e71aa630504 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
>  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
>  				     const struct spi_mem_op *op)
>  {
> -	bool all_false;
> +	struct spi_mem_controller_caps caps = {};
>  
>  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
>  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
>  	if (op->addr.nbytes > 7)
>  		return false;
>  
> -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> -		    !op->data.dtr;
> +	caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;

Are you sure that's what you want to do? spi_mem_controller_caps is
supposed to encode the controller capabilities, not whether the
operation contains a DTR cycle or not. I'd expect this caps object to be
statically defined, with possibly one instance per-compat if the caps
depend on the HW revision.

>  
> -	if (all_false)
> -		return spi_mem_default_supports_op(mem, op);
> -	else
> -		return spi_mem_dtr_supports_op(mem, op);
> +	return spi_mem_generic_supports_op(mem, op, &caps);
>  }
>  
>  static int mxic_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
Miquel Raynal Dec. 15, 2021, 5:44 p.m. UTC | #2
Hi Boris,

boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100:

> On Tue, 14 Dec 2021 12:41:39 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > This driver can be simplified a little bit by using
> > spi_mem_generic_supports_op() instead of the
> > spi_mem_default/dtr_supports_op() couple. The all_false boolean is
> > inverted to become a dtr boolean, which checks if at least one of the
> > operation member uses dtr mode. The idea behind this change is to
> > simplify the introduction of the pipelined ECC engine.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/spi/spi-mxic.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > index 485a7f2afb44..5e71aa630504 100644
> > --- a/drivers/spi/spi-mxic.c
> > +++ b/drivers/spi/spi-mxic.c
> > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
> >  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> >  				     const struct spi_mem_op *op)
> >  {
> > -	bool all_false;
> > +	struct spi_mem_controller_caps caps = {};
> >  
> >  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> >  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> >  	if (op->addr.nbytes > 7)
> >  		return false;
> >  
> > -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > -		    !op->data.dtr;
> > +	caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;  
> 
> Are you sure that's what you want to do? spi_mem_controller_caps is
> supposed to encode the controller capabilities, not whether the
> operation contains a DTR cycle or not. I'd expect this caps object to be
> statically defined, with possibly one instance per-compat if the caps
> depend on the HW revision.

In order to keep the series easy to review I decided to go for the
following approach:
* Introduce the spi_mem_generic_supports_op_helper() which takes a
  capabilities structure. This helper gathers all the checks from
  spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These
  two helpers now call the new one with either a NULL pointer in the
  former case, or a structure with the .dtr parameter set to true in
  the latter.
* Change the API of spi_mem_default_supports_op(), this involves
  updating many different drivers so this change does only that in a
  very transparent way, with no functional changes at all. All the
  drivers provide a NULL parameter for the capabilities structure.
* Actually make use of the new parameter of
  spi_mem_default_supports_op() in the drivers Cadence and Macronix,
  which do have DTR support. This kills the spi_mem_dtr_supports_op()
  helper.
* Kill the temporary spi_mem_generic_supports_op() helper by moving
  all the logic back into spi_mem_default_supports_op().

This approach is really straightforward and easily bisectable if
needed. While working on this, I fixed the check we discussed on IRC
about the command parameter when in a DTR operation. I also reverted
the logic in the various checks, as you suggested.

Thanks,
Miquèl
Boris Brezillon Dec. 15, 2021, 6:45 p.m. UTC | #3
On Wed, 15 Dec 2021 18:44:26 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100:
> 
> > On Tue, 14 Dec 2021 12:41:39 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > This driver can be simplified a little bit by using
> > > spi_mem_generic_supports_op() instead of the
> > > spi_mem_default/dtr_supports_op() couple. The all_false boolean is
> > > inverted to become a dtr boolean, which checks if at least one of the
> > > operation member uses dtr mode. The idea behind this change is to
> > > simplify the introduction of the pipelined ECC engine.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/spi/spi-mxic.c | 10 +++-------
> > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > > index 485a7f2afb44..5e71aa630504 100644
> > > --- a/drivers/spi/spi-mxic.c
> > > +++ b/drivers/spi/spi-mxic.c
> > > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
> > >  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > >  				     const struct spi_mem_op *op)
> > >  {
> > > -	bool all_false;
> > > +	struct spi_mem_controller_caps caps = {};
> > >  
> > >  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> > >  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> > > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > >  	if (op->addr.nbytes > 7)
> > >  		return false;
> > >  
> > > -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > > -		    !op->data.dtr;
> > > +	caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;    
> > 
> > Are you sure that's what you want to do? spi_mem_controller_caps is
> > supposed to encode the controller capabilities, not whether the
> > operation contains a DTR cycle or not. I'd expect this caps object to be
> > statically defined, with possibly one instance per-compat if the caps
> > depend on the HW revision.  
> 
> In order to keep the series easy to review I decided to go for the
> following approach:
> * Introduce the spi_mem_generic_supports_op_helper() which takes a
>   capabilities structure. This helper gathers all the checks from
>   spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These
>   two helpers now call the new one with either a NULL pointer in the
>   former case, or a structure with the .dtr parameter set to true in
>   the latter.
> * Change the API of spi_mem_default_supports_op(), this involves
>   updating many different drivers so this change does only that in a
>   very transparent way, with no functional changes at all. All the
>   drivers provide a NULL parameter for the capabilities structure.
> * Actually make use of the new parameter of
>   spi_mem_default_supports_op() in the drivers Cadence and Macronix,
>   which do have DTR support. This kills the spi_mem_dtr_supports_op()
>   helper.
> * Kill the temporary spi_mem_generic_supports_op() helper by moving
>   all the logic back into spi_mem_default_supports_op().
> 
> This approach is really straightforward and easily bisectable if
> needed.

I don't really see the benefit of converting drivers one by one for
such a trivial/mechanical change to be honest. After all, we're talking
about adding a caps argument to spi_mem_default_supports_op() and
making all existing callers pass a caps object that's zero initialized.
I hope we can get this right in one shot...
Boris Brezillon Dec. 15, 2021, 6:52 p.m. UTC | #4
On Wed, 15 Dec 2021 18:44:26 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> In order to keep the series easy to review I decided to go for the
> following approach:
> * Introduce the spi_mem_generic_supports_op_helper() which takes a
>   capabilities structure. This helper gathers all the checks from
>   spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These
>   two helpers now call the new one with either a NULL pointer in the
>   former case, or a structure with the .dtr parameter set to true in
>   the latter.

Is there a benefit adding an extra NULL check when you could make sure
all callers pass a zero-initialized caps object when they don't support
fancy features like DTR or ECC.
Boris Brezillon Dec. 15, 2021, 7:05 p.m. UTC | #5
On Wed, 15 Dec 2021 18:44:26 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100:
> 
> > On Tue, 14 Dec 2021 12:41:39 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > This driver can be simplified a little bit by using
> > > spi_mem_generic_supports_op() instead of the
> > > spi_mem_default/dtr_supports_op() couple. The all_false boolean is
> > > inverted to become a dtr boolean, which checks if at least one of the
> > > operation member uses dtr mode. The idea behind this change is to
> > > simplify the introduction of the pipelined ECC engine.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/spi/spi-mxic.c | 10 +++-------
> > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > > index 485a7f2afb44..5e71aa630504 100644
> > > --- a/drivers/spi/spi-mxic.c
> > > +++ b/drivers/spi/spi-mxic.c
> > > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
> > >  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > >  				     const struct spi_mem_op *op)
> > >  {
> > > -	bool all_false;
> > > +	struct spi_mem_controller_caps caps = {};
> > >  
> > >  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> > >  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> > > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > >  	if (op->addr.nbytes > 7)
> > >  		return false;
> > >  
> > > -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > > -		    !op->data.dtr;
> > > +	caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;    
> > 
> > Are you sure that's what you want to do? spi_mem_controller_caps is
> > supposed to encode the controller capabilities, not whether the
> > operation contains a DTR cycle or not. I'd expect this caps object to be
> > statically defined, with possibly one instance per-compat if the caps
> > depend on the HW revision.  
> 
> In order to keep the series easy to review I decided to go for the
> following approach:
> * Introduce the spi_mem_generic_supports_op_helper() which takes a
>   capabilities structure. This helper gathers all the checks from
>   spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These
>   two helpers now call the new one with either a NULL pointer in the
>   former case, or a structure with the .dtr parameter set to true in
>   the latter.
> * Change the API of spi_mem_default_supports_op(), this involves
>   updating many different drivers so this change does only that in a
>   very transparent way, with no functional changes at all. All the
>   drivers provide a NULL parameter for the capabilities structure.
> * Actually make use of the new parameter of
>   spi_mem_default_supports_op() in the drivers Cadence and Macronix,
>   which do have DTR support. This kills the spi_mem_dtr_supports_op()
>   helper.
> * Kill the temporary spi_mem_generic_supports_op() helper by moving
>   all the logic back into spi_mem_default_supports_op().
> 
> This approach is really straightforward and easily bisectable if
> needed.

There's also a second option that doesn't involve patching existing
users: add a spi_mem_controller_caps to the spi_controller struct, and
check this instance in your spi_mem_default_supports_op()
implementation. Note that the buswidth check done in the generic
helper is already based on caps exposed by the controller
through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits).

> While working on this, I fixed the check we discussed on IRC
> about the command parameter when in a DTR operation. I also reverted
> the logic in the various checks, as you suggested.
> 
> Thanks,
> Miquèl
Mark Brown Dec. 15, 2021, 7:19 p.m. UTC | #6
On Wed, Dec 15, 2021 at 08:05:48PM +0100, Boris Brezillon wrote:

> There's also a second option that doesn't involve patching existing
> users: add a spi_mem_controller_caps to the spi_controller struct, and
> check this instance in your spi_mem_default_supports_op()
> implementation. Note that the buswidth check done in the generic
> helper is already based on caps exposed by the controller
> through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits).

This approach is quite nice for things like this - having things as data
rather than code.  The only issue is if any of the caps end up varying
by operation and we need different capabilities but that doesn't look
too likely here I think?
Miquel Raynal Dec. 16, 2021, 8:07 a.m. UTC | #7
Hi Boris, Mark,

broonie@kernel.org wrote on Wed, 15 Dec 2021 19:19:11 +0000:

> On Wed, Dec 15, 2021 at 08:05:48PM +0100, Boris Brezillon wrote:
> 
> > There's also a second option that doesn't involve patching existing
> > users: add a spi_mem_controller_caps to the spi_controller struct, and
> > check this instance in your spi_mem_default_supports_op()
> > implementation. Note that the buswidth check done in the generic
> > helper is already based on caps exposed by the controller
> > through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits).  
> 
> This approach is quite nice for things like this - having things as data
> rather than code.  The only issue is if any of the caps end up varying
> by operation and we need different capabilities but that doesn't look
> too likely here I think?

Sure, that's a good idea, I'll look into it.

Thanks,
Miquèl
Miquel Raynal Dec. 16, 2021, 8:11 a.m. UTC | #8
Hi Boris,

boris.brezillon@collabora.com wrote on Wed, 15 Dec 2021 19:45:38 +0100:

> On Wed, 15 Dec 2021 18:44:26 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100:
> >   
> > > On Tue, 14 Dec 2021 12:41:39 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > This driver can be simplified a little bit by using
> > > > spi_mem_generic_supports_op() instead of the
> > > > spi_mem_default/dtr_supports_op() couple. The all_false boolean is
> > > > inverted to become a dtr boolean, which checks if at least one of the
> > > > operation member uses dtr mode. The idea behind this change is to
> > > > simplify the introduction of the pipelined ECC engine.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/spi/spi-mxic.c | 10 +++-------
> > > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > > > index 485a7f2afb44..5e71aa630504 100644
> > > > --- a/drivers/spi/spi-mxic.c
> > > > +++ b/drivers/spi/spi-mxic.c
> > > > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
> > > >  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > > >  				     const struct spi_mem_op *op)
> > > >  {
> > > > -	bool all_false;
> > > > +	struct spi_mem_controller_caps caps = {};
> > > >  
> > > >  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> > > >  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> > > > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > > >  	if (op->addr.nbytes > 7)
> > > >  		return false;
> > > >  
> > > > -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > > > -		    !op->data.dtr;
> > > > +	caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;      
> > > 
> > > Are you sure that's what you want to do? spi_mem_controller_caps is
> > > supposed to encode the controller capabilities, not whether the
> > > operation contains a DTR cycle or not. I'd expect this caps object to be
> > > statically defined, with possibly one instance per-compat if the caps
> > > depend on the HW revision.    
> > 
> > In order to keep the series easy to review I decided to go for the
> > following approach:
> > * Introduce the spi_mem_generic_supports_op_helper() which takes a
> >   capabilities structure. This helper gathers all the checks from
> >   spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These
> >   two helpers now call the new one with either a NULL pointer in the
> >   former case, or a structure with the .dtr parameter set to true in
> >   the latter.
> > * Change the API of spi_mem_default_supports_op(), this involves
> >   updating many different drivers so this change does only that in a
> >   very transparent way, with no functional changes at all. All the
> >   drivers provide a NULL parameter for the capabilities structure.
> > * Actually make use of the new parameter of
> >   spi_mem_default_supports_op() in the drivers Cadence and Macronix,
> >   which do have DTR support. This kills the spi_mem_dtr_supports_op()
> >   helper.
> > * Kill the temporary spi_mem_generic_supports_op() helper by moving
> >   all the logic back into spi_mem_default_supports_op().
> > 
> > This approach is really straightforward and easily bisectable if
> > needed.  
> 
> I don't really see the benefit of converting drivers one by one for
> such a trivial/mechanical change to be honest. After all, we're talking
> about adding a caps argument to spi_mem_default_supports_op() and
> making all existing callers pass a caps object that's zero initialized.
> I hope we can get this right in one shot...

I'm not converting them one by one, but all in one patch. One by one is
not even an option because it would completely break bisectability.
What I meant is that in a single patch, I was converting all the
drivers using spi_mem_default_supports_op() to pass a third parameter.
In *all* cases, this parameter would be NULL (because I find this
cleaner than providing empty structures for 95% of the drivers). In the
next patch, I would actually convert the users of
spi_mem_dtr_supports_op() to use spi_mem_default_supports_op() by
providing their static capabilities definition.

Thanks,
Miquèl
Miquel Raynal Dec. 16, 2021, 8:14 a.m. UTC | #9
Hi Boris,

boris.brezillon@collabora.com wrote on Wed, 15 Dec 2021 19:52:19 +0100:

> On Wed, 15 Dec 2021 18:44:26 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > In order to keep the series easy to review I decided to go for the
> > following approach:
> > * Introduce the spi_mem_generic_supports_op_helper() which takes a
> >   capabilities structure. This helper gathers all the checks from
> >   spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These
> >   two helpers now call the new one with either a NULL pointer in the
> >   former case, or a structure with the .dtr parameter set to true in
> >   the latter.  
> 
> Is there a benefit adding an extra NULL check when you could make sure
> all callers pass a zero-initialized caps object when they don't support
> fancy features like DTR or ECC.

That's exactly my point, I really don't like the creation of 15 empty
and useless structures while we could just have a check. If the
controller provides no capabilities, we assure he has none. I don't
think checking "if (caps && caps->PARAM)" hurts. Anyway, if we go for
the spi_mem controller internal structure approach, we might just not
need those.

Thanks,
Miquèl
Miquel Raynal Dec. 16, 2021, 9:01 a.m. UTC | #10
Hi Mark,

broonie@kernel.org wrote on Wed, 15 Dec 2021 19:19:11 +0000:

> On Wed, Dec 15, 2021 at 08:05:48PM +0100, Boris Brezillon wrote:
> 
> > There's also a second option that doesn't involve patching existing
> > users: add a spi_mem_controller_caps to the spi_controller struct, and
> > check this instance in your spi_mem_default_supports_op()
> > implementation. Note that the buswidth check done in the generic
> > helper is already based on caps exposed by the controller
> > through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits).  
> 
> This approach is quite nice for things like this - having things as data
> rather than code.  The only issue is if any of the caps end up varying
> by operation and we need different capabilities but that doesn't look
> too likely here I think?

Indeed that was the main point of the original review from Boris, the
capabilities should be fixed on the controller's lifetime. So I believe
we are safe.

I think I am going to propose the following:
	const struct spi_controller_mem_ops *mem_ops;
+	struct spi_controller_mem_caps mem_caps;

As the structure is not supposed to enlarge dramatically in the near
future, I guess it's fine to have it defined statically. Please tell me
if you prefer a *mem_caps pointer.

I'll send a proposal soon.

Thanks,
Miquèl
Miquel Raynal Dec. 16, 2021, 9:57 a.m. UTC | #11
miquel.raynal@bootlin.com wrote on Thu, 16 Dec 2021 10:01:47 +0100:

> Hi Mark,
> 
> broonie@kernel.org wrote on Wed, 15 Dec 2021 19:19:11 +0000:
> 
> > On Wed, Dec 15, 2021 at 08:05:48PM +0100, Boris Brezillon wrote:
> >   
> > > There's also a second option that doesn't involve patching existing
> > > users: add a spi_mem_controller_caps to the spi_controller struct, and
> > > check this instance in your spi_mem_default_supports_op()
> > > implementation. Note that the buswidth check done in the generic
> > > helper is already based on caps exposed by the controller
> > > through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits).    
> > 
> > This approach is quite nice for things like this - having things as data
> > rather than code.  The only issue is if any of the caps end up varying
> > by operation and we need different capabilities but that doesn't look
> > too likely here I think?  
> 
> Indeed that was the main point of the original review from Boris, the
> capabilities should be fixed on the controller's lifetime. So I believe
> we are safe.
> 
> I think I am going to propose the following:
> 	const struct spi_controller_mem_ops *mem_ops;
> +	struct spi_controller_mem_caps mem_caps;
> 
> As the structure is not supposed to enlarge dramatically in the near
> future, I guess it's fine to have it defined statically. Please tell me
> if you prefer a *mem_caps pointer.

Actually as the spi-mem.h header is not included in spi.h, it makes
defining a static mem_caps entry harder. I'll go for another approach.
Maybe putting the capabilities within the spi_controller_mem_ops
structure, as these are highly related.

> I'll send a proposal soon.
> 
> Thanks,
> Miquèl


Thanks,
Miquèl
Mark Brown Dec. 16, 2021, 2:04 p.m. UTC | #12
On Thu, Dec 16, 2021 at 10:57:39AM +0100, Miquel Raynal wrote:

> Actually as the spi-mem.h header is not included in spi.h, it makes
> defining a static mem_caps entry harder. I'll go for another approach.
> Maybe putting the capabilities within the spi_controller_mem_ops
> structure, as these are highly related.

Yeah, or putting a pointer to a static declaration of the caps in there
rather than the caps directly.
Miquel Raynal Dec. 16, 2021, 2:27 p.m. UTC | #13
Hi Mark,

broonie@kernel.org wrote on Thu, 16 Dec 2021 14:04:18 +0000:

> On Thu, Dec 16, 2021 at 10:57:39AM +0100, Miquel Raynal wrote:
> 
> > Actually as the spi-mem.h header is not included in spi.h, it makes
> > defining a static mem_caps entry harder. I'll go for another approach.
> > Maybe putting the capabilities within the spi_controller_mem_ops
> > structure, as these are highly related.  
> 
> Yeah, or putting a pointer to a static declaration of the caps in there
> rather than the caps directly.

Yeah, that's what I ended up doing. Each controller driver supporting
mem-ops must provide a capabilities structure. Drivers without specific
capabilities can just reference the static &spi_mem_no_caps struct
instead of defining their empty one.

Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 485a7f2afb44..5e71aa630504 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -452,7 +452,7 @@  static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
 static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
 				     const struct spi_mem_op *op)
 {
-	bool all_false;
+	struct spi_mem_controller_caps caps = {};
 
 	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
 	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
@@ -465,13 +465,9 @@  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
 	if (op->addr.nbytes > 7)
 		return false;
 
-	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
-		    !op->data.dtr;
+	caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;
 
-	if (all_false)
-		return spi_mem_default_supports_op(mem, op);
-	else
-		return spi_mem_dtr_supports_op(mem, op);
+	return spi_mem_generic_supports_op(mem, op, &caps);
 }
 
 static int mxic_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)