LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Maxime Ripard <maxime@cerno.tech>
To: Kevin Tang <kevin3.tang@gmail.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
	Daniel Vetter <daniel@ffwll.ch>, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	pony1.wu@gmail.com, Orson Zhai <orsonzhai@gmail.com>,
	Chunyan Zhang <zhang.lyra@gmail.com>,
	"Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>,
	ML dri-devel <dri-devel@lists.freedesktop.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v6 6/6] drm/sprd: add Unisoc's drm mipi dsi&dphy driver
Date: Tue, 28 Sep 2021 11:28:05 +0200	[thread overview]
Message-ID: <20210928092805.wbc4ev3ze7a7zgqr@gilmour> (raw)
In-Reply-To: <CAFPSGXZbqh0f6kEoQaq_Nt677ksVS6QPdAa5==KVVAszSAuasw@mail.gmail.com>

On Sun, Sep 26, 2021 at 10:31:53PM +0800, Kevin Tang wrote:
> Maxime Ripard <maxime@cerno.tech> 于2021年9月17日周五 下午11:40写道:
> > > +static void sprd_dsi_encoder_mode_set(struct drm_encoder *encoder,
> > > +                              struct drm_display_mode *mode,
> > > +                              struct drm_display_mode *adj_mode)
> > > +{
> > > +     struct sprd_dsi *dsi = encoder_to_dsi(encoder);
> > > +
> > > +     drm_dbg(dsi->drm, "%s() set mode: %s\n", __func__, dsi->mode->name);
> > > +}
> >
> > You don't need that function?
> No need for now. need to delete it?

Yes

> > > +static int sprd_dsi_encoder_atomic_check(struct drm_encoder *encoder,
> > > +                                 struct drm_crtc_state *crtc_state,
> > > +                                 struct drm_connector_state *conn_state)
> > > +{
> > > +     return 0;
> > > +}
> >
> > Ditto
>
> No need for now. need to delete it?

Yep

> > > +static int sprd_dsi_find_panel(struct sprd_dsi *dsi)
> > > +{
> > > +     struct device *dev = dsi->host.dev;
> > > +     struct device_node *child, *lcds_node;
> > > +     struct drm_panel *panel;
> > > +
> > > +     /* search /lcds child node first */
> > > +     lcds_node = of_find_node_by_path("/lcds");
> > > +     for_each_child_of_node(lcds_node, child) {
> > > +             panel = of_drm_find_panel(child);
> > > +             if (!IS_ERR(panel)) {
> > > +                     dsi->panel = panel;
> > > +                     return 0;
> > > +             }
> > > +     }
> > > +
> > > +     /*
> > > +      * If /lcds child node search failed, we search
> > > +      * the child of dsi host node.
> > > +      */
> > > +     for_each_child_of_node(dev->of_node, child) {
> > > +             panel = of_drm_find_panel(child);
> > > +             if (!IS_ERR(panel)) {
> > > +                     dsi->panel = panel;
> > > +                     return 0;
> > > +             }
> > > +     }
> > > +
> > > +     drm_err(dsi->drm, "of_drm_find_panel() failed\n");
> > > +     return -ENODEV;
> > > +}
> >
> > Just use devm_drm_of_get_bridge there
>
> We use drm_panel_init and drm_panel_add API to add panel, so here is a
> panel device, not a bridge.

Like Sam said, the panel API is on its way out and is being superseded
by bridge_panels.

