diff mbox

[v2,2/5] drxd: Fix warning caused by new entries in an enum

Message ID 4DC714EC.2060606@linuxtv.org (mailing list archive)
State Superseded
Headers show

Commit Message

Andreas Oberritter May 8, 2011, 10:10 p.m. UTC 
On 05/08/2011 09:17 PM, Steve Kerrison wrote:
> Additional bandwidth modes have been added in frontend.h
> drxd_hard.c had no default case so the compiler was warning about
> a non-exhausive switch statement.
> 
> This has been fixed by making the default behaviour the same as
> BANDWIDTH_AUTO, with the addition of a printk to notify if this
> ever happens.
> 
> Signed-off-by: Steve Kerrison <steve@stevekerrison.com>
> ---
>  drivers/media/dvb/frontends/drxd_hard.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/dvb/frontends/drxd_hard.c b/drivers/media/dvb/frontends/drxd_hard.c
> index 30a78af..b3b0704 100644
> --- a/drivers/media/dvb/frontends/drxd_hard.c
> +++ b/drivers/media/dvb/frontends/drxd_hard.c
> @@ -2325,6 +2325,10 @@ static int DRX_Start(struct drxd_state *state, s32 off)
>  		   InitEC and ResetEC
>  		   functions */
>  		switch (p->bandwidth) {
> +		default:
> +			printk(KERN_INFO "drxd: Unsupported bandwidth mode %u, reverting to default\n",
> +				p->bandwidth);
> +			/* Fall back to auto */

I'd prefer returning -EINVAL for unsupported parameters.

>  		case BANDWIDTH_AUTO:
>  		case BANDWIDTH_8_MHZ:
>  			/* (64/7)*(8/8)*1000000 */

I already had a patch for this, but forgot to submit it together with the frontend.h bits.

From 73d630b57f584d7e35cac5e27149cbc564aedde2 Mon Sep 17 00:00:00 2001
From: Andreas Oberritter <obi@linuxtv.org>
Date: Fri, 8 Apr 2011 16:39:20 +0000
Subject: [PATCH 2/2] DVB: drxd_hard: handle new bandwidths by returning -EINVAL

Signed-off-by: Andreas Oberritter <obi@linuxtv.org>
---
 drivers/media/dvb/frontends/drxd_hard.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Comments

Steve Kerrison May 9, 2011, 9:15 a.m. UTC | #1 
Hi Andreas,

> I'd prefer returning -EINVAL for unsupported parameters.
>
> [snip]
> 
> I already had a patch for this, but forgot to submit it together with
> the frontend.h bits.

That seems reasonable. Do I need to do anything with this? I'm happy for
Mauro to scrub my drxd and mxl patches and use yours instead.

> Btw., "status = status;" looks odd.

Heh, yes it does. I wonder if that was put in to deal with an "unused
variable" compiler warning before the switch statement had a default
case? Otherwise, perhaps it's from the department of redundancy
department.

Regards,
Mauro Carvalho Chehab May 9, 2011, 9:25 a.m. UTC | #2 
Em 09-05-2011 11:15, Steve Kerrison escreveu:
> Hi Andreas,
> 
>> I'd prefer returning -EINVAL for unsupported parameters.
>>
>> [snip]
>>
>> I already had a patch for this, but forgot to submit it together with
>> the frontend.h bits.
> 
> That seems reasonable. Do I need to do anything with this? I'm happy for
> Mauro to scrub my drxd and mxl patches and use yours instead.
> 
>> Btw., "status = status;" looks odd.
> 
> Heh, yes it does. I wonder if that was put in to deal with an "unused
> variable" compiler warning before the switch statement had a default
> case? Otherwise, perhaps it's from the department of redundancy
> department.

Yes, there is. Linux defines a macro for it:
	uninitialized_var()

(it basically will do status = status internally with newer gcc versions,
but it helps to document what's happening there)

it is sometimes better to initialize the var, as the warning may help
to detect troubles after some changes. 

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

Patch

diff --git a/drivers/media/dvb/frontends/drxd_hard.c b/drivers/media/dvb/frontends/drxd_hard.c
index 30a78af..53319f4 100644
--- a/drivers/media/dvb/frontends/drxd_hard.c
+++ b/drivers/media/dvb/frontends/drxd_hard.c
@@ -2348,6 +2348,9 @@  static int DRX_Start(struct drxd_state *state, s32 off)
 			status = Write16(state,
 					 FE_AG_REG_IND_DEL__A, 71, 0x0000);
 			break;
+		default:
+			status = -EINVAL;
+			break;
 		}
 		status = status;
 		if (status < 0)