Messages in this thread | | | Date | Wed, 29 Jun 2022 17:09:28 +0100 | Subject | Re: [PATCH 2/4] ASoC: codecs: add wsa883x amplifier support | From | Srinivas Kandagatla <> |
| |
Thanks Pierre,
On 29/06/2022 16:03, Pierre-Louis Bossart wrote: > >> +/* 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? > 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
> 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.
Yes, that might be possible, I can spin a fix for this.
thanks, --srini > >> + >> + usleep_range(20000, 20010); >> + regcache_cache_only(regmap, false); >> + regcache_sync(regmap); >> + >> + return 0; >> +} >
| |