> > > +static int sprd_dsi_host_init(struct sprd_dsi *dsi, struct device *dev)
> > > +{
> > > +     int ret;
> > > +
> > > +     dsi->host.dev = dev;
> > > +     dsi->host.ops = &sprd_dsi_host_ops;
> > > +
> > > +     ret = mipi_dsi_host_register(&dsi->host);
> > > +     if (ret)
> > > +             drm_err(dsi->drm, "failed to register dsi host\n");
> > > +
> > > +     return ret;
> > > +}
> > >
> > > [...]
> > >
> > > +static int sprd_dsi_connector_init(struct drm_device *drm, struct sprd_dsi *dsi)
> > > +{
> > > +     struct drm_encoder *encoder = &dsi->encoder;
> > > +     struct drm_connector *connector = &dsi->connector;
> > > +     int ret;
> > > +
> > > +     connector->polled = DRM_CONNECTOR_POLL_HPD;
> > > +
> > > +     ret = drm_connector_init(drm, connector,
> > > +                              &sprd_dsi_atomic_connector_funcs,
> > > +                              DRM_MODE_CONNECTOR_DSI);
> > > +     if (ret) {
> > > +             drm_err(drm, "drm_connector_init() failed\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     drm_connector_helper_add(connector,
> > > +                              &sprd_dsi_connector_helper_funcs);
> > > +
> > > +     drm_connector_attach_encoder(connector, encoder);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int sprd_dsi_context_init(struct sprd_dsi *dsi,
> > > +                     struct device *dev)
> > > +{
> > > +     struct platform_device *pdev = to_platform_device(dev);
> > > +     struct dsi_context *ctx = &dsi->ctx;
> > > +     struct resource *res;
> > > +
> > > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +     ctx->base = devm_ioremap(dev, res->start, resource_size(res));
> > > +     if (!ctx->base) {
> > > +             drm_err(dsi->drm, "failed to map dsi host registers\n");
> > > +             return -ENXIO;
> > > +     }
> > > +
> > > +     ctx->pll = devm_kzalloc(dev, sizeof(*ctx->pll), GFP_KERNEL);
> > > +     if (!ctx->pll)
> > > +             return -ENOMEM;
> > > +
> > > +     ctx->regmap = devm_regmap_init(dev, &regmap_tst_io, dsi, &byte_config);
> > > +     if (IS_ERR(ctx->regmap)) {
> > > +             drm_err(dsi->drm, "dphy regmap init failed\n");
> > > +             return PTR_ERR(ctx->regmap);
> > > +     }
> > > +
> > > +     ctx->data_hs2lp = 120;
> > > +     ctx->data_lp2hs = 500;
> > > +     ctx->clk_hs2lp = 4;
> > > +     ctx->clk_lp2hs = 15;
> > > +     ctx->max_rd_time = 6000;
> > > +     ctx->int0_mask = 0xffffffff;
> > > +     ctx->int1_mask = 0xffffffff;
> > > +     ctx->enabled = true;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int sprd_dsi_bind(struct device *dev, struct device *master, void *data)
> > > +{
> > > +     struct drm_device *drm = data;
> > > +     struct sprd_dsi *dsi;
> > > +     int ret;
> > > +
> > > +     dsi = sprd_dsi_encoder_init(drm, dev);
> > > +     if (IS_ERR(dsi))
> > > +             return PTR_ERR(dsi);
> > > +
> > > +     dsi->drm = drm;
> > > +     dev_set_drvdata(dev, dsi);
> > > +
> > > +     ret = sprd_dsi_connector_init(drm, dsi);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = sprd_dsi_context_init(dsi, dev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = sprd_dsi_host_init(dsi, dev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void sprd_dsi_unbind(struct device *dev,
> > > +                     struct device *master, void *data)
> > > +{
> > > +     struct sprd_dsi *dsi = dev_get_drvdata(dev);
> > > +
> > > +     mipi_dsi_host_unregister(&dsi->host);
> > > +}
> > > +
> > > +static const struct component_ops dsi_component_ops = {
> > > +     .bind   = sprd_dsi_bind,
> > > +     .unbind = sprd_dsi_unbind,
> > > +};
> > > +
> > > +static const struct of_device_id dsi_match_table[] = {
> > > +     { .compatible = "sprd,sharkl3-dsi-host" },
> > > +     { /* sentinel */ },
> > > +};
> > > +
> > > +static int sprd_dsi_probe(struct platform_device *pdev)
> > > +{
> > > +     return component_add(&pdev->dev, &dsi_component_ops);
> >
> > In order to prevent probe issues, you need to register you mipi_dsi_host
> > here, see:
> > https://lore.kernel.org/dri-devel/20210910101218.1632297-3-maxime@cerno.tech/
>
> We register mipi_dsi_hot on our panel driver, like this:
> 
> 1092   ret = mipi_dsi_attach(slave);
> 1093   if (ret) {
> 1094   DRM_ERROR("failed to attach dsi panel to host\n");
> 1095   drm_panel_remove(&panel->base);
> 1096   return ret;
> 1097   }

It's not about when you attach, but when you call
mipi_dsi_host_register. You're doing it in sprd_dsi_host_init that you
call in bind(), which is against the best practices and will create
probing issues in the future.

Maxime

  reply	other threads:[~2021-09-28  9:28 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13 14:52 [PATCH v6 0/6] Add Unisoc's drm kms module Kevin Tang
2021-08-13 14:52 ` [PATCH v6 1/6] dt-bindings: display: add Unisoc's drm master bindings Kevin Tang
2021-08-13 14:52 ` [PATCH v6 2/6] drm/sprd: add Unisoc's drm kms master Kevin Tang
2021-08-13 14:52 ` [PATCH v6 3/6] dt-bindings: display: add Unisoc's dpu bindings Kevin Tang
2021-08-13 14:53 ` [PATCH v6 4/6] drm/sprd: add Unisoc's drm display controller driver Kevin Tang
2021-09-17 14:58   ` Maxime Ripard
2021-09-26 13:44     ` Kevin Tang
2021-08-13 14:53 ` [PATCH v6 5/6] dt-bindings: display: add Unisoc's mipi dsi controller bindings Kevin Tang
2021-08-13 14:53 ` [PATCH v6 6/6] drm/sprd: add Unisoc's drm mipi dsi&dphy driver Kevin Tang
2021-09-17 15:40   ` Maxime Ripard
2021-09-26 14:31     ` Kevin Tang
2021-09-28  9:28       ` Maxime Ripard [this message]
2021-10-06 17:54         ` Kevin Tang
2021-10-20 10:09         ` Kevin Tang
2021-10-21  8:00           ` Maxime Ripard
     [not found]       ` <YVCgznmOA97v30wG@ravnborg.org>
2021-10-06 18:25         ` Kevin Tang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210928092805.wbc4ev3ze7a7zgqr@gilmour \
    --to=maxime@cerno.tech \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=kevin3.tang@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mark.rutland@arm.com \
    --cc=orsonzhai@gmail.com \
    --cc=pony1.wu@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=sean@poorly.run \
    --cc=zhang.lyra@gmail.com \
    --subject='Re: [PATCH v6 6/6] drm/sprd: add Unisoc'\''s drm mipi dsi&dphy driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).