diff mbox series

[v2,8/8,RFC] drm/ingenic: convert to component framework for jz4780 hdmi

Message ID 77554dd2612f418f6ab74a8be06c82b71410e0e6.1628172477.git.hns@goldelico.com (mailing list archive)
State New, archived
Headers show
Series MIPS: jz4780 HDMI | expand

Commit Message

H. Nikolaus Schaller Aug. 5, 2021, 2:07 p.m. UTC 
This patch attempts to convert the ingenic-dw-hdmi driver
into a version that uses the component framework.

Unfortunately the new version does not work.

Debugging shows that ingenic_dw_hdmi_bind() is never called.

Suggestions for reasons and fixes are welcome.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Co-authored-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 57 ++++++++++++++++++-----
 1 file changed, 46 insertions(+), 11 deletions(-)

Comments

Laurent Pinchart Aug. 5, 2021, 3:04 p.m. UTC | #1 
Hi Nikolaus,

Thank you for the patch.

On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller wrote:
> This patch attempts to convert the ingenic-dw-hdmi driver
> into a version that uses the component framework.

Why ? What problem would this solve ?

> Unfortunately the new version does not work.
> 
> Debugging shows that ingenic_dw_hdmi_bind() is never called.
> 
> Suggestions for reasons and fixes are welcome.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Co-authored-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 57 ++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> index 61e7a57d7cec1..a5ba0b69baa8c 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> @@ -1,17 +1,24 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
> - * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
> + * Copyright (C) 2019, 2020, 2021 Paul Boddie <paul@boddie.org.uk>
>   *
>   * Derived from dw_hdmi-imx.c with i.MX portions removed.
> - * Probe and remove operations derived from rcar_dw_hdmi.c.
>   */
>  
> +#include <linux/component.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  
>  #include <drm/bridge/dw_hdmi.h>
>  #include <drm/drm_of.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_simple_kms_helper.h>
> +
> +struct ingenic_dw_hdmi_encoder {
> +	struct drm_encoder encoder;
> +	struct dw_hdmi *hdmi;
> +};
>  
>  static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
>  	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
> @@ -87,24 +94,52 @@ static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
>  
> -static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
> +static int ingenic_dw_hdmi_bind(struct device *dev, struct device *master,
> +				void *data)
>  {
> -	struct dw_hdmi *hdmi;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm = data;
> +	struct drm_encoder *enc;
> +	struct ingenic_dw_hdmi_encoder *hdmi_encoder;
>  
> -	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
> -	if (IS_ERR(hdmi))
> -		return PTR_ERR(hdmi);
> +	hdmi_encoder = drmm_simple_encoder_alloc(drm, struct ingenic_dw_hdmi_encoder,
> +						 encoder, DRM_MODE_ENCODER_TMDS);
> +	if (IS_ERR(hdmi_encoder))
> +		return PTR_ERR(hdmi_encoder);
>  
> -	platform_set_drvdata(pdev, hdmi);
> +	enc = &hdmi_encoder->encoder;
> +	drm_encoder_helper_add(enc, NULL);
> +	hdmi_encoder->hdmi = dw_hdmi_bind(pdev, enc, &ingenic_dw_hdmi_plat_data);
> +
> +	if (IS_ERR(hdmi_encoder->hdmi))
> +		return PTR_ERR(hdmi_encoder->hdmi);
> +
> +	dev_set_drvdata(dev, hdmi_encoder->hdmi);
>  
>  	return 0;
>  }
>  
> -static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
> +static void ingenic_dw_hdmi_unbind(struct device *dev, struct device *master,
> +				   void *data)
> +{
> +	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
> +
> +	dw_hdmi_unbind(hdmi);
> +}
> +
> +static const struct component_ops ingenic_dw_hdmi_ops = {
> +	.bind	= ingenic_dw_hdmi_bind,
> +	.unbind	= ingenic_dw_hdmi_unbind,
> +};
> +
> +static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
>  {
> -	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
> +	return component_add(&pdev->dev, &ingenic_dw_hdmi_ops);
> +}
>  
> -	dw_hdmi_remove(hdmi);
> +static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
> +{
> +	component_del(&pdev->dev, &ingenic_dw_hdmi_ops);
>  
>  	return 0;
>  }
H. Nikolaus Schaller Aug. 5, 2021, 4:07 p.m. UTC | #2 
Hi Laurent,

