On 12/06/2023 12:18, Srinivas Kandagatla wrote:
Thanks Krzyztof,
few minor nits below.
Please trim unrelated context - it's easy to miss a comment between huge quoted text.
+static const char * const wsa884x_dev_mode_text[] = {
- "Speaker", "Receiver"
+};
+enum wsa884x_variant {
- WSA8840 = 0x0,
- WSA8845 = 0x5,
- WSA8845H = 0xc,
A proper defines for this makes more sense than a enum.
OK. I will also put the define next to respective register define.
O_REG(2000), },
- { WSA884X_ANA_WO_CTL_1, 0x00 },
- { WSA884X_OTP_REG_38, 0x00 },
- { WSA884X_OTP_REG_40, 0x8 << WSA884X_OTP_REG_40_ISENSE_RESCAL_SHIFT },
FIELD macros?
Ack.
+};
+static void wsa884x_set_gain_parameters(struct wsa884x_priv *wsa884x) +{
- struct regmap *regmap = wsa884x->regmap;
- unsigned int min_gain, igain, vgain, comp_offset;
- /*
* Downstream sets gain parameters customized per boards per use-case.* Choose here some sane values matching knowon users, like QRD8550* board:.** Values match here downstream:* For WSA884X_RECEIVER - G_7P5_DB system gain* For WSA884X_SPEAKER - G_21_DB system gain*/- if (wsa884x->dev_mode == WSA884X_RECEIVER) {
comp_offset = COMP_OFFSET4;min_gain = G_M6_DB;igain = ISENSE_18_DB;vgain = VSENSE_M12_DB;- } else {
/* WSA884X_SPEAKER */comp_offset = COMP_OFFSET0;min_gain = G_0_DB;igain = ISENSE_12_DB;vgain = VSENSE_M24_DB;- }
- regmap_update_bits(regmap, WSA884X_ISENSE2,
WSA884X_ISENSE2_ISENSE_GAIN_CTL_MASK,igain << WSA884X_ISENSE2_ISENSE_GAIN_CTL_SHIFT);- regmap_update_bits(regmap, WSA884X_VSENSE1,
WSA884X_VSENSE1_GAIN_VSENSE_FE_MASK,vgain << WSA884X_VSENSE1_GAIN_VSENSE_FE_SHIFT);- regmap_update_bits(regmap, WSA884X_GAIN_RAMPING_MIN,
WSA884X_GAIN_RAMPING_MIN_MIN_GAIN_MASK,min_gain << WSA884X_GAIN_RAMPING_MIN_MIN_GAIN_SHIFT);- if (wsa884x->port_enable[WSA884X_PORT_COMP]) {
regmap_update_bits(regmap, WSA884X_DRE_CTL_0,WSA884X_DRE_CTL_0_OFFSET_MASK,comp_offset);regmap_update_bits(regmap, WSA884X_DRE_CTL_1,WSA884X_DRE_CTL_1_CSR_GAIN_EN_MASK,0x0);- } else {
regmap_update_bits(regmap, WSA884X_DRE_CTL_1,WSA884X_DRE_CTL_1_CSR_GAIN_EN_MASK,0x1);- }
+}
+static void wsa884x_init(struct wsa884x_priv *wsa884x) +{
- unsigned int wo_ctl_0;
- unsigned int variant = 0;
- if (!regmap_read(wsa884x->regmap, WSA884X_OTP_REG_0, &variant))
wsa884x->variant = variant & WSA884X_OTP_REG_0_ID_MASK;- regmap_multi_reg_write(wsa884x->regmap, wsa884x_reg_init,
ARRAY_SIZE(wsa884x_reg_init));- wo_ctl_0 = 0xc;
- wo_ctl_0 |= WSA884X_ANA_WO_CTL_0_MODE_SPEAKER << WSA884X_ANA_WO_CTL_0_MODE_SHIFT;
FIELD helpers?
ack
- /* Assume that compander is enabled by default unless it is haptics sku */
- if (wsa884x->variant == WSA8845H)
wo_ctl_0 |= WSA884X_ANA_WO_CTL_0_PA_AUX_18_DB << WSA884X_ANA_WO_CTL_0_PA_AUX_SHIFT;- else
wo_ctl_0 |= WSA884X_ANA_WO_CTL_0_PA_AUX_0_DB << WSA884X_ANA_WO_CTL_0_PA_AUX_SHIFT;new line
It's write of previous created/computed value, so it's unusual to split these with new line.
- regmap_write(wsa884x->regmap, WSA884X_ANA_WO_CTL_0, wo_ctl_0);
- wsa884x_set_gain_parameters(wsa884x);
- wsa884x->hw_init = false;
+}
+static int wsa884x_update_status(struct sdw_slave *slave,
enum sdw_slave_status status)+{
- struct wsa884x_priv *wsa884x = dev_get_drvdata(&slave->dev);
- int ret;
- if (status == SDW_SLAVE_UNATTACHED)
should we not put the regmap in to cache state when device is un-attached?
Makes sense. I checked few other drivers and I did not see such pattern, but it seems reasonable.
wsa884x->hw_init = false;- if (wsa884x->hw_init || status != SDW_SLAVE_ATTACHED)
return 0;- regcache_cache_only(wsa884x->regmap, false);
- ret = regcache_sync(wsa884x->regmap);
- if (ret < 0) {
dev_err(&slave->dev, "Cannot sync regmap cache\n");return ret;- }
- wsa884x_init(wsa884x);
- return 0;
+}
+static int wsa884x_port_prep(struct sdw_slave *slave,
struct sdw_prepare_ch *prepare_ch,enum sdw_port_prep_ops state)+{
- struct wsa884x_priv *wsa884x = dev_get_drvdata(&slave->dev);
- if (state == SDW_OPS_PORT_POST_PREP)
wsa884x->port_prepared[prepare_ch->num - 1] = true;- else
wsa884x->port_prepared[prepare_ch->num - 1] = false;- return 0;
+}
+static const struct sdw_slave_ops wsa884x_slave_ops = {
- .update_status = wsa884x_update_status,
- .port_prep = wsa884x_port_prep,
+};
+static int wsa884x_dev_mode_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
- struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(component);
- ucontrol->value.enumerated.item[0] = wsa884x->dev_mode;
- return 0;
+}
+static int wsa884x_dev_mode_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
- struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(component);
- if (wsa884x->dev_mode == ucontrol->value.enumerated.item[0])
return 0;- wsa884x->dev_mode = ucontrol->value.enumerated.item[0];
- return 1;
+}
+static int wsa884x_get_swr_port(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
- struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(comp);
- struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
- int portidx = mixer->reg;
- ucontrol->value.integer.value[0] = wsa884x->port_enable[portidx];
- return 0;
+}
+static int wsa884x_set_swr_port(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
- struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(comp);
- struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
- int portidx = mixer->reg;
- if (ucontrol->value.integer.value[0]) {
if (wsa884x->port_enable[portidx])return 0;wsa884x->port_enable[portidx] = true;- } else {
if (!wsa884x->port_enable[portidx])return 0;wsa884x->port_enable[portidx] = false;- }
- return 1;
+}
+static int wsa884x_codec_probe(struct snd_soc_component *comp) +{
- struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(comp);
- snd_soc_component_init_regmap(comp, wsa884x->regmap);
- return 0;
+}
+static void wsa884x_spkr_post_pmu(struct snd_soc_component *component,
struct wsa884x_priv *wsa884x)+{
- unsigned int curr_limit, curr_ovrd_en;
- wsa884x_set_gain_parameters(wsa884x);
- if (wsa884x->dev_mode == WSA884X_RECEIVER) {
snd_soc_component_write_field(component, WSA884X_DRE_CTL_0,WSA884X_DRE_CTL_0_PROG_DELAY_MASK, 0x3);snd_soc_component_write_field(component, WSA884X_CDC_PATH_MODE,WSA884X_CDC_PATH_MODE_RXD_MODE_MASK,0x1);snd_soc_component_write_field(component, WSA884X_PWM_CLK_CTL,WSA884X_PWM_CLK_CTL_PWM_CLK_FREQ_SEL_MASK,0x1);- } else {
/* WSA884X_SPEAKER */snd_soc_component_write_field(component, WSA884X_DRE_CTL_0,WSA884X_DRE_CTL_0_PROG_DELAY_MASK, 0xf);- }
- if (wsa884x->port_enable[WSA884X_PORT_PBR]) {
curr_ovrd_en = 0x0;curr_limit = 0x15;Can we define these hardcoded values?
I don't know their meaning.
- } else {
curr_ovrd_en = 0x1;if (wsa884x->dev_mode == WSA884X_RECEIVER)curr_limit = 0x9;elsecurr_limit = 0x15;- }
- snd_soc_component_update_bits(component, WSA884X_CURRENT_LIMIT,
WSA884X_CURRENT_LIMIT_CURRENT_LIMIT_OVRD_EN_MASK,curr_ovrd_en);- snd_soc_component_update_bits(component, WSA884X_CURRENT_LIMIT,
WSA884X_CURRENT_LIMIT_CURRENT_LIMIT_MASK,curr_limit << WSA884X_CURRENT_LIMIT_CURRENT_LIMIT_SHIFT);field apis should make this bit cleaner.
ack
Best regards, Krzysztof