Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 15 Dec 2023 14:19:50 +0100 | From | Neil Armstrong <> | Subject | Re: [PATCH v3 4/5] ASoC: codecs: Add WCD939x Soundwire devices driver |
| |
On 13/12/2023 19:31, Mark Brown wrote: > On Thu, Dec 07, 2023 at 11:28:07AM +0100, Neil Armstrong wrote: >> Add Soundwire Slave driver for the WCD9390/WCD9395 Audio Codec. >> >> The WCD9390/WCD9395 Soundwire devices will be used by the >> main WCD9390/WCD9395 Audio Codec driver to access registers >> and configure Soundwire RX and TX ports. > >> +static const struct reg_default wcd939x_defaults[] = { > >> + { WCD939X_DIGITAL_MODE_STATUS_0, 0x00 }, >> + { WCD939X_DIGITAL_MODE_STATUS_1, 0x00 }, > > There's a bunch of registers like this which look like they should be > volatile and are actually volatile which makes supplying defaults rather > strange - in general volatile registers shouldn't have defaults.
Indeed I'll clean those up
> >> + { WCD939X_DIGITAL_EFUSE_REG_0, 0x00 }, >> + { WCD939X_DIGITAL_EFUSE_REG_1, 0xff }, >> + { WCD939X_DIGITAL_EFUSE_REG_2, 0xff }, > > With the fuse registers even though I'd expect them to be cachable the > whole point is usually that these are programmable per device and > therefore I'd not expect defaults, I'd expect them to be cached on first > use.
Ack
> >> +static bool wcd939x_readonly_register(struct device *dev, unsigned int reg) >> +{ > >> + case WCD939X_DIGITAL_CHIP_ID0: >> + case WCD939X_DIGITAL_CHIP_ID1: >> + case WCD939X_DIGITAL_CHIP_ID2: >> + case WCD939X_DIGITAL_CHIP_ID3: > >> + case WCD939X_DIGITAL_EFUSE_REG_0: >> + case WCD939X_DIGITAL_EFUSE_REG_1: >> + case WCD939X_DIGITAL_EFUSE_REG_2: > >> + /* Consider all readonly registers as volatile */ >> + .volatile_reg = wcd939x_readonly_register, > > There's a bunch of the readonly registers that I'd expect to be cachable > at runtime - I *hope* the chip ID doesn't change at runtime! OTOH it > likely doesn't matter so perhaps it's fine but the comment could use > some improvement.
I'll improve this
> >> +static int wcd939x_sdw_component_bind(struct device *dev, struct device *master, >> + void *data) >> +{ >> + /* Bind is required by component framework */ >> + return 0; >> +} >> + >> +static void wcd939x_sdw_component_unbind(struct device *dev, >> + struct device *master, void *data) >> +{ >> + /* Unbind is required by component framework */ >> +} >> + >> +static const struct component_ops wcd939x_sdw_component_ops = { >> + .bind = wcd939x_sdw_component_bind, >> + .unbind = wcd939x_sdw_component_unbind, >> +}; > > So what exactly is the component framework *doing* here then? It really > would be better to get this fixed in the component framework if this is > a sensible usage.
So the component framework is here to synchronize probes of the main codec and soundwire devices, because the main codec needs the soundwire devices to access registers. I assume this design was chosen to limit probe defer infinite loops waiting for the soundwire devices to probe
I'll propose a change on the component framework, without any insurance it would be accepted.
> >> +static int __maybe_unused wcd939x_sdw_runtime_resume(struct device *dev) >> +{ >> + struct wcd939x_sdw_priv *wcd = dev_get_drvdata(dev); >> + >> + if (wcd->regmap) { >> + regcache_cache_only(wcd->regmap, false); >> + regcache_sync(wcd->regmap); >> + } >> + >> + pm_runtime_mark_last_busy(dev); > > The pm_runtime_mark_last_busy() in the resume function is a bit of a > weird pattern - usually this is something that the user updates and more > normally when releasing a runtime PM reference.
I took this from wcd938x_sd, I'll check the rationale of it in the resume function.
Thanks, Neil
| ![\](/images/icornerr.gif) |