> Am 05.08.2021 um 17:04 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 
> Hi Nikolaus,
> 
> Thank you for the patch.
> 
> On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller wrote:
>> This patch attempts to convert the ingenic-dw-hdmi driver
>> into a version that uses the component framework.
> 
> Why ? What problem would this solve ?

Well, it was suggested in a v1 we did post several months ago. I have not
looked up by whom and do not exactly remember the reasons.

We now simply thought that it is common style since dome dw-hdmi drivers
make use of it but some others don't. And we got it working without.

If it is not needed/requested by anyone, we can drop it from v3 (or add later).

BR and thanks,
Nikolaus
Paul Cercueil Aug. 5, 2021, 4:17 p.m. UTC | #3 
Hi Nikolaus and Laurent,

Le jeu., août 5 2021 at 18:07:20 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Laurent,
> 
>>  Am 05.08.2021 um 17:04 schrieb Laurent Pinchart 
>> <laurent.pinchart@ideasonboard.com>:
>> 
>>  Hi Nikolaus,
>> 
>>  Thank you for the patch.
>> 
>>  On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller 
>> wrote:
>>>  This patch attempts to convert the ingenic-dw-hdmi driver
>>>  into a version that uses the component framework.
>> 
>>  Why ? What problem would this solve ?
> 
> Well, it was suggested in a v1 we did post several months ago. I have 
> not
> looked up by whom and do not exactly remember the reasons.
> 
> We now simply thought that it is common style since dome dw-hdmi 
> drivers
> make use of it but some others don't. And we got it working without.
> 
> If it is not needed/requested by anyone, we can drop it from v3 (or 
> add later).

I don't remember exactly TBH - the only reason to use a component is to 
have access to the main driver's "drm_device" structure. The IPU needs 
it for instance, to register planes; but I don't think this HDMI driver 
needs it as it registers a bridge.

Cheers,
-Paul
H. Nikolaus Schaller Aug. 5, 2021, 4:22 p.m. UTC | #4 
Hi Paul,

> Am 05.08.2021 um 18:17 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus and Laurent,
> 
> Le jeu., août 5 2021 at 18:07:20 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Laurent,
>>> Am 05.08.2021 um 17:04 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>>> Hi Nikolaus,
>>> Thank you for the patch.
>>> On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller wrote:
>>>> This patch attempts to convert the ingenic-dw-hdmi driver
>>>> into a version that uses the component framework.
>>> Why ? What problem would this solve ?
>> Well, it was suggested in a v1 we did post several months ago. I have not
>> looked up by whom and do not exactly remember the reasons.
>> We now simply thought that it is common style since dome dw-hdmi drivers
>> make use of it but some others don't. And we got it working without.
>> If it is not needed/requested by anyone, we can drop it from v3 (or add later).
> 
> I don't remember exactly TBH - the only reason to use a component is to have access to the main driver's "drm_device" structure. The IPU needs it for instance, to register planes; but I don't think this HDMI driver needs it as it registers a bridge.
> 
> Cheers,
> -Paul

Ok, fine! We'll drop it and don't waste time.

