[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Neil Armstrong]

From: Russell King <rmk+kernel@armlinux.org.uk>

Add CEC notifier support to the HDMI bridge driver, so that the CEC
part of the IP can receive its physical address.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Hi Archit, Hans,

This is repost of Russell's patch from his dw-hdmi CEC patchset.

Since his CEC implementation will be integrated in the bridge driver,
this notifier patch won't be re-posted.

But the Amlogic Platform needs a notifier since it uses a standalone CEC
controller.

Thanks,
Neil

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index ead1124..9c03846 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -33,6 +33,8 @@
 #include <uapi/linux/media-bus-format.h>
 #include <uapi/linux/videodev2.h>
 
+#include <media/cec-notifier.h>
+
 #include "dw-hdmi.h"
 #include "dw-hdmi-audio.h"
 
@@ -175,6 +177,8 @@ struct dw_hdmi {
 	struct regmap *regm;
 	void (*enable_audio)(struct dw_hdmi *hdmi);
 	void (*disable_audio)(struct dw_hdmi *hdmi);
+
+	struct cec_notifier *cec_notifier;
 };
 
 #define HDMI_IH_PHY_STAT0_RX_SENSE \
@@ -1896,6 +1900,7 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 		hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
 		hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
 		drm_mode_connector_update_edid_property(connector, edid);
+		cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier, edid);
 		ret = drm_add_edid_modes(connector, edid);
 		/* Store the ELD */
 		drm_edid_to_eld(connector, edid);
@@ -2077,6 +2082,10 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
 		dw_hdmi_update_phy_mask(hdmi);
 	}
 	mutex_unlock(&hdmi->mutex);
+
+	if (!rx_sense && !hpd)
+		cec_notifier_set_phys_addr(hdmi->cec_notifier,
+					   CEC_PHYS_ADDR_INVALID);
 }
 
 void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
@@ -2376,6 +2385,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
 	if (ret)
 		goto err_iahb;
 
