[PATCH v4] drm/bridge/sii8620: add remote control support

[PATCH v4] drm/bridge/sii8620: add remote control support

[Maciej Purski]

MHL specification defines Remote Control Protocol(RCP) to
send input events between MHL devices.
The driver now recognizes RCP messages and reacts to them
by reporting key events to input subsystem, allowing
a user to control a device using TV remote control.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---

Changes in v2:
- use RC subsystem (including CEC keymap)
- RC device initialized in attach drm_bridge callback and
  removed in detach callback. This is necessary, because RC_CORE,
  which is needed during rc_dev init, is loaded after sii8620.
  DRM bridge is binded later which solves the problem.
- add RC_CORE dependency

Changes in v3:
- fix error handling in init_rcp and in attach callback

Changes in v4:
- usage of rc-core API compatible with upcoming changes
- fix error handling in init_rcp
- fix commit message
---
 drivers/gpu/drm/bridge/Kconfig       |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c | 96 ++++++++++++++++++++++++++++++++++--
 include/drm/bridge/mhl.h             |  4 ++
 3 files changed, 96 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index adf9ae0..6ef901c 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -71,7 +71,7 @@ config DRM_PARADE_PS8622
 
 config DRM_SIL_SII8620
 	tristate "Silicon Image SII8620 HDMI/MHL bridge"
-	depends on OF
+	depends on OF && RC_CORE
 	select DRM_KMS_HELPER
 	help
 	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 2d51a22..ecb26c4 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -28,6 +28,8 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
+#include <media/rc-core.h>
+
 #include "sil-sii8620.h"
 
 #define SII8620_BURST_BUF_LEN 288
@@ -58,6 +60,7 @@ enum sii8620_mt_state {
 struct sii8620 {
 	struct drm_bridge bridge;
 	struct device *dev;
+	struct rc_dev *rc_dev;
 	struct clk *clk_xtal;
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_int;
@@ -431,6 +434,16 @@ static void sii8620_mt_rap(struct sii8620 *ctx, u8 code)
 	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RAP, code);
 }
 
+static void sii8620_mt_rcpk(struct sii8620 *ctx, u8 code)
+{
+	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPK, code);
+}
+
+static void sii8620_mt_rcpe(struct sii8620 *ctx, u8 code)
+{
+	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPE, code);
+}
+
 static void sii8620_mt_read_devcap_send(struct sii8620 *ctx,
 					struct sii8620_mt_msg *msg)
 {
@@ -1753,6 +1766,25 @@ static void sii8620_send_features(struct sii8620 *ctx)
 	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
 }
 
+static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
+{
+	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
+
+	scancode &= MHL_RCP_KEY_ID_MASK;
+
+	if (!ctx->rc_dev) {
+		dev_dbg(ctx->dev, "RCP input device not initialized\n");
+		return false;
+	}
+
+	if (pressed)
+		rc_keydown(ctx->rc_dev, RC_PROTO_CEC, scancode, 0);
+	else
+		rc_keyup(ctx->rc_dev);
+
+	return true;
+}
+
 static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
 {
 	u8 ints[MHL_INT_SIZE];
@@ -1804,19 +1836,25 @@ static void sii8620_msc_mt_done(struct sii8620 *ctx)
 
 static void sii8620_msc_mr_msc_msg(struct sii8620 *ctx)
 {
-	struct sii8620_mt_msg *msg = sii8620_msc_msg_first(ctx);
+	struct sii8620_mt_msg *msg;
 	u8 buf[2];
 
-	if (!msg)
-		return;
-
 	sii8620_read_buf(ctx, REG_MSC_MR_MSC_MSG_RCVD_1ST_DATA, buf, 2);
 
 	switch (buf[0]) {
 	case MHL_MSC_MSG_RAPK:
+		msg = sii8620_msc_msg_first(ctx);
+		if (!msg)
+			return;
 		msg->ret = buf[1];
 		ctx->mt_state = MT_STATE_DONE;
 		break;
+	case MHL_MSC_MSG_RCP:
+		if (!sii8620_rcp_consume(ctx, buf[1]))
+			sii8620_mt_rcpe(ctx,
+					MHL_RCPE_STATUS_INEFFECTIVE_KEY_CODE);
+		sii8620_mt_rcpk(ctx, buf[1]);
+		break;
 	default:
 		dev_err(ctx->dev, "%s message type %d,%d not supported",
 			__func__, buf[0], buf[1]);
@@ -2102,11 +2140,57 @@ static void sii8620_cable_in(struct sii8620 *ctx)
 	enable_irq(to_i2c_client(ctx->dev)->irq);
 }
 
+static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
+{
+	struct rc_dev *rc_dev;
+	int ret;
+
+	rc_dev = rc_allocate_device(RC_DRIVER_SCANCODE);
+	if (!rc_dev) {
+		dev_err(ctx->dev, "Failed to allocate RC device\n");
+		ctx->error = -ENOMEM;
+		return;
+	}
+
+	rc_dev->input_phys = "sii8620/input0";
+	rc_dev->input_id.bustype = BUS_VIRTUAL;
+	rc_dev->map_name = RC_MAP_CEC;
+	rc_dev->allowed_protocols = RC_PROTO_BIT_CEC;
+	rc_dev->driver_name = "sii8620";
+	rc_dev->device_name = "sii8620";
+
+	ret = rc_register_device(rc_dev);
+
+	if (ret) {
+		dev_err(ctx->dev, "Failed to register RC device\n");
+		ctx->error = ret;
+		rc_free_device(ctx->rc_dev);
+		return;
+	}
+	ctx->rc_dev = rc_dev;
+}
+
 static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct sii8620, bridge);
 }
 
+static int sii8620_attach(struct drm_bridge *bridge)
+{
+	struct sii8620 *ctx = bridge_to_sii8620(bridge);
+
+	sii8620_init_rcp_input_dev(ctx);
+
+	return sii8620_clear_error(ctx);
+}
+
+static void sii8620_detach(struct drm_bridge *bridge)
+{
+	struct sii8620 *ctx = bridge_to_sii8620(bridge);
+
+	rc_unregister_device(ctx->rc_dev);
+}
+
 static bool sii8620_mode_fixup(struct drm_bridge *bridge,
 			       const struct drm_display_mode *mode,
 			       struct drm_display_mode *adjusted_mode)
@@ -2151,6 +2235,8 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
 }
 
 static const struct drm_bridge_funcs sii8620_bridge_funcs = {
+	.attach = sii8620_attach,
+	.detach = sii8620_detach,
 	.mode_fixup = sii8620_mode_fixup,
 };
 
@@ -2217,8 +2303,8 @@ static int sii8620_remove(struct i2c_client *client)
 	struct sii8620 *ctx = i2c_get_clientdata(client);
 
 	disable_irq(to_i2c_client(ctx->dev)->irq);
-	drm_bridge_remove(&ctx->bridge);
 	sii8620_hw_off(ctx);
+	drm_bridge_remove(&ctx->bridge);
 
 	return 0;
 }
diff --git a/include/drm/bridge/mhl.h b/include/drm/bridge/mhl.h
index fbdfc8d..96a5e0f 100644
--- a/include/drm/bridge/mhl.h
+++ b/include/drm/bridge/mhl.h
@@ -262,6 +262,10 @@ enum {
 #define MHL_RAPK_UNSUPPORTED	0x02	/* Rcvd RAP action code not supported */
 #define MHL_RAPK_BUSY		0x03	/* Responder too busy to respond */
 
+/* Bit masks for RCP messages */
+#define MHL_RCP_KEY_RELEASED_MASK	0x80
+#define MHL_RCP_KEY_ID_MASK		0x7F
+
 /*
  * Error status codes for RCPE messages
  */
-- 
2.7.4

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Sean Young]

On Thu, Aug 24, 2017 at 10:58:07AM +0200, Maciej Purski wrote:
> MHL specification defines Remote Control Protocol(RCP) to
> send input events between MHL devices.
> The driver now recognizes RCP messages and reacts to them
> by reporting key events to input subsystem, allowing
> a user to control a device using TV remote control.
> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
> 
> Changes in v2:
> - use RC subsystem (including CEC keymap)
> - RC device initialized in attach drm_bridge callback and
>   removed in detach callback. This is necessary, because RC_CORE,
>   which is needed during rc_dev init, is loaded after sii8620.
>   DRM bridge is binded later which solves the problem.
> - add RC_CORE dependency
> 
> Changes in v3:
> - fix error handling in init_rcp and in attach callback
> 
> Changes in v4:
> - usage of rc-core API compatible with upcoming changes
> - fix error handling in init_rcp
> - fix commit message

Looks good.

Acked-by: Sean Young <sean@mess.org>

