lkml.org 
[lkml]   [2018]   [Jul]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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?
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2018-07-24 14:00    [W:2.608 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site