Re: [PATCH 1/4] drm/i915: Add audio hotplug info struct

[David Henningsson <david.henningsson@canonical.com>]

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?

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx