| Message ID | 20210118235256.29748-2-matt@traverse.com.au |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for SPI-NAND issues on LS1088A | expand |
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
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
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__ */
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