> ---
>  drivers/gpu/drm/bridge/Kconfig       |  2 +-
>  drivers/gpu/drm/bridge/sil-sii8620.c | 96 ++++++++++++++++++++++++++++++++++--
>  include/drm/bridge/mhl.h             |  4 ++
>  3 files changed, 96 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index adf9ae0..6ef901c 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -71,7 +71,7 @@ config DRM_PARADE_PS8622
>  
>  config DRM_SIL_SII8620
>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
> -	depends on OF
> +	depends on OF && RC_CORE
>  	select DRM_KMS_HELPER
>  	help
>  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 2d51a22..ecb26c4 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -28,6 +28,8 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  
> +#include <media/rc-core.h>
> +
>  #include "sil-sii8620.h"
>  
>  #define SII8620_BURST_BUF_LEN 288
> @@ -58,6 +60,7 @@ enum sii8620_mt_state {
>  struct sii8620 {
>  	struct drm_bridge bridge;
>  	struct device *dev;
> +	struct rc_dev *rc_dev;
>  	struct clk *clk_xtal;
>  	struct gpio_desc *gpio_reset;
>  	struct gpio_desc *gpio_int;
> @@ -431,6 +434,16 @@ static void sii8620_mt_rap(struct sii8620 *ctx, u8 code)
>  	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RAP, code);
>  }
>  
> +static void sii8620_mt_rcpk(struct sii8620 *ctx, u8 code)
> +{
> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPK, code);
> +}
> +
> +static void sii8620_mt_rcpe(struct sii8620 *ctx, u8 code)
> +{
> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPE, code);
> +}
> +
>  static void sii8620_mt_read_devcap_send(struct sii8620 *ctx,
>  					struct sii8620_mt_msg *msg)
>  {
> @@ -1753,6 +1766,25 @@ static void sii8620_send_features(struct sii8620 *ctx)
>  	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
>  }
>  
> +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
> +{
> +	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
> +
> +	scancode &= MHL_RCP_KEY_ID_MASK;
> +
> +	if (!ctx->rc_dev) {
> +		dev_dbg(ctx->dev, "RCP input device not initialized\n");
> +		return false;
> +	}
> +
> +	if (pressed)
> +		rc_keydown(ctx->rc_dev, RC_PROTO_CEC, scancode, 0);
> +	else
> +		rc_keyup(ctx->rc_dev);
> +
> +	return true;
> +}
> +
>  static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
>  {
>  	u8 ints[MHL_INT_SIZE];
> @@ -1804,19 +1836,25 @@ static void sii8620_msc_mt_done(struct sii8620 *ctx)
>  
>  static void sii8620_msc_mr_msc_msg(struct sii8620 *ctx)
>  {
> -	struct sii8620_mt_msg *msg = sii8620_msc_msg_first(ctx);
> +	struct sii8620_mt_msg *msg;
>  	u8 buf[2];
>  
> -	if (!msg)
> -		return;
> -
>  	sii8620_read_buf(ctx, REG_MSC_MR_MSC_MSG_RCVD_1ST_DATA, buf, 2);
>  
>  	switch (buf[0]) {
>  	case MHL_MSC_MSG_RAPK:
> +		msg = sii8620_msc_msg_first(ctx);
> +		if (!msg)
> +			return;
>  		msg->ret = buf[1];
>  		ctx->mt_state = MT_STATE_DONE;
>  		break;
> +	case MHL_MSC_MSG_RCP:
> +		if (!sii8620_rcp_consume(ctx, buf[1]))
> +			sii8620_mt_rcpe(ctx,
> +					MHL_RCPE_STATUS_INEFFECTIVE_KEY_CODE);
> +		sii8620_mt_rcpk(ctx, buf[1]);
> +		break;
>  	default:
>  		dev_err(ctx->dev, "%s message type %d,%d not supported",
>  			__func__, buf[0], buf[1]);
> @@ -2102,11 +2140,57 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>  	enable_irq(to_i2c_client(ctx->dev)->irq);
>  }
>  
> +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
> +{
> +	struct rc_dev *rc_dev;
> +	int ret;
> +
> +	rc_dev = rc_allocate_device(RC_DRIVER_SCANCODE);
> +	if (!rc_dev) {
> +		dev_err(ctx->dev, "Failed to allocate RC device\n");
> +		ctx->error = -ENOMEM;
> +		return;
> +	}
> +
> +	rc_dev->input_phys = "sii8620/input0";
> +	rc_dev->input_id.bustype = BUS_VIRTUAL;
> +	rc_dev->map_name = RC_MAP_CEC;
> +	rc_dev->allowed_protocols = RC_PROTO_BIT_CEC;
> +	rc_dev->driver_name = "sii8620";
> +	rc_dev->device_name = "sii8620";
> +
> +	ret = rc_register_device(rc_dev);
> +
> +	if (ret) {
> +		dev_err(ctx->dev, "Failed to register RC device\n");
> +		ctx->error = ret;
> +		rc_free_device(ctx->rc_dev);
> +		return;
> +	}
> +	ctx->rc_dev = rc_dev;
> +}
> +
>  static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sii8620, bridge);
>  }
>  
> +static int sii8620_attach(struct drm_bridge *bridge)
> +{
> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
> +
> +	sii8620_init_rcp_input_dev(ctx);
> +
> +	return sii8620_clear_error(ctx);
> +}
> +
> +static void sii8620_detach(struct drm_bridge *bridge)
> +{
> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
> +
> +	rc_unregister_device(ctx->rc_dev);
> +}
> +
>  static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>  			       const struct drm_display_mode *mode,
>  			       struct drm_display_mode *adjusted_mode)
> @@ -2151,6 +2235,8 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>  }
>  
>  static const struct drm_bridge_funcs sii8620_bridge_funcs = {
> +	.attach = sii8620_attach,
> +	.detach = sii8620_detach,
>  	.mode_fixup = sii8620_mode_fixup,
>  };
>  
> @@ -2217,8 +2303,8 @@ static int sii8620_remove(struct i2c_client *client)
>  	struct sii8620 *ctx = i2c_get_clientdata(client);
>  
>  	disable_irq(to_i2c_client(ctx->dev)->irq);
> -	drm_bridge_remove(&ctx->bridge);
>  	sii8620_hw_off(ctx);
> +	drm_bridge_remove(&ctx->bridge);
>  
>  	return 0;
>  }
> diff --git a/include/drm/bridge/mhl.h b/include/drm/bridge/mhl.h
> index fbdfc8d..96a5e0f 100644
> --- a/include/drm/bridge/mhl.h
> +++ b/include/drm/bridge/mhl.h
> @@ -262,6 +262,10 @@ enum {
>  #define MHL_RAPK_UNSUPPORTED	0x02	/* Rcvd RAP action code not supported */
>  #define MHL_RAPK_BUSY		0x03	/* Responder too busy to respond */
>  
> +/* Bit masks for RCP messages */
> +#define MHL_RCP_KEY_RELEASED_MASK	0x80
> +#define MHL_RCP_KEY_ID_MASK		0x7F
> +
>  /*
>   * Error status codes for RCPE messages
>   */
> -- 
> 2.7.4

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Mauro Carvalho Chehab]

Em Thu, 24 Aug 2017 10:58:07 +0200
Maciej Purski <m.purski@samsung.com> escreveu:

> MHL specification defines Remote Control Protocol(RCP) to
> send input events between MHL devices.
> The driver now recognizes RCP messages and reacts to them
> by reporting key events to input subsystem, allowing
> a user to control a device using TV remote control.
> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>

Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>


> ---
> 
> Changes in v2:
> - use RC subsystem (including CEC keymap)
> - RC device initialized in attach drm_bridge callback and
>   removed in detach callback. This is necessary, because RC_CORE,
>   which is needed during rc_dev init, is loaded after sii8620.
>   DRM bridge is binded later which solves the problem.
> - add RC_CORE dependency
> 
> Changes in v3:
> - fix error handling in init_rcp and in attach callback
> 
> Changes in v4:
> - usage of rc-core API compatible with upcoming changes
> - fix error handling in init_rcp
> - fix commit message
> ---
>  drivers/gpu/drm/bridge/Kconfig       |  2 +-
>  drivers/gpu/drm/bridge/sil-sii8620.c | 96 ++++++++++++++++++++++++++++++++++--
>  include/drm/bridge/mhl.h             |  4 ++
>  3 files changed, 96 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index adf9ae0..6ef901c 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -71,7 +71,7 @@ config DRM_PARADE_PS8622
>  
>  config DRM_SIL_SII8620
>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
> -	depends on OF
> +	depends on OF && RC_CORE
>  	select DRM_KMS_HELPER
>  	help
>  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 2d51a22..ecb26c4 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -28,6 +28,8 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  
> +#include <media/rc-core.h>
> +
>  #include "sil-sii8620.h"
>  
>  #define SII8620_BURST_BUF_LEN 288
> @@ -58,6 +60,7 @@ enum sii8620_mt_state {
>  struct sii8620 {
>  	struct drm_bridge bridge;
>  	struct device *dev;
> +	struct rc_dev *rc_dev;
>  	struct clk *clk_xtal;
>  	struct gpio_desc *gpio_reset;
>  	struct gpio_desc *gpio_int;
> @@ -431,6 +434,16 @@ static void sii8620_mt_rap(struct sii8620 *ctx, u8 code)
>  	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RAP, code);
>  }
>  
> +static void sii8620_mt_rcpk(struct sii8620 *ctx, u8 code)
> +{
> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPK, code);
> +}
> +
> +static void sii8620_mt_rcpe(struct sii8620 *ctx, u8 code)
> +{
> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPE, code);
> +}
> +
>  static void sii8620_mt_read_devcap_send(struct sii8620 *ctx,
>  					struct sii8620_mt_msg *msg)
>  {
> @@ -1753,6 +1766,25 @@ static void sii8620_send_features(struct sii8620 *ctx)
>  	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
>  }
>  
> +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
> +{
> +	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
> +
> +	scancode &= MHL_RCP_KEY_ID_MASK;
> +
> +	if (!ctx->rc_dev) {
> +		dev_dbg(ctx->dev, "RCP input device not initialized\n");
> +		return false;
> +	}
> +
> +	if (pressed)
> +		rc_keydown(ctx->rc_dev, RC_PROTO_CEC, scancode, 0);
> +	else
> +		rc_keyup(ctx->rc_dev);
> +
> +	return true;
> +}
> +
>  static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
>  {
>  	u8 ints[MHL_INT_SIZE];
> @@ -1804,19 +1836,25 @@ static void sii8620_msc_mt_done(struct sii8620 *ctx)
>  
>  static void sii8620_msc_mr_msc_msg(struct sii8620 *ctx)
>  {
> -	struct sii8620_mt_msg *msg = sii8620_msc_msg_first(ctx);
> +	struct sii8620_mt_msg *msg;
>  	u8 buf[2];
>  
> -	if (!msg)
> -		return;
> -
>  	sii8620_read_buf(ctx, REG_MSC_MR_MSC_MSG_RCVD_1ST_DATA, buf, 2);
>  
>  	switch (buf[0]) {
>  	case MHL_MSC_MSG_RAPK:
> +		msg = sii8620_msc_msg_first(ctx);
> +		if (!msg)
> +			return;
>  		msg->ret = buf[1];
>  		ctx->mt_state = MT_STATE_DONE;
>  		break;
> +	case MHL_MSC_MSG_RCP:
> +		if (!sii8620_rcp_consume(ctx, buf[1]))
> +			sii8620_mt_rcpe(ctx,
> +					MHL_RCPE_STATUS_INEFFECTIVE_KEY_CODE);
> +		sii8620_mt_rcpk(ctx, buf[1]);
> +		break;
>  	default:
>  		dev_err(ctx->dev, "%s message type %d,%d not supported",
>  			__func__, buf[0], buf[1]);
> @@ -2102,11 +2140,57 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>  	enable_irq(to_i2c_client(ctx->dev)->irq);
>  }
>  
> +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
> +{
> +	struct rc_dev *rc_dev;
> +	int ret;
> +
> +	rc_dev = rc_allocate_device(RC_DRIVER_SCANCODE);
> +	if (!rc_dev) {
> +		dev_err(ctx->dev, "Failed to allocate RC device\n");
> +		ctx->error = -ENOMEM;
> +		return;
> +	}
> +
> +	rc_dev->input_phys = "sii8620/input0";
> +	rc_dev->input_id.bustype = BUS_VIRTUAL;
> +	rc_dev->map_name = RC_MAP_CEC;
> +	rc_dev->allowed_protocols = RC_PROTO_BIT_CEC;
> +	rc_dev->driver_name = "sii8620";
> +	rc_dev->device_name = "sii8620";
> +
> +	ret = rc_register_device(rc_dev);
> +
> +	if (ret) {
> +		dev_err(ctx->dev, "Failed to register RC device\n");
> +		ctx->error = ret;
> +		rc_free_device(ctx->rc_dev);
> +		return;
> +	}
> +	ctx->rc_dev = rc_dev;
> +}
> +
>  static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sii8620, bridge);
>  }
>  
> +static int sii8620_attach(struct drm_bridge *bridge)
> +{
> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
> +
> +	sii8620_init_rcp_input_dev(ctx);
> +
> +	return sii8620_clear_error(ctx);
> +}
> +
> +static void sii8620_detach(struct drm_bridge *bridge)
> +{
> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
> +
> +	rc_unregister_device(ctx->rc_dev);
> +}
> +
>  static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>  			       const struct drm_display_mode *mode,
>  			       struct drm_display_mode *adjusted_mode)
> @@ -2151,6 +2235,8 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>  }
>  
>  static const struct drm_bridge_funcs sii8620_bridge_funcs = {
> +	.attach = sii8620_attach,
> +	.detach = sii8620_detach,
>  	.mode_fixup = sii8620_mode_fixup,
>  };
>  
> @@ -2217,8 +2303,8 @@ static int sii8620_remove(struct i2c_client *client)
>  	struct sii8620 *ctx = i2c_get_clientdata(client);
>  
>  	disable_irq(to_i2c_client(ctx->dev)->irq);
> -	drm_bridge_remove(&ctx->bridge);
>  	sii8620_hw_off(ctx);
> +	drm_bridge_remove(&ctx->bridge);
>  
>  	return 0;
>  }
> diff --git a/include/drm/bridge/mhl.h b/include/drm/bridge/mhl.h
> index fbdfc8d..96a5e0f 100644
> --- a/include/drm/bridge/mhl.h
> +++ b/include/drm/bridge/mhl.h
> @@ -262,6 +262,10 @@ enum {
>  #define MHL_RAPK_UNSUPPORTED	0x02	/* Rcvd RAP action code not supported */
>  #define MHL_RAPK_BUSY		0x03	/* Responder too busy to respond */
>  
> +/* Bit masks for RCP messages */
> +#define MHL_RCP_KEY_RELEASED_MASK	0x80
> +#define MHL_RCP_KEY_ID_MASK		0x7F
> +
>  /*
>   * Error status codes for RCPE messages
>   */



Thanks,
Mauro

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Hans Verkuil]

Hi Maciej,

On 24/08/17 10:58, Maciej Purski wrote:
> MHL specification defines Remote Control Protocol(RCP) to
> send input events between MHL devices.
> The driver now recognizes RCP messages and reacts to them
> by reporting key events to input subsystem, allowing
> a user to control a device using TV remote control.

Before I Ack this I would like to know how this behaves w.r.t. autorepeat.

If you keep pressing a remote key, what RCP messages do you receive and
at what time intervals? If that's similar to what CEC does, then it is
very likely that the same rules apply and I will have to review this patch
again with that in mind.

See the commit log for the patching fixing the CEC 'Press and Hold' behavior:

https://git.linuxtv.org/media_tree.git/commit/drivers/media/cec/cec-adap.c?id=a9a249a2c997506a64eaee22f1458fda893f62a8

If you have access to the HDMI 2.0 specification, then that spec describes the CEC
'Press and Hold' behavior in detail.

Regards,

	Hans

> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
> 
> Changes in v2:
> - use RC subsystem (including CEC keymap)
> - RC device initialized in attach drm_bridge callback and
>   removed in detach callback. This is necessary, because RC_CORE,
>   which is needed during rc_dev init, is loaded after sii8620.
>   DRM bridge is binded later which solves the problem.
> - add RC_CORE dependency
> 
> Changes in v3:
> - fix error handling in init_rcp and in attach callback
> 
> Changes in v4:
> - usage of rc-core API compatible with upcoming changes
> - fix error handling in init_rcp
> - fix commit message
> ---
>  drivers/gpu/drm/bridge/Kconfig       |  2 +-
>  drivers/gpu/drm/bridge/sil-sii8620.c | 96 ++++++++++++++++++++++++++++++++++--
>  include/drm/bridge/mhl.h             |  4 ++
>  3 files changed, 96 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index adf9ae0..6ef901c 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -71,7 +71,7 @@ config DRM_PARADE_PS8622
>  
>  config DRM_SIL_SII8620
>  	tristate "Silicon Image SII8620 HDMI/MHL bridge"
> -	depends on OF
> +	depends on OF && RC_CORE
>  	select DRM_KMS_HELPER
>  	help
>  	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 2d51a22..ecb26c4 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -28,6 +28,8 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  
> +#include <media/rc-core.h>
> +
>  #include "sil-sii8620.h"
>  
>  #define SII8620_BURST_BUF_LEN 288
> @@ -58,6 +60,7 @@ enum sii8620_mt_state {
>  struct sii8620 {
>  	struct drm_bridge bridge;
>  	struct device *dev;
> +	struct rc_dev *rc_dev;
>  	struct clk *clk_xtal;
>  	struct gpio_desc *gpio_reset;
>  	struct gpio_desc *gpio_int;
> @@ -431,6 +434,16 @@ static void sii8620_mt_rap(struct sii8620 *ctx, u8 code)
>  	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RAP, code);
>  }
>  
> +static void sii8620_mt_rcpk(struct sii8620 *ctx, u8 code)
> +{
> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPK, code);
> +}
> +
> +static void sii8620_mt_rcpe(struct sii8620 *ctx, u8 code)
> +{
> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPE, code);
> +}
> +
>  static void sii8620_mt_read_devcap_send(struct sii8620 *ctx,
>  					struct sii8620_mt_msg *msg)
>  {
> @@ -1753,6 +1766,25 @@ static void sii8620_send_features(struct sii8620 *ctx)
>  	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
>  }
>  
> +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
> +{
> +	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
> +
> +	scancode &= MHL_RCP_KEY_ID_MASK;
> +
> +	if (!ctx->rc_dev) {
> +		dev_dbg(ctx->dev, "RCP input device not initialized\n");
> +		return false;
> +	}
> +
> +	if (pressed)
> +		rc_keydown(ctx->rc_dev, RC_PROTO_CEC, scancode, 0);
> +	else
> +		rc_keyup(ctx->rc_dev);
> +
> +	return true;
> +}
> +
>  static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
>  {
>  	u8 ints[MHL_INT_SIZE];
> @@ -1804,19 +1836,25 @@ static void sii8620_msc_mt_done(struct sii8620 *ctx)
>  
>  static void sii8620_msc_mr_msc_msg(struct sii8620 *ctx)
>  {
> -	struct sii8620_mt_msg *msg = sii8620_msc_msg_first(ctx);
> +	struct sii8620_mt_msg *msg;
>  	u8 buf[2];
>  
> -	if (!msg)
> -		return;
> -
>  	sii8620_read_buf(ctx, REG_MSC_MR_MSC_MSG_RCVD_1ST_DATA, buf, 2);
>  
>  	switch (buf[0]) {
>  	case MHL_MSC_MSG_RAPK:
> +		msg = sii8620_msc_msg_first(ctx);
> +		if (!msg)
> +			return;
>  		msg->ret = buf[1];
>  		ctx->mt_state = MT_STATE_DONE;
>  		break;
> +	case MHL_MSC_MSG_RCP:
> +		if (!sii8620_rcp_consume(ctx, buf[1]))
> +			sii8620_mt_rcpe(ctx,
> +					MHL_RCPE_STATUS_INEFFECTIVE_KEY_CODE);
> +		sii8620_mt_rcpk(ctx, buf[1]);
> +		break;
>  	default:
>  		dev_err(ctx->dev, "%s message type %d,%d not supported",
>  			__func__, buf[0], buf[1]);
> @@ -2102,11 +2140,57 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>  	enable_irq(to_i2c_client(ctx->dev)->irq);
>  }
>  
> +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
> +{
> +	struct rc_dev *rc_dev;
> +	int ret;
> +
> +	rc_dev = rc_allocate_device(RC_DRIVER_SCANCODE);
> +	if (!rc_dev) {
> +		dev_err(ctx->dev, "Failed to allocate RC device\n");
> +		ctx->error = -ENOMEM;
> +		return;
> +	}
> +
> +	rc_dev->input_phys = "sii8620/input0";
> +	rc_dev->input_id.bustype = BUS_VIRTUAL;
> +	rc_dev->map_name = RC_MAP_CEC;
> +	rc_dev->allowed_protocols = RC_PROTO_BIT_CEC;
> +	rc_dev->driver_name = "sii8620";
> +	rc_dev->device_name = "sii8620";
> +
> +	ret = rc_register_device(rc_dev);
> +
> +	if (ret) {
> +		dev_err(ctx->dev, "Failed to register RC device\n");
> +		ctx->error = ret;
> +		rc_free_device(ctx->rc_dev);
> +		return;
> +	}
> +	ctx->rc_dev = rc_dev;
> +}
> +
>  static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sii8620, bridge);
>  }
>  
> +static int sii8620_attach(struct drm_bridge *bridge)
> +{
> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
> +
> +	sii8620_init_rcp_input_dev(ctx);
> +
> +	return sii8620_clear_error(ctx);
> +}
> +
> +static void sii8620_detach(struct drm_bridge *bridge)
> +{
> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
> +
> +	rc_unregister_device(ctx->rc_dev);
> +}
> +
>  static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>  			       const struct drm_display_mode *mode,
>  			       struct drm_display_mode *adjusted_mode)
> @@ -2151,6 +2235,8 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>  }
>  
>  static const struct drm_bridge_funcs sii8620_bridge_funcs = {
> +	.attach = sii8620_attach,
> +	.detach = sii8620_detach,
>  	.mode_fixup = sii8620_mode_fixup,
>  };
>  
> @@ -2217,8 +2303,8 @@ static int sii8620_remove(struct i2c_client *client)
>  	struct sii8620 *ctx = i2c_get_clientdata(client);
>  
>  	disable_irq(to_i2c_client(ctx->dev)->irq);
> -	drm_bridge_remove(&ctx->bridge);
>  	sii8620_hw_off(ctx);
> +	drm_bridge_remove(&ctx->bridge);
>  
>  	return 0;
>  }
> diff --git a/include/drm/bridge/mhl.h b/include/drm/bridge/mhl.h
> index fbdfc8d..96a5e0f 100644
> --- a/include/drm/bridge/mhl.h
> +++ b/include/drm/bridge/mhl.h
> @@ -262,6 +262,10 @@ enum {
>  #define MHL_RAPK_UNSUPPORTED	0x02	/* Rcvd RAP action code not supported */
>  #define MHL_RAPK_BUSY		0x03	/* Responder too busy to respond */
>  
> +/* Bit masks for RCP messages */
> +#define MHL_RCP_KEY_RELEASED_MASK	0x80
> +#define MHL_RCP_KEY_ID_MASK		0x7F
> +
>  /*
>   * Error status codes for RCPE messages
>   */
> 

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Maciej Purski]

Hi Hans,

According to my tests, when pressing 'Press and Hold' key, the messages received are
always the same until the button is released. The second message is received after
~550 ms and each next message is received every ~100 ms.

Regards,

	Maciej

On 27/08/2017 14:40, Hans Verkuil wrote:

