Re: [PATCH v3 4/5] ASoC: codecs: Add WCD939x Soundwire devices driver

[Neil Armstrong <neil.armstrong@linaro.org>]

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