Re: [PATCH 2/3] spi: Remove uses of #ifdef __U_BOOT__ from spi-mem.c

On 8/7/20 12:48 PM, Pratyush Yadav wrote:

Hi Sean,

...

Subject: [PATCH 2/3] spi: Remove uses of #ifdef __U_BOOT__ from spi-mem.c

Maybe s/ifdef/ifndef/

Sure.

On 07/08/20 09:30AM, Sean Anderson wrote:

...

Preprocessing out large sections of the file is confusing and makes it difficult to follow the control flow. Presumably these were initially added to make porting easier, but this code has not been synced with Linux since it was introduced two years ago.

Signed-off-by: Sean Anderson seanga2@gmail.com

drivers/spi/spi-mem.c | 283 ------------------------------------------ 1 file changed, 283 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index c095ae9505..36f3eb7f1e 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -6,109 +6,8 @@

• Author: Boris Brezillon boris.brezillon@bootlin.com

*/

-#ifndef __UBOOT__ -#include <log.h> -#include <dm/devres.h> -#include <linux/dmaengine.h> -#include <linux/pm_runtime.h> -#include "internals.h" -#else -#include <common.h> -#include <dm.h> -#include <errno.h> -#include <malloc.h> -#include <spi.h> #include <spi.h> #include <spi-mem.h> -#include <dm/device_compat.h>

Why remove these headers? They are not part of a #ifndef __UBOOT__. The build fails with this patch applied on latest master:

Hm, I'm not sure how those got deleted. I originally planned to submit this in another series, so perhaps I made an error when cherry-picking. I will resubmit this with those headers included.

drivers/spi/spi-mem.c: In function ‘spi_mem_supports_op’: drivers/spi/spi-mem.c:87:34: error: dereferencing pointer to incomplete type ‘struct udevice’ struct udevice *bus = slave->dev->parent; ^~ drivers/spi/spi-mem.c: In function ‘spi_mem_exec_op’: drivers/spi/spi-mem.c:195:3: warning: implicit declaration of function ‘debug’; did you mean ‘pr_debug’? [-Wimplicit-function-declaration] debug("%02x ", op_buf[i]); ^~~~~ pr_debug

...

-#endif

-#ifndef __UBOOT__ -/**

• • spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a

• • 			  memory operation
    

• • @ctlr: the SPI controller requesting this dma_map()
• • @op: the memory operation containing the buffer to map
• • @sgt: a pointer to a non-initialized sg_table that will be filled by this

• • function
    

• • Some controllers might want to do DMA on the data buffer embedded in @op.
• • This helper prepares everything for you and provides a ready-to-use
• • sg_table. This function is not intended to be called from spi drivers.
• • Only SPI controller drivers should use it.
• • Note that the caller must ensure the memory region pointed by
• • op->data.buf.{in,out} is DMA-able before calling this function.
• • Return: 0 in case of success, a negative error code otherwise.
• */

-int spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,

• 		       const struct spi_mem_op *op,
  

• 		       struct sg_table *sgt)
  

-{

• struct device *dmadev;
• if (!op->data.nbytes)

• return -EINVAL;
  

• if (op->data.dir == SPI_MEM_DATA_OUT && ctlr->dma_tx)

• dmadev = ctlr->dma_tx->device->dev;
  

• else if (op->data.dir == SPI_MEM_DATA_IN && ctlr->dma_rx)

• dmadev = ctlr->dma_rx->device->dev;
  

• else

• dmadev = ctlr->dev.parent;
  

• if (!dmadev)

• return -EINVAL;
  

• return spi_map_buf(ctlr, dmadev, sgt, op->data.buf.in, op->data.nbytes,

• 	   op->data.dir == SPI_MEM_DATA_IN ?
  

• 	   DMA_FROM_DEVICE : DMA_TO_DEVICE);
  

-} -EXPORT_SYMBOL_GPL(spi_controller_dma_map_mem_op_data);

-/**

• • spi_controller_dma_unmap_mem_op_data() - DMA-unmap the buffer attached to a

• • 			    memory operation
    

• • @ctlr: the SPI controller requesting this dma_unmap()
• • @op: the memory operation containing the buffer to unmap
• • @sgt: a pointer to an sg_table previously initialized by

• • spi_controller_dma_map_mem_op_data()
    

• • Some controllers might want to do DMA on the data buffer embedded in @op.
• • This helper prepares things so that the CPU can access the
• • op->data.buf.{in,out} buffer again.
• • This function is not intended to be called from SPI drivers. Only SPI
• • controller drivers should use it.
• • This function should be called after the DMA operation has finished and is
• • only valid if the previous spi_controller_dma_map_mem_op_data() call
• • returned 0.
• • Return: 0 in case of success, a negative error code otherwise.
• */

-void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,

• 			  const struct spi_mem_op *op,
  

• 			  struct sg_table *sgt)
  

-{

• struct device *dmadev;
• if (!op->data.nbytes)

• return;
  

• if (op->data.dir == SPI_MEM_DATA_OUT && ctlr->dma_tx)

• dmadev = ctlr->dma_tx->device->dev;
  

• else if (op->data.dir == SPI_MEM_DATA_IN && ctlr->dma_rx)

• dmadev = ctlr->dma_rx->device->dev;
  

• else

• dmadev = ctlr->dev.parent;
  

• spi_unmap_buf(ctlr, dmadev, sgt,

•       op->data.dir == SPI_MEM_DATA_IN ?
  

•       DMA_FROM_DEVICE : DMA_TO_DEVICE);
  

-} -EXPORT_SYMBOL_GPL(spi_controller_dma_unmap_mem_op_data); -#endif /* __UBOOT__ */

static int spi_check_buswidth_req(struct spi_slave *slave, u8 buswidth, bool tx) { @@ -166,7 +65,6 @@ bool spi_mem_default_supports_op(struct spi_slave *slave,

return true; } -EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);

Nitpick: Since this patch is about removing things in #ifndef __UBOOT_, please don't remove things outside of it. Do them as a separate patch instead. Same for all other places below.

Ok, I can move the removal of these lines to its own patch.

I am neither endorsing this patch nor opposing it, but please at least make sure it builds.

Thanks for the feedback.

--Sean