[1/3] mem: spi-mem: define spi_mem_default_supports_op

Message ID 20210118235256.29748-2-matt@traverse.com.au
State New
Headers show
Series
  • Fixes for SPI-NAND issues on LS1088A
Related show

Commit Message

Mathew McBride Jan. 18, 2021, 11:52 p.m.
spi_mem_default_supports_op is used internally by controller
drivers to verify operation semantics are correct.

Signed-off-by: Mathew McBride <matt@traverse.com.au>

---
 include/spi-mem.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.30.0

Comments

Pratyush Yadav Jan. 19, 2021, 12:06 p.m. | #1
Hi Matthew,

> Subject: [PATCH 1/3] mem: spi-mem: define spi_mem_default_supports_op


Nitpick: You are declaring spi_mem_default_supports_op() here. It is 
already defined.

On 18/01/21 11:52PM, Mathew McBride wrote:
> spi_mem_default_supports_op is used internally by controller

> drivers to verify operation semantics are correct.

> 

> Signed-off-by: Mathew McBride <matt@traverse.com.au>

> ---

>  include/spi-mem.h | 10 ++++++++++

>  1 file changed, 10 insertions(+)

> 

> diff --git a/include/spi-mem.h b/include/spi-mem.h

> index ca0f55c8fd..92c640dabe 100644

> --- a/include/spi-mem.h

> +++ b/include/spi-mem.h

> @@ -216,6 +216,10 @@ int spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,

>  void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,

>  					  const struct spi_mem_op *op,

>  					  struct sg_table *sg);

> +

> +bool spi_mem_default_supports_op(struct spi_mem *mem,

> +				 const struct spi_mem_op *op);

> +


This block of code was imported verbatim from the Linux driver and then 
wrapped around with a #ifndef __UBOOT__ to avoid compilation errors. So 
it will never get "enabled" in U-Boot ever. No driver can use the 
prototypes you have added.

And I tested this by applying your patch series and building the 
fsl_qspi driver using ls1012aqds_qspi_defconfig. Sure enough, the 
compiler reported "implicit declaration of function 
spi_mem_default_supports_op". Strangely, the linker did not complain and 
went through without errors. Not sure which function it would end up 
linking there.

Move the declaration outside this ifdef, right beside where
spi_mem_supports_op() is declared. No need to have the variant below. It 
is safe to assume CONFIG_SPI_MEM is enabled if spi-mem.h is included.

>  #else

>  static inline int

>  spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,

> @@ -231,6 +235,12 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,

>  				     struct sg_table *sg)

>  {

>  }

> +

> +bool spi_mem_default_supports_op(struct spi_mem *mem,

> +				 const struct spi_mem_op *op)

> +{

> +	return false;

> +}

>  #endif /* CONFIG_SPI_MEM */

>  #endif /* __UBOOT__ */

>  

> -- 

> 2.30.0

> 


-- 
Regards,
Pratyush Yadav
Texas Instruments India
Mathew McBride Jan. 25, 2021, 4 a.m. | #2
Hello Pratyush,
On Tue, Jan 19, 2021, at 11:06 PM, Pratyush Yadav wrote:
> Hi Matthew,

> 

> > Subject: [PATCH 1/3] mem: spi-mem: define spi_mem_default_supports_op

> 

> Nitpick: You are declaring spi_mem_default_supports_op() here. It is 

> already defined.

> [snip]

> 

> This block of code was imported verbatim from the Linux driver and then 

> wrapped around with a #ifndef __UBOOT__ to avoid compilation errors. So 

> it will never get "enabled" in U-Boot ever. No driver can use the 

> prototypes you have added.

> 

> And I tested this by applying your patch series and building the 

> fsl_qspi driver using ls1012aqds_qspi_defconfig. Sure enough, the 

> compiler reported "implicit declaration of function 

> spi_mem_default_supports_op". Strangely, the linker did not complain and 

> went through without errors. Not sure which function it would end up 

> linking there.

> 

> Move the declaration outside this ifdef, right beside where

> spi_mem_supports_op() is declared. No need to have the variant below. It 

> is safe to assume CONFIG_SPI_MEM is enabled if spi-mem.h is included.

> 

Many thanks for your feedback - I did not account for the differences in the kernel and U-Boot here. 

My revised patch should handle this correctly.

Best Regards,
Mathew

Patch

diff --git a/include/spi-mem.h b/include/spi-mem.h
index ca0f55c8fd..92c640dabe 100644
--- a/include/spi-mem.h
+++ b/include/spi-mem.h
@@ -216,6 +216,10 @@  int spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,
 void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
 					  const struct spi_mem_op *op,
 					  struct sg_table *sg);
+
+bool spi_mem_default_supports_op(struct spi_mem *mem,
+				 const struct spi_mem_op *op);
+
 #else
 static inline int
 spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,
@@ -231,6 +235,12 @@  spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
 				     struct sg_table *sg)
 {
 }
+
+bool spi_mem_default_supports_op(struct spi_mem *mem,
+				 const struct spi_mem_op *op)
+{
+	return false;
+}
 #endif /* CONFIG_SPI_MEM */
 #endif /* __UBOOT__ */