Re: [PATCH v8 1/5] video: rmk's HDMI notification prototype

[Russell King - ARM Linux <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org>]

On Thu, Aug 11, 2016 at 12:49:21PM +0200, Hans Verkuil wrote:
> On 08/11/16 12:39, Russell King - ARM Linux wrote:
> > On Thu, Aug 11, 2016 at 12:30:03PM +0200, Hans Verkuil wrote:
> >> Hi Russell,
> >>
> >> I like this approach and it is something I will need for other CEC drivers
> >> (not yet merged) as well.
> >>
> >> Just a few comments:
> >>
> >> On 08/11/16 11:20, Philipp Zabel wrote:
> >>> This is Russell's HDMI notification prototype [1], currently waiting
> >>> for the HDMI CEC situation to resolve.
> >>>
> >>> The use case for the notifications on MediaTek MT8173 is to let the
> >>> (dis)connection notifications control an ALSA jack object.
> >>>
> >>> No Signed-off-by since this is not my code, and still up for discussion.
> >>>
> >>> [1] https://patchwork.kernel.org/patch/8351501/
> >>> ---
> >>>  drivers/video/Kconfig         |  3 +++
> >>>  drivers/video/Makefile        |  1 +
> >>>  drivers/video/hdmi-notifier.c | 61 +++++++++++++++++++++++++++++++++++++++++++
> >>>  include/linux/hdmi-notifier.h | 44 +++++++++++++++++++++++++++++++
> >>>  4 files changed, 109 insertions(+)
> >>>  create mode 100644 drivers/video/hdmi-notifier.c
> >>>  create mode 100644 include/linux/hdmi-notifier.h
> >>>
> >>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> >>> index 3c20af9..1ee7b9f 100644
> >>> --- a/drivers/video/Kconfig
> >>> +++ b/drivers/video/Kconfig
> >>> @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS
> >>>  config HDMI
> >>>  	bool
> >>>  
> >>> +config HDMI_NOTIFIERS
> >>> +	bool
> >>> +
> >>>  if VT
> >>>  	source "drivers/video/console/Kconfig"
> >>>  endif
> >>> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> >>> index 9ad3c17..65f5649 100644
> >>> --- a/drivers/video/Makefile
> >>> +++ b/drivers/video/Makefile
> >>> @@ -1,5 +1,6 @@
> >>>  obj-$(CONFIG_VGASTATE)            += vgastate.o
> >>>  obj-$(CONFIG_HDMI)                += hdmi.o
> >>> +obj-$(CONFIG_HDMI_NOTIFIERS)      += hdmi-notifier.o
> >>>  
> >>>  obj-$(CONFIG_VT)		  += console/
> >>>  obj-$(CONFIG_LOGO)		  += logo/
> >>> diff --git a/drivers/video/hdmi-notifier.c b/drivers/video/hdmi-notifier.c
> >>> new file mode 100644
> >>> index 0000000..a233359
> >>> --- /dev/null
> >>> +++ b/drivers/video/hdmi-notifier.c
> >>> @@ -0,0 +1,61 @@
> >>> +#include <linux/export.h>
> >>> +#include <linux/hdmi-notifier.h>
> >>> +#include <linux/notifier.h>
> >>> +#include <linux/string.h>
> >>> +
> >>> +static BLOCKING_NOTIFIER_HEAD(hdmi_notifier);
> >>> +
> >>> +int hdmi_register_notifier(struct notifier_block *nb)
> >>> +{
> >>> +	return blocking_notifier_chain_register(&hdmi_notifier, nb);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(hdmi_register_notifier);
> >>> +
> >>> +int hdmi_unregister_notifier(struct notifier_block *nb)
> >>> +{
> >>> +	return blocking_notifier_chain_unregister(&hdmi_notifier, nb);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(hdmi_unregister_notifier);
> >>> +
> >>> +void hdmi_event_connect(struct device *dev)
> >>> +{
> >>> +	struct hdmi_event_base base;
> >>> +
> >>> +	base.source = dev;
> >>> +
> >>> +	blocking_notifier_call_chain(&hdmi_notifier, HDMI_CONNECTED, &base);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(hdmi_event_connect);
> >>> +
> >>> +void hdmi_event_disconnect(struct device *dev)
> >>> +{
> >>> +	struct hdmi_event_base base;
> >>> +
> >>> +	base.source = dev;
> >>> +
> >>> +	blocking_notifier_call_chain(&hdmi_notifier, HDMI_DISCONNECTED, &base);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(hdmi_event_disconnect);
> >>> +
> >>> +void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size)
> >>
> >> I would prefer const u8 *edid. It is after all just a u8 array and not some
> >> opaque data structure.
> > 
> > In DRM, the edid is of type "struct edid" (defined in
> > include/drm/drm_edid.h).  So, making it "const u8 *" means that all
> > DRM drivers will have to explicitly cast it - not something I'm in
> > favour of.
> 
> I thought this was the raw EDID data. But if you pass a struct edid around,
> then why not make this const struct edid *edid? I fail to see why you would
> want to use a void pointer here.

struct edid is a DRM thing - it's not generic.  Using struct edid here
would force everyone to use the DRM structure, whereas other subsystems
use, eg, unsigned char.

> Also struct edid is for the first EDID block only, whereas CEC drivers need
> the CEA-861 block to get the physical address.

That may be, but DRM parses the HDMI vendor block too while still using
struct edid to represent it.  See drm_find_edid_extension() in
drivers/gpu/drm/drm_edid.c

This is just how DRM is... I'm not sure what the design decisions were
that came up with this, but it's what we have, and I'm not sure whether
DRM would be open to changing it.

> > The ELD stands for EDID-like data.  See information on HDA039.  It's a
> > method of conveying the EDID data specific to audio drivers to them
> > without having the complexities of parsing the full EDID each time.
> > It's also exported to userland so that userland can determine the
> > capabilities of the audio path, again, without having to have full
> > EDID parsers and exporting the full EDID data block through audio
> > drivers.
> > 
> 
> OK. Since eld is an unsigned char array I would suggest changing void to
> unsigned char here.
> 
> Or perhaps const unsigned char eld[128].

Yes, I'll make that change.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.