[0/9] PL08x further cleanups

Commit Message

Vinod Koul July 25, 2011, 1:38 p.m. UTC

On Thu, 2011-07-21 at 17:08 +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 14, 2011 at 04:35:28AM +0530, Koul, Vinod wrote:
> > On Tue, 2011-07-05 at 14:10 +0100, Russell King - ARM Linux wrote:
> > > This patch series is the remainder of the PL08x work I did earlier
> > > this year while trying to get the ARM platforms working with DMA.
> > > 
> > > Unfortunately, due to the state of the hardware, I was not able to
> > > properly test these changes.  However, they do compile, and I think
> > > if others are going to be looking at the PL08x DMA engine driver,
> > > they're worth having.
> > > 
> > > Linus, can you check these out on your hardware please?
> > 
> > Russell,
> > 
> > The fail to build for me, on line 550 it refers to bd.llil_bus whereas
> > the bd is a pointer. Further the WIDTH macros seem to be missing.
> 
> Grr, I've updated the patches and will post the two updated ones.  I
> have them in my git tree - would you like them via git instead, or
> would you prefer to provide acks and let me push them upstream?
> 
Russell,
Thanks I have applied this

Although gcc didnt like not handling other enums so warned:

drivers/dma/amba-pl08x.c: In function 'pl08x_width':
drivers/dma/amba-pl08x.c:1119: warning: enumeration value
'DMA_SLAVE_BUSWIDTH_UNDEFINED' not handled in switch
drivers/dma/amba-pl08x.c:1119: warning: enumeration value
'DMA_SLAVE_BUSWIDTH_8_BYTES' not handled in switch

which can be fixed as:

 }

If you are okay, pls ack it

Comments

Russell King - ARM Linux July 25, 2011, 1:43 p.m. UTC | #1

On Mon, Jul 25, 2011 at 07:08:31PM +0530, Vinod Koul wrote:
> Although gcc didnt like not handling other enums so warned:
> 
> drivers/dma/amba-pl08x.c: In function 'pl08x_width':
> drivers/dma/amba-pl08x.c:1119: warning: enumeration value
> 'DMA_SLAVE_BUSWIDTH_UNDEFINED' not handled in switch
> drivers/dma/amba-pl08x.c:1119: warning: enumeration value
> 'DMA_SLAVE_BUSWIDTH_8_BYTES' not handled in switch

Those must be new since I wrote the patch.

> which can be fixed as:
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 9aa2bd4..4925e0d 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -1123,6 +1123,8 @@ static u32 pl08x_width(enum dma_slave_buswidth
> width)
>  		return PL080_WIDTH_16BIT;
>  	case DMA_SLAVE_BUSWIDTH_4_BYTES:
>  		return PL080_WIDTH_32BIT;
> +	default:
> +		return -EINVAL;
>  	}
>  	return ~0;
>  }
> 
> If you are okay, pls ack it

No.  This function is used as:

+       width = pl08x_width(addr_width);
+       if (width == ~0) {
                dev_err(&pl08x->adev->dev,
                        "bad runtime_config: alien address width\n");
                return -EINVAL;
        }

Notice that it returns a u32 so negative errnos don't make sense.  It
returns ~0 to indicate error.

The code is actually correct as it stands, it's just gcc deciding to
emit a warning for an unhandled enum value which isn't really unhandled.
Just move the 'return ~0;' at the end of the function inside the switch
as a default case to shut it up.

Vinod Koul July 25, 2011, 1:51 p.m. UTC | #2

On Mon, 2011-07-25 at 14:43 +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 25, 2011 at 07:08:31PM +0530, Vinod Koul wrote:
> > Although gcc didnt like not handling other enums so warned:
> > 
> > drivers/dma/amba-pl08x.c: In function 'pl08x_width':
> > drivers/dma/amba-pl08x.c:1119: warning: enumeration value
> > 'DMA_SLAVE_BUSWIDTH_UNDEFINED' not handled in switch
> > drivers/dma/amba-pl08x.c:1119: warning: enumeration value
> > 'DMA_SLAVE_BUSWIDTH_8_BYTES' not handled in switch
> 
> Those must be new since I wrote the patch.
> 
> > which can be fixed as:
> > 
> > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> > index 9aa2bd4..4925e0d 100644
> > --- a/drivers/dma/amba-pl08x.c
> > +++ b/drivers/dma/amba-pl08x.c
> > @@ -1123,6 +1123,8 @@ static u32 pl08x_width(enum dma_slave_buswidth
> > width)
> >  		return PL080_WIDTH_16BIT;
> >  	case DMA_SLAVE_BUSWIDTH_4_BYTES:
> >  		return PL080_WIDTH_32BIT;
> > +	default:
> > +		return -EINVAL;
> >  	}
> >  	return ~0;
> >  }
> > 
> > If you are okay, pls ack it
> 
> No.  This function is used as:
> 
> +       width = pl08x_width(addr_width);
> +       if (width == ~0) {
>                 dev_err(&pl08x->adev->dev,
>                         "bad runtime_config: alien address width\n");
>                 return -EINVAL;
>         }
> 
> Notice that it returns a u32 so negative errnos don't make sense.  It
> returns ~0 to indicate error.
> 
> The code is actually correct as it stands, it's just gcc deciding to
> emit a warning for an unhandled enum value which isn't really unhandled.
> Just move the 'return ~0;' at the end of the function inside the switch
> as a default case to shut it up.
Okay but shouldn't this ideally check for width < 0, that way we can
return proper errors?

