Received: by 10.192.165.148 with SMTP id m20csp67256imm; Fri, 20 Apr 2018 03:09:26 -0700 (PDT) X-Google-Smtp-Source: AIpwx4+mfQVA1Jesc5H8/YosLE+xTPN7OaNoiEkN3Ykm/YNDtiSN3hhwPv1TPGsuCyJkSf2Oz18e X-Received: by 10.101.71.132 with SMTP id e4mr5137535pgs.196.1524218966207; Fri, 20 Apr 2018 03:09:26 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524218966; cv=none; d=google.com; s=arc-20160816; b=VO0KRR2G2gEyWB8D1B5SEz8UyyTrYOC+GBkUJlNgvm/FakpFzHZGQ/sCZ4MLGT9oCQ str/x2/C6z5ZT+GbVQ1OY1AFp+O7goFFGehi2kS0g67Fw/yD9joarcFWzBxWMu6HR38l dDjw9ntNkYVxwnbmOW6K4hScxlHykA7zJjVZfuzSHXEKyXLOySIAieSteh5t/64ovJ/P qKF1S+WwQNw9rly/Nga3qG6YRfycaifwsXbyNeGlJjU60/zydrxY2Odv9XnR4EnvQdQ0 bhovWEgyYNYEKPHxKG5swysy3YTDyAT/HVuLVbNdm3rk6jR2sTxqcHzr9//UWdw8JxID Ed4A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:organization:message-id:date:subject:cc:to :from:dkim-signature:arc-authentication-results; bh=QEtFiHzU6BhSqkvpGnKN/leSO9oEjKl5LUvXpi0epCI=; b=KcAgfCWQFK3Bi850GUyopE33JEKJOQbrN6lRdl+Gg61GbK02jJM4wHEUb6fkgpFjOo +/g8K/WYfCpygrS1FQxxkTaZ7r0gq4LRIvaDmJrWyICQuoeANb6VrSRQL1lvmPkyQyPU xsXzr1PyaRQnkfDA0LIRxt5O2Y0mneIGN32GYSLrFL6bFUDtYk2UqtLEzxCjjcTmna9+ EEnH7M4AjJcK2vyDAfTvIdW8tH+uGCh/itAxd2xiLJfk4oW3zxOWX4XzXBlVSl13+Q4a 9ZyGhPhQEj1LZ2vazD54zwTN/hWuGBEYa+IVXxGuE7h2opREylmp7CkoADGT6OsrkYxb SOkw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=nPESFeHB; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 132si4597496pgb.470.2018.04.20.03.09.11; Fri, 20 Apr 2018 03:09:26 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass (test mode) header.i=@ideasonboard.com header.s=mail header.b=nPESFeHB; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754611AbeDTKGp (ORCPT + 99 others); Fri, 20 Apr 2018 06:06:45 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:39450 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754458AbeDTKGl (ORCPT ); Fri, 20 Apr 2018 06:06:41 -0400 Received: from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi [IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id B243860A2; Fri, 20 Apr 2018 12:06:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1524218799; bh=VWpUF0nXDEw3EXpL7qhRLHTjwACzZL5fRtZHzinzkec=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=nPESFeHBO4JIjoKxHQ0oGZn1Ak0aUQ7BKuUMp6WHRJxGvCbor44+g3MP97m/EZr0v tGuTC42edL+bBHKBP3xIGCr/+Way0v7HQ59/diRyhg8hGJvZitEEKUfsUObOm36W90 VWyLzh9/vXr3tLWyzhITtyGceeTzxdSX2Be3iCOw= From: Laurent Pinchart To: dri-devel@lists.freedesktop.org Cc: Peter Rosin , linux-kernel@vger.kernel.org, Mark Rutland , Boris Brezillon , Alexandre Belloni , Jacopo Mondi , devicetree@vger.kernel.org, David Airlie , Nicolas Ferre , Russell King , Rob Herring , Daniel Vetter , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 7/7] drm/i2c: tda998x: register as a drm bridge Date: Fri, 20 Apr 2018 13:06:49 +0300 Message-ID: <2556566.3fxMPoIyx0@avalon> Organization: Ideas on Board Oy In-Reply-To: <20180419162751.25223-8-peda@axentia.se> References: <20180419162751.25223-1-peda@axentia.se> <20180419162751.25223-8-peda@axentia.se> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Peter, Thank you for the patch. On Thursday, 19 April 2018 19:27:51 EEST Peter Rosin wrote: > This makes this driver work with all(?) drivers that are not > componentized and instead expect to connect to a panel/bridge. That > said, the only one tested is atmel_hlcdc. > > This hooks the relevant work function previously called by the encoder > and the component also to the bridge, since the encoder goes away when > connecting to the bridge interface of the driver and the equivalent of > bind/unbind of the component is handled by bridge attach/detach. > > The lifetime requirements of a bridge and a component are slightly > different, which is the reason for struct tda998x_bridge. Couldn't you move the allocation and initialization (tda998x_create) of the tda998x_priv structure to probe time ? I think you wouldn't need a separate structure in that case. Unless I'm mistaken there would be an added benefit of separating component and bridge initialization, resulting in the encoder not being initialized at all if the component isn't used. You wouldn't need to add a local_encoder parameter to the tda998x_init() function. > Signed-off-by: Peter Rosin > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 157 ++++++++++++++++++++++++++++++++--- > 1 file changed, 137 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > b/drivers/gpu/drm/i2c/tda998x_drv.c index 9c78f7bde49c..012dee61d817 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -36,6 +36,14 @@ struct tda998x_audio_port { > u8 config; /* AP value */ > }; > > +struct tda998x_priv; > + > +struct tda998x_bridge { > + struct tda998x_priv *priv; > + struct device *dev; > + struct drm_bridge bridge; > +}; > + > struct tda998x_priv { > struct i2c_client *cec; > struct i2c_client *hdmi; > @@ -63,6 +71,8 @@ struct tda998x_priv { > wait_queue_head_t edid_delay_waitq; > bool edid_delay_active; > > + struct tda998x_bridge *bridge; > + bool local_encoder; > struct drm_encoder encoder; > struct drm_connector connector; > > @@ -75,6 +85,9 @@ struct tda998x_priv { > #define enc_to_tda998x_priv(x) \ > container_of(x, struct tda998x_priv, encoder) > > +#define bridge_to_tda998x_bridge(x) \ > + container_of(x, struct tda998x_bridge, bridge) > + > /* The TDA9988 series of devices use a paged register scheme.. to simplify > * things we encode the page # in upper bits of the register #. To read/ > * write a given register, we need to make sure CURPAGE register is set > @@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev, > void *data, struct hdmi_codec_daifmt *daifmt, > struct hdmi_codec_params *params) > { > - struct tda998x_priv *priv = dev_get_drvdata(dev); > + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > + struct tda998x_priv *priv = bridge->priv; > int i, ret; > struct tda998x_audio_params audio = { > .sample_width = params->sample_width, > @@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev, > void *data, > > static void tda998x_audio_shutdown(struct device *dev, void *data) > { > - struct tda998x_priv *priv = dev_get_drvdata(dev); > + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > + struct tda998x_priv *priv = bridge->priv; > > mutex_lock(&priv->audio_mutex); > > @@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev, > void *data) > > int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable) > { > - struct tda998x_priv *priv = dev_get_drvdata(dev); > + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > + struct tda998x_priv *priv = bridge->priv; > > mutex_lock(&priv->audio_mutex); > > @@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void > *data, bool enable) static int tda998x_audio_get_eld(struct device *dev, > void *data, > uint8_t *buf, size_t len) > { > - struct tda998x_priv *priv = dev_get_drvdata(dev); > + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > + struct tda998x_priv *priv = bridge->priv; > > mutex_lock(&priv->audio_mutex); > memcpy(buf, priv->connector.eld, > @@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector > *connector) { > struct tda998x_priv *priv = conn_to_tda998x_priv(connector); > > - return &priv->encoder; > + if (priv->local_encoder) > + return &priv->encoder; > + else > + return priv->bridge->bridge.encoder; > } > > static > @@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv > *priv, struct drm_device *drm) > { > struct drm_connector *connector = &priv->connector; > + struct drm_encoder *encoder; > int ret; > > connector->interlace_allowed = 1; > @@ -1156,7 +1177,8 @@ static int tda998x_connector_init(struct tda998x_priv > *priv, if (ret) > return ret; > > - drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder); > + encoder = tda998x_connector_best_encoder(&priv->connector); > + drm_mode_connector_attach_encoder(&priv->connector, encoder); > > return 0; > } > @@ -1668,8 +1690,10 @@ static void tda998x_set_config(struct tda998x_priv > *priv, priv->audio_params = p->audio_params; > } > > -static int tda998x_init(struct device *dev, struct drm_device *drm) > +static int tda998x_init(struct device *dev, struct drm_device *drm, > + bool local_encoder) > { > + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > struct tda998x_encoder_params *params = dev->platform_data; > struct i2c_client *client = to_i2c_client(dev); > struct tda998x_priv *priv; > @@ -1680,7 +1704,9 @@ static int tda998x_init(struct device *dev, struct > drm_device *drm) if (!priv) > return -ENOMEM; > > - dev_set_drvdata(dev, priv); > + bridge->priv = priv; > + priv->bridge = bridge; > + priv->local_encoder = local_encoder; > > if (dev->of_node) > crtcs = drm_of_find_possible_crtcs(drm, dev->of_node); > @@ -1691,7 +1717,8 @@ static int tda998x_init(struct device *dev, struct > drm_device *drm) crtcs = 1 << 0; > } > > - priv->encoder.possible_crtcs = crtcs; > + if (local_encoder) > + priv->encoder.possible_crtcs = crtcs; > > ret = tda998x_create(client, priv); > if (ret) > @@ -1700,11 +1727,15 @@ static int tda998x_init(struct device *dev, struct > drm_device *drm) if (!dev->of_node && params) > tda998x_set_config(priv, params); > > - drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs); > - ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs, > - DRM_MODE_ENCODER_TMDS, NULL); > - if (ret) > - goto err_encoder; > + if (local_encoder) { > + drm_encoder_helper_add(&priv->encoder, > + &tda998x_encoder_helper_funcs); > + ret = drm_encoder_init(drm, &priv->encoder, > + &tda998x_encoder_funcs, > + DRM_MODE_ENCODER_TMDS, NULL); > + if (ret) > + goto err_encoder; > + } > > ret = tda998x_connector_init(priv, drm); > if (ret) > @@ -1713,7 +1744,8 @@ static int tda998x_init(struct device *dev, struct > drm_device *drm) return 0; > > err_connector: > - drm_encoder_cleanup(&priv->encoder); > + if (local_encoder) > + drm_encoder_cleanup(&priv->encoder); > err_encoder: > tda998x_destroy(priv); > return ret; > @@ -1721,10 +1753,12 @@ static int tda998x_init(struct device *dev, struct > drm_device *drm) > > static void tda998x_fini(struct device *dev) > { > - struct tda998x_priv *priv = dev_get_drvdata(dev); > + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > + struct tda998x_priv *priv = bridge->priv; > > drm_connector_cleanup(&priv->connector); > - drm_encoder_cleanup(&priv->encoder); > + if (priv->local_encoder) > + drm_encoder_cleanup(&priv->encoder); > tda998x_destroy(priv); > } > > @@ -1732,7 +1766,7 @@ static int tda998x_bind(struct device *dev, struct > device *master, void *data) { > struct drm_device *drm = data; > > - return tda998x_init(dev, drm); > + return tda998x_init(dev, drm, true); > } > > static void tda998x_unbind(struct device *dev, struct device *master, > @@ -1746,19 +1780,102 @@ static const struct component_ops tda998x_ops = { > .unbind = tda998x_unbind, > }; > > +/* DRM bridge functions */ > + > +static int tda998x_bridge_attach(struct drm_bridge *dbridge) > +{ > + struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge); > + struct device *dev = bridge->dev; > + struct drm_device *drm = bridge->bridge.dev; > + > + dev_info(dev, "attach\n"); You can drop all the dev_info messages. > + return tda998x_init(dev, drm, false); > +} > + > +static void tda998x_bridge_detach(struct drm_bridge *dbridge) > +{ > + struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge); > + struct device *dev = bridge->dev; > + > + dev_info(dev, "detach\n"); > + tda998x_fini(dev); > +} > + > +static void tda998x_bridge_enable(struct drm_bridge *dbridge) > +{ > + struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge); > + struct tda998x_priv *priv = bridge->priv; > + > + dev_info(bridge->dev, "enable\n"); > + tda998x_dpms(priv, DRM_MODE_DPMS_ON); > +} > + > +static void tda998x_bridge_disable(struct drm_bridge *dbridge) > +{ > + struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge); > + struct tda998x_priv *priv = bridge->priv; > + > + dev_info(bridge->dev, "disable\n"); > + tda998x_dpms(priv, DRM_MODE_DPMS_OFF); > +} > + > +static void tda998x_bridge_mode_set(struct drm_bridge *dbridge, > + struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode) > +{ > + struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge); > + struct tda998x_priv *priv = bridge->priv; > + > + tda998x_mode_set(priv, mode, adjusted_mode); > +} > + > +static const struct drm_bridge_funcs tda998x_bridge_funcs = { > + .attach = tda998x_bridge_attach, > + .detach = tda998x_bridge_detach, > + .enable = tda998x_bridge_enable, > + .disable = tda998x_bridge_disable, > + .mode_set = tda998x_bridge_mode_set, > +}; > + > static int > tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id) > { > + struct device *dev = &client->dev; > + struct tda998x_bridge *bridge; > + int ret; > + > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > dev_warn(&client->dev, "adapter does not support I2C\n"); > return -EIO; > } > - return component_add(&client->dev, &tda998x_ops); > + > + bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL); > + if (!bridge) > + return -ENOMEM; > + > + bridge->dev = dev; > + dev_set_drvdata(dev, bridge); > + > + bridge->bridge.funcs = &tda998x_bridge_funcs; > + bridge->bridge.of_node = dev->of_node; > + drm_bridge_add(&bridge->bridge); > + > + ret = component_add(dev, &tda998x_ops); > + > + if (ret) > + drm_bridge_remove(&bridge->bridge); > + > + return ret; > } > > static int tda998x_remove(struct i2c_client *client) > { > - component_del(&client->dev, &tda998x_ops); > + struct device *dev = &client->dev; > + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > + > + drm_bridge_remove(&bridge->bridge); > + component_del(dev, &tda998x_ops); > + > return 0; > } -- Regards, Laurent Pinchart