| Message ID | 77554dd2612f418f6ab74a8be06c82b71410e0e6.1628172477.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: jz4780 HDMI | expand |
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; > }
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
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
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
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 --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; }