| Message ID | 20220629090644.67982-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | ASoC: codecs: add WSA883x support | expand |
On Wed, Jun 29, 2022 at 10:06:43AM +0100, Srinivas Kandagatla wrote: > +static int wsa_dev_mode_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); > + struct wsa883x_priv *wsa883x = snd_soc_component_get_drvdata(component); > + > + if (wsa883x->dev_mode == ucontrol->value.enumerated.item[0]) > + return 0; > + > + wsa883x->dev_mode = ucontrol->value.enumerated.item[0]; > + > + return 0; > +} This isn't returning 1 when the value changes so will miss generating events, please test the driver with mixer-test. > + switch (event) { > + case SND_SOC_DAPM_POST_PMU: > + if (wsa883x->dev_mode == RECEIVER) { > + snd_soc_component_write_field(component, WSA883X_CDC_PATH_MODE, > + WSA883X_RXD_MODE_MASK, > + WSA883X_RXD_MODE_HIFI); > + snd_soc_component_write_field(component, WSA883X_SPKR_PWM_CLK_CTL, > + WSA883X_SPKR_PWM_FREQ_SEL_MASK, > + WSA883X_SPKR_PWM_FREQ_F600KHZ); > + snd_soc_component_write_field(component, WSA883X_DRE_CTL_0, > + WSA883X_DRE_PROG_DELAY_MASK, 0x0); > + } else if (wsa883x->dev_mode == SPEAKER) { This looks like it'd be better written as a switch statement. > +static const struct snd_kcontrol_new wsa883x_snd_controls[] = { > + SOC_SINGLE_RANGE_TLV("PA Gain", WSA883X_DRE_CTL_1, 1, > + 0x0, 0x1f, 1, pa_gain), Volume controls should end in Volume, mixer-test should also have caught this.
> +/* 4 ports */ > +static struct sdw_dpn_prop wsa_sink_dpn_prop[WSA883X_MAX_SWR_PORTS] = { > + { > + /* DAC */ > + .num = 1, > + .type = SDW_DPN_SIMPLE, > + .min_ch = 1, > + .max_ch = 1, > + .simple_ch_prep_sm = true, > + .read_only_wordlength = true, > + }, { nit-pick: it's unusual to see such opening brackets for structure initialization, usually there are on a new line. > + /* COMP */ > + .num = 2, > + .type = SDW_DPN_SIMPLE, > + .min_ch = 1, > + .max_ch = 1, > + .simple_ch_prep_sm = true, > + .read_only_wordlength = true, > + }, { > + /* BOOST */ > + .num = 3, > + .type = SDW_DPN_SIMPLE, > + .min_ch = 1, > + .max_ch = 1, > + .simple_ch_prep_sm = true, > + .read_only_wordlength = true, > + }, { > + /* VISENSE */ > + .num = 4, > + .type = SDW_DPN_SIMPLE, > + .min_ch = 1, > + .max_ch = 1, > + .simple_ch_prep_sm = true, > + .read_only_wordlength = true, > + } > +}; > +static int wsa883x_update_status(struct sdw_slave *slave, > + enum sdw_slave_status status) > +{ > + struct wsa883x_priv *wsa883x = dev_get_drvdata(&slave->dev); > + > + if (status == SDW_SLAVE_ATTACHED && slave->dev_num > 0) do you actually need to test if slave->dev_num is > 0? if I look at drivers/soundwire/bus.c, update_status cannot really be invoked with dev_num == 0. > + wsa883x_init(wsa883x); > + > + return 0; > +} > + > +static int __maybe_unused wsa883x_runtime_resume(struct device *dev) > +{ > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > + struct regmap *regmap = dev_get_regmap(dev, NULL); > + struct wsa883x_priv *wsa883x = dev_get_drvdata(dev); > + int ret; > + > + ret = regulator_enable(wsa883x->vdd); > + if (ret) { > + dev_err(dev, "Failed to enable vdd regulator (%d)\n", ret); > + return ret; > + } > + > + gpiod_direction_output(wsa883x->sd_n, 1); > + > + wait_for_completion_timeout(&slave->initialization_complete, > + msecs_to_jiffies(WSA883X_PROBE_TIMEOUT)); check for success? You don't want to enable regmap sync below if the device never successfully attached and initialized. > + > + usleep_range(20000, 20010); > + regcache_cache_only(regmap, false); > + regcache_sync(regmap); > + > + return 0; > +}
>>> +static int wsa883x_update_status(struct sdw_slave *slave, >>> + enum sdw_slave_status status) >>> +{ >>> + struct wsa883x_priv *wsa883x = dev_get_drvdata(&slave->dev); >>> + >>> + if (status == SDW_SLAVE_ATTACHED && slave->dev_num > 0) >> >> do you actually need to test if slave->dev_num is > 0? >> > Few years back I think it was you who asked me to add this check.. :-) > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2073074.html Oops! My comment was valid in general but at the bus level. With the benefit of hindsight, I don't think this comment is valid in this callback. update_status is either called with UNATTACHED, or with ATTACHED/ALERT after programming dev_num to a value > 0. It's not wrong to leave the code as is, but it's likely to be an always-true condition.