linux-kernel - Re: [PATCH 07/12] ASoC: wcd9335: add CLASS-H Controller support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180724115908.GC2414@sirena.org.uk>
Date:   Tue, 24 Jul 2018 12:59:08 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     lee.jones@...aro.org, robh+dt@...nel.org, mark.rutland@....com,
        lgirdwood@...il.com, tiwai@...e.com, bgoswami@...eaurora.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        vkoul@...nel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH 07/12] ASoC: wcd9335: add CLASS-H Controller support

On Mon, Jul 23, 2018 at 04:54:05PM +0100, Srinivas Kandagatla wrote:

> +static inline int wcd_clsh_get_int_mode(struct wcd_clsh_ctrl *ctrl,
> +					int clsh_state)
> +{
> +	int mode;
> +
> +	if ((clsh_state != WCD_CLSH_STATE_EAR) &&
> +	    (clsh_state != WCD_CLSH_STATE_HPHL) &&
> +	    (clsh_state != WCD_CLSH_STATE_HPHR) &&
> +	    (clsh_state != WCD_CLSH_STATE_LO))
> +		mode = CLS_NONE;
> +	else
> +		mode = ctrl->interpolator_modes[ffs(clsh_state)];
> +
> +	return mode;

This looks like it wants to be a switch statement.

> +static void wcd_clsh_state_hph_lo(struct wcd_clsh_ctrl *ctrl, int req_state,
> +				  bool is_enable, int mode)
> +{
> +	struct snd_soc_component *comp = ctrl->comp;
> +	int hph_mode = 0;
> +
> +	if (is_enable) {
> +		/*
> +		 * If requested state is LO, put regulator
> +		 * in class-AB or if requested state is HPH,
> +		 * which means LO is already enabled, keep
> +		 * the regulator config the same at class-AB
> +		 * and just set the power modes for flyback
> +		 * and buck.
> +		 */
> +		if (req_state == WCD_CLSH_STATE_LO)
> +			wcd_clsh_set_buck_regulator_mode(comp, CLS_AB);

This seems like there's a pretty confusing state machine, or possibly
that we might end up in different states depending on how we transition.
Whatever is going on it really feels like this code is more complex than
it needs to be.  Some of this is the use of lots of nested if statements,
some of it is the lack of any clear description of what we're trying to
achieve.  It's hard to tell if the code is doing what's expected because
it's hard to tell what it is expected to do.

> +		else {

If there's braces on one side of an if/else there should be braces on
both sides.

> +			if (req_state == WCD_CLSH_STATE_HPHL)
> +				snd_soc_component_update_bits(comp,
> +					WCD9XXX_A_CDC_RX1_RX_PATH_CFG0,
> +					WCD9XXX_A_CDC_RX_PATH_CLSH_EN_MASK,
> +					WCD9XXX_A_CDC_RX_PATH_CLSH_ENABLE);
> +			if (req_state == WCD_CLSH_STATE_HPHR)
> +				snd_soc_component_update_bits(comp,
> +					WCD9XXX_A_CDC_RX2_RX_PATH_CFG0,
> +					WCD9XXX_A_CDC_RX_PATH_CLSH_EN_MASK,
> +					WCD9XXX_A_CDC_RX_PATH_CLSH_ENABLE);
> +		}

Switch statement?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)