BR and thanks,
NIkolaus
Daniel Vetter Aug. 10, 2021, 9:31 a.m. UTC | #5 
On Thu, Aug 05, 2021 at 06:17:32PM +0200, Paul Cercueil wrote:
> Hi Nikolaus and Laurent,
> 
> Le jeu., août 5 2021 at 18:07:20 +0200, H. Nikolaus Schaller
> <hns@goldelico.com> a écrit :
> > Hi Laurent,
> > 
> > >  Am 05.08.2021 um 17:04 schrieb Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com>:
> > > 
> > >  Hi Nikolaus,
> > > 
> > >  Thank you for the patch.
> > > 
> > >  On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller
> > > wrote:
> > > >  This patch attempts to convert the ingenic-dw-hdmi driver
> > > >  into a version that uses the component framework.
> > > 
> > >  Why ? What problem would this solve ?
> > 
> > Well, it was suggested in a v1 we did post several months ago. I have
> > not
> > looked up by whom and do not exactly remember the reasons.
> > 
> > We now simply thought that it is common style since dome dw-hdmi drivers
> > make use of it but some others don't. And we got it working without.
> > 
> > If it is not needed/requested by anyone, we can drop it from v3 (or add
> > later).
> 
> I don't remember exactly TBH - the only reason to use a component is to have
> access to the main driver's "drm_device" structure. The IPU needs it for
> instance, to register planes; but I don't think this HDMI driver needs it as
> it registers a bridge.

Imo for bridges/panels we really should move _away_ from component, not
towards it. If there's a gap in the bridge/panel api (I think there's some
patches floating around exactly to make this more a multi-step process for
reasons like the above) then we need to fix that.

Unfortunately the dw-hdmi and also dw-dsi drivers are very much built on
top of component.c and side-step the bridge stuff quite a lot. That
results in quite bad integration pains all around as we add more users of
these.

The other unfortunate thing is that there's not many people working in
this area, so fundamental improvements to the core design take a long time
to make and then even longer to roll out. It's a bit a tough spot, but
also, help very much welcome :-)
-Daniel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
index 61e7a57d7cec1..a5ba0b69baa8c 100644
--- a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
+++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
@@ -1,17 +1,24 @@ 
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
- * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
+ * Copyright (C) 2019, 2020, 2021 Paul Boddie <paul@boddie.org.uk>
  *
  * Derived from dw_hdmi-imx.c with i.MX portions removed.
- * Probe and remove operations derived from rcar_dw_hdmi.c.
  */
 
+#include <linux/component.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
 #include <drm/bridge/dw_hdmi.h>
 #include <drm/drm_of.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_simple_kms_helper.h>
+
+struct ingenic_dw_hdmi_encoder {
+	struct drm_encoder encoder;
+	struct dw_hdmi *hdmi;
+};
 
 static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
 	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
@@ -87,24 +94,52 @@  static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
 
-static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
+static int ingenic_dw_hdmi_bind(struct device *dev, struct device *master,
+				void *data)
 {
-	struct dw_hdmi *hdmi;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *drm = data;
+	struct drm_encoder *enc;
+	struct ingenic_dw_hdmi_encoder *hdmi_encoder;
 
-	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
-	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+	hdmi_encoder = drmm_simple_encoder_alloc(drm, struct ingenic_dw_hdmi_encoder,
+						 encoder, DRM_MODE_ENCODER_TMDS);
+	if (IS_ERR(hdmi_encoder))
+		return PTR_ERR(hdmi_encoder);
 
-	platform_set_drvdata(pdev, hdmi);
+	enc = &hdmi_encoder->encoder;
+	drm_encoder_helper_add(enc, NULL);
+	hdmi_encoder->hdmi = dw_hdmi_bind(pdev, enc, &ingenic_dw_hdmi_plat_data);
+
+	if (IS_ERR(hdmi_encoder->hdmi))
+		return PTR_ERR(hdmi_encoder->hdmi);
+
+	dev_set_drvdata(dev, hdmi_encoder->hdmi);
 
 	return 0;
 }
 
-static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
+static void ingenic_dw_hdmi_unbind(struct device *dev, struct device *master,
+				   void *data)
+{
+	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(hdmi);
+}
+
+static const struct component_ops ingenic_dw_hdmi_ops = {
+	.bind	= ingenic_dw_hdmi_bind,
+	.unbind	= ingenic_dw_hdmi_unbind,
+};
+
+static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
 {
-	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
+	return component_add(&pdev->dev, &ingenic_dw_hdmi_ops);
+}
 
-	dw_hdmi_remove(hdmi);
+static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &ingenic_dw_hdmi_ops);
 
 	return 0;
 }