ASoC: adav80x: mark expected switch fall-through
diff mbox series

Message ID 20180808191933.GA27767@embeddedor.com
State Accepted
Headers show
Series
  • ASoC: adav80x: mark expected switch fall-through
Related show

Commit Message

Gustavo A. R. Silva Aug. 8, 2018, 7:19 p.m. UTC
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Addresses-Coverity-ID: 1056531 ("Missing break in switch")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 sound/soc/codecs/adav80x.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Mark Brown Aug. 9, 2018, 9:30 a.m. UTC | #1
On Wed, Aug 08, 2018 at 02:19:33PM -0500, Gustavo A. R. Silva wrote:

> @@ -648,6 +648,7 @@ static int adav80x_set_pll(struct snd_soc_component *component, int pll_id,
>  			pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
>  			break;
>  		}
> +		/* fall through */
>  	default:
>  		return -EINVAL;
>  	}

Are you *positive* this is a deliberate fall through?
Lars-Peter Clausen Aug. 9, 2018, 11:23 a.m. UTC | #2
On 08/09/2018 11:30 AM, Mark Brown wrote:
> On Wed, Aug 08, 2018 at 02:19:33PM -0500, Gustavo A. R. Silva wrote:
> 
>> @@ -648,6 +648,7 @@ static int adav80x_set_pll(struct snd_soc_component *component, int pll_id,
>>  			pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
>>  			break;
>>  		}
>> +		/* fall through */
>>  	default:
>>  		return -EINVAL;
>>  	}
> 
> Are you *positive* this is a deliberate fall through?

It is. But it might make sense to re-order the code to look like

    case 54000000:
        if (source != ADAV80X_PLL_SRC_XIN)
            return -EINVAL;

        pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
        break;

It is just as many lines added when adding the /* fall through */, but it
makes it more obvious what is going on.

Either way:

Acked-by: Lars-Peter Clausen <lars@metafoo.de>
Mark Brown Aug. 9, 2018, 1:46 p.m. UTC | #3
On Thu, Aug 09, 2018 at 01:23:58PM +0200, Lars-Peter Clausen wrote:
> On 08/09/2018 11:30 AM, Mark Brown wrote:

> It is. But it might make sense to re-order the code to look like
> 
>     case 54000000:
>         if (source != ADAV80X_PLL_SRC_XIN)
>             return -EINVAL;
> 
>         pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
>         break;

> It is just as many lines added when adding the /* fall through */, but it
> makes it more obvious what is going on.

Yes, it's much more obvious.

Patch
diff mbox series

diff --git a/sound/soc/codecs/adav80x.c b/sound/soc/codecs/adav80x.c
index db21ecb..8b9ca7e 100644
--- a/sound/soc/codecs/adav80x.c
+++ b/sound/soc/codecs/adav80x.c
@@ -648,6 +648,7 @@  static int adav80x_set_pll(struct snd_soc_component *component, int pll_id,
 			pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
 			break;
 		}
+		/* fall through */
 	default:
 		return -EINVAL;
 	}