> Hi Maciej,
>
> On 24/08/17 10:58, Maciej Purski wrote:
>> MHL specification defines Remote Control Protocol(RCP) to
>> send input events between MHL devices.
>> The driver now recognizes RCP messages and reacts to them
>> by reporting key events to input subsystem, allowing
>> a user to control a device using TV remote control.
> Before I Ack this I would like to know how this behaves w.r.t. autorepeat.
>
> If you keep pressing a remote key, what RCP messages do you receive and
> at what time intervals? If that's similar to what CEC does, then it is
> very likely that the same rules apply and I will have to review this patch
> again with that in mind.
>
> See the commit log for the patching fixing the CEC 'Press and Hold' behavior:
>
> https://git.linuxtv.org/media_tree.git/commit/drivers/media/cec/cec-adap.c?id=a9a249a2c997506a64eaee22f1458fda893f62a8
>
> If you have access to the HDMI 2.0 specification, then that spec describes the CEC
> 'Press and Hold' behavior in detail.
>
> Regards,
>
> 	Hans
>
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> ---
>>
>> Changes in v2:
>> - use RC subsystem (including CEC keymap)
>> - RC device initialized in attach drm_bridge callback and
>>    removed in detach callback. This is necessary, because RC_CORE,
>>    which is needed during rc_dev init, is loaded after sii8620.
>>    DRM bridge is binded later which solves the problem.
>> - add RC_CORE dependency
>>
>> Changes in v3:
>> - fix error handling in init_rcp and in attach callback
>>
>> Changes in v4:
>> - usage of rc-core API compatible with upcoming changes
>> - fix error handling in init_rcp
>> - fix commit message
>> ---
>>   drivers/gpu/drm/bridge/Kconfig       |  2 +-
>>   drivers/gpu/drm/bridge/sil-sii8620.c | 96 ++++++++++++++++++++++++++++++++++--
>>   include/drm/bridge/mhl.h             |  4 ++
>>   3 files changed, 96 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index adf9ae0..6ef901c 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -71,7 +71,7 @@ config DRM_PARADE_PS8622
>>   
>>   config DRM_SIL_SII8620
>>   	tristate "Silicon Image SII8620 HDMI/MHL bridge"
>> -	depends on OF
>> +	depends on OF && RC_CORE
>>   	select DRM_KMS_HELPER
>>   	help
>>   	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
>> index 2d51a22..ecb26c4 100644
>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>> @@ -28,6 +28,8 @@
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/slab.h>
>>   
>> +#include <media/rc-core.h>
>> +
>>   #include "sil-sii8620.h"
>>   
>>   #define SII8620_BURST_BUF_LEN 288
>> @@ -58,6 +60,7 @@ enum sii8620_mt_state {
>>   struct sii8620 {
>>   	struct drm_bridge bridge;
>>   	struct device *dev;
>> +	struct rc_dev *rc_dev;
>>   	struct clk *clk_xtal;
>>   	struct gpio_desc *gpio_reset;
>>   	struct gpio_desc *gpio_int;
>> @@ -431,6 +434,16 @@ static void sii8620_mt_rap(struct sii8620 *ctx, u8 code)
>>   	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RAP, code);
>>   }
>>   
>> +static void sii8620_mt_rcpk(struct sii8620 *ctx, u8 code)
>> +{
>> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPK, code);
>> +}
>> +
>> +static void sii8620_mt_rcpe(struct sii8620 *ctx, u8 code)
>> +{
>> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPE, code);
>> +}
>> +
>>   static void sii8620_mt_read_devcap_send(struct sii8620 *ctx,
>>   					struct sii8620_mt_msg *msg)
>>   {
>> @@ -1753,6 +1766,25 @@ static void sii8620_send_features(struct sii8620 *ctx)
>>   	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
>>   }
>>   
>> +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
>> +{
>> +	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
>> +
>> +	scancode &= MHL_RCP_KEY_ID_MASK;
>> +
>> +	if (!ctx->rc_dev) {
>> +		dev_dbg(ctx->dev, "RCP input device not initialized\n");
>> +		return false;
>> +	}
>> +
>> +	if (pressed)
>> +		rc_keydown(ctx->rc_dev, RC_PROTO_CEC, scancode, 0);
>> +	else
>> +		rc_keyup(ctx->rc_dev);
>> +
>> +	return true;
>> +}
>> +
>>   static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
>>   {
>>   	u8 ints[MHL_INT_SIZE];
>> @@ -1804,19 +1836,25 @@ static void sii8620_msc_mt_done(struct sii8620 *ctx)
>>   
>>   static void sii8620_msc_mr_msc_msg(struct sii8620 *ctx)
>>   {
>> -	struct sii8620_mt_msg *msg = sii8620_msc_msg_first(ctx);
>> +	struct sii8620_mt_msg *msg;
>>   	u8 buf[2];
>>   
>> -	if (!msg)
>> -		return;
>> -
>>   	sii8620_read_buf(ctx, REG_MSC_MR_MSC_MSG_RCVD_1ST_DATA, buf, 2);
>>   
>>   	switch (buf[0]) {
>>   	case MHL_MSC_MSG_RAPK:
>> +		msg = sii8620_msc_msg_first(ctx);
>> +		if (!msg)
>> +			return;
>>   		msg->ret = buf[1];
>>   		ctx->mt_state = MT_STATE_DONE;
>>   		break;
>> +	case MHL_MSC_MSG_RCP:
>> +		if (!sii8620_rcp_consume(ctx, buf[1]))
>> +			sii8620_mt_rcpe(ctx,
>> +					MHL_RCPE_STATUS_INEFFECTIVE_KEY_CODE);
>> +		sii8620_mt_rcpk(ctx, buf[1]);
>> +		break;
>>   	default:
>>   		dev_err(ctx->dev, "%s message type %d,%d not supported",
>>   			__func__, buf[0], buf[1]);
>> @@ -2102,11 +2140,57 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>>   	enable_irq(to_i2c_client(ctx->dev)->irq);
>>   }
>>   
>> +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
>> +{
>> +	struct rc_dev *rc_dev;
>> +	int ret;
>> +
>> +	rc_dev = rc_allocate_device(RC_DRIVER_SCANCODE);
>> +	if (!rc_dev) {
>> +		dev_err(ctx->dev, "Failed to allocate RC device\n");
>> +		ctx->error = -ENOMEM;
>> +		return;
>> +	}
>> +
>> +	rc_dev->input_phys = "sii8620/input0";
>> +	rc_dev->input_id.bustype = BUS_VIRTUAL;
>> +	rc_dev->map_name = RC_MAP_CEC;
>> +	rc_dev->allowed_protocols = RC_PROTO_BIT_CEC;
>> +	rc_dev->driver_name = "sii8620";
>> +	rc_dev->device_name = "sii8620";
>> +
>> +	ret = rc_register_device(rc_dev);
>> +
>> +	if (ret) {
>> +		dev_err(ctx->dev, "Failed to register RC device\n");
>> +		ctx->error = ret;
>> +		rc_free_device(ctx->rc_dev);
>> +		return;
>> +	}
>> +	ctx->rc_dev = rc_dev;
>> +}
>> +
>>   static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
>>   {
>>   	return container_of(bridge, struct sii8620, bridge);
>>   }
>>   
>> +static int sii8620_attach(struct drm_bridge *bridge)
>> +{
>> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>> +
>> +	sii8620_init_rcp_input_dev(ctx);
>> +
>> +	return sii8620_clear_error(ctx);
>> +}
>> +
>> +static void sii8620_detach(struct drm_bridge *bridge)
>> +{
>> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>> +
>> +	rc_unregister_device(ctx->rc_dev);
>> +}
>> +
>>   static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>>   			       const struct drm_display_mode *mode,
>>   			       struct drm_display_mode *adjusted_mode)
>> @@ -2151,6 +2235,8 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>>   }
>>   
>>   static const struct drm_bridge_funcs sii8620_bridge_funcs = {
>> +	.attach = sii8620_attach,
>> +	.detach = sii8620_detach,
>>   	.mode_fixup = sii8620_mode_fixup,
>>   };
>>   
>> @@ -2217,8 +2303,8 @@ static int sii8620_remove(struct i2c_client *client)
>>   	struct sii8620 *ctx = i2c_get_clientdata(client);
>>   
>>   	disable_irq(to_i2c_client(ctx->dev)->irq);
>> -	drm_bridge_remove(&ctx->bridge);
>>   	sii8620_hw_off(ctx);
>> +	drm_bridge_remove(&ctx->bridge);
>>   
>>   	return 0;
>>   }
>> diff --git a/include/drm/bridge/mhl.h b/include/drm/bridge/mhl.h
>> index fbdfc8d..96a5e0f 100644
>> --- a/include/drm/bridge/mhl.h
>> +++ b/include/drm/bridge/mhl.h
>> @@ -262,6 +262,10 @@ enum {
>>   #define MHL_RAPK_UNSUPPORTED	0x02	/* Rcvd RAP action code not supported */
>>   #define MHL_RAPK_BUSY		0x03	/* Responder too busy to respond */
>>   
>> +/* Bit masks for RCP messages */
>> +#define MHL_RCP_KEY_RELEASED_MASK	0x80
>> +#define MHL_RCP_KEY_ID_MASK		0x7F
>> +
>>   /*
>>    * Error status codes for RCPE messages
>>    */
>>
>
>
>

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Maciej Purski]

Hi Hans,
some time ago in reply to your email I described what messages does
the MHL driver receive and at what time intervals.
Regarding that information, do you think that a similar solution as
in [1] is required? Would it be OK, if I just set REP_DELAY and REP_PERIOD
to values, which I presented in my last email?


Best Regards,

	Maciej

[1] https://git.linuxtv.org/media_tree.git/commit/drivers/media/cec/cec-adap.c?id=a9a249a2c997506a64eaee22f1458fda893f62a8

On 08/27/2017 02:40 PM, Hans Verkuil wrote:

