From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector Date: Mon, 24 Mar 2014 23:39:01 +0100 Message-ID: <1458827.cQ6aDWdh1W@avalon> References: Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 93C0789E39 for ; Mon, 24 Mar 2014 15:37:09 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: dri-devel@lists.freedesktop.org Cc: linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Russell King List-Id: dri-devel@lists.freedesktop.org Hi Jean-Fran=E7ois, Thank you for the patch. On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote: > The 'slave encoder' structure of the tda998x driver asks for glue > between the DRM driver and the encoder/connector structures. > = > This patch changes the driver to a normal DRM encoder/connector > thanks to the infrastructure for componentised subsystems. I like the idea, but I'm not really happy with the implementation. Let me t= ry = to explain why below. > Signed-off-by: Jean-Francois Moine > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++-------------= --- > 1 file changed, 188 insertions(+), 135 deletions(-) > = > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c [snip] > @@ -44,10 +45,14 @@ struct tda998x_priv { > = > wait_queue_head_t wq_edid; > volatile int wq_edid_wait; > - struct drm_encoder *encoder; > + struct drm_encoder encoder; > + struct drm_connector connector; > }; [snip] > -static int > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id) > +static int tda_bind(struct device *dev, struct device *master, void *dat= a) > { > + struct drm_device *drm =3D data; This is the part that bothers me. You're making two assumptions here, that = the = DRM driver will pass a struct drm_device pointer to the bind operation, and = that the I2C encoder driver can take control of DRM encoder and connector = creation. Although it could become problematic later, the first assumption = isn't too much of an issue for now. I'll thus focus on the second one. The component framework isolate the encoder and DRM master drivers as far a= s = component creation and binding is concerned, but doesn't provide a way for = the = two drivers to communicate together (nor should it). You're solving this by = passing a pointer to the DRM device to the encoder bind operation, making t= he = encoder driver create a DRM encoder and connector, and relying on the DRM c= ore = to orchestrate CRTCs, encoders and connectors. You thus assume that the = encoder hardware should be represented by a DRM encoder object, and that it= s = output is connected to a connector that should be represented by a DRM = connector object. While this can work in your use case, that won't always h= old = true. Hardware encoders can be chained together, while DRM encoders can't. = The = DRM core has recently received support for bridge objects to overcome that = limitation. Depending on the hardware topology, a given hardware encoder = should be modeled as a DRM encoder or as a DRM bridge. That decision should= n't = be taken by the encoder driver but by the DRM master driver. The I2C encode= r = driver thus shouldn't create the DRM encoder and DRM connector itself. I believe the encoder/master communication problem should be solved = differently. Instead of passing a pointer to the DRM device to the encoder = driver and making the encoder driver control DRM encoder and connector = creation, the encoder driver should instead create an object not visible to = userspace that can be retrieved by the DRM master driver (possibly through = registration with the DRM core, or by going through drvdata in the encoder'= s = struct device). The DRM master could use that object to communicate with th= e = encoder, and would register the DRM encoder and DRM connector itself based = on = hardware topology. > + struct i2c_client *i2c_client =3D to_i2c_client(dev); > + struct tda998x_priv *priv =3D i2c_get_clientdata(i2c_client); > + struct drm_connector *connector =3D &priv->connector; > + struct drm_encoder *encoder =3D &priv->encoder; > + int ret; > + > + if (!try_module_get(THIS_MODULE)) { > + dev_err(dev, "cannot get module %s\n", THIS_MODULE->name); > + return -EINVAL; > + } > + > + ret =3D drm_connector_init(drm, connector, > + &connector_funcs, > + DRM_MODE_CONNECTOR_HDMIA); This is one example of the shortcomings I've explained above. An encoder = driver can't always know what connector type it is connected to. If I'm not = mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should b= e = up to the master driver to select the connector type based on its overall v= iew = of the hardware, or even to a connector driver that would be bound to a = connector DT node (as proposed in https://www.mail-archive.com/devicetree@v= ger.kernel.org/msg16585.html). > + if (ret < 0) > + return ret; > + drm_connector_helper_add(connector, &connector_helper_funcs); > + > + ret =3D drm_encoder_init(drm, encoder, > + &encoder_funcs, > + DRM_MODE_ENCODER_TMDS); > + > + encoder->possible_crtcs =3D 1; // 1 << lcd_num > + > + if (ret < 0) > + goto err; > + drm_encoder_helper_add(encoder, &encoder_helper_funcs); > + > + ret =3D drm_mode_connector_attach_encoder(connector, encoder); > + if (ret < 0) > + goto err; > + connector->encoder =3D encoder; > + > + drm_sysfs_connector_add(connector); > + > + drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF); > + ret =3D drm_object_property_set_value(&connector->base, > + drm->mode_config.dpms_property, > + DRM_MODE_DPMS_OFF); > + > + if (priv->hdmi->irq) > + connector->polled =3D DRM_CONNECTOR_POLL_HPD; > + else > + connector->polled =3D DRM_CONNECTOR_POLL_CONNECT | > + DRM_CONNECTOR_POLL_DISCONNECT; > return 0; > + > +err: > + if (encoder->dev) > + drm_encoder_cleanup(encoder); > + if (connector->dev) > + drm_connector_cleanup(connector); > + return ret; > } > = > -static int > -tda998x_remove(struct i2c_client *client) > +static void tda_unbind(struct device *dev, struct device *master, void > *data) { > - return 0; > + module_put(THIS_MODULE); > } > = > +static const struct component_ops comp_ops =3D { > + .bind =3D tda_bind, > + .unbind =3D tda_unbind, > +}; > + > static int > -tda998x_encoder_init(struct i2c_client *client, > - struct drm_device *dev, > - struct drm_encoder_slave *encoder_slave) > +tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id) > { > struct tda998x_priv *priv; > struct device_node *np =3D client->dev.of_node; > @@ -1239,6 +1291,8 @@ tda998x_encoder_init(struct i2c_client *client, > if (!priv) > return -ENOMEM; > = > + i2c_set_clientdata(client, priv); > + > priv->vip_cntrl_0 =3D VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3); > priv->vip_cntrl_1 =3D VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); > priv->vip_cntrl_2 =3D VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5); > @@ -1250,13 +1304,8 @@ tda998x_encoder_init(struct i2c_client *client, > kfree(priv); > return -ENODEV; > } > - > - priv->encoder =3D &encoder_slave->base; > priv->dpms =3D DRM_MODE_DPMS_OFF; > = > - encoder_slave->slave_priv =3D priv; > - encoder_slave->slave_funcs =3D &tda998x_encoder_funcs; > - > /* wake up the device: */ > cec_write(priv, REG_CEC_ENAMODS, > CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI); > @@ -1340,31 +1389,55 @@ tda998x_encoder_init(struct i2c_client *client, > /* enable EDID read irq: */ > reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); > = > - if (!np) > - return 0; /* non-DT */ > + if (np) { /* if DT */ > = > - /* get the optional video properties */ > - ret =3D of_property_read_u32(np, "video-ports", &video); > - if (ret =3D=3D 0) { > - priv->vip_cntrl_0 =3D video >> 16; > - priv->vip_cntrl_1 =3D video >> 8; > - priv->vip_cntrl_2 =3D video; > + /* get the optional video properties */ > + ret =3D of_property_read_u32(np, "video-ports", &video); > + if (ret =3D=3D 0) { > + priv->vip_cntrl_0 =3D video >> 16; > + priv->vip_cntrl_1 =3D video >> 8; > + priv->vip_cntrl_2 =3D video; > + } > + } else { > + struct tda998x_encoder_params *params =3D > + (struct tda998x_encoder_params) > + client->dev.platform_data; > + > + if (params) > + tda998x_encoder_set_config(priv, params); > } > = > + /* tda998x video component ready */ > + component_add(&client->dev, &comp_ops); > + > return 0; > = > fail: > - /* if encoder_init fails, the encoder slave is never registered, > - * so cleanup here: > - */ > if (priv->cec) > i2c_unregister_device(priv->cec); > kfree(priv); > - encoder_slave->slave_priv =3D NULL; > - encoder_slave->slave_funcs =3D NULL; > return -ENXIO; > } > = > +static int > +tda998x_remove(struct i2c_client *client) > +{ > + struct tda998x_priv *priv =3D i2c_get_clientdata(client); > + > + /* disable all IRQs and free the IRQ handler */ > + cec_write(priv, REG_CEC_RXSHPDINTENA, 0); > + reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); > + if (priv->hdmi->irq) > + free_irq(priv->hdmi->irq, priv); > + > + component_del(&client->dev, &comp_ops); > + > + if (priv->cec) > + i2c_unregister_device(priv->cec); > + kfree(priv); > + return 0; > +} > + > #ifdef CONFIG_OF > static const struct of_device_id tda998x_dt_ids[] =3D { > { .compatible =3D "nxp,tda9989", }, > @@ -1381,38 +1454,18 @@ static struct i2c_device_id tda998x_ids[] =3D { > }; > MODULE_DEVICE_TABLE(i2c, tda998x_ids); > = > -static struct drm_i2c_encoder_driver tda998x_driver =3D { > - .i2c_driver =3D { > - .probe =3D tda998x_probe, > - .remove =3D tda998x_remove, > - .driver =3D { > - .name =3D "tda998x", > - .of_match_table =3D of_match_ptr(tda998x_dt_ids), > - }, > - .id_table =3D tda998x_ids, > +static struct i2c_driver tda998x_driver =3D { > + .probe =3D tda998x_probe, > + .remove =3D tda998x_remove, > + .driver =3D { > + .name =3D "tda998x", > + .of_match_table =3D of_match_ptr(tda998x_dt_ids), > }, > - .encoder_init =3D tda998x_encoder_init, > + .id_table =3D tda998x_ids, > }; -- = Regards, Laurent Pinchart