diff mbox

[v2,02/10] spi: Expose spi_{map,unmap}_buf() for internal use

Message ID 20180410224439.9260-3-boris.brezillon@bootlin.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris Brezillon April 10, 2018, 10:44 p.m. UTC 
spi_{map,unmap}_buf() are needed by the spi-mem logic that is about to
be introduced to prepare data buffer for DMA operations.

Remove the static specifier on these functions and add their prototypes
to include/linux/spi/spi.h. We do not export the symbols here because
both SPI_MEM and SPI can't be enabled as modules and we'd like to
prevent controller/device drivers from using these functions.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
I see 2 other options to not expose internal stuff to spi users or
controller drivers:

1/ we do not expose spi_map/unmap_buf() and instead implement
   spi_controller_dma_map/unmap_mem_op_data() directly in spi.c
2/ we create a new header (drivers/spi/internals.h?) for all
   definitions that are meant for internal use (shared between spi.c
   and spi-mem.c)

I personally prefer #2 as it provides a better separation between spi
and spi-mem code.
---
 drivers/spi/spi.c       | 23 +++++------------------
 include/linux/spi/spi.h | 26 ++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 18 deletions(-)

Comments

Mark Brown April 16, 2018, 12:11 p.m. UTC | #1 
On Wed, Apr 11, 2018 at 12:44:31AM +0200, Boris Brezillon wrote:

> 2/ we create a new header (drivers/spi/internals.h?) for all
>    definitions that are meant for internal use (shared between spi.c
>    and spi-mem.c)

> I personally prefer #2 as it provides a better separation between spi
> and spi-mem code.
> ---
>  drivers/spi/spi.c       | 23 +++++------------------
>  include/linux/spi/spi.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 18 deletions(-)

I prefer that too (if you prefer it you should go with it so it's the
option on the table!).  I'd go with drivers/spi/internals.h though so
that it's even more obvious if something is being silly trying to use
it when they shouldn't.
Boris Brezillon April 18, 2018, 2:20 p.m. UTC | #2 
Hi Mark,

On Mon, 16 Apr 2018 13:11:27 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Apr 11, 2018 at 12:44:31AM +0200, Boris Brezillon wrote:
> 
> > 2/ we create a new header (drivers/spi/internals.h?) for all
> >    definitions that are meant for internal use (shared between spi.c
> >    and spi-mem.c)  
> 
> > I personally prefer #2 as it provides a better separation between spi
> > and spi-mem code.
> > ---
> >  drivers/spi/spi.c       | 23 +++++------------------
> >  include/linux/spi/spi.h | 26 ++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+), 18 deletions(-)  
> 
> I prefer that too (if you prefer it you should go with it so it's the
> option on the table!).

I hesitated for long and went for the solution I didn't like, mainly
because everything is already put in a single header file so I thought
you would prefer that approach.

> I'd go with drivers/spi/internals.h though so
> that it's even more obvious if something is being silly trying to use
> it when they shouldn't.

Okay, I'll move those definitions to drivers/spi/internals.h.

Thanks,

Boris


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 24369e437c6b..308e4c2114d8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -740,9 +740,9 @@  static void spi_set_cs(struct spi_device *spi, bool enable)
 }
 
 #ifdef CONFIG_HAS_DMA
-static int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
-		       struct sg_table *sgt, void *buf, size_t len,
-		       enum dma_data_direction dir)
+int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
+		struct sg_table *sgt, void *buf, size_t len,
+		enum dma_data_direction dir)
 {
 	const bool vmalloced_buf = is_vmalloc_addr(buf);
 	unsigned int max_seg_size = dma_get_max_seg_size(dev);
@@ -821,8 +821,8 @@  static int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
 	return 0;
 }
 
-static void spi_unmap_buf(struct spi_controller *ctlr, struct device *dev,
-			  struct sg_table *sgt, enum dma_data_direction dir)
+void spi_unmap_buf(struct spi_controller *ctlr, struct device *dev,
+		   struct sg_table *sgt, enum dma_data_direction dir)
 {
 	if (sgt->orig_nents) {
 		dma_unmap_sg(dev, sgt->sgl, sgt->orig_nents, dir);
@@ -907,19 +907,6 @@  static int __spi_unmap_msg(struct spi_controller *ctlr, struct spi_message *msg)
 	return 0;
 }
 #else /* !CONFIG_HAS_DMA */
-static inline int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
-			      struct sg_table *sgt, void *buf, size_t len,
-			      enum dma_data_direction dir)
-{
-	return -EINVAL;
-}
-
-static inline void spi_unmap_buf(struct spi_controller *ctlr,
-				 struct device *dev, struct sg_table *sgt,
-				 enum dma_data_direction dir)
-{
-}
-
 static inline int __spi_map_msg(struct spi_controller *ctlr,
 				struct spi_message *msg)
 {
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index bc6bb325d1bf..de6fd95a61c5 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -16,6 +16,7 @@ 
 #define __LINUX_SPI_H
 
 #include <linux/device.h>
+#include <linux/dma-direction.h>
 #include <linux/mod_devicetable.h>
 #include <linux/slab.h>
 #include <linux/kthread.h>
@@ -614,6 +615,31 @@  static inline bool spi_controller_is_slave(struct spi_controller *ctlr)
 extern int spi_controller_suspend(struct spi_controller *ctlr);
 extern int spi_controller_resume(struct spi_controller *ctlr);
 
+/*
+ * Helpers needed by the spi-mem logic. Should not be used outside of
+ * spi-mem.c
+ */
+#ifdef CONFIG_HAS_DMA
+int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
+		struct sg_table *sgt, void *buf, size_t len,
+		enum dma_data_direction dir);
+void spi_unmap_buf(struct spi_controller *ctlr, struct device *dev,
+		   struct sg_table *sgt, enum dma_data_direction dir);
+#else /* !CONFIG_HAS_DMA */
+static inline int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
+			      struct sg_table *sgt, void *buf, size_t len,
+			      enum dma_data_direction dir)
+{
+	return -EINVAL;
+}
+
+static inline void spi_unmap_buf(struct spi_controller *ctlr,
+				 struct device *dev, struct sg_table *sgt,
+				 enum dma_data_direction dir)
+{
+}
+#endif /* CONFIG_HAS_DMA */
+
 /* Calls the driver make to interact with the message queue */
 extern struct spi_message *spi_get_next_queued_message(struct spi_controller *ctlr);
 extern void spi_finalize_current_message(struct spi_controller *ctlr);