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