> Hi Maciej,
>
> On 24/08/17 10:58, Maciej Purski wrote:
>> MHL specification defines Remote Control Protocol(RCP) to
>> send input events between MHL devices.
>> The driver now recognizes RCP messages and reacts to them
>> by reporting key events to input subsystem, allowing
>> a user to control a device using TV remote control.
> Before I Ack this I would like to know how this behaves w.r.t. autorepeat.
>
> If you keep pressing a remote key, what RCP messages do you receive and
> at what time intervals? If that's similar to what CEC does, then it is
> very likely that the same rules apply and I will have to review this patch
> again with that in mind.
>
> See the commit log for the patching fixing the CEC 'Press and Hold' behavior:
>
> https://git.linuxtv.org/media_tree.git/commit/drivers/media/cec/cec-adap.c?id=a9a249a2c997506a64eaee22f1458fda893f62a8
>
> If you have access to the HDMI 2.0 specification, then that spec describes the CEC
> 'Press and Hold' behavior in detail.
>
> Regards,
>
> 	Hans
>
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> ---
>>
>> Changes in v2:
>> - use RC subsystem (including CEC keymap)
>> - RC device initialized in attach drm_bridge callback and
>>    removed in detach callback. This is necessary, because RC_CORE,
>>    which is needed during rc_dev init, is loaded after sii8620.
>>    DRM bridge is binded later which solves the problem.
>> - add RC_CORE dependency
>>
>> Changes in v3:
>> - fix error handling in init_rcp and in attach callback
>>
>> Changes in v4:
>> - usage of rc-core API compatible with upcoming changes
>> - fix error handling in init_rcp
>> - fix commit message
>> ---
>>   drivers/gpu/drm/bridge/Kconfig       |  2 +-
>>   drivers/gpu/drm/bridge/sil-sii8620.c | 96 ++++++++++++++++++++++++++++++++++--
>>   include/drm/bridge/mhl.h             |  4 ++
>>   3 files changed, 96 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index adf9ae0..6ef901c 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -71,7 +71,7 @@ config DRM_PARADE_PS8622
>>   
>>   config DRM_SIL_SII8620
>>   	tristate "Silicon Image SII8620 HDMI/MHL bridge"
>> -	depends on OF
>> +	depends on OF && RC_CORE
>>   	select DRM_KMS_HELPER
>>   	help
>>   	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
>> index 2d51a22..ecb26c4 100644
>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>> @@ -28,6 +28,8 @@
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/slab.h>
>>   
>> +#include <media/rc-core.h>
>> +
>>   #include "sil-sii8620.h"
>>   
>>   #define SII8620_BURST_BUF_LEN 288
>> @@ -58,6 +60,7 @@ enum sii8620_mt_state {
>>   struct sii8620 {
>>   	struct drm_bridge bridge;
>>   	struct device *dev;
>> +	struct rc_dev *rc_dev;
>>   	struct clk *clk_xtal;
>>   	struct gpio_desc *gpio_reset;
>>   	struct gpio_desc *gpio_int;
>> @@ -431,6 +434,16 @@ static void sii8620_mt_rap(struct sii8620 *ctx, u8 code)
>>   	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RAP, code);
>>   }
>>   
>> +static void sii8620_mt_rcpk(struct sii8620 *ctx, u8 code)
>> +{
>> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPK, code);
>> +}
>> +
>> +static void sii8620_mt_rcpe(struct sii8620 *ctx, u8 code)
>> +{
>> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPE, code);
>> +}
>> +
>>   static void sii8620_mt_read_devcap_send(struct sii8620 *ctx,
>>   					struct sii8620_mt_msg *msg)
>>   {
>> @@ -1753,6 +1766,25 @@ static void sii8620_send_features(struct sii8620 *ctx)
>>   	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
>>   }
>>   
>> +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
>> +{
>> +	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
>> +
>> +	scancode &= MHL_RCP_KEY_ID_MASK;
>> +
>> +	if (!ctx->rc_dev) {
>> +		dev_dbg(ctx->dev, "RCP input device not initialized\n");
>> +		return false;
>> +	}
>> +
>> +	if (pressed)
>> +		rc_keydown(ctx->rc_dev, RC_PROTO_CEC, scancode, 0);
>> +	else
>> +		rc_keyup(ctx->rc_dev);
>> +
>> +	return true;
>> +}
>> +
>>   static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
>>   {
>>   	u8 ints[MHL_INT_SIZE];
>> @@ -1804,19 +1836,25 @@ static void sii8620_msc_mt_done(struct sii8620 *ctx)
>>   
>>   static void sii8620_msc_mr_msc_msg(struct sii8620 *ctx)
>>   {
>> -	struct sii8620_mt_msg *msg = sii8620_msc_msg_first(ctx);
>> +	struct sii8620_mt_msg *msg;
>>   	u8 buf[2];
>>   
>> -	if (!msg)
>> -		return;
>> -
>>   	sii8620_read_buf(ctx, REG_MSC_MR_MSC_MSG_RCVD_1ST_DATA, buf, 2);
>>   
>>   	switch (buf[0]) {
>>   	case MHL_MSC_MSG_RAPK:
>> +		msg = sii8620_msc_msg_first(ctx);
>> +		if (!msg)
>> +			return;
>>   		msg->ret = buf[1];
>>   		ctx->mt_state = MT_STATE_DONE;
>>   		break;
>> +	case MHL_MSC_MSG_RCP:
>> +		if (!sii8620_rcp_consume(ctx, buf[1]))
>> +			sii8620_mt_rcpe(ctx,
>> +					MHL_RCPE_STATUS_INEFFECTIVE_KEY_CODE);
>> +		sii8620_mt_rcpk(ctx, buf[1]);
>> +		break;
>>   	default:
>>   		dev_err(ctx->dev, "%s message type %d,%d not supported",
>>   			__func__, buf[0], buf[1]);
>> @@ -2102,11 +2140,57 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>>   	enable_irq(to_i2c_client(ctx->dev)->irq);
>>   }
>>   
>> +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
>> +{
>> +	struct rc_dev *rc_dev;
>> +	int ret;
>> +
>> +	rc_dev = rc_allocate_device(RC_DRIVER_SCANCODE);
>> +	if (!rc_dev) {
>> +		dev_err(ctx->dev, "Failed to allocate RC device\n");
>> +		ctx->error = -ENOMEM;
>> +		return;
>> +	}
>> +
>> +	rc_dev->input_phys = "sii8620/input0";
>> +	rc_dev->input_id.bustype = BUS_VIRTUAL;
>> +	rc_dev->map_name = RC_MAP_CEC;
>> +	rc_dev->allowed_protocols = RC_PROTO_BIT_CEC;
>> +	rc_dev->driver_name = "sii8620";
>> +	rc_dev->device_name = "sii8620";
>> +
>> +	ret = rc_register_device(rc_dev);
>> +
>> +	if (ret) {
>> +		dev_err(ctx->dev, "Failed to register RC device\n");
>> +		ctx->error = ret;
>> +		rc_free_device(ctx->rc_dev);
>> +		return;
>> +	}
>> +	ctx->rc_dev = rc_dev;
>> +}
>> +
>>   static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
>>   {
>>   	return container_of(bridge, struct sii8620, bridge);
>>   }
>>   
>> +static int sii8620_attach(struct drm_bridge *bridge)
>> +{
>> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>> +
>> +	sii8620_init_rcp_input_dev(ctx);
>> +
>> +	return sii8620_clear_error(ctx);
>> +}
>> +
>> +static void sii8620_detach(struct drm_bridge *bridge)
>> +{
>> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>> +
>> +	rc_unregister_device(ctx->rc_dev);
>> +}
>> +
>>   static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>>   			       const struct drm_display_mode *mode,
>>   			       struct drm_display_mode *adjusted_mode)
>> @@ -2151,6 +2235,8 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>>   }
>>   
>>   static const struct drm_bridge_funcs sii8620_bridge_funcs = {
>> +	.attach = sii8620_attach,
>> +	.detach = sii8620_detach,
>>   	.mode_fixup = sii8620_mode_fixup,
>>   };
>>   
>> @@ -2217,8 +2303,8 @@ static int sii8620_remove(struct i2c_client *client)
>>   	struct sii8620 *ctx = i2c_get_clientdata(client);
>>   
>>   	disable_irq(to_i2c_client(ctx->dev)->irq);
>> -	drm_bridge_remove(&ctx->bridge);
>>   	sii8620_hw_off(ctx);
>> +	drm_bridge_remove(&ctx->bridge);
>>   
>>   	return 0;
>>   }
>> diff --git a/include/drm/bridge/mhl.h b/include/drm/bridge/mhl.h
>> index fbdfc8d..96a5e0f 100644
>> --- a/include/drm/bridge/mhl.h
>> +++ b/include/drm/bridge/mhl.h
>> @@ -262,6 +262,10 @@ enum {
>>   #define MHL_RAPK_UNSUPPORTED	0x02	/* Rcvd RAP action code not supported */
>>   #define MHL_RAPK_BUSY		0x03	/* Responder too busy to respond */
>>   
>> +/* Bit masks for RCP messages */
>> +#define MHL_RCP_KEY_RELEASED_MASK	0x80
>> +#define MHL_RCP_KEY_ID_MASK		0x7F
>> +
>>   /*
>>    * Error status codes for RCPE messages
>>    */
>>
>
>
>

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Hans Verkuil]

On 09/18/2017 04:15 PM, Maciej Purski wrote:
> Hi Hans,
> some time ago in reply to your email I described what messages does
> the MHL driver receive and at what time intervals.
> Regarding that information, do you think that a similar solution as
> in [1] is required? Would it be OK, if I just set REP_DELAY and REP_PERIOD
> to values, which I presented in my last email?

Based on the timings you measured I would say that there is a 99% chance that MHL
uses exactly the same "Press and Hold" mechanism as CEC. Since you already specify
RC_PROTO_BIT_CEC in the driver, it will set REP_DELAY and REP_PERIOD to the right
values automatically.

You will have to implement the same code as in cec-adap.c for the key press handling,
though. It's a bit tricky to get it right.