+	hdmi->cec_notifier = cec_notifier_get(dev);
+	if (!hdmi->cec_notifier) {
+		ret = -ENOMEM;
+		goto err_iahb;
+	}
+
 	/*
 	 * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
 	 * N and cts values before enabling phy
@@ -2452,6 +2467,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
 		hdmi->ddc = NULL;
 	}
 
+	if (hdmi->cec_notifier)
+		cec_notifier_put(hdmi->cec_notifier);
+
 	clk_disable_unprepare(hdmi->iahb_clk);
 err_isfr:
 	clk_disable_unprepare(hdmi->isfr_clk);
-- 
1.9.1

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Jose Abreu]

Hi Neil,


On 06-07-2017 11:33, Neil Armstrong wrote:
> From: Russell King <rmk+kernel@armlinux.org.uk>
>
> Add CEC notifier support to the HDMI bridge driver, so that the CEC
> part of the IP can receive its physical address.
>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> Hi Archit, Hans,
>
> This is repost of Russell's patch from his dw-hdmi CEC patchset.
>
> Since his CEC implementation will be integrated in the bridge driver,
> this notifier patch won't be re-posted.
>
> But the Amlogic Platform needs a notifier since it uses a standalone CEC
> controller.
>
> Thanks,
> Neil
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index ead1124..9c03846 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -33,6 +33,8 @@
>  #include <uapi/linux/media-bus-format.h>
>  #include <uapi/linux/videodev2.h>
>  
> +#include <media/cec-notifier.h>

Don't you also have to "select CEC_NOTIFIER" in Kconfig? Or is it
not needed anymore with the recent Kconfig changes of CEC?

Best regards,
Jose Miguel Abreu

> +
>  #include "dw-hdmi.h"
>  #include "dw-hdmi-audio.h"
>  
> @@ -175,6 +177,8 @@ struct dw_hdmi {
>  	struct regmap *regm;
>  	void (*enable_audio)(struct dw_hdmi *hdmi);
>  	void (*disable_audio)(struct dw_hdmi *hdmi);
> +
> +	struct cec_notifier *cec_notifier;
>  };
>  
>  #define HDMI_IH_PHY_STAT0_RX_SENSE \
> @@ -1896,6 +1900,7 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>  		hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
>  		hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
>  		drm_mode_connector_update_edid_property(connector, edid);
> +		cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier, edid);
>  		ret = drm_add_edid_modes(connector, edid);
>  		/* Store the ELD */
>  		drm_edid_to_eld(connector, edid);
> @@ -2077,6 +2082,10 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
>  		dw_hdmi_update_phy_mask(hdmi);
>  	}
>  	mutex_unlock(&hdmi->mutex);
> +
> +	if (!rx_sense && !hpd)
> +		cec_notifier_set_phys_addr(hdmi->cec_notifier,
> +					   CEC_PHYS_ADDR_INVALID);
>  }
>  
>  void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
> @@ -2376,6 +2385,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
>  	if (ret)
>  		goto err_iahb;
>  
> +	hdmi->cec_notifier = cec_notifier_get(dev);
> +	if (!hdmi->cec_notifier) {
> +		ret = -ENOMEM;
> +		goto err_iahb;
> +	}
> +
>  	/*
>  	 * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
>  	 * N and cts values before enabling phy
> @@ -2452,6 +2467,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
>  		hdmi->ddc = NULL;
>  	}
>  
> +	if (hdmi->cec_notifier)
> +		cec_notifier_put(hdmi->cec_notifier);
> +
>  	clk_disable_unprepare(hdmi->iahb_clk);
>  err_isfr:
>  	clk_disable_unprepare(hdmi->isfr_clk);

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Neil Armstrong]

On 07/06/2017 12:43 PM, Jose Abreu wrote:
> Hi Neil,
> 
> 
> On 06-07-2017 11:33, Neil Armstrong wrote:
>> From: Russell King <rmk+kernel@armlinux.org.uk>
>>
>> Add CEC notifier support to the HDMI bridge driver, so that the CEC
>> part of the IP can receive its physical address.
>>
>> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
>> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> Hi Archit, Hans,
>>
>> This is repost of Russell's patch from his dw-hdmi CEC patchset.
>>
>> Since his CEC implementation will be integrated in the bridge driver,
>> this notifier patch won't be re-posted.
>>
>> But the Amlogic Platform needs a notifier since it uses a standalone CEC
>> controller.
>>
>> Thanks,
>> Neil
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index ead1124..9c03846 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -33,6 +33,8 @@
>>  #include <uapi/linux/media-bus-format.h>
>>  #include <uapi/linux/videodev2.h>
>>  
>> +#include <media/cec-notifier.h>
> 
> Don't you also have to "select CEC_NOTIFIER" in Kconfig? Or is it
> not needed anymore with the recent Kconfig changes of CEC?

Hi Jose,

Seems no, since the cec functions are protected with :
#if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)

and replaced with dummy inline functions.

Neil

> 
> Best regards,
> Jose Miguel Abreu
> 
>> +
>>  #include "dw-hdmi.h"
>>  #include "dw-hdmi-audio.h"
>>  
>> @@ -175,6 +177,8 @@ struct dw_hdmi {
>>  	struct regmap *regm;
>>  	void (*enable_audio)(struct dw_hdmi *hdmi);
>>  	void (*disable_audio)(struct dw_hdmi *hdmi);
>> +
>> +	struct cec_notifier *cec_notifier;
>>  };
>>  
>>  #define HDMI_IH_PHY_STAT0_RX_SENSE \
>> @@ -1896,6 +1900,7 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>>  		hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
>>  		hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
>>  		drm_mode_connector_update_edid_property(connector, edid);
>> +		cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier, edid);
>>  		ret = drm_add_edid_modes(connector, edid);
>>  		/* Store the ELD */
>>  		drm_edid_to_eld(connector, edid);
>> @@ -2077,6 +2082,10 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
>>  		dw_hdmi_update_phy_mask(hdmi);
>>  	}
>>  	mutex_unlock(&hdmi->mutex);
>> +
>> +	if (!rx_sense && !hpd)
>> +		cec_notifier_set_phys_addr(hdmi->cec_notifier,
>> +					   CEC_PHYS_ADDR_INVALID);
>>  }
>>  
>>  void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
>> @@ -2376,6 +2385,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
>>  	if (ret)
>>  		goto err_iahb;
>>  
>> +	hdmi->cec_notifier = cec_notifier_get(dev);
>> +	if (!hdmi->cec_notifier) {
>> +		ret = -ENOMEM;
>> +		goto err_iahb;
>> +	}
>> +
>>  	/*
>>  	 * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
>>  	 * N and cts values before enabling phy
>> @@ -2452,6 +2467,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
>>  		hdmi->ddc = NULL;
>>  	}
>>  
>> +	if (hdmi->cec_notifier)
>> +		cec_notifier_put(hdmi->cec_notifier);
>> +
>>  	clk_disable_unprepare(hdmi->iahb_clk);
>>  err_isfr:
>>  	clk_disable_unprepare(hdmi->isfr_clk);
> 

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Russell King - ARM Linux]

On Thu, Jul 06, 2017 at 12:33:06PM +0200, Neil Armstrong wrote:
> From: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Add CEC notifier support to the HDMI bridge driver, so that the CEC
> part of the IP can receive its physical address.
> 
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> Hi Archit, Hans,
> 
> This is repost of Russell's patch from his dw-hdmi CEC patchset.
> 
> Since his CEC implementation will be integrated in the bridge driver,
> this notifier patch won't be re-posted.
> 
> But the Amlogic Platform needs a notifier since it uses a standalone CEC
> controller.

Without this, the dw-hdmi CEC support will be totally useless anyway,
and it's pointless to merge it without this patch.

-- 
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.

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Neil Armstrong]

On 07/06/2017 01:05 PM, Russell King - ARM Linux wrote:
> On Thu, Jul 06, 2017 at 12:33:06PM +0200, Neil Armstrong wrote:
>> From: Russell King <rmk+kernel@armlinux.org.uk>
>>
>> Add CEC notifier support to the HDMI bridge driver, so that the CEC
>> part of the IP can receive its physical address.
>>
>> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
>> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> Hi Archit, Hans,
>>
>> This is repost of Russell's patch from his dw-hdmi CEC patchset.
>>
>> Since his CEC implementation will be integrated in the bridge driver,
>> this notifier patch won't be re-posted.
>>
>> But the Amlogic Platform needs a notifier since it uses a standalone CEC
>> controller.
> 
> Without this, the dw-hdmi CEC support will be totally useless anyway,
> and it's pointless to merge it without this patch.
> 

Hi Russell,

While following discussion on your last patchset, it seems you agreed with Hans
to no more rely on the cec-notifier but directly call the dw-hdmi-cec functions.

Anyway, if this patch is merged separately and you still depend on it, you could
still rebase on it when it appears on drm-misc-next...

Neil

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Russell King - ARM Linux]

On Thu, Jul 06, 2017 at 01:29:54PM +0200, Neil Armstrong wrote:
> On 07/06/2017 01:05 PM, Russell King - ARM Linux wrote:
> > On Thu, Jul 06, 2017 at 12:33:06PM +0200, Neil Armstrong wrote:
> >> From: Russell King <rmk+kernel@armlinux.org.uk>
> >>
> >> Add CEC notifier support to the HDMI bridge driver, so that the CEC
> >> part of the IP can receive its physical address.
> >>
> >> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> >> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >> ---
> >>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >>
> >> Hi Archit, Hans,
> >>
> >> This is repost of Russell's patch from his dw-hdmi CEC patchset.
> >>
> >> Since his CEC implementation will be integrated in the bridge driver,
> >> this notifier patch won't be re-posted.
> >>
> >> But the Amlogic Platform needs a notifier since it uses a standalone CEC
> >> controller.
> > 
> > Without this, the dw-hdmi CEC support will be totally useless anyway,
> > and it's pointless to merge it without this patch.
> > 
> 
> Hi Russell,
> 
> While following discussion on your last patchset, it seems you agreed with Hans
> to no more rely on the cec-notifier but directly call the dw-hdmi-cec functions.

Incorrect.  I think you're looking at the discussion in "thread mode"
and assuming that messages are stored in date order...

There was a discussion about removing the cec-notifier which happened
on June 1st/2nd, based off the covering email.

However, there was a later discussion on June 9th (sparked by your
requirement) which changed the resolution from "lets remove the
notifier" to "we need to keep the notifier".  This was based on
patch 2/4.

I never posted an updated patch set, because of the dependencies, but
Hans decided on June 12th to do I-don't-know-what with the patches I
sent, resulting in what we have queued up today.  This brought the
sub-thread containing the cec-notifier removal discussion to be _after_
the June 9th discussion, which is probably what's causing some
confusion here.

However, I can assure you that the resolution of the discussion with
the cec-notifier was that it should remain in place - this is from
the last few messages in the discussion:

Hans wrote on 9th June:
| You wrote on 9th June:
| > It won't since the Meson platform needs it...
|
| Ah, I wasn't aware of that when I wrote my original comments. In that case
| we do need the notifier. Which is fine, as long as the reason for that is
| documented.

>From what you're saying, it sounds like the CEC dw-hdmi-cec.c still
relies on the notifier, but the patch which adds the CEC notifier
to dw-hdmi.c was omited from what Hans did, which will result in the
whole thing being a total waste of time.

> Anyway, if this patch is merged separately and you still depend on it,
> you could still rebase on it when it appears on drm-misc-next...

Well, from what I can see in 4.12, the cec-notifier stuff is rather
broken (tda998x has stopped working as its stuck with a physical
address of f.f.f.f) so I think the whole thing is rather moot right
now.  I don't yet know what's going on with that, other than the
notifier stuff seems to not be working, despite being enabled in
the .config.

-- 
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.

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Neil Armstrong]

On 07/06/2017 01:45 PM, Russell King - ARM Linux wrote:
> On Thu, Jul 06, 2017 at 01:29:54PM +0200, Neil Armstrong wrote:
>> On 07/06/2017 01:05 PM, Russell King - ARM Linux wrote:
>>> On Thu, Jul 06, 2017 at 12:33:06PM +0200, Neil Armstrong wrote:
>>>> From: Russell King <rmk+kernel@armlinux.org.uk>
>>>>
>>>> Add CEC notifier support to the HDMI bridge driver, so that the CEC
>>>> part of the IP can receive its physical address.
>>>>
>>>> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++
>>>>  1 file changed, 18 insertions(+)
>>>>
>>>> Hi Archit, Hans,
>>>>
>>>> This is repost of Russell's patch from his dw-hdmi CEC patchset.
>>>>
>>>> Since his CEC implementation will be integrated in the bridge driver,
>>>> this notifier patch won't be re-posted.
>>>>
>>>> But the Amlogic Platform needs a notifier since it uses a standalone CEC
>>>> controller.
>>>
>>> Without this, the dw-hdmi CEC support will be totally useless anyway,
>>> and it's pointless to merge it without this patch.
>>>
>>
>> Hi Russell,
>>
>> While following discussion on your last patchset, it seems you agreed with Hans
>> to no more rely on the cec-notifier but directly call the dw-hdmi-cec functions.
> 
> Incorrect.  I think you're looking at the discussion in "thread mode"
> and assuming that messages are stored in date order...
> 
> There was a discussion about removing the cec-notifier which happened
> on June 1st/2nd, based off the covering email.
> 
> However, there was a later discussion on June 9th (sparked by your
> requirement) which changed the resolution from "lets remove the
> notifier" to "we need to keep the notifier".  This was based on
> patch 2/4.
> 
> I never posted an updated patch set, because of the dependencies, but
> Hans decided on June 12th to do I-don't-know-what with the patches I
> sent, resulting in what we have queued up today.  This brought the
> sub-thread containing the cec-notifier removal discussion to be _after_
> the June 9th discussion, which is probably what's causing some
> confusion here.
> 
> However, I can assure you that the resolution of the discussion with
> the cec-notifier was that it should remain in place - this is from
> the last few messages in the discussion:
> 
> Hans wrote on 9th June:
> | You wrote on 9th June:
> | > It won't since the Meson platform needs it...
> |
> | Ah, I wasn't aware of that when I wrote my original comments. In that case
> | we do need the notifier. Which is fine, as long as the reason for that is
> | documented.
> 
> From what you're saying, it sounds like the CEC dw-hdmi-cec.c still
> relies on the notifier, but the patch which adds the CEC notifier
> to dw-hdmi.c was omited from what Hans did, which will result in the
> whole thing being a total waste of time.
> 
>> Anyway, if this patch is merged separately and you still depend on it,
>> you could still rebase on it when it appears on drm-misc-next...
> 
> Well, from what I can see in 4.12, the cec-notifier stuff is rather
> broken (tda998x has stopped working as its stuck with a physical
> address of f.f.f.f) so I think the whole thing is rather moot right
> now.  I don't yet know what's going on with that, other than the
> notifier stuff seems to not be working, despite being enabled in
> the .config.
> 

Indeed, I missed some parts of the thread, sorry for the confusion.

Anyway, is it a showstopper to have this patch merged separately ?

If you prefer re-posting a serie with this patch inside, no problem,
but the Amlogic platform still needs this patch to have CEC working.

Maybe it's because of fixes introduced in 4.13, but using next-20170706
and this patch, make things works perfectly.

Neil

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Russell King - ARM Linux]

On Thu, Jul 06, 2017 at 01:55:58PM +0200, Neil Armstrong wrote:
> On 07/06/2017 01:45 PM, Russell King - ARM Linux wrote:
> > Well, from what I can see in 4.12, the cec-notifier stuff is rather
> > broken (tda998x has stopped working as its stuck with a physical
> > address of f.f.f.f) so I think the whole thing is rather moot right
> > now.  I don't yet know what's going on with that, other than the
> > notifier stuff seems to not be working, despite being enabled in
> > the .config.
> > 
> 
> Indeed, I missed some parts of the thread, sorry for the confusion.
> 
> Anyway, is it a showstopper to have this patch merged separately ?

It shouldn't be provided it makes _this_ merge window or the -rc
following, but it doesn't really comprise a "fix" in that the feature
never worked in the previous kernel (and we are supposed to have the
rule that features are merged prior to the merge window.)

It's up to the maintainers to decide what to do at this point.

> If you prefer re-posting a serie with this patch inside, no problem,
> but the Amlogic platform still needs this patch to have CEC working.

I suspect (because I've not seen what has been queued to be merged yet)
that even the dw-hdmi "native" side will be similarly broken - unless
Hans merged some other changes to make it work.

So, I suggest we all sit tight and wait until what was merged appears
in Linus' tree.

-- 
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.

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Russell King - ARM Linux]

On Thu, Jul 06, 2017 at 12:45:55PM +0100, Russell King - ARM Linux wrote:
> Well, from what I can see in 4.12, the cec-notifier stuff is rather
> broken (tda998x has stopped working as its stuck with a physical
> address of f.f.f.f) so I think the whole thing is rather moot right
> now.  I don't yet know what's going on with that, other than the
> notifier stuff seems to not be working, despite being enabled in
> the .config.

The problem there appears to be the changes that were made with the
way the config works - which IMHO are totally broken.

Let's take this scenario:

- You have a HDMI bridge, and you build that into the kernel, because you
  want the display to come up early.
- You have a CEC driver, which you build as a module.

If the HDMI bridge driver selects CEC_NOTIFIER and the CEC driver selects
both CEC_NOTIFIER and CEC_CORE, you end up with CEC_NOTIFIER=y and
CEC_CORE=m.

We now come to this:

#if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)

The definition of IS_REACHABLE() is that it is false if the config symbol
is selected as a module.  So, in this case, we end up compiling out all
the CEC notifier functions from the HDMI bridge, and building them into
the CEC driver.

The CEC notifier also gets built as a module, meaning that there's no way
for the built-in HDMI bridge could ever call the notifier.

The overall result of this is that such a configuration completely breaks
such a setup - a setup that worked fine before the CEC Kconfig changes.

This isn't limited to tda998x - I'd expect the same to be true of dw-hdmi.

-- 
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.

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Russell King - ARM Linux]

On Thu, Jul 06, 2017 at 12:56:43PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 06, 2017 at 12:45:55PM +0100, Russell King - ARM Linux wrote:
> > Well, from what I can see in 4.12, the cec-notifier stuff is rather
> > broken (tda998x has stopped working as its stuck with a physical
> > address of f.f.f.f) so I think the whole thing is rather moot right
> > now.  I don't yet know what's going on with that, other than the
> > notifier stuff seems to not be working, despite being enabled in
> > the .config.
> 
> The problem there appears to be the changes that were made with the
> way the config works - which IMHO are totally broken.
> 
> Let's take this scenario:
> 
> - You have a HDMI bridge, and you build that into the kernel, because you
>   want the display to come up early.
> - You have a CEC driver, which you build as a module.
> 
> If the HDMI bridge driver selects CEC_NOTIFIER and the CEC driver selects
> both CEC_NOTIFIER and CEC_CORE, you end up with CEC_NOTIFIER=y and
> CEC_CORE=m.
> 
> We now come to this:
> 
> #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
> 
> The definition of IS_REACHABLE() is that it is false if the config symbol
> is selected as a module.  So, in this case, we end up compiling out all
> the CEC notifier functions from the HDMI bridge, and building them into
> the CEC driver.
> 
> The CEC notifier also gets built as a module, meaning that there's no way
> for the built-in HDMI bridge could ever call the notifier.
> 
> The overall result of this is that such a configuration completely breaks
> such a setup - a setup that worked fine before the CEC Kconfig changes.
> 
> This isn't limited to tda998x - I'd expect the same to be true of dw-hdmi.

Fixing this so cec-notifier is built-in isn't sufficient, because we
also need the cec-edid parsing code as well, which is currently part
of cec-core, and that function gets stubbed out if cec-core is not
built-in...

The patch below works for me.

 drivers/media/Makefile       | 2 +-
 drivers/media/cec/Makefile   | 6 +++---
 drivers/media/cec/cec-core.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index 044503aa8801..0c02fbe4b9c7 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -24,7 +24,7 @@ obj-$(CONFIG_DVB_CORE)  += dvb-core/
 # There are both core and drivers at RC subtree - merge before drivers
 obj-y += rc/
 
-obj-$(CONFIG_CEC_CORE) += cec/
+obj-y += cec/
 
 #
 # Finally, merge the drivers that require the core
diff --git a/drivers/media/cec/Makefile b/drivers/media/cec/Makefile
index eaf408e64669..58394b77a328 100644
--- a/drivers/media/cec/Makefile
+++ b/drivers/media/cec/Makefile
@@ -1,7 +1,7 @@
 cec-objs := cec-core.o cec-adap.o cec-api.o cec-edid.o
 
-ifeq ($(CONFIG_CEC_NOTIFIER),y)
-  cec-objs += cec-notifier.o
-endif
+obj-$(CONFIG_CEC_NOTIFIER) += cec-notifier.o cec-edid.o
+# Remove cec-edid from cec-core if the notifier is enabled
+cec-objs := $(filter-out $(obj-y) $(obj-m), $(cec-objs))
 
 obj-$(CONFIG_CEC_CORE) += cec.o
diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 2f87748ba4fc..bce26b94c348 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -187,7 +187,7 @@ static void cec_devnode_unregister(struct cec_devnode *devnode)
 	put_device(&devnode->dev);
 }
 
-#ifdef CONFIG_CEC_NOTIFIER
+#if IS_ENABLED(CONFIG_CEC_NOTIFIER)
 static void cec_cec_notify(struct cec_adapter *adap, u16 pa)
 {
 	cec_s_phys_addr(adap, pa, false);


-- 
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.

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Russell King - ARM Linux]

On Thu, Jul 06, 2017 at 02:38:41PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 06, 2017 at 12:56:43PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Jul 06, 2017 at 12:45:55PM +0100, Russell King - ARM Linux wrote:
> > > Well, from what I can see in 4.12, the cec-notifier stuff is rather
> > > broken (tda998x has stopped working as its stuck with a physical
> > > address of f.f.f.f) so I think the whole thing is rather moot right
> > > now.  I don't yet know what's going on with that, other than the
> > > notifier stuff seems to not be working, despite being enabled in
> > > the .config.
> > 
> > The problem there appears to be the changes that were made with the
> > way the config works - which IMHO are totally broken.
> > 
> > Let's take this scenario:
> > 
> > - You have a HDMI bridge, and you build that into the kernel, because you
> >   want the display to come up early.
> > - You have a CEC driver, which you build as a module.
> > 
> > If the HDMI bridge driver selects CEC_NOTIFIER and the CEC driver selects
> > both CEC_NOTIFIER and CEC_CORE, you end up with CEC_NOTIFIER=y and
> > CEC_CORE=m.
> > 
> > We now come to this:
> > 
> > #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
> > 
> > The definition of IS_REACHABLE() is that it is false if the config symbol
> > is selected as a module.  So, in this case, we end up compiling out all
> > the CEC notifier functions from the HDMI bridge, and building them into
> > the CEC driver.
> > 
> > The CEC notifier also gets built as a module, meaning that there's no way
> > for the built-in HDMI bridge could ever call the notifier.
> > 
> > The overall result of this is that such a configuration completely breaks
> > such a setup - a setup that worked fine before the CEC Kconfig changes.
> > 
> > This isn't limited to tda998x - I'd expect the same to be true of dw-hdmi.
> 
> Fixing this so cec-notifier is built-in isn't sufficient, because we
> also need the cec-edid parsing code as well, which is currently part
> of cec-core, and that function gets stubbed out if cec-core is not
> built-in...
> 
> The patch below works for me.

I missed the include/media changes...

 drivers/media/Makefile       |  2 +-
 drivers/media/cec/Makefile   |  6 ++---
 drivers/media/cec/cec-core.c |  2 +-
 include/media/cec-notifier.h |  2 +-
 include/media/cec.h          | 54 ++++++++++++++++++++++++--------------------
 5 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index 044503aa8801..0c02fbe4b9c7 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -24,7 +24,7 @@ obj-$(CONFIG_DVB_CORE)  += dvb-core/
 # There are both core and drivers at RC subtree - merge before drivers
 obj-y += rc/
 
-obj-$(CONFIG_CEC_CORE) += cec/
+obj-y += cec/
 
 #
 # Finally, merge the drivers that require the core
diff --git a/drivers/media/cec/Makefile b/drivers/media/cec/Makefile
index eaf408e64669..58394b77a328 100644
--- a/drivers/media/cec/Makefile
+++ b/drivers/media/cec/Makefile
@@ -1,7 +1,7 @@
 cec-objs := cec-core.o cec-adap.o cec-api.o cec-edid.o
 
-ifeq ($(CONFIG_CEC_NOTIFIER),y)
-  cec-objs += cec-notifier.o
-endif
+obj-$(CONFIG_CEC_NOTIFIER) += cec-notifier.o cec-edid.o
+
+cec-objs := $(filter-out $(obj-y) $(obj-m), $(cec-objs))
 
 obj-$(CONFIG_CEC_CORE) += cec.o
diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 2f87748ba4fc..bce26b94c348 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -187,7 +187,7 @@ static void cec_devnode_unregister(struct cec_devnode *devnode)
 	put_device(&devnode->dev);
 }
 
-#ifdef CONFIG_CEC_NOTIFIER
+#if IS_ENABLED(CONFIG_CEC_NOTIFIER)
 static void cec_cec_notify(struct cec_adapter *adap, u16 pa)
 {
 	cec_s_phys_addr(adap, pa, false);
diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
index 298f996969df..83bdc221d0a0 100644
--- a/include/media/cec-notifier.h
+++ b/include/media/cec-notifier.h
@@ -29,7 +29,7 @@ struct edid;
 struct cec_adapter;
 struct cec_notifier;
 
-#if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
+#if IS_ENABLED(CONFIG_CEC_NOTIFIER)
 
 /**
  * cec_notifier_get - find or create a new cec_notifier for the given device.
diff --git a/include/media/cec.h b/include/media/cec.h
index 201f060978da..039aad98462d 100644
--- a/include/media/cec.h
+++ b/include/media/cec.h
@@ -225,6 +225,36 @@ void cec_transmit_done(struct cec_adapter *adap, u8 status, u8 arb_lost_cnt,
 		       u8 nack_cnt, u8 low_drive_cnt, u8 error_cnt);
 void cec_received_msg(struct cec_adapter *adap, struct cec_msg *msg);
 
+#if IS_ENABLED(CONFIG_CEC_NOTIFIER)
+void cec_register_cec_notifier(struct cec_adapter *adap,
+			       struct cec_notifier *notifier);
+#endif
+
+#else
+
+static inline int cec_register_adapter(struct cec_adapter *adap,
+				       struct device *parent)
+{
+	return 0;
+}
+
+static inline void cec_unregister_adapter(struct cec_adapter *adap)
+{
+}
+
+static inline void cec_delete_adapter(struct cec_adapter *adap)
+{
+}
+
+static inline void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr,
+				   bool block)
+{
+}
+
+#endif
+
+#if IS_REACHABLE(CONFIG_CEC_CORE) || IS_REACHABLE(CONFIG_CEC_NOTIFIER)
+
 /**
  * cec_get_edid_phys_addr() - find and return the physical address
  *
@@ -300,32 +330,8 @@ u16 cec_phys_addr_for_input(u16 phys_addr, u8 input);
  */
 int cec_phys_addr_validate(u16 phys_addr, u16 *parent, u16 *port);
 
