[5/5] ASoC: Intel: sof_sdw: clarify operator precedence
diff mbox series

Message ID 20200813175839.59422-6-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • ASoC: SOF/Intel: fix cppcheck warnings
Related show

Commit Message

Pierre-Louis Bossart Aug. 13, 2020, 5:58 p.m. UTC
Fix cppcheck warning:

sound/soc/intel/boards/sof_sdw.c:739:46: style: Clarify calculation
precedence for '&' and '?'. [clarifyCalculation]
 hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
                                             ^

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/sof_sdw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mark Brown Aug. 13, 2020, 6:45 p.m. UTC | #1
On Thu, Aug 13, 2020 at 12:58:39PM -0500, Pierre-Louis Bossart wrote:

> -	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
> +	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
>  				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;

Or better yet, just don't abuse the ternery operator like this and write
normal conditional statements.
Pierre-Louis Bossart Aug. 13, 2020, 7:43 p.m. UTC | #2
On 8/13/20 1:45 PM, Mark Brown wrote:
> On Thu, Aug 13, 2020 at 12:58:39PM -0500, Pierre-Louis Bossart wrote:
> 
>> -	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
>> +	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
>>   				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;
> 
> Or better yet, just don't abuse the ternery operator like this and write
> normal conditional statements.

I count 795 uses of the ternary operator in sound/soc and 68776 in my 
local kernel branch.
Can you clarify in what way this is an abuse? I don't mind changing 
this, I wasn't aware this is frowned upon.
Mark Brown Aug. 13, 2020, 7:49 p.m. UTC | #3
On Thu, Aug 13, 2020 at 02:43:50PM -0500, Pierre-Louis Bossart wrote:
> On 8/13/20 1:45 PM, Mark Brown wrote:
> > On Thu, Aug 13, 2020 at 12:58:39PM -0500, Pierre-Louis Bossart wrote:

> > > -	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
> > > +	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
> > >   				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;

> > Or better yet, just don't abuse the ternery operator like this and write
> > normal conditional statements.

> I count 795 uses of the ternary operator in sound/soc and 68776 in my local
> kernel branch.
> Can you clarify in what way this is an abuse? I don't mind changing this, I
> wasn't aware this is frowned upon.

If you write a normal conditional statement then not only is the
precedence clear but it's just generally easier to read.  There are
cases where it can help make things clearer (eg, avoiding the use of
scratch variables to hold results) but this is most definitely not one
of them and I don't understand everyone's enthusiasm for trying to put
them in.
Pierre-Louis Bossart Aug. 13, 2020, 7:57 p.m. UTC | #4
On 8/13/20 2:49 PM, Mark Brown wrote:
> On Thu, Aug 13, 2020 at 02:43:50PM -0500, Pierre-Louis Bossart wrote:
>> On 8/13/20 1:45 PM, Mark Brown wrote:
>>> On Thu, Aug 13, 2020 at 12:58:39PM -0500, Pierre-Louis Bossart wrote:
> 
>>>> -	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
>>>> +	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
>>>>    				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;
> 
>>> Or better yet, just don't abuse the ternery operator like this and write
>>> normal conditional statements.
> 
>> I count 795 uses of the ternary operator in sound/soc and 68776 in my local
>> kernel branch.
>> Can you clarify in what way this is an abuse? I don't mind changing this, I
>> wasn't aware this is frowned upon.
> 
> If you write a normal conditional statement then not only is the
> precedence clear but it's just generally easier to read.  There are
> cases where it can help make things clearer (eg, avoiding the use of
> scratch variables to hold results) but this is most definitely not one
> of them and I don't understand everyone's enthusiasm for trying to put
> them in.

That's fair, I am not a big fan either.
Please drop this patch and we'll rework this machine driver. There's a 
set of updates planned anyways and we can add this cleanup in a separate 
set. Thanks!

Patch
diff mbox series

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 2463d432bf4d..baff9e99f626 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -728,7 +728,7 @@  static int sof_card_dai_links_create(struct device *dev,
 	for (i = 0; i < ARRAY_SIZE(codec_info_list); i++)
 		codec_info_list[i].amp_num = 0;
 
-	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
+	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
 				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;
 
 	ssp_mask = SOF_SSP_GET_PORT(sof_sdw_quirk);