[PATCH] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[PATCH] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Nathan Chancellor]

Clang warns when one enumerated type is implicitly converted to another.

drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:428:58: note: expanded from macro
'dma_map_sg'
\#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
                               ~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
                dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
\#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
\#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
3 warnings generated.

dma_{,un}map_sg expects an enum of type dma_data_direction but this
driver uses dma_transfer_direction for everything. Converting to
dma_data_direction would be desirable but there are a few shared
structures that expect dma_transfer_direction so it is just simpler to
change the parameter here. dma_transfer_direction and dma_data_direction
are different sizes but this driver only uses the 1 and 2 values which
mean the same thing so this change is safe.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/spi/spi-ep93xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
index f1526757aaf6..189fc2225b69 100644
--- a/drivers/spi/spi-ep93xx.c
+++ b/drivers/spi/spi-ep93xx.c
@@ -256,8 +256,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
  * in case of failure.
  */
 static struct dma_async_tx_descriptor *
-ep93xx_spi_dma_prepare(struct spi_master *master,
-		       enum dma_transfer_direction dir)
+ep93xx_spi_dma_prepare(struct spi_master *master, int dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct spi_transfer *xfer = master->cur_msg->state;
@@ -359,8 +358,7 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
  * Function finishes with the DMA transfer. After this, the DMA buffer is
  * unmapped.
  */
-static void ep93xx_spi_dma_finish(struct spi_master *master,
-				  enum dma_transfer_direction dir)
+static void ep93xx_spi_dma_finish(struct spi_master *master, int dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct dma_chan *chan;
-- 
2.19.0

[PATCH v2] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Nathan Chancellor]

Clang warns when one enumerated type is implicitly converted to another.

drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:428:58: note: expanded from macro
'dma_map_sg'
#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
                               ~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
                dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
3 warnings generated.

dma_{,un}map_sg expects an enum of type dma_data_direction but this
driver uses dma_transfer_direction for everything. Converting to
dma_data_direction would be desirable but there are a few shared
structures that expect dma_transfer_direction so it is just simpler to
change the parameter here. dma_transfer_direction and dma_data_direction
are different sizes but this driver only uses the 1 and 2 values which
mean the same thing so this change is safe.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Fix escaped hash symbols for '#define' lines

 drivers/spi/spi-ep93xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
index f1526757aaf6..189fc2225b69 100644
--- a/drivers/spi/spi-ep93xx.c
+++ b/drivers/spi/spi-ep93xx.c
@@ -256,8 +256,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
  * in case of failure.
  */
 static struct dma_async_tx_descriptor *
-ep93xx_spi_dma_prepare(struct spi_master *master,
-		       enum dma_transfer_direction dir)
+ep93xx_spi_dma_prepare(struct spi_master *master, int dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct spi_transfer *xfer = master->cur_msg->state;
@@ -359,8 +358,7 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
  * Function finishes with the DMA transfer. After this, the DMA buffer is
  * unmapped.
  */
-static void ep93xx_spi_dma_finish(struct spi_master *master,
-				  enum dma_transfer_direction dir)
+static void ep93xx_spi_dma_finish(struct spi_master *master, int dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct dma_chan *chan;
-- 
2.19.0

Re: [PATCH v2] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Nick Desaulniers]

On Wed, Oct 3, 2018 at 7:41 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]

I'm just trying to think what this change would look like if this
driver ONLY used `enum dma_data_direction`?  There's an additional
warning in the header that a return value is messed up there, too:

In file included from drivers/spi/spi-ep93xx.c:34:
./include/linux/platform_data/dma-ep93xx.h:88:10: warning: implicit
conversion from
      enumeration type 'enum dma_data_direction' to different
enumeration type 'enum
      dma_transfer_direction' [-Wenum-conversion]
                return DMA_NONE;
                ~~~~~~ ^~~~~~~~

>         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> 'dma_map_sg'
> #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>                                ~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> 3 warnings generated.
>
> dma_{,un}map_sg expects an enum of type dma_data_direction but this
> driver uses dma_transfer_direction for everything. Converting to
> dma_data_direction would be desirable but there are a few shared
> structures that expect dma_transfer_direction so it is just simpler to
> change the parameter here. dma_transfer_direction and dma_data_direction
> are different sizes but this driver only uses the 1 and 2 values which
> mean the same thing so this change is safe.
>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Fix escaped hash symbols for '#define' lines
>
>  drivers/spi/spi-ep93xx.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> index f1526757aaf6..189fc2225b69 100644
> --- a/drivers/spi/spi-ep93xx.c
> +++ b/drivers/spi/spi-ep93xx.c
> @@ -256,8 +256,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
>   * in case of failure.
>   */
>  static struct dma_async_tx_descriptor *
> -ep93xx_spi_dma_prepare(struct spi_master *master,
> -                      enum dma_transfer_direction dir)
> +ep93xx_spi_dma_prepare(struct spi_master *master, int dir)
>  {
>         struct ep93xx_spi *espi = spi_master_get_devdata(master);
>         struct spi_transfer *xfer = master->cur_msg->state;
> @@ -359,8 +358,7 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
>   * Function finishes with the DMA transfer. After this, the DMA buffer is
>   * unmapped.
>   */
> -static void ep93xx_spi_dma_finish(struct spi_master *master,
> -                                 enum dma_transfer_direction dir)
> +static void ep93xx_spi_dma_finish(struct spi_master *master, int dir)
>  {
>         struct ep93xx_spi *espi = spi_master_get_devdata(master);
>         struct dma_chan *chan;
> --
> 2.19.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Nathan Chancellor]

On Thu, Oct 04, 2018 at 02:32:48PM -0700, Nick Desaulniers wrote:
> On Wed, Oct 3, 2018 at 7:41 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns when one enumerated type is implicitly converted to another.
> >
> > drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> 
> I'm just trying to think what this change would look like if this
> driver ONLY used `enum dma_data_direction`?  There's an additional

I can work up a proof of concept patch but it was kind of convoluted
when I tried before this patch because dmaengine_prep_slave_sg and the
direction member in struct dma_slave_config expect an enum of type
dma_transfer_direction so there are two explicit casts needed and I
thought there were one or two more places that it complained.

This was the cleanest solution to me but I'm happy to rework it in any
way that I need to.

> warning in the header that a return value is messed up there, too:
> 
> In file included from drivers/spi/spi-ep93xx.c:34:
> ./include/linux/platform_data/dma-ep93xx.h:88:10: warning: implicit
> conversion from
>       enumeration type 'enum dma_data_direction' to different
> enumeration type 'enum
>       dma_transfer_direction' [-Wenum-conversion]
>                 return DMA_NONE;
>                 ~~~~~~ ^~~~~~~~
> 

This is fixed in the dma tree and that function isn't used in this file:
https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git/commit/?id=9524d6b265f9b2b9a61fceb2ee2ce1c2a83e39ca

> >         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> > 'dma_map_sg'
> > #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
> >                                ~~~~~~~~~~~~~~~~          ^
> > drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > 'dma_unmap_sg'
> > #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> >                                  ~~~~~~~~~~~~~~~~~~          ^
> > drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > 'dma_unmap_sg'
> > #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> >                                  ~~~~~~~~~~~~~~~~~~          ^
> > 3 warnings generated.
> >
> > dma_{,un}map_sg expects an enum of type dma_data_direction but this
> > driver uses dma_transfer_direction for everything. Converting to
> > dma_data_direction would be desirable but there are a few shared
> > structures that expect dma_transfer_direction so it is just simpler to
> > change the parameter here. dma_transfer_direction and dma_data_direction
> > are different sizes but this driver only uses the 1 and 2 values which
> > mean the same thing so this change is safe.
> >
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >
> > v1 -> v2:
> >
> > * Fix escaped hash symbols for '#define' lines
> >
> >  drivers/spi/spi-ep93xx.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> > index f1526757aaf6..189fc2225b69 100644
> > --- a/drivers/spi/spi-ep93xx.c
> > +++ b/drivers/spi/spi-ep93xx.c
> > @@ -256,8 +256,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
> >   * in case of failure.
> >   */
> >  static struct dma_async_tx_descriptor *
> > -ep93xx_spi_dma_prepare(struct spi_master *master,
> > -                      enum dma_transfer_direction dir)
> > +ep93xx_spi_dma_prepare(struct spi_master *master, int dir)
> >  {
> >         struct ep93xx_spi *espi = spi_master_get_devdata(master);
> >         struct spi_transfer *xfer = master->cur_msg->state;
> > @@ -359,8 +358,7 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
> >   * Function finishes with the DMA transfer. After this, the DMA buffer is
> >   * unmapped.
> >   */
> > -static void ep93xx_spi_dma_finish(struct spi_master *master,
> > -                                 enum dma_transfer_direction dir)
> > +static void ep93xx_spi_dma_finish(struct spi_master *master, int dir)
> >  {
> >         struct ep93xx_spi *espi = spi_master_get_devdata(master);
> >         struct dma_chan *chan;
> > --
> > 2.19.0
> >
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers

Re: [PATCH v2] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Nick Desaulniers]

On Thu, Oct 4, 2018 at 2:47 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Oct 04, 2018 at 02:32:48PM -0700, Nick Desaulniers wrote:
> > On Wed, Oct 3, 2018 at 7:41 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns when one enumerated type is implicitly converted to another.
> > >
> > > drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> >
> > I'm just trying to think what this change would look like if this
> > driver ONLY used `enum dma_data_direction`?  There's an additional
>
> I can work up a proof of concept patch but it was kind of convoluted
> when I tried before this patch because dmaengine_prep_slave_sg and the
> direction member in struct dma_slave_config expect an enum of type
> dma_transfer_direction so there are two explicit casts needed and I
> thought there were one or two more places that it complained.

If you made the attempt already and found this to be a better
solution, I trust your judgement.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> This was the cleanest solution to me but I'm happy to rework it in any
> way that I need to.
>
> > warning in the header that a return value is messed up there, too:
> >
> > In file included from drivers/spi/spi-ep93xx.c:34:
> > ./include/linux/platform_data/dma-ep93xx.h:88:10: warning: implicit
> > conversion from
> >       enumeration type 'enum dma_data_direction' to different
> > enumeration type 'enum
> >       dma_transfer_direction' [-Wenum-conversion]
> >                 return DMA_NONE;
> >                 ~~~~~~ ^~~~~~~~
> >
>
> This is fixed in the dma tree and that function isn't used in this file:
> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git/commit/?id=9524d6b265f9b2b9a61fceb2ee2ce1c2a83e39ca

Heh, I should have remembered!

>
> > >         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> > > 'dma_map_sg'
> > > #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
> > >                                ~~~~~~~~~~~~~~~~          ^
> > > drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> > >                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > > 'dma_unmap_sg'
> > > #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> > >                                  ~~~~~~~~~~~~~~~~~~          ^
> > > drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> > >         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > > 'dma_unmap_sg'
> > > #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> > >                                  ~~~~~~~~~~~~~~~~~~          ^
> > > 3 warnings generated.
> > >
> > > dma_{,un}map_sg expects an enum of type dma_data_direction but this
> > > driver uses dma_transfer_direction for everything. Converting to
> > > dma_data_direction would be desirable but there are a few shared
> > > structures that expect dma_transfer_direction so it is just simpler to
> > > change the parameter here. dma_transfer_direction and dma_data_direction
> > > are different sizes but this driver only uses the 1 and 2 values which
> > > mean the same thing so this change is safe.
> > >
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >
> > > v1 -> v2:
> > >
> > > * Fix escaped hash symbols for '#define' lines
> > >
> > >  drivers/spi/spi-ep93xx.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> > > index f1526757aaf6..189fc2225b69 100644
> > > --- a/drivers/spi/spi-ep93xx.c
> > > +++ b/drivers/spi/spi-ep93xx.c
> > > @@ -256,8 +256,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
> > >   * in case of failure.
> > >   */
> > >  static struct dma_async_tx_descriptor *
> > > -ep93xx_spi_dma_prepare(struct spi_master *master,
> > > -                      enum dma_transfer_direction dir)
> > > +ep93xx_spi_dma_prepare(struct spi_master *master, int dir)
> > >  {
> > >         struct ep93xx_spi *espi = spi_master_get_devdata(master);
> > >         struct spi_transfer *xfer = master->cur_msg->state;
> > > @@ -359,8 +358,7 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
> > >   * Function finishes with the DMA transfer. After this, the DMA buffer is
> > >   * unmapped.
> > >   */
> > > -static void ep93xx_spi_dma_finish(struct spi_master *master,
> > > -                                 enum dma_transfer_direction dir)
> > > +static void ep93xx_spi_dma_finish(struct spi_master *master, int dir)
> > >  {
> > >         struct ep93xx_spi *espi = spi_master_get_devdata(master);
> > >         struct dma_chan *chan;
> > > --
> > > 2.19.0
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

[PATCH v3] spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}

[Nathan Chancellor]

Clang warns when one enumerated type is implicitly converted to another.

drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:428:58: note: expanded from macro
'dma_map_sg'
#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
                               ~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
                dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
3 warnings generated.

dma_{,un}map_sg expect an enum of type dma_data_direction but this
driver uses dma_transfer_direction for everything. Convert the driver to
use dma_data_direction for these two functions.

There are two places that strictly require an enum of type
dma_transfer_direction: the direction member in struct dma_slave_config
and the direction parameter in dmaengine_prep_slave_sg. To avoid using
an explicit cast, add a simple function, ep93xx_dma_data_to_trans_dir,
to safely map between the two types because they are not 1 to 1 in
meaning.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Fix escaped hash symbols for '#define' lines (\# -> #)

v2 -> v3:

* Instead of changing the dir parameter in the prepare and finish
  functions, convert the driver to use dma_data_direction for everywhere
  that makes sense. Add a helper function to convert between the two
  types as there is still a couple of locations that need a transfer
  enum (and no such function exists). Alternatively, the function could
  just convert from transfer -> data and be used in dma_{,un}map_sg and
  the rest of the driver be left alone. Hopefully I understood what Mika
  wanted!

 drivers/spi/spi-ep93xx.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
index f1526757aaf6..667da7964d63 100644
--- a/drivers/spi/spi-ep93xx.c
+++ b/drivers/spi/spi-ep93xx.c
@@ -246,6 +246,19 @@ static int ep93xx_spi_read_write(struct spi_master *master)
 	return -EINPROGRESS;
 }
 
+static enum dma_transfer_direction
+ep93xx_dma_data_to_trans_dir(enum dma_data_direction dir)
+{
+	switch (dir) {
+	case DMA_TO_DEVICE:
+		return DMA_MEM_TO_DEV;
+	case DMA_FROM_DEVICE:
+		return DMA_DEV_TO_MEM;
+	default:
+		return DMA_TRANS_NONE;
+	}
+}
+
 /**
  * ep93xx_spi_dma_prepare() - prepares a DMA transfer
  * @master: SPI master
@@ -257,7 +270,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
  */
 static struct dma_async_tx_descriptor *
 ep93xx_spi_dma_prepare(struct spi_master *master,
-		       enum dma_transfer_direction dir)
+		       enum dma_data_direction dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct spi_transfer *xfer = master->cur_msg->state;
@@ -277,9 +290,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
 		buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
 
 	memset(&conf, 0, sizeof(conf));
-	conf.direction = dir;
+	conf.direction = ep93xx_dma_data_to_trans_dir(dir);
 
-	if (dir == DMA_DEV_TO_MEM) {
+	if (dir == DMA_FROM_DEVICE) {
 		chan = espi->dma_rx;
 		buf = xfer->rx_buf;
 		sgt = &espi->rx_sgt;
@@ -343,7 +356,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
 	if (!nents)
 		return ERR_PTR(-ENOMEM);
 
-	txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, dir, DMA_CTRL_ACK);
+	txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents,
+				      ep93xx_dma_data_to_trans_dir(dir),
+				      DMA_CTRL_ACK);
 	if (!txd) {
 		dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
 		return ERR_PTR(-ENOMEM);
@@ -360,13 +375,13 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
  * unmapped.
  */
 static void ep93xx_spi_dma_finish(struct spi_master *master,
-				  enum dma_transfer_direction dir)
+				  enum dma_data_direction dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct dma_chan *chan;
 	struct sg_table *sgt;
 
-	if (dir == DMA_DEV_TO_MEM) {
+	if (dir == DMA_FROM_DEVICE) {
 		chan = espi->dma_rx;
 		sgt = &espi->rx_sgt;
 	} else {
@@ -381,8 +396,8 @@ static void ep93xx_spi_dma_callback(void *callback_param)
 {
 	struct spi_master *master = callback_param;
 
-	ep93xx_spi_dma_finish(master, DMA_MEM_TO_DEV);
-	ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
+	ep93xx_spi_dma_finish(master, DMA_TO_DEVICE);
+	ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
 
 	spi_finalize_current_transfer(master);
 }
@@ -392,15 +407,15 @@ static int ep93xx_spi_dma_transfer(struct spi_master *master)
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct dma_async_tx_descriptor *rxd, *txd;
 
-	rxd = ep93xx_spi_dma_prepare(master, DMA_DEV_TO_MEM);
+	rxd = ep93xx_spi_dma_prepare(master, DMA_FROM_DEVICE);
 	if (IS_ERR(rxd)) {
 		dev_err(&master->dev, "DMA RX failed: %ld\n", PTR_ERR(rxd));
 		return PTR_ERR(rxd);
 	}
 
-	txd = ep93xx_spi_dma_prepare(master, DMA_MEM_TO_DEV);
+	txd = ep93xx_spi_dma_prepare(master, DMA_TO_DEVICE);
 	if (IS_ERR(txd)) {
-		ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
+		ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
 		dev_err(&master->dev, "DMA TX failed: %ld\n", PTR_ERR(txd));
 		return PTR_ERR(txd);
 	}
-- 
2.19.0

Re: [PATCH v3] spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}

[Nick Desaulniers]

Nathan,
Thanks for your patience and continued support working towards an
optimal solution to fix this warning. Some thoughts below.

On Fri, Oct 5, 2018 at 12:29 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> 'dma_map_sg'
> #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>                                ~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> 3 warnings generated.
>
> dma_{,un}map_sg expect an enum of type dma_data_direction but this
> driver uses dma_transfer_direction for everything. Convert the driver to
> use dma_data_direction for these two functions.
>
> There are two places that strictly require an enum of type
> dma_transfer_direction: the direction member in struct dma_slave_config
> and the direction parameter in dmaengine_prep_slave_sg. To avoid using
> an explicit cast, add a simple function, ep93xx_dma_data_to_trans_dir,
> to safely map between the two types because they are not 1 to 1 in
> meaning.
>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Fix escaped hash symbols for '#define' lines (\# -> #)
>
> v2 -> v3:
>
> * Instead of changing the dir parameter in the prepare and finish
>   functions, convert the driver to use dma_data_direction for everywhere
>   that makes sense. Add a helper function to convert between the two
>   types as there is still a couple of locations that need a transfer
>   enum (and no such function exists). Alternatively, the function could
>   just convert from transfer -> data and be used in dma_{,un}map_sg and
>   the rest of the driver be left alone. Hopefully I understood what Mika
>   wanted!
>
>  drivers/spi/spi-ep93xx.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> index f1526757aaf6..667da7964d63 100644
> --- a/drivers/spi/spi-ep93xx.c
> +++ b/drivers/spi/spi-ep93xx.c
> @@ -246,6 +246,19 @@ static int ep93xx_spi_read_write(struct spi_master *master)
>         return -EINPROGRESS;
>  }
>
> +static enum dma_transfer_direction
> +ep93xx_dma_data_to_trans_dir(enum dma_data_direction dir)
> +{
> +       switch (dir) {
> +       case DMA_TO_DEVICE:
> +               return DMA_MEM_TO_DEV;
> +       case DMA_FROM_DEVICE:
> +               return DMA_DEV_TO_MEM;
> +       default:
> +               return DMA_TRANS_NONE;
> +       }
> +}
> +
>  /**
>   * ep93xx_spi_dma_prepare() - prepares a DMA transfer
>   * @master: SPI master
> @@ -257,7 +270,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
>   */
>  static struct dma_async_tx_descriptor *
>  ep93xx_spi_dma_prepare(struct spi_master *master,
> -                      enum dma_transfer_direction dir)
> +                      enum dma_data_direction dir)
>  {
>         struct ep93xx_spi *espi = spi_master_get_devdata(master);
>         struct spi_transfer *xfer = master->cur_msg->state;
> @@ -277,9 +290,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
>                 buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
>
>         memset(&conf, 0, sizeof(conf));
> -       conf.direction = dir;
> +       conf.direction = ep93xx_dma_data_to_trans_dir(dir);
>
> -       if (dir == DMA_DEV_TO_MEM) {
> +       if (dir == DMA_FROM_DEVICE) {
>                 chan = espi->dma_rx;
>                 buf = xfer->rx_buf;
>                 sgt = &espi->rx_sgt;
> @@ -343,7 +356,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
>         if (!nents)
>                 return ERR_PTR(-ENOMEM);
>
> -       txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, dir, DMA_CTRL_ACK);
> +       txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents,
> +                                     ep93xx_dma_data_to_trans_dir(dir),

So we've already calculated this once already in this function, and
the value of `dir` hasn't changed since.  While the compiler might be
able to optimize out this common sub expression (CSE or GVN I think
are the compiler passes that could optimize this out), I think it
would be better to guarantee that this precalculated value is always
used by replacing the rematerialization of the value
(`ep93xx_dma_data_to_trans_dir(dir)`) with `conf.direction`.

> +                                     DMA_CTRL_ACK);
>         if (!txd) {
>                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 return ERR_PTR(-ENOMEM);
> @@ -360,13 +375,13 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
>   * unmapped.
>   */
>  static void ep93xx_spi_dma_finish(struct spi_master *master,
> -                                 enum dma_transfer_direction dir)
> +                                 enum dma_data_direction dir)
>  {
>         struct ep93xx_spi *espi = spi_master_get_devdata(master);
>         struct dma_chan *chan;
>         struct sg_table *sgt;
>
> -       if (dir == DMA_DEV_TO_MEM) {
> +       if (dir == DMA_FROM_DEVICE) {
>                 chan = espi->dma_rx;
>                 sgt = &espi->rx_sgt;
>         } else {
> @@ -381,8 +396,8 @@ static void ep93xx_spi_dma_callback(void *callback_param)
>  {
>         struct spi_master *master = callback_param;
>
> -       ep93xx_spi_dma_finish(master, DMA_MEM_TO_DEV);
> -       ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
> +       ep93xx_spi_dma_finish(master, DMA_TO_DEVICE);
> +       ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
>
>         spi_finalize_current_transfer(master);
>  }
> @@ -392,15 +407,15 @@ static int ep93xx_spi_dma_transfer(struct spi_master *master)
>         struct ep93xx_spi *espi = spi_master_get_devdata(master);
>         struct dma_async_tx_descriptor *rxd, *txd;
>
> -       rxd = ep93xx_spi_dma_prepare(master, DMA_DEV_TO_MEM);
> +       rxd = ep93xx_spi_dma_prepare(master, DMA_FROM_DEVICE);
>         if (IS_ERR(rxd)) {
>                 dev_err(&master->dev, "DMA RX failed: %ld\n", PTR_ERR(rxd));
>                 return PTR_ERR(rxd);
>         }
>
> -       txd = ep93xx_spi_dma_prepare(master, DMA_MEM_TO_DEV);
> +       txd = ep93xx_spi_dma_prepare(master, DMA_TO_DEVICE);
>         if (IS_ERR(txd)) {
> -               ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
> +               ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
>                 dev_err(&master->dev, "DMA TX failed: %ld\n", PTR_ERR(txd));
>                 return PTR_ERR(txd);
>         }
> --
> 2.19.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v3] spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}

[Nathan Chancellor]

On Fri, Oct 05, 2018 at 01:41:19PM -0700, Nick Desaulniers wrote:
> Nathan,
> Thanks for your patience and continued support working towards an
> optimal solution to fix this warning. Some thoughts below.
> 
> On Fri, Oct 5, 2018 at 12:29 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns when one enumerated type is implicitly converted to another.
> >
> > drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> > 'dma_map_sg'
> > #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
> >                                ~~~~~~~~~~~~~~~~          ^
> > drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > 'dma_unmap_sg'
> > #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> >                                  ~~~~~~~~~~~~~~~~~~          ^
> > drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > 'dma_unmap_sg'
> > #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> >                                  ~~~~~~~~~~~~~~~~~~          ^
> > 3 warnings generated.
> >
> > dma_{,un}map_sg expect an enum of type dma_data_direction but this
> > driver uses dma_transfer_direction for everything. Convert the driver to
> > use dma_data_direction for these two functions.
> >
> > There are two places that strictly require an enum of type
> > dma_transfer_direction: the direction member in struct dma_slave_config
> > and the direction parameter in dmaengine_prep_slave_sg. To avoid using
> > an explicit cast, add a simple function, ep93xx_dma_data_to_trans_dir,
> > to safely map between the two types because they are not 1 to 1 in
> > meaning.
> >
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >
> > v1 -> v2:
> >
> > * Fix escaped hash symbols for '#define' lines (\# -> #)
> >
> > v2 -> v3:
> >
> > * Instead of changing the dir parameter in the prepare and finish
> >   functions, convert the driver to use dma_data_direction for everywhere
> >   that makes sense. Add a helper function to convert between the two
> >   types as there is still a couple of locations that need a transfer
> >   enum (and no such function exists). Alternatively, the function could
> >   just convert from transfer -> data and be used in dma_{,un}map_sg and
> >   the rest of the driver be left alone. Hopefully I understood what Mika
> >   wanted!
> >
> >  drivers/spi/spi-ep93xx.c | 37 ++++++++++++++++++++++++++-----------
> >  1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> > index f1526757aaf6..667da7964d63 100644
> > --- a/drivers/spi/spi-ep93xx.c
> > +++ b/drivers/spi/spi-ep93xx.c
> > @@ -246,6 +246,19 @@ static int ep93xx_spi_read_write(struct spi_master *master)
> >         return -EINPROGRESS;
> >  }
> >
> > +static enum dma_transfer_direction
> > +ep93xx_dma_data_to_trans_dir(enum dma_data_direction dir)
> > +{
> > +       switch (dir) {
> > +       case DMA_TO_DEVICE:
> > +               return DMA_MEM_TO_DEV;
> > +       case DMA_FROM_DEVICE:
> > +               return DMA_DEV_TO_MEM;
> > +       default:
> > +               return DMA_TRANS_NONE;
> > +       }
> > +}
> > +
> >  /**
> >   * ep93xx_spi_dma_prepare() - prepares a DMA transfer
> >   * @master: SPI master
> > @@ -257,7 +270,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
> >   */
> >  static struct dma_async_tx_descriptor *
> >  ep93xx_spi_dma_prepare(struct spi_master *master,
> > -                      enum dma_transfer_direction dir)
> > +                      enum dma_data_direction dir)
> >  {
> >         struct ep93xx_spi *espi = spi_master_get_devdata(master);
> >         struct spi_transfer *xfer = master->cur_msg->state;
> > @@ -277,9 +290,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
> >                 buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
> >
> >         memset(&conf, 0, sizeof(conf));
> > -       conf.direction = dir;
> > +       conf.direction = ep93xx_dma_data_to_trans_dir(dir);
> >
> > -       if (dir == DMA_DEV_TO_MEM) {
> > +       if (dir == DMA_FROM_DEVICE) {
> >                 chan = espi->dma_rx;
> >                 buf = xfer->rx_buf;
> >                 sgt = &espi->rx_sgt;
> > @@ -343,7 +356,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
> >         if (!nents)
> >                 return ERR_PTR(-ENOMEM);
> >
> > -       txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, dir, DMA_CTRL_ACK);
> > +       txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents,
> > +                                     ep93xx_dma_data_to_trans_dir(dir),
> 
> So we've already calculated this once already in this function, and
> the value of `dir` hasn't changed since.  While the compiler might be
> able to optimize out this common sub expression (CSE or GVN I think
> are the compiler passes that could optimize this out), I think it
> would be better to guarantee that this precalculated value is always
> used by replacing the rematerialization of the value
> (`ep93xx_dma_data_to_trans_dir(dir)`) with `conf.direction`.
> 

Hi Nick,

That's a good point, I will change that for v4 (I'll wait for Mika and
Hartley to chime in before sending it out).

Thanks for the review,
Nathan

> > +                                     DMA_CTRL_ACK);
> >         if (!txd) {
> >                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 return ERR_PTR(-ENOMEM);
> > @@ -360,13 +375,13 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
> >   * unmapped.
> >   */
> >  static void ep93xx_spi_dma_finish(struct spi_master *master,
> > -                                 enum dma_transfer_direction dir)
> > +                                 enum dma_data_direction dir)
> >  {
> >         struct ep93xx_spi *espi = spi_master_get_devdata(master);
> >         struct dma_chan *chan;
> >         struct sg_table *sgt;
> >
> > -       if (dir == DMA_DEV_TO_MEM) {
> > +       if (dir == DMA_FROM_DEVICE) {
> >                 chan = espi->dma_rx;
> >                 sgt = &espi->rx_sgt;
> >         } else {
> > @@ -381,8 +396,8 @@ static void ep93xx_spi_dma_callback(void *callback_param)
> >  {
> >         struct spi_master *master = callback_param;
> >
> > -       ep93xx_spi_dma_finish(master, DMA_MEM_TO_DEV);
> > -       ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
> > +       ep93xx_spi_dma_finish(master, DMA_TO_DEVICE);
> > +       ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
> >
> >         spi_finalize_current_transfer(master);
> >  }
> > @@ -392,15 +407,15 @@ static int ep93xx_spi_dma_transfer(struct spi_master *master)
> >         struct ep93xx_spi *espi = spi_master_get_devdata(master);
> >         struct dma_async_tx_descriptor *rxd, *txd;
> >
> > -       rxd = ep93xx_spi_dma_prepare(master, DMA_DEV_TO_MEM);
> > +       rxd = ep93xx_spi_dma_prepare(master, DMA_FROM_DEVICE);
> >         if (IS_ERR(rxd)) {
> >                 dev_err(&master->dev, "DMA RX failed: %ld\n", PTR_ERR(rxd));
> >                 return PTR_ERR(rxd);
> >         }
> >
> > -       txd = ep93xx_spi_dma_prepare(master, DMA_MEM_TO_DEV);
> > +       txd = ep93xx_spi_dma_prepare(master, DMA_TO_DEVICE);
> >         if (IS_ERR(txd)) {
> > -               ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
> > +               ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
> >                 dev_err(&master->dev, "DMA TX failed: %ld\n", PTR_ERR(txd));
> >                 return PTR_ERR(txd);
> >         }
> > --
> > 2.19.0
> >
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers

Re: [PATCH v3] spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}

[Mika Westerberg]

On Fri, Oct 05, 2018 at 12:25:09PM -0700, Nathan Chancellor wrote:
> Clang warns when one enumerated type is implicitly converted to another.
> 
> drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> 'dma_map_sg'
> #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>                                ~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> 3 warnings generated.
> 
> dma_{,un}map_sg expect an enum of type dma_data_direction but this
> driver uses dma_transfer_direction for everything. Convert the driver to
> use dma_data_direction for these two functions.
> 
> There are two places that strictly require an enum of type
> dma_transfer_direction: the direction member in struct dma_slave_config
> and the direction parameter in dmaengine_prep_slave_sg. To avoid using
> an explicit cast, add a simple function, ep93xx_dma_data_to_trans_dir,
> to safely map between the two types because they are not 1 to 1 in
> meaning.
> 
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

Looks good to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v4] spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}

[Nathan Chancellor]

Clang warns when one enumerated type is implicitly converted to another.

drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:428:58: note: expanded from macro
'dma_map_sg'
#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
                               ~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
                dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
3 warnings generated.

dma_{,un}map_sg expect an enum of type dma_data_direction but this
driver uses dma_transfer_direction for everything. Convert the driver to
use dma_data_direction for these two functions.

There are two places that strictly require an enum of type
dma_transfer_direction: the direction member in struct dma_slave_config
and the direction parameter in dmaengine_prep_slave_sg. To avoid using
an explicit cast, add a simple function, ep93xx_dma_data_to_trans_dir,
to safely map between the two types because they are not 1 to 1 in
meaning.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Fix escaped hash symbols for '#define' lines (\# -> #).

v2 -> v3:

* Instead of changing the dir parameter in the prepare and finish
  functions, convert the driver to use dma_data_direction for everywhere
  that makes sense, thanks to review from Mika. Add a helper function to
  convert between the two types as there is still a couple of locations
  that need a transfer enum (and no such function exists).

v3 -> v4:

* Use 'conf.direction' in dmaengine_prep_slave_sg instead of the new
  function since the value was already set earlier in the function and
  never changed, thanks to review from Nick.

 drivers/spi/spi-ep93xx.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
index f1526757aaf6..79fc3940245a 100644
--- a/drivers/spi/spi-ep93xx.c
+++ b/drivers/spi/spi-ep93xx.c
@@ -246,6 +246,19 @@ static int ep93xx_spi_read_write(struct spi_master *master)
 	return -EINPROGRESS;
 }
 
+static enum dma_transfer_direction
+ep93xx_dma_data_to_trans_dir(enum dma_data_direction dir)
+{
+	switch (dir) {
+	case DMA_TO_DEVICE:
+		return DMA_MEM_TO_DEV;
+	case DMA_FROM_DEVICE:
+		return DMA_DEV_TO_MEM;
+	default:
+		return DMA_TRANS_NONE;
+	}
+}
+
 /**
  * ep93xx_spi_dma_prepare() - prepares a DMA transfer
  * @master: SPI master
@@ -257,7 +270,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
  */
 static struct dma_async_tx_descriptor *
 ep93xx_spi_dma_prepare(struct spi_master *master,
-		       enum dma_transfer_direction dir)
+		       enum dma_data_direction dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct spi_transfer *xfer = master->cur_msg->state;
@@ -277,9 +290,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
 		buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
 
 	memset(&conf, 0, sizeof(conf));
-	conf.direction = dir;
+	conf.direction = ep93xx_dma_data_to_trans_dir(dir);
 
-	if (dir == DMA_DEV_TO_MEM) {
+	if (dir == DMA_FROM_DEVICE) {
 		chan = espi->dma_rx;
 		buf = xfer->rx_buf;
 		sgt = &espi->rx_sgt;
@@ -343,7 +356,8 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
 	if (!nents)
 		return ERR_PTR(-ENOMEM);
 
-	txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, dir, DMA_CTRL_ACK);
+	txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, conf.direction,
+				      DMA_CTRL_ACK);
 	if (!txd) {
 		dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
 		return ERR_PTR(-ENOMEM);
@@ -360,13 +374,13 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
  * unmapped.
  */
 static void ep93xx_spi_dma_finish(struct spi_master *master,
-				  enum dma_transfer_direction dir)
+				  enum dma_data_direction dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct dma_chan *chan;
 	struct sg_table *sgt;
 
-	if (dir == DMA_DEV_TO_MEM) {
+	if (dir == DMA_FROM_DEVICE) {
 		chan = espi->dma_rx;
 		sgt = &espi->rx_sgt;
 	} else {
@@ -381,8 +395,8 @@ static void ep93xx_spi_dma_callback(void *callback_param)
 {
 	struct spi_master *master = callback_param;
 
-	ep93xx_spi_dma_finish(master, DMA_MEM_TO_DEV);
-	ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
+	ep93xx_spi_dma_finish(master, DMA_TO_DEVICE);
+	ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
 
 	spi_finalize_current_transfer(master);
 }
@@ -392,15 +406,15 @@ static int ep93xx_spi_dma_transfer(struct spi_master *master)
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct dma_async_tx_descriptor *rxd, *txd;
 
-	rxd = ep93xx_spi_dma_prepare(master, DMA_DEV_TO_MEM);
+	rxd = ep93xx_spi_dma_prepare(master, DMA_FROM_DEVICE);
 	if (IS_ERR(rxd)) {
 		dev_err(&master->dev, "DMA RX failed: %ld\n", PTR_ERR(rxd));
 		return PTR_ERR(rxd);
 	}
 
-	txd = ep93xx_spi_dma_prepare(master, DMA_MEM_TO_DEV);
+	txd = ep93xx_spi_dma_prepare(master, DMA_TO_DEVICE);
 	if (IS_ERR(txd)) {
-		ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
+		ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
 		dev_err(&master->dev, "DMA TX failed: %ld\n", PTR_ERR(txd));
 		return PTR_ERR(txd);
 	}
-- 
2.19.0

Re: [PATCH v4] spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}

[Nick Desaulniers]

On Mon, Oct 8, 2018 at 11:09 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> 'dma_map_sg'
> #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>                                ~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> 3 warnings generated.
>
> dma_{,un}map_sg expect an enum of type dma_data_direction but this
> driver uses dma_transfer_direction for everything. Convert the driver to
> use dma_data_direction for these two functions.
>
> There are two places that strictly require an enum of type
> dma_transfer_direction: the direction member in struct dma_slave_config
> and the direction parameter in dmaengine_prep_slave_sg. To avoid using
> an explicit cast, add a simple function, ep93xx_dma_data_to_trans_dir,
> to safely map between the two types because they are not 1 to 1 in
> meaning.
>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Fix escaped hash symbols for '#define' lines (\# -> #).
>
> v2 -> v3:
>
> * Instead of changing the dir parameter in the prepare and finish
>   functions, convert the driver to use dma_data_direction for everywhere
>   that makes sense, thanks to review from Mika. Add a helper function to
>   convert between the two types as there is still a couple of locations
>   that need a transfer enum (and no such function exists).
>
> v3 -> v4:
>
> * Use 'conf.direction' in dmaengine_prep_slave_sg instead of the new
>   function since the value was already set earlier in the function and
>   never changed, thanks to review from Nick.

Thanks Nathan!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
>  drivers/spi/spi-ep93xx.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> index f1526757aaf6..79fc3940245a 100644
> --- a/drivers/spi/spi-ep93xx.c
> +++ b/drivers/spi/spi-ep93xx.c
> @@ -246,6 +246,19 @@ static int ep93xx_spi_read_write(struct spi_master *master)
>         return -EINPROGRESS;
>  }
>
> +static enum dma_transfer_direction
> +ep93xx_dma_data_to_trans_dir(enum dma_data_direction dir)
> +{
> +       switch (dir) {
> +       case DMA_TO_DEVICE:
> +               return DMA_MEM_TO_DEV;
> +       case DMA_FROM_DEVICE:
> +               return DMA_DEV_TO_MEM;
> +       default:
> +               return DMA_TRANS_NONE;
> +       }
> +}
> +
>  /**
>   * ep93xx_spi_dma_prepare() - prepares a DMA transfer
>   * @master: SPI master
> @@ -257,7 +270,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
>   */
>  static struct dma_async_tx_descriptor *
>  ep93xx_spi_dma_prepare(struct spi_master *master,
> -                      enum dma_transfer_direction dir)
> +                      enum dma_data_direction dir)
>  {
>         struct ep93xx_spi *espi = spi_master_get_devdata(master);
>         struct spi_transfer *xfer = master->cur_msg->state;
> @@ -277,9 +290,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
>                 buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
>
>         memset(&conf, 0, sizeof(conf));
> -       conf.direction = dir;
> +       conf.direction = ep93xx_dma_data_to_trans_dir(dir);
>
> -       if (dir == DMA_DEV_TO_MEM) {
> +       if (dir == DMA_FROM_DEVICE) {
>                 chan = espi->dma_rx;
>                 buf = xfer->rx_buf;
>                 sgt = &espi->rx_sgt;
> @@ -343,7 +356,8 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
>         if (!nents)
>                 return ERR_PTR(-ENOMEM);
>
> -       txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, dir, DMA_CTRL_ACK);
> +       txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, conf.direction,
> +                                     DMA_CTRL_ACK);
>         if (!txd) {
>                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 return ERR_PTR(-ENOMEM);
> @@ -360,13 +374,13 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
>   * unmapped.
>   */
>  static void ep93xx_spi_dma_finish(struct spi_master *master,
> -                                 enum dma_transfer_direction dir)
> +                                 enum dma_data_direction dir)
>  {
>         struct ep93xx_spi *espi = spi_master_get_devdata(master);
>         struct dma_chan *chan;
>         struct sg_table *sgt;
>
> -       if (dir == DMA_DEV_TO_MEM) {
> +       if (dir == DMA_FROM_DEVICE) {
>                 chan = espi->dma_rx;
>                 sgt = &espi->rx_sgt;
>         } else {
> @@ -381,8 +395,8 @@ static void ep93xx_spi_dma_callback(void *callback_param)
>  {
>         struct spi_master *master = callback_param;
>
> -       ep93xx_spi_dma_finish(master, DMA_MEM_TO_DEV);
> -       ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
> +       ep93xx_spi_dma_finish(master, DMA_TO_DEVICE);
> +       ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
>
>         spi_finalize_current_transfer(master);
>  }
> @@ -392,15 +406,15 @@ static int ep93xx_spi_dma_transfer(struct spi_master *master)
>         struct ep93xx_spi *espi = spi_master_get_devdata(master);
>         struct dma_async_tx_descriptor *rxd, *txd;
>
> -       rxd = ep93xx_spi_dma_prepare(master, DMA_DEV_TO_MEM);
> +       rxd = ep93xx_spi_dma_prepare(master, DMA_FROM_DEVICE);
>         if (IS_ERR(rxd)) {
>                 dev_err(&master->dev, "DMA RX failed: %ld\n", PTR_ERR(rxd));
>                 return PTR_ERR(rxd);
>         }
>
> -       txd = ep93xx_spi_dma_prepare(master, DMA_MEM_TO_DEV);
> +       txd = ep93xx_spi_dma_prepare(master, DMA_TO_DEVICE);
>         if (IS_ERR(txd)) {
> -               ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
> +               ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
>                 dev_err(&master->dev, "DMA TX failed: %ld\n", PTR_ERR(txd));
>                 return PTR_ERR(txd);
>         }
> --
> 2.19.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4] spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}

[Mika Westerberg]

On Mon, Oct 08, 2018 at 11:08:47AM -0700, Nathan Chancellor wrote:
> Clang warns when one enumerated type is implicitly converted to another.
> 
> drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> 'dma_map_sg'
> #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>                                ~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> 3 warnings generated.
> 
> dma_{,un}map_sg expect an enum of type dma_data_direction but this
> driver uses dma_transfer_direction for everything. Convert the driver to
> use dma_data_direction for these two functions.
> 
> There are two places that strictly require an enum of type
> dma_transfer_direction: the direction member in struct dma_slave_config
> and the direction parameter in dmaengine_prep_slave_sg. To avoid using
> an explicit cast, add a simple function, ep93xx_dma_data_to_trans_dir,
> to safely map between the two types because they are not 1 to 1 in
> meaning.
> 
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied "spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}" to the spi tree

[Mark Brown]

The patch

   spi: spi-ep93xx: Use dma_data_direction for ep93xx_spi_dma_{finish,prepare}

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From a1108c7b2efb892350ba6a0e932dfd45622f4e2b Mon Sep 17 00:00:00 2001
From: Nathan Chancellor <natechancellor@gmail.com>
Date: Mon, 8 Oct 2018 11:08:47 -0700
Subject: [PATCH] spi: spi-ep93xx: Use dma_data_direction for
 ep93xx_spi_dma_{finish,prepare}

Clang warns when one enumerated type is implicitly converted to another.

drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:428:58: note: expanded from macro
'dma_map_sg'
#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
                               ~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
                dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
enumeration type 'enum dma_transfer_direction' to different enumeration
type 'enum dma_data_direction' [-Wenum-conversion]
        dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
./include/linux/dma-mapping.h:429:62: note: expanded from macro
'dma_unmap_sg'
#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
                                 ~~~~~~~~~~~~~~~~~~          ^
3 warnings generated.

dma_{,un}map_sg expect an enum of type dma_data_direction but this
driver uses dma_transfer_direction for everything. Convert the driver to
use dma_data_direction for these two functions.

There are two places that strictly require an enum of type
dma_transfer_direction: the direction member in struct dma_slave_config
and the direction parameter in dmaengine_prep_slave_sg. To avoid using
an explicit cast, add a simple function, ep93xx_dma_data_to_trans_dir,
to safely map between the two types because they are not 1 to 1 in
meaning.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-ep93xx.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
index f1526757aaf6..79fc3940245a 100644
--- a/drivers/spi/spi-ep93xx.c
+++ b/drivers/spi/spi-ep93xx.c
@@ -246,6 +246,19 @@ static int ep93xx_spi_read_write(struct spi_master *master)
 	return -EINPROGRESS;
 }
 
+static enum dma_transfer_direction
+ep93xx_dma_data_to_trans_dir(enum dma_data_direction dir)
+{
+	switch (dir) {
+	case DMA_TO_DEVICE:
+		return DMA_MEM_TO_DEV;
+	case DMA_FROM_DEVICE:
+		return DMA_DEV_TO_MEM;
+	default:
+		return DMA_TRANS_NONE;
+	}
+}
+
 /**
  * ep93xx_spi_dma_prepare() - prepares a DMA transfer
  * @master: SPI master
@@ -257,7 +270,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
  */
 static struct dma_async_tx_descriptor *
 ep93xx_spi_dma_prepare(struct spi_master *master,
-		       enum dma_transfer_direction dir)
+		       enum dma_data_direction dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct spi_transfer *xfer = master->cur_msg->state;
@@ -277,9 +290,9 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
 		buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
 
 	memset(&conf, 0, sizeof(conf));
-	conf.direction = dir;
+	conf.direction = ep93xx_dma_data_to_trans_dir(dir);
 
-	if (dir == DMA_DEV_TO_MEM) {
+	if (dir == DMA_FROM_DEVICE) {
 		chan = espi->dma_rx;
 		buf = xfer->rx_buf;
 		sgt = &espi->rx_sgt;
@@ -343,7 +356,8 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
 	if (!nents)
 		return ERR_PTR(-ENOMEM);
 
-	txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, dir, DMA_CTRL_ACK);
+	txd = dmaengine_prep_slave_sg(chan, sgt->sgl, nents, conf.direction,
+				      DMA_CTRL_ACK);
 	if (!txd) {
 		dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
 		return ERR_PTR(-ENOMEM);
@@ -360,13 +374,13 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
  * unmapped.
  */
 static void ep93xx_spi_dma_finish(struct spi_master *master,
-				  enum dma_transfer_direction dir)
+				  enum dma_data_direction dir)
 {
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct dma_chan *chan;
 	struct sg_table *sgt;
 
-	if (dir == DMA_DEV_TO_MEM) {
+	if (dir == DMA_FROM_DEVICE) {
 		chan = espi->dma_rx;
 		sgt = &espi->rx_sgt;
 	} else {
@@ -381,8 +395,8 @@ static void ep93xx_spi_dma_callback(void *callback_param)
 {
 	struct spi_master *master = callback_param;
 
-	ep93xx_spi_dma_finish(master, DMA_MEM_TO_DEV);
-	ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
+	ep93xx_spi_dma_finish(master, DMA_TO_DEVICE);
+	ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
 
 	spi_finalize_current_transfer(master);
 }
@@ -392,15 +406,15 @@ static int ep93xx_spi_dma_transfer(struct spi_master *master)
 	struct ep93xx_spi *espi = spi_master_get_devdata(master);
 	struct dma_async_tx_descriptor *rxd, *txd;
 
-	rxd = ep93xx_spi_dma_prepare(master, DMA_DEV_TO_MEM);
+	rxd = ep93xx_spi_dma_prepare(master, DMA_FROM_DEVICE);
 	if (IS_ERR(rxd)) {
 		dev_err(&master->dev, "DMA RX failed: %ld\n", PTR_ERR(rxd));
 		return PTR_ERR(rxd);
 	}
 
-	txd = ep93xx_spi_dma_prepare(master, DMA_MEM_TO_DEV);
+	txd = ep93xx_spi_dma_prepare(master, DMA_TO_DEVICE);
 	if (IS_ERR(txd)) {
-		ep93xx_spi_dma_finish(master, DMA_DEV_TO_MEM);
+		ep93xx_spi_dma_finish(master, DMA_FROM_DEVICE);
 		dev_err(&master->dev, "DMA TX failed: %ld\n", PTR_ERR(txd));
 		return PTR_ERR(txd);
 	}
-- 
2.19.0

Re: [PATCH] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3771 bytes --]

On Wed, Oct 03, 2018 at 07:39:26PM -0700, Nathan Chancellor wrote:
> Clang warns when one enumerated type is implicitly converted to another.
> 
> drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration

Please remember to CC driver maintainers and authors on patch
submissions so they can review things, copying in Hartley and Mika.

> type 'enum dma_data_direction' [-Wenum-conversion]
>         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> 'dma_map_sg'
> \#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>                                ~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> enumeration type 'enum dma_transfer_direction' to different enumeration
> type 'enum dma_data_direction' [-Wenum-conversion]
>         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> 'dma_unmap_sg'
> \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>                                  ~~~~~~~~~~~~~~~~~~          ^
> 3 warnings generated.
> 
> dma_{,un}map_sg expects an enum of type dma_data_direction but this
> driver uses dma_transfer_direction for everything. Converting to
> dma_data_direction would be desirable but there are a few shared
> structures that expect dma_transfer_direction so it is just simpler to
> change the parameter here. dma_transfer_direction and dma_data_direction
> are different sizes but this driver only uses the 1 and 2 values which
> mean the same thing so this change is safe.
> 
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/spi/spi-ep93xx.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> index f1526757aaf6..189fc2225b69 100644
> --- a/drivers/spi/spi-ep93xx.c
> +++ b/drivers/spi/spi-ep93xx.c
> @@ -256,8 +256,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
>   * in case of failure.
>   */
>  static struct dma_async_tx_descriptor *
> -ep93xx_spi_dma_prepare(struct spi_master *master,
> -		       enum dma_transfer_direction dir)
> +ep93xx_spi_dma_prepare(struct spi_master *master, int dir)
>  {
>  	struct ep93xx_spi *espi = spi_master_get_devdata(master);
>  	struct spi_transfer *xfer = master->cur_msg->state;
> @@ -359,8 +358,7 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
>   * Function finishes with the DMA transfer. After this, the DMA buffer is
>   * unmapped.
>   */
> -static void ep93xx_spi_dma_finish(struct spi_master *master,
> -				  enum dma_transfer_direction dir)
> +static void ep93xx_spi_dma_finish(struct spi_master *master, int dir)
>  {
>  	struct ep93xx_spi *espi = spi_master_get_devdata(master);
>  	struct dma_chan *chan;
> -- 
> 2.19.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Nathan Chancellor]

On Thu, Oct 04, 2018 at 11:32:47AM +0100, Mark Brown wrote:
> On Wed, Oct 03, 2018 at 07:39:26PM -0700, Nathan Chancellor wrote:
> > Clang warns when one enumerated type is implicitly converted to another.
> > 
> > drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> 
> Please remember to CC driver maintainers and authors on patch
> submissions so they can review things, copying in Hartley and Mika.
> 

Hi Mark,

Thank you for that, I usually just rely on get_maintainer.pl but I'll be
better about looking at git history for major patch authors as well.

$ ./scripts/get_maintainer.pl drivers/spi/spi-ep93xx.c
Mark Brown <broonie@kernel.org> (maintainer:SPI SUBSYSTEM)
linux-spi@vger.kernel.org (open list:SPI SUBSYSTEM)
linux-kernel@vger.kernel.org (open list)

Nathan

> > type 'enum dma_data_direction' [-Wenum-conversion]
> >         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> > 'dma_map_sg'
> > \#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
> >                                ~~~~~~~~~~~~~~~~          ^
> > drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > 'dma_unmap_sg'
> > \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> >                                  ~~~~~~~~~~~~~~~~~~          ^
> > drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > 'dma_unmap_sg'
> > \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> >                                  ~~~~~~~~~~~~~~~~~~          ^
> > 3 warnings generated.
> > 
> > dma_{,un}map_sg expects an enum of type dma_data_direction but this
> > driver uses dma_transfer_direction for everything. Converting to
> > dma_data_direction would be desirable but there are a few shared
> > structures that expect dma_transfer_direction so it is just simpler to
> > change the parameter here. dma_transfer_direction and dma_data_direction
> > are different sizes but this driver only uses the 1 and 2 values which
> > mean the same thing so this change is safe.
> > 
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/spi/spi-ep93xx.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> > index f1526757aaf6..189fc2225b69 100644
> > --- a/drivers/spi/spi-ep93xx.c
> > +++ b/drivers/spi/spi-ep93xx.c
> > @@ -256,8 +256,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
> >   * in case of failure.
> >   */
> >  static struct dma_async_tx_descriptor *
> > -ep93xx_spi_dma_prepare(struct spi_master *master,
> > -		       enum dma_transfer_direction dir)
> > +ep93xx_spi_dma_prepare(struct spi_master *master, int dir)
> >  {
> >  	struct ep93xx_spi *espi = spi_master_get_devdata(master);
> >  	struct spi_transfer *xfer = master->cur_msg->state;
> > @@ -359,8 +358,7 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
> >   * Function finishes with the DMA transfer. After this, the DMA buffer is
> >   * unmapped.
> >   */
> > -static void ep93xx_spi_dma_finish(struct spi_master *master,
> > -				  enum dma_transfer_direction dir)
> > +static void ep93xx_spi_dma_finish(struct spi_master *master, int dir)
> >  {
> >  	struct ep93xx_spi *espi = spi_master_get_devdata(master);
> >  	struct dma_chan *chan;
> > -- 
> > 2.19.0
> > 

Re: [PATCH] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Nick Desaulniers]

On Thu, Oct 4, 2018 at 9:04 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Oct 04, 2018 at 11:32:47AM +0100, Mark Brown wrote:
> > On Wed, Oct 03, 2018 at 07:39:26PM -0700, Nathan Chancellor wrote:
> > > Clang warns when one enumerated type is implicitly converted to another.
> > >
> > > drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> >
> > Please remember to CC driver maintainers and authors on patch
> > submissions so they can review things, copying in Hartley and Mika.
> >
>
> Hi Mark,
>
> Thank you for that, I usually just rely on get_maintainer.pl but I'll be

If get_maintainer.pl doesn't cc the right person, does that signal
that the MAINTAINERS file has some omissions?

> better about looking at git history for major patch authors as well.
>
> $ ./scripts/get_maintainer.pl drivers/spi/spi-ep93xx.c
> Mark Brown <broonie@kernel.org> (maintainer:SPI SUBSYSTEM)
> linux-spi@vger.kernel.org (open list:SPI SUBSYSTEM)
> linux-kernel@vger.kernel.org (open list)
>
> Nathan
>
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> > >         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> > > 'dma_map_sg'
> > > \#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
> > >                                ~~~~~~~~~~~~~~~~          ^
> > > drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> > >                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > > 'dma_unmap_sg'
> > > \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> > >                                  ~~~~~~~~~~~~~~~~~~          ^
> > > drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> > >         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > > 'dma_unmap_sg'
> > > \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> > >                                  ~~~~~~~~~~~~~~~~~~          ^
> > > 3 warnings generated.
> > >
> > > dma_{,un}map_sg expects an enum of type dma_data_direction but this
> > > driver uses dma_transfer_direction for everything. Converting to
> > > dma_data_direction would be desirable but there are a few shared
> > > structures that expect dma_transfer_direction so it is just simpler to
> > > change the parameter here. dma_transfer_direction and dma_data_direction
> > > are different sizes but this driver only uses the 1 and 2 values which
> > > mean the same thing so this change is safe.
> > >
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  drivers/spi/spi-ep93xx.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
> > > index f1526757aaf6..189fc2225b69 100644
> > > --- a/drivers/spi/spi-ep93xx.c
> > > +++ b/drivers/spi/spi-ep93xx.c
> > > @@ -256,8 +256,7 @@ static int ep93xx_spi_read_write(struct spi_master *master)
> > >   * in case of failure.
> > >   */
> > >  static struct dma_async_tx_descriptor *
> > > -ep93xx_spi_dma_prepare(struct spi_master *master,
> > > -                  enum dma_transfer_direction dir)
> > > +ep93xx_spi_dma_prepare(struct spi_master *master, int dir)
> > >  {
> > >     struct ep93xx_spi *espi = spi_master_get_devdata(master);
> > >     struct spi_transfer *xfer = master->cur_msg->state;
> > > @@ -359,8 +358,7 @@ ep93xx_spi_dma_prepare(struct spi_master *master,
> > >   * Function finishes with the DMA transfer. After this, the DMA buffer is
> > >   * unmapped.
> > >   */
> > > -static void ep93xx_spi_dma_finish(struct spi_master *master,
> > > -                             enum dma_transfer_direction dir)
> > > +static void ep93xx_spi_dma_finish(struct spi_master *master, int dir)
> > >  {
> > >     struct ep93xx_spi *espi = spi_master_get_devdata(master);
> > >     struct dma_chan *chan;
> > > --
> > > 2.19.0
> > >
>
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

On Thu, Oct 04, 2018 at 09:41:20AM -0700, Nick Desaulniers wrote:
> On Thu, Oct 4, 2018 at 9:04 AM Nathan Chancellor
> > On Thu, Oct 04, 2018 at 11:32:47AM +0100, Mark Brown wrote:

> > > Please remember to CC driver maintainers and authors on patch
> > > submissions so they can review things, copying in Hartley and Mika.

> > Thank you for that, I usually just rely on get_maintainer.pl but I'll be

> If get_maintainer.pl doesn't cc the right person, does that signal
> that the MAINTAINERS file has some omissions?

It can do but there's always cases where it misses people - someone
working very actively on a bit of code for the short term for example.
Especially with drivers if you're only seeing subsystem maintainers then
it's worth checking.  The --git option helps a lot here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Mika Westerberg]

Hi,

On Thu, Oct 04, 2018 at 11:32:47AM +0100, Mark Brown wrote:
> On Wed, Oct 03, 2018 at 07:39:26PM -0700, Nathan Chancellor wrote:
> > Clang warns when one enumerated type is implicitly converted to another.
> > 
> > drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> 
> Please remember to CC driver maintainers and authors on patch
> submissions so they can review things, copying in Hartley and Mika.

Thanks for copying me.

> > type 'enum dma_data_direction' [-Wenum-conversion]
> >         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> > 'dma_map_sg'
> > \#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
> >                                ~~~~~~~~~~~~~~~~          ^
> > drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > 'dma_unmap_sg'
> > \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> >                                  ~~~~~~~~~~~~~~~~~~          ^
> > drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> > enumeration type 'enum dma_transfer_direction' to different enumeration
> > type 'enum dma_data_direction' [-Wenum-conversion]
> >         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > 'dma_unmap_sg'
> > \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> >                                  ~~~~~~~~~~~~~~~~~~          ^
> > 3 warnings generated.
> > 
> > dma_{,un}map_sg expects an enum of type dma_data_direction but this
> > driver uses dma_transfer_direction for everything. Converting to
> > dma_data_direction would be desirable but there are a few shared
> > structures that expect dma_transfer_direction so it is just simpler to
> > change the parameter here. dma_transfer_direction and dma_data_direction
> > are different sizes but this driver only uses the 1 and 2 values which
> > mean the same thing so this change is safe.

I would rather do the conversion than passing "int" to the function even
if both enums happen to have same value now. I would be surprised if
there is no helper function already for the conversion :)

Re: [PATCH] spi: spi-ep93xx: Change dir type in ep93xx_spi_dma_{finish,prepare}

[Nathan Chancellor]

On Fri, Oct 05, 2018 at 11:28:44AM +0300, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Oct 04, 2018 at 11:32:47AM +0100, Mark Brown wrote:
> > On Wed, Oct 03, 2018 at 07:39:26PM -0700, Nathan Chancellor wrote:
> > > Clang warns when one enumerated type is implicitly converted to another.
> > > 
> > > drivers/spi/spi-ep93xx.c:342:62: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> > 
> > Please remember to CC driver maintainers and authors on patch
> > submissions so they can review things, copying in Hartley and Mika.
> 
> Thanks for copying me.
> 
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> > >         nents = dma_map_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:428:58: note: expanded from macro
> > > 'dma_map_sg'
> > > \#define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
> > >                                ~~~~~~~~~~~~~~~~          ^
> > > drivers/spi/spi-ep93xx.c:348:57: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> > >                 dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > > 'dma_unmap_sg'
> > > \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> > >                                  ~~~~~~~~~~~~~~~~~~          ^
> > > drivers/spi/spi-ep93xx.c:377:56: warning: implicit conversion from
> > > enumeration type 'enum dma_transfer_direction' to different enumeration
> > > type 'enum dma_data_direction' [-Wenum-conversion]
> > >         dma_unmap_sg(chan->device->dev, sgt->sgl, sgt->nents, dir);
> > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> > > ./include/linux/dma-mapping.h:429:62: note: expanded from macro
> > > 'dma_unmap_sg'
> > > \#define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
> > >                                  ~~~~~~~~~~~~~~~~~~          ^
> > > 3 warnings generated.
> > > 
> > > dma_{,un}map_sg expects an enum of type dma_data_direction but this
> > > driver uses dma_transfer_direction for everything. Converting to
> > > dma_data_direction would be desirable but there are a few shared
> > > structures that expect dma_transfer_direction so it is just simpler to
> > > change the parameter here. dma_transfer_direction and dma_data_direction
> > > are different sizes but this driver only uses the 1 and 2 values which
> > > mean the same thing so this change is safe.
> 
> I would rather do the conversion than passing "int" to the function even
> if both enums happen to have same value now. I would be surprised if
> there is no helper function already for the conversion :)

Hi Mika,

I will go ahead and spin up a v3 in the morning for review and copy you
and Hartley, thanks for the comments!

Nathan