-#ifdef CONFIG_CEC_NOTIFIER
-void cec_register_cec_notifier(struct cec_adapter *adap,
-			       struct cec_notifier *notifier);
-#endif
-
 #else
 
-static inline int cec_register_adapter(struct cec_adapter *adap,
-				       struct device *parent)
-{
-	return 0;
-}
-
-static inline void cec_unregister_adapter(struct cec_adapter *adap)
-{
-}
-
-static inline void cec_delete_adapter(struct cec_adapter *adap)
-{
-}
-
-static inline void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr,
-				   bool block)
-{
-}
-
 static inline u16 cec_get_edid_phys_addr(const u8 *edid, unsigned int size,
 					 unsigned int *offset)
 {


-- 
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.

[PATCH] drm/bridge: dw_hdmi: add cec notifier support

[Neil Armstrong]

From: Russell King <rmk+kernel@armlinux.org.uk>

Add CEC notifier support to the HDMI bridge driver, so that the CEC
part of the IP can receive its physical address.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
[narmstrong: added kconfig tweak to get the right modules dependencies]
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/bridge/synopsys/Kconfig   |  1 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

Hi Archit, Hans,

This is second repost of Russell's patch from his dw-hdmi CEC patchset.

Latest russell's thread :
https://lkml.kernel.org/r/E1dFi3E-0004lh-Pu at rmk-PC.armlinux.org.uk

Previous repost :
https://lkml.kernel.org/r/1499337186-24429-1-git-send-email-narmstrong at baylibre.com

It includes the Kconfig tweak to ensure the correct modules dependencies.

Neil

diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig
index 53e78d0..351db00 100644
--- a/drivers/gpu/drm/bridge/synopsys/Kconfig
+++ b/drivers/gpu/drm/bridge/synopsys/Kconfig
@@ -2,6 +2,7 @@ config DRM_DW_HDMI
 	tristate
 	select DRM_KMS_HELPER
 	select REGMAP_MMIO
+	select CEC_CORE if CEC_NOTIFIER
 
 config DRM_DW_HDMI_AHB_AUDIO
 	tristate "Synopsys Designware AHB Audio interface"
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index ead1124..9c03846 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -33,6 +33,8 @@
 #include <uapi/linux/media-bus-format.h>
 #include <uapi/linux/videodev2.h>
 
+#include <media/cec-notifier.h>
+
 #include "dw-hdmi.h"
 #include "dw-hdmi-audio.h"
 
@@ -175,6 +177,8 @@ struct dw_hdmi {
 	struct regmap *regm;
 	void (*enable_audio)(struct dw_hdmi *hdmi);
 	void (*disable_audio)(struct dw_hdmi *hdmi);
+
+	struct cec_notifier *cec_notifier;
 };
 
 #define HDMI_IH_PHY_STAT0_RX_SENSE \
@@ -1896,6 +1900,7 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 		hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
 		hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
 		drm_mode_connector_update_edid_property(connector, edid);
+		cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier, edid);
 		ret = drm_add_edid_modes(connector, edid);
 		/* Store the ELD */
 		drm_edid_to_eld(connector, edid);
@@ -2077,6 +2082,10 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense)
 		dw_hdmi_update_phy_mask(hdmi);
 	}
 	mutex_unlock(&hdmi->mutex);
+
+	if (!rx_sense && !hpd)
+		cec_notifier_set_phys_addr(hdmi->cec_notifier,
+					   CEC_PHYS_ADDR_INVALID);
 }
 
 void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense)
@@ -2376,6 +2385,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
 	if (ret)
 		goto err_iahb;
 
+	hdmi->cec_notifier = cec_notifier_get(dev);
+	if (!hdmi->cec_notifier) {
+		ret = -ENOMEM;
+		goto err_iahb;
+	}
+
 	/*
 	 * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
 	 * N and cts values before enabling phy
@@ -2452,6 +2467,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
 		hdmi->ddc = NULL;
 	}
 
+	if (hdmi->cec_notifier)
+		cec_notifier_put(hdmi->cec_notifier);
+
 	clk_disable_unprepare(hdmi->iahb_clk);
 err_isfr:
 	clk_disable_unprepare(hdmi->isfr_clk);
-- 
1.9.1