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

[Neil Armstrong <neil.armstrong@xxxxxxxxxx>]

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