[alsa-devel] [PATCH 1/4] drm/i915: Add audio hotplug info struct

[Takashi Iwai]

On Tue, 21 Jul 2015 09:57:24 +0200,
David Henningsson wrote:
> 
> This struct will be used to transfer information from the i915
> driver to the hda driver on HDMI hotplug events.
> 
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>

Looks good to me, just a few nitpicking:

> ---
>  include/drm/i915_component.h |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..4fc0db3 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -24,8 +24,22 @@
>  #ifndef _I915_COMPONENT_H_
>  #define _I915_COMPONENT_H_
>  
> +struct hdac_bus;
> +
> +struct i915_audio_hotplug_info {
> +	int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
> +	int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
> +	int port; /* Used for mapping to affected nid */
> +	int port_multi_stream_device; /* For DP multi-streaming */
> +
> +	bool plugged_in;
> +	uint8_t *eld;

Use u8 or just unsigned char as it's a in-kernel API.
Also, safer to add const, since this is read-only for audio side.

> +	int eld_size;
> +};
> +
>  struct i915_audio_component {
>  	struct device *dev;
> +	struct hdac_bus *hdac_bus;

If we want to be more generic, using a struct device would be better,
e.g.
	struct device *audio_dev;

>  	const struct i915_audio_component_ops {
>  		struct module *owner;
> @@ -34,6 +48,11 @@ struct i915_audio_component {
>  		void (*codec_wake_override)(struct device *, bool enable);
>  		int (*get_cdclk_freq)(struct device *);
>  	} *ops;
> +
> +	const struct i915_audio_component_cb_ops {
> +		struct module *owner;

Do we need the owner field at all?

> +		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
> +	} *cb_ops;

cb_ops doesn't sound intuitive.  Any better name?


thanks,

Takashi