Re: [net-next v4 10/12] ASoC: SOF: Introduce descriptors for SOF client

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2145 bytes --]

On Wed, May 20, 2020 at 12:02:25AM -0700, Jeff Kirsher wrote:
> From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> 
> A client in the SOF (Sound Open Firmware) context is a
> device that needs to communicate with the DSP via IPC

As please send patches to the maintainers for the code you would like to
change.  :(

> +config SND_SOC_SOF_CLIENT
> +	tristate
> +	select VIRTUAL_BUS
> +	help
> +	  This option is not user-selectable but automagically handled by
> +	  'select' statements at a higher level

VIRTUAL_BUS is both user visible and selected here (not that the select
will do much since this option is not user visible) - which is it?

> +config SND_SOC_SOF_CLIENT_SUPPORT
> +	bool "SOF enable clients"
> +	depends on SND_SOC_SOF
> +	help
> +	  This adds support for client support with Sound Open Firmware.
> +	  The SOF driver adds the capability to separate out the debug
> +	  functionality for IPC tests, probes etc. into separate client
> +	  devices. This option would also allow adding client devices
> +	  based on DSP FW capabilities and ACPI/OF device information.
> +	  Say Y if you want to enable clients with SOF.
> +	  If unsure select "N".

I'm not sure that's going to mean much to users, nor can I see why you'd
ever have a SOF system without this enabled?

> +	/*
> +	 * Register virtbus device for the client.
> +	 * The error path in virtbus_register_device() calls put_device(),
> +	 * which will free cdev in the release callback.
> +	 */
> +	ret = virtbus_register_device(vdev);
> +	if (ret < 0)
> +		return ret;

> +	/* make sure the probe is complete before updating client list */
> +	timeout = msecs_to_jiffies(SOF_CLIENT_PROBE_TIMEOUT_MS);
> +	time = wait_for_completion_timeout(&cdev->probe_complete, timeout);
> +	if (!time) {
> +		dev_err(sdev->dev, "error: probe of virtbus dev %s timed out\n",
> +			name);
> +		virtbus_unregister_device(vdev);
> +		return -ETIMEDOUT;
> +	}

This seems wrong - what happens if the driver isn't loaded yet for
example?  Why does the device registration care what's going on with
driver binding at all?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]