Re: [PATCH v3 2/7] ASoC: codecs: wcd-clsh: add new version support

[Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>]

On 19/03/2021 15:23, Pierre-Louis Bossart wrote:
> > +static void wcd_clsh_v3_set_hph_mode(struct snd_soc_component 
> > *component,
> > +                  int mode)
> > +{
> > +    u8 val = 0;
>
> initialization not needed.
I agree, will remove this in next spin.

> > +
> > +    switch (mode) {
> > +    case CLS_H_NORMAL:
> > +        val = 0x00;
> > +        break;
> > +    case CLS_AB:
> > +    case CLS_H_ULP:
> > +        val = 0x0C;
> > +        break;
> > +    case CLS_AB_HIFI:
> > +    case CLS_H_HIFI:
> > +        val = 0x08;
> > +        break;
> > +    case CLS_H_LP:
> > +    case CLS_H_LOHIFI:
> > +    case CLS_AB_LP:
> > +    case CLS_AB_LOHIFI:
> > +        val = 0x04;
> > +        break;
> > +    default:
> > +        dev_err(component->dev, "%s:Invalid mode %d\n", __func__, mode);
> > +        return;
> > +    };
> > +
> > +    snd_soc_component_update_bits(component, WCD9XXX_ANA_HPH, 0x0C, 
> > val);
> > +}
> > +
> > +
>
> extra line
will remove this in next spin.

> > +void wcd_clsh_set_hph_mode(struct wcd_clsh_ctrl *ctrl, int mode)
> > +{
> > +    struct snd_soc_component *comp = ctrl->comp;
> > +
> > +    if (ctrl->codec_version >= WCD937X)
> > +        wcd_clsh_v3_set_hph_mode(comp, mode);
> > +    else
> > +        wcd_clsh_v2_set_hph_mode(comp, mode);
> > +
> > +}
> > +
> >   static void wcd_clsh_set_flyback_current(struct snd_soc_component 
> > *comp,
> >                        int mode)
> >   {
> > @@ -289,6 +388,130 @@ static void 
> > wcd_clsh_set_buck_regulator_mode(struct snd_soc_component *comp,
> >                       WCD9XXX_A_ANA_RX_REGULATOR_MODE_CLS_H);
> >   }
> > +static void wcd_clsh_v3_set_buck_regulator_mode(struct 
> > snd_soc_component *component,
> > +                        int mode)
> > +{
> > +    snd_soc_component_update_bits(component, WCD9XXX_ANA_RX_SUPPLIES,
> > +                0x02, 0x00);
> > +}
> > +
> > +static inline void wcd_clsh_v3_set_flyback_mode(struct 
> > snd_soc_component *component,
> > +                        int mode)
> > +{
> > +    if (mode == CLS_H_HIFI || mode == CLS_H_LOHIFI ||
> > +        mode == CLS_AB_HIFI || mode == CLS_AB_LOHIFI) {
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_ANA_RX_SUPPLIES,
> > +                0x04, 0x04);
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_FLYBACK_VNEG_CTRL_4,
> > +                0xF0, 0x80);
> > +    } else {
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_ANA_RX_SUPPLIES,
> > +                0x04, 0x00); /* set to Default */
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_FLYBACK_VNEG_CTRL_4,
> > +                0xF0, 0x70);
> > +    }
> > +}
> > +
> > +static inline void wcd_clsh_v3_force_iq_ctl(struct snd_soc_component 
> > *component,
> > +                     int mode, bool enable)
> > +{
> > +    if (enable) {
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_FLYBACK_VNEGDAC_CTRL_2,
> > +                0xE0, 0xA0);
> > +        /* 100usec delay is needed as per HW requirement */
> > +        usleep_range(100, 110);
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_CLASSH_MODE_3,
> > +                0x02, 0x02);
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_CLASSH_MODE_2,
> > +                0xFF, 0x1C);
> > +        if (mode == CLS_H_LOHIFI || mode == CLS_AB_LOHIFI) {
> > +            snd_soc_component_update_bits(component,
> > +                    WCD9XXX_HPH_NEW_INT_PA_MISC2,
> > +                    0x20, 0x20);
> > +            snd_soc_component_update_bits(component,
> > +                    WCD9XXX_RX_BIAS_HPH_LOWPOWER,
> > +                    0xF0, 0xC0);
> > +            snd_soc_component_update_bits(component,
> > +                    WCD9XXX_HPH_PA_CTL1,
> > +                    0x0E, 0x02);
> > +        }
> > +    } else {
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_HPH_NEW_INT_PA_MISC2,
> > +                0x20, 0x00);
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_RX_BIAS_HPH_LOWPOWER,
> > +                0xF0, 0x80);
> > +        snd_soc_component_update_bits(component,
> > +                WCD9XXX_HPH_PA_CTL1,
> > +                0x0E, 0x06);
> > +    }
> > +}
>
> do you need the inline for the two functions above?
yes, these are totally unnecessary.

--srini
>