On 2015-07-22 10:22, Takashi Iwai wrote:
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@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.
Ok.
- 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;
Does this work? If we want to have the hdac_bus.dev ptr instead of a hdac_bus ptr, there does not seem to be an obvious way to go from the audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an arbitrary dev pointer).
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?
It was merely for symmetry. I'll remove it for v2.
void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);- } *cb_ops;
cb_ops doesn't sound intuitive. Any better name?
I was thinking of it as "callback ops", i e, calls that go in the reverse direction compared to the already existing "ops".
But if we call the device "audio_dev" as you suggested above, then maybe "audio_ops" would be nice and symmetric?