[PATCH v3] drm/bridge/sii8620: add remote control support
Sean Young
sean at mess.org
Wed Aug 23 08:39:25 UTC 2017
Hi,
Some review comments below.
On Wed, Aug 23, 2017 at 09:03:18AM +0200, Hans Verkuil wrote:
> Maciej,
>
> I'm cross-posting this to linux-media and the rc maintainer Sean Young.
>
> Note that various RC defines have been renamed in the upcoming 4.14. It might be
> better to base your code on top of https://git.linuxtv.org/media_tree.git/
> which has those patches merged.
>
> Regards,
>
> Hans
>
> On 08/21/2017 11:33 AM, 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.
> >
> > 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
> >
> > Signed-off-by: Maciej Purski <m.purski at samsung.com>
> > ---
> > drivers/gpu/drm/bridge/Kconfig | 2 +-
> > drivers/gpu/drm/bridge/sil-sii8620.c | 101 +++++++++++++++++++++++++++++++++--
> > include/drm/bridge/mhl.h | 4 ++
> > 3 files changed, 101 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..e072bca 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_TYPE_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,62 @@ 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_BIT_CEC;
> > + rc_dev->driver_name = "sii8620";
> > + rc_dev->input_name = "sii8620";
input_name is already renamed to device_name (just like RC_BIT_CEC is
RC_PROTO_BIT_CEC now, as Hans pointed out already).
> > +
> > + ret = rc_register_device(rc_dev);
> > + ctx->rc_dev = rc_dev;
> > +
> > + if (ret) {
> > + dev_err(ctx->dev, "Failed to register RC device\n");
> > + ctx->error = ret;
> > + rc_free_device(ctx->rc_dev);
> > + }
In this error path, ctx->rc_dev is still set but contains a pointer to
something which is already freed.
> > +}
> > +
> > +static void sii8620_remove_rcp_input_dev(struct sii8620 *ctx)
> > +{
> > + rc_unregister_device(ctx->rc_dev);
> > + rc_free_device(ctx->rc_dev);
rc_unregister_device() already frees the rc_dev, don't call rc_free_device()
as well.
> > +}
> > +
> > 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);
> > +
> > + sii8620_remove_rcp_input_dev(ctx);
> > +}
> > +
> > static bool sii8620_mode_fixup(struct drm_bridge *bridge,
> > const struct drm_display_mode *mode,
> > struct drm_display_mode *adjusted_mode)
> > @@ -2151,6 +2240,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 +2308,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
> > */
> >
More information about the dri-devel
mailing list