Since you will have to do the same thing as I do in CEC, I wonder if it would make
more sense to move this code to the RC core. Basically it ensures that repeat mode
doesn't turn on until you get two identical key presses 550ms or less apart. This
is independent of REP_DELAY which can be changed by userspace.

Sean, what do you think?

Regards,

	Hans

> 
> 
> Best Regards,
> 
> 	Maciej
> 
> [1] https://git.linuxtv.org/media_tree.git/commit/drivers/media/cec/cec-adap.c?id=a9a249a2c997506a64eaee22f1458fda893f62a8
> 
> On 08/27/2017 02:40 PM, Hans Verkuil wrote:
> 
>> Hi Maciej,
>>
>> On 24/08/17 10:58, Maciej Purski wrote:
>>> MHL specification defines Remote Control Protocol(RCP) to
>>> send input events between MHL devices.
>>> The driver now recognizes RCP messages and reacts to them
>>> by reporting key events to input subsystem, allowing
>>> a user to control a device using TV remote control.
>> Before I Ack this I would like to know how this behaves w.r.t. autorepeat.
>>
>> If you keep pressing a remote key, what RCP messages do you receive and
>> at what time intervals? If that's similar to what CEC does, then it is
>> very likely that the same rules apply and I will have to review this patch
>> again with that in mind.
>>
>> See the commit log for the patching fixing the CEC 'Press and Hold' behavior:
>>
>> https://git.linuxtv.org/media_tree.git/commit/drivers/media/cec/cec-adap.c?id=a9a249a2c997506a64eaee22f1458fda893f62a8
>>
>> If you have access to the HDMI 2.0 specification, then that spec describes the CEC
>> 'Press and Hold' behavior in detail.
>>
>> Regards,
>>
>> 	Hans
>>
>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>> ---
>>>
>>> Changes in v2:
>>> - use RC subsystem (including CEC keymap)
>>> - RC device initialized in attach drm_bridge callback and
>>>    removed in detach callback. This is necessary, because RC_CORE,
>>>    which is needed during rc_dev init, is loaded after sii8620.
>>>    DRM bridge is binded later which solves the problem.
>>> - add RC_CORE dependency
>>>
>>> Changes in v3:
>>> - fix error handling in init_rcp and in attach callback
>>>
>>> Changes in v4:
>>> - usage of rc-core API compatible with upcoming changes
>>> - fix error handling in init_rcp
>>> - fix commit message
>>> ---
>>>   drivers/gpu/drm/bridge/Kconfig       |  2 +-
>>>   drivers/gpu/drm/bridge/sil-sii8620.c | 96 ++++++++++++++++++++++++++++++++++--
>>>   include/drm/bridge/mhl.h             |  4 ++
>>>   3 files changed, 96 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index adf9ae0..6ef901c 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -71,7 +71,7 @@ config DRM_PARADE_PS8622
>>>   
>>>   config DRM_SIL_SII8620
>>>   	tristate "Silicon Image SII8620 HDMI/MHL bridge"
>>> -	depends on OF
>>> +	depends on OF && RC_CORE
>>>   	select DRM_KMS_HELPER
>>>   	help
>>>   	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> index 2d51a22..ecb26c4 100644
>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>>> @@ -28,6 +28,8 @@
>>>   #include <linux/regulator/consumer.h>
>>>   #include <linux/slab.h>
>>>   
>>> +#include <media/rc-core.h>
>>> +
>>>   #include "sil-sii8620.h"
>>>   
>>>   #define SII8620_BURST_BUF_LEN 288
>>> @@ -58,6 +60,7 @@ enum sii8620_mt_state {
>>>   struct sii8620 {
>>>   	struct drm_bridge bridge;
>>>   	struct device *dev;
>>> +	struct rc_dev *rc_dev;
>>>   	struct clk *clk_xtal;
>>>   	struct gpio_desc *gpio_reset;
>>>   	struct gpio_desc *gpio_int;
>>> @@ -431,6 +434,16 @@ static void sii8620_mt_rap(struct sii8620 *ctx, u8 code)
>>>   	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RAP, code);
>>>   }
>>>   
>>> +static void sii8620_mt_rcpk(struct sii8620 *ctx, u8 code)
>>> +{
>>> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPK, code);
>>> +}
>>> +
>>> +static void sii8620_mt_rcpe(struct sii8620 *ctx, u8 code)
>>> +{
>>> +	sii8620_mt_msc_msg(ctx, MHL_MSC_MSG_RCPE, code);
>>> +}
>>> +
>>>   static void sii8620_mt_read_devcap_send(struct sii8620 *ctx,
>>>   					struct sii8620_mt_msg *msg)
>>>   {
>>> @@ -1753,6 +1766,25 @@ static void sii8620_send_features(struct sii8620 *ctx)
>>>   	sii8620_write_buf(ctx, REG_MDT_XMIT_WRITE_PORT, buf, ARRAY_SIZE(buf));
>>>   }
>>>   
>>> +static bool sii8620_rcp_consume(struct sii8620 *ctx, u8 scancode)
>>> +{
>>> +	bool pressed = !(scancode & MHL_RCP_KEY_RELEASED_MASK);
>>> +
>>> +	scancode &= MHL_RCP_KEY_ID_MASK;
>>> +
>>> +	if (!ctx->rc_dev) {
>>> +		dev_dbg(ctx->dev, "RCP input device not initialized\n");
>>> +		return false;
>>> +	}
>>> +
>>> +	if (pressed)
>>> +		rc_keydown(ctx->rc_dev, RC_PROTO_CEC, scancode, 0);
>>> +	else
>>> +		rc_keyup(ctx->rc_dev);
>>> +
>>> +	return true;
>>> +}
>>> +
>>>   static void sii8620_msc_mr_set_int(struct sii8620 *ctx)
>>>   {
>>>   	u8 ints[MHL_INT_SIZE];
>>> @@ -1804,19 +1836,25 @@ static void sii8620_msc_mt_done(struct sii8620 *ctx)
>>>   
>>>   static void sii8620_msc_mr_msc_msg(struct sii8620 *ctx)
>>>   {
>>> -	struct sii8620_mt_msg *msg = sii8620_msc_msg_first(ctx);
>>> +	struct sii8620_mt_msg *msg;
>>>   	u8 buf[2];
>>>   
>>> -	if (!msg)
>>> -		return;
>>> -
>>>   	sii8620_read_buf(ctx, REG_MSC_MR_MSC_MSG_RCVD_1ST_DATA, buf, 2);
>>>   
>>>   	switch (buf[0]) {
>>>   	case MHL_MSC_MSG_RAPK:
>>> +		msg = sii8620_msc_msg_first(ctx);
>>> +		if (!msg)
>>> +			return;
>>>   		msg->ret = buf[1];
>>>   		ctx->mt_state = MT_STATE_DONE;
>>>   		break;
>>> +	case MHL_MSC_MSG_RCP:
>>> +		if (!sii8620_rcp_consume(ctx, buf[1]))
>>> +			sii8620_mt_rcpe(ctx,
>>> +					MHL_RCPE_STATUS_INEFFECTIVE_KEY_CODE);
>>> +		sii8620_mt_rcpk(ctx, buf[1]);
>>> +		break;
>>>   	default:
>>>   		dev_err(ctx->dev, "%s message type %d,%d not supported",
>>>   			__func__, buf[0], buf[1]);
>>> @@ -2102,11 +2140,57 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>>>   	enable_irq(to_i2c_client(ctx->dev)->irq);
>>>   }
>>>   
>>> +static void sii8620_init_rcp_input_dev(struct sii8620 *ctx)
>>> +{
>>> +	struct rc_dev *rc_dev;
>>> +	int ret;
>>> +
>>> +	rc_dev = rc_allocate_device(RC_DRIVER_SCANCODE);
>>> +	if (!rc_dev) {
>>> +		dev_err(ctx->dev, "Failed to allocate RC device\n");
>>> +		ctx->error = -ENOMEM;
>>> +		return;
>>> +	}
>>> +
>>> +	rc_dev->input_phys = "sii8620/input0";
>>> +	rc_dev->input_id.bustype = BUS_VIRTUAL;
>>> +	rc_dev->map_name = RC_MAP_CEC;
>>> +	rc_dev->allowed_protocols = RC_PROTO_BIT_CEC;
>>> +	rc_dev->driver_name = "sii8620";
>>> +	rc_dev->device_name = "sii8620";
>>> +
>>> +	ret = rc_register_device(rc_dev);
>>> +
>>> +	if (ret) {
>>> +		dev_err(ctx->dev, "Failed to register RC device\n");
>>> +		ctx->error = ret;
>>> +		rc_free_device(ctx->rc_dev);
>>> +		return;
>>> +	}
>>> +	ctx->rc_dev = rc_dev;
>>> +}
>>> +
>>>   static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
>>>   {
>>>   	return container_of(bridge, struct sii8620, bridge);
>>>   }
>>>   
>>> +static int sii8620_attach(struct drm_bridge *bridge)
>>> +{
>>> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>>> +
>>> +	sii8620_init_rcp_input_dev(ctx);
>>> +
>>> +	return sii8620_clear_error(ctx);
>>> +}
>>> +
>>> +static void sii8620_detach(struct drm_bridge *bridge)
>>> +{
>>> +	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>>> +
>>> +	rc_unregister_device(ctx->rc_dev);
>>> +}
>>> +
>>>   static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>>>   			       const struct drm_display_mode *mode,
>>>   			       struct drm_display_mode *adjusted_mode)
>>> @@ -2151,6 +2235,8 @@ static bool sii8620_mode_fixup(struct drm_bridge *bridge,
>>>   }
>>>   
>>>   static const struct drm_bridge_funcs sii8620_bridge_funcs = {
>>> +	.attach = sii8620_attach,
>>> +	.detach = sii8620_detach,
>>>   	.mode_fixup = sii8620_mode_fixup,
>>>   };
>>>   
>>> @@ -2217,8 +2303,8 @@ static int sii8620_remove(struct i2c_client *client)
>>>   	struct sii8620 *ctx = i2c_get_clientdata(client);
>>>   
>>>   	disable_irq(to_i2c_client(ctx->dev)->irq);
>>> -	drm_bridge_remove(&ctx->bridge);
>>>   	sii8620_hw_off(ctx);
>>> +	drm_bridge_remove(&ctx->bridge);
>>>   
>>>   	return 0;
>>>   }
>>> diff --git a/include/drm/bridge/mhl.h b/include/drm/bridge/mhl.h
>>> index fbdfc8d..96a5e0f 100644
>>> --- a/include/drm/bridge/mhl.h
>>> +++ b/include/drm/bridge/mhl.h
>>> @@ -262,6 +262,10 @@ enum {
>>>   #define MHL_RAPK_UNSUPPORTED	0x02	/* Rcvd RAP action code not supported */
>>>   #define MHL_RAPK_BUSY		0x03	/* Responder too busy to respond */
>>>   
>>> +/* Bit masks for RCP messages */
>>> +#define MHL_RCP_KEY_RELEASED_MASK	0x80
>>> +#define MHL_RCP_KEY_ID_MASK		0x7F
>>> +
>>>   /*
>>>    * Error status codes for RCPE messages
>>>    */
>>>
>>
>>
>>
> 

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Sean Young]

