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@xxxxxxxxxxx> > > --- > > 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 > > */ > >