If yo

Vinod Koul July 25, 2011, 1:51 p.m. UTC | #3

On Mon, 2011-07-25 at 14:43 +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 25, 2011 at 07:08:31PM +0530, Vinod Koul wrote:
> > Although gcc didnt like not handling other enums so warned:
> > 
> > drivers/dma/amba-pl08x.c: In function 'pl08x_width':
> > drivers/dma/amba-pl08x.c:1119: warning: enumeration value
> > 'DMA_SLAVE_BUSWIDTH_UNDEFINED' not handled in switch
> > drivers/dma/amba-pl08x.c:1119: warning: enumeration value
> > 'DMA_SLAVE_BUSWIDTH_8_BYTES' not handled in switch
> 
> Those must be new since I wrote the patch.
> 
> > which can be fixed as:
> > 
> > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> > index 9aa2bd4..4925e0d 100644
> > --- a/drivers/dma/amba-pl08x.c
> > +++ b/drivers/dma/amba-pl08x.c
> > @@ -1123,6 +1123,8 @@ static u32 pl08x_width(enum dma_slave_buswidth
> > width)
> >  		return PL080_WIDTH_16BIT;
> >  	case DMA_SLAVE_BUSWIDTH_4_BYTES:
> >  		return PL080_WIDTH_32BIT;
> > +	default:
> > +		return -EINVAL;
> >  	}
> >  	return ~0;
> >  }
> > 
> > If you are okay, pls ack it
> 
> No.  This function is used as:
> 
> +       width = pl08x_width(addr_width);
> +       if (width == ~0) {
>                 dev_err(&pl08x->adev->dev,
>                         "bad runtime_config: alien address width\n");
>                 return -EINVAL;
>         }
> 
> Notice that it returns a u32 so negative errnos don't make sense.  It
> returns ~0 to indicate error.
> 
> The code is actually correct as it stands, it's just gcc deciding to
> emit a warning for an unhandled enum value which isn't really unhandled.
> Just move the 'return ~0;' at the end of the function inside the switch
> as a default case to shut it up.
Okay but shouldn't this ideally check for width < 0, that way we can
return proper errors?

Russell King - ARM Linux July 25, 2011, 2:22 p.m. UTC | #4

On Mon, Jul 25, 2011 at 07:21:12PM +0530, Vinod Koul wrote:
> On Mon, 2011-07-25 at 14:43 +0100, Russell King - ARM Linux wrote:
> > No.  This function is used as:
> > 
> > +       width = pl08x_width(addr_width);
> > +       if (width == ~0) {
> >                 dev_err(&pl08x->adev->dev,
> >                         "bad runtime_config: alien address width\n");
> >                 return -EINVAL;
> >         }
> > 
> > Notice that it returns a u32 so negative errnos don't make sense.  It
> > returns ~0 to indicate error.
> > 
> > The code is actually correct as it stands, it's just gcc deciding to
> > emit a warning for an unhandled enum value which isn't really unhandled.
> > Just move the 'return ~0;' at the end of the function inside the switch
> > as a default case to shut it up.
> Okay but shouldn't this ideally check for width < 0, that way we can
> return proper errors?

No.  Two reasons:

1. u32 < 0 does not exist.

2. It's returning a sub bitmask value for the register, so signed numbers
   conceptually don't make sense.

Vinod Koul July 25, 2011, 2:38 p.m. UTC | #5

On Mon, 2011-07-25 at 15:22 +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 25, 2011 at 07:21:12PM +0530, Vinod Koul wrote:
> > On Mon, 2011-07-25 at 14:43 +0100, Russell King - ARM Linux wrote:
> > > No.  This function is used as:
> > > 
> > > +       width = pl08x_width(addr_width);
> > > +       if (width == ~0) {
> > >                 dev_err(&pl08x->adev->dev,
> > >                         "bad runtime_config: alien address width\n");
> > >                 return -EINVAL;
> > >         }
> > > 
> > > Notice that it returns a u32 so negative errnos don't make sense.  It
> > > returns ~0 to indicate error.
> > > 
> > > The code is actually correct as it stands, it's just gcc deciding to
> > > emit a warning for an unhandled enum value which isn't really unhandled.
> > > Just move the 'return ~0;' at the end of the function inside the switch
> > > as a default case to shut it up.
> > Okay but shouldn't this ideally check for width < 0, that way we can
> > return proper errors?
> 
> No.  Two reasons:
> 
> 1. u32 < 0 does not exist.
> 
> 2. It's returning a sub bitmask value for the register, so signed numbers
>    conceptually don't make sense.
Agreed, Pushed this with return ~0;
You should be able to see your patchset along with this change in my
tree now

Patch

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 9aa2bd4..4925e0d 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1123,6 +1123,8 @@  static u32 pl08x_width(enum dma_slave_buswidth
width)
 		return PL080_WIDTH_16BIT;
 	case DMA_SLAVE_BUSWIDTH_4_BYTES:
 		return PL080_WIDTH_32BIT;
+	default:
+		return -EINVAL;
 	}
 	return ~0;