On Mon, Sep 18, 2017 at 04:37:52PM +0200, Hans Verkuil wrote:
> On 09/18/2017 04:15 PM, Maciej Purski wrote:
> > Hi Hans,
> > some time ago in reply to your email I described what messages does
> > the MHL driver receive and at what time intervals.
> > Regarding that information, do you think that a similar solution as
> > in [1] is required? Would it be OK, if I just set REP_DELAY and REP_PERIOD
> > to values, which I presented in my last email?
> 
> Based on the timings you measured I would say that there is a 99% chance that MHL
> uses exactly the same "Press and Hold" mechanism as CEC. Since you already specify
> RC_PROTO_BIT_CEC in the driver, it will set REP_DELAY and REP_PERIOD to the right
> values automatically.
> 
> You will have to implement the same code as in cec-adap.c for the key press handling,
> though. It's a bit tricky to get it right.
> 
> Since you will have to do the same thing as I do in CEC, I wonder if it would make
> more sense to move this code to the RC core. Basically it ensures that repeat mode
> doesn't turn on until you get two identical key presses 550ms or less apart. This
> is independent of REP_DELAY which can be changed by userspace.
> 
> Sean, what do you think?

Yes, this makes sense. In fact, since IR protocols have different repeat
times, I would like to make this protocol dependant anyway.

To be honest I find REP_DELAY of 500ms too long and REP_PERIOD of 125ms
too short; IOW it takes too long for a key to start repeating, and once
it starts repeating it is very hard to control. If I try to increase the
volume with my remote it first hardly changes at all and then after 500ms
the volume shoots up far too quickly, same thing when navigating menus.

CEC dictates a delay period of 550ms, which is not great for the user IMO.

Anyway I'll have a look at this over the weekend.


Sean

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Sean Young]

Hi,

On Thu, Sep 21, 2017 at 12:46:04PM +0100, Sean Young wrote:
> On Mon, Sep 18, 2017 at 04:37:52PM +0200, Hans Verkuil wrote:
> > On 09/18/2017 04:15 PM, Maciej Purski wrote:
> > > Hi Hans,
> > > some time ago in reply to your email I described what messages does
> > > the MHL driver receive and at what time intervals.
> > > Regarding that information, do you think that a similar solution as
> > > in [1] is required? Would it be OK, if I just set REP_DELAY and REP_PERIOD
> > > to values, which I presented in my last email?
> > 
> > Based on the timings you measured I would say that there is a 99% chance that MHL
> > uses exactly the same "Press and Hold" mechanism as CEC. Since you already specify
> > RC_PROTO_BIT_CEC in the driver, it will set REP_DELAY and REP_PERIOD to the right
> > values automatically.
> > 
> > You will have to implement the same code as in cec-adap.c for the key press handling,
> > though. It's a bit tricky to get it right.
> > 
> > Since you will have to do the same thing as I do in CEC, I wonder if it would make
> > more sense to move this code to the RC core. Basically it ensures that repeat mode
> > doesn't turn on until you get two identical key presses 550ms or less apart. This
> > is independent of REP_DELAY which can be changed by userspace.
> > 
> > Sean, what do you think?
> 
> Yes, this makes sense. In fact, since IR protocols have different repeat
> times, I would like to make this protocol dependant anyway.
> 
> To be honest I find REP_DELAY of 500ms too long and REP_PERIOD of 125ms
> too short; IOW it takes too long for a key to start repeating, and once
> it starts repeating it is very hard to control. If I try to increase the
> volume with my remote it first hardly changes at all and then after 500ms
> the volume shoots up far too quickly, same thing when navigating menus.
> 
> CEC dictates a delay period of 550ms, which is not great for the user IMO.
> 
> Anyway I'll have a look at this over the weekend.

I have started on this, but I haven't gotten it in a state where I'm
happy to submit it. I hope to have it ready for v4.15; in the mean time,
there is no reason to block this patch on this.


Sean

Re: [PATCH v4] drm/bridge/sii8620: add remote control support

[Andrzej Hajda]

On 09.10.2017 10:44, Sean Young wrote:
> Hi,
>
> On Thu, Sep 21, 2017 at 12:46:04PM +0100, Sean Young wrote:
>> On Mon, Sep 18, 2017 at 04:37:52PM +0200, Hans Verkuil wrote:
>>> On 09/18/2017 04:15 PM, Maciej Purski wrote:
>>>> Hi Hans,
>>>> some time ago in reply to your email I described what messages does
>>>> the MHL driver receive and at what time intervals.
>>>> Regarding that information, do you think that a similar solution as
>>>> in [1] is required? Would it be OK, if I just set REP_DELAY and REP_PERIOD
>>>> to values, which I presented in my last email?
>>> Based on the timings you measured I would say that there is a 99% chance that MHL
>>> uses exactly the same "Press and Hold" mechanism as CEC. Since you already specify
>>> RC_PROTO_BIT_CEC in the driver, it will set REP_DELAY and REP_PERIOD to the right
>>> values automatically.
>>>
>>> You will have to implement the same code as in cec-adap.c for the key press handling,
>>> though. It's a bit tricky to get it right.
>>>
>>> Since you will have to do the same thing as I do in CEC, I wonder if it would make
>>> more sense to move this code to the RC core. Basically it ensures that repeat mode
>>> doesn't turn on until you get two identical key presses 550ms or less apart. This
>>> is independent of REP_DELAY which can be changed by userspace.
>>>
>>> Sean, what do you think?
>> Yes, this makes sense. In fact, since IR protocols have different repeat
>> times, I would like to make this protocol dependant anyway.
>>
>> To be honest I find REP_DELAY of 500ms too long and REP_PERIOD of 125ms
>> too short; IOW it takes too long for a key to start repeating, and once
>> it starts repeating it is very hard to control. If I try to increase the
>> volume with my remote it first hardly changes at all and then after 500ms
>> the volume shoots up far too quickly, same thing when navigating menus.
>>
>> CEC dictates a delay period of 550ms, which is not great for the user IMO.
>>
>> Anyway I'll have a look at this over the weekend.
> I have started on this, but I haven't gotten it in a state where I'm
> happy to submit it. I hope to have it ready for v4.15; in the mean time,
> there is no reason to block this patch on this.
>
>
> Sean
>
>
>

Queued to drm-misc-next.

Thanks
Andrzej