| Message ID | 20211213121929.3377752-1-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Jagan, Thank you for the patch. On Mon, Dec 13, 2021 at 05:49:29PM +0530, Jagan Teki wrote: > Replace the manual panel handling code by a drm panel_bridge via > devm_drm_of_get_bridge(). > > Adding panel_bridge handling, > > - Drops drm_connector and related operations as drm_bridge_attach > creates connector during attachment. > > - Drops panel pointer and panel healpers. > > This simplifies the driver and allows all components in the display > pipeline to be treated as bridges. > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > drivers/gpu/drm/bridge/tc358764.c | 99 ++----------------------------- > 1 file changed, 6 insertions(+), 93 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c > index c1e35bdf9232..28480bdc4287 100644 > --- a/drivers/gpu/drm/bridge/tc358764.c > +++ b/drivers/gpu/drm/bridge/tc358764.c > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { > struct tc358764 { > struct device *dev; > struct drm_bridge bridge; > - struct drm_connector connector; > + struct drm_bridge *panel_bridge; s/panel_bridge/next_bridge/ as it may not be a panel. > struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; > struct gpio_desc *gpio_reset; > - struct drm_panel *panel; Are there #includes that you can drop ? > int error; > }; > > @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge) > return container_of(bridge, struct tc358764, bridge); > } > > -static inline > -struct tc358764 *connector_to_tc358764(struct drm_connector *connector) > -{ > - return container_of(connector, struct tc358764, connector); > -} > - > static int tc358764_init(struct tc358764 *ctx) > { > u32 v = 0; > @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx) > usleep_range(1000, 2000); > } > > -static int tc358764_get_modes(struct drm_connector *connector) > -{ > - struct tc358764 *ctx = connector_to_tc358764(connector); > - > - return drm_panel_get_modes(ctx->panel, connector); > -} > - > -static const > -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = { > - .get_modes = tc358764_get_modes, > -}; > - > -static const struct drm_connector_funcs tc358764_connector_funcs = { > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = drm_connector_cleanup, > - .reset = drm_atomic_helper_connector_reset, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -static void tc358764_disable(struct drm_bridge *bridge) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel); > - > - if (ret < 0) > - dev_err(ctx->dev, "error disabling panel (%d)\n", ret); > -} > - > static void tc358764_post_disable(struct drm_bridge *bridge) > { > struct tc358764 *ctx = bridge_to_tc358764(bridge); > int ret; > > - ret = drm_panel_unprepare(ctx->panel); > - if (ret < 0) > - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret); > tc358764_reset(ctx); > usleep_range(10000, 15000); > ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); > @@ -335,72 +296,25 @@ static void tc358764_pre_enable(struct drm_bridge *bridge) > ret = tc358764_init(ctx); > if (ret < 0) > dev_err(ctx->dev, "error initializing bridge (%d)\n", ret); > - ret = drm_panel_prepare(ctx->panel); > - if (ret < 0) > - dev_err(ctx->dev, "error preparing panel (%d)\n", ret); > -} > - > -static void tc358764_enable(struct drm_bridge *bridge) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - int ret = drm_panel_enable(ctx->panel); > - > - if (ret < 0) > - dev_err(ctx->dev, "error enabling panel (%d)\n", ret); > } > > static int tc358764_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - struct drm_device *drm = bridge->dev; > - int ret; > - > - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { > - DRM_ERROR("Fix bridge driver to make connector optional!"); > - return -EINVAL; > - } > - > - ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; > - ret = drm_connector_init(drm, &ctx->connector, > - &tc358764_connector_funcs, > - DRM_MODE_CONNECTOR_LVDS); > - if (ret) { > - DRM_ERROR("Failed to initialize connector\n"); > - return ret; > - } > - > - drm_connector_helper_add(&ctx->connector, > - &tc358764_connector_helper_funcs); > - drm_connector_attach_encoder(&ctx->connector, bridge->encoder); > - ctx->connector.funcs->reset(&ctx->connector); > - drm_connector_register(&ctx->connector); > - > - return 0; > -} > - > -static void tc358764_detach(struct drm_bridge *bridge) > { > struct tc358764 *ctx = bridge_to_tc358764(bridge); > > - drm_connector_unregister(&ctx->connector); > - ctx->panel = NULL; > - drm_connector_put(&ctx->connector); > + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, bridge, flags); > } > > static const struct drm_bridge_funcs tc358764_bridge_funcs = { > - .disable = tc358764_disable, > .post_disable = tc358764_post_disable, > - .enable = tc358764_enable, > .pre_enable = tc358764_pre_enable, > .attach = tc358764_attach, > - .detach = tc358764_detach, > }; > > static int tc358764_parse_dt(struct tc358764 *ctx) > { > struct device *dev = ctx->dev; > - int ret; > > ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(ctx->gpio_reset)) { > @@ -408,12 +322,11 @@ static int tc358764_parse_dt(struct tc358764 *ctx) > return PTR_ERR(ctx->gpio_reset); > } > > - ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel, > - NULL); > - if (ret && ret != -EPROBE_DEFER) > - dev_err(dev, "cannot find panel (%d)\n", ret); > + ctx->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); > + if (IS_ERR(ctx->panel_bridge)) > + return PTR_ERR(ctx->panel_bridge); > > - return ret; > + return 0; > } > > static int tc358764_configure_regulators(struct tc358764 *ctx)
Hi Laurent, On Mon, Dec 13, 2021 at 6:02 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jagan, > > Thank you for the patch. > > On Mon, Dec 13, 2021 at 05:49:29PM +0530, Jagan Teki wrote: > > Replace the manual panel handling code by a drm panel_bridge via > > devm_drm_of_get_bridge(). > > > > Adding panel_bridge handling, > > > > - Drops drm_connector and related operations as drm_bridge_attach > > creates connector during attachment. > > > > - Drops panel pointer and panel healpers. > > > > This simplifies the driver and allows all components in the display > > pipeline to be treated as bridges. > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > drivers/gpu/drm/bridge/tc358764.c | 99 ++----------------------------- > > 1 file changed, 6 insertions(+), 93 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c > > index c1e35bdf9232..28480bdc4287 100644 > > --- a/drivers/gpu/drm/bridge/tc358764.c > > +++ b/drivers/gpu/drm/bridge/tc358764.c > > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { > > struct tc358764 { > > struct device *dev; > > struct drm_bridge bridge; > > - struct drm_connector connector; > > + struct drm_bridge *panel_bridge; > > s/panel_bridge/next_bridge/ as it may not be a panel. Sometime, I'm a strong believer of my own notation (I may be wrong) based on my understanding. This is downstream bridge and the only option it to connect is panel and panel in bridge terminology are treated as panel_bridge. This is the reason I have used panel_bridge. next_bridge notation will be used if the bridge connected to any downstream bridge, like we can use next_bridge notation in host bridge drivers as host bridge can be an option of connecting downstream bridge or panel. This is what I understood so-far with DRM bridges. May be you can correct if I'm wrong. > > > struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; > > struct gpio_desc *gpio_reset; > > - struct drm_panel *panel; > > Are there #includes that you can drop ? I think, yes. I will update it in v2. Thanks, Jagan.
On Mon, Dec 13, 2021 at 06:09:23PM +0530, Jagan Teki wrote: > Hi Laurent, > > On Mon, Dec 13, 2021 at 6:02 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Jagan, > > > > Thank you for the patch. > > > > On Mon, Dec 13, 2021 at 05:49:29PM +0530, Jagan Teki wrote: > > > Replace the manual panel handling code by a drm panel_bridge via > > > devm_drm_of_get_bridge(). > > > > > > Adding panel_bridge handling, > > > > > > - Drops drm_connector and related operations as drm_bridge_attach > > > creates connector during attachment. > > > > > > - Drops panel pointer and panel healpers. > > > > > > This simplifies the driver and allows all components in the display > > > pipeline to be treated as bridges. > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > --- > > > drivers/gpu/drm/bridge/tc358764.c | 99 ++----------------------------- > > > 1 file changed, 6 insertions(+), 93 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c > > > index c1e35bdf9232..28480bdc4287 100644 > > > --- a/drivers/gpu/drm/bridge/tc358764.c > > > +++ b/drivers/gpu/drm/bridge/tc358764.c > > > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { > > > struct tc358764 { > > > struct device *dev; > > > struct drm_bridge bridge; > > > - struct drm_connector connector; > > > + struct drm_bridge *panel_bridge; > > > > s/panel_bridge/next_bridge/ as it may not be a panel. > > Sometime, I'm a strong believer of my own notation (I may be wrong) > based on my understanding. This is downstream bridge and the only > option it to connect is panel and panel in bridge terminology are > treated as panel_bridge. This is the reason I have used panel_bridge. > next_bridge notation will be used if the bridge connected to any > downstream bridge, like we can use next_bridge notation in host bridge > drivers as host bridge can be an option of connecting downstream > bridge or panel. The downstream bridge doesn't have to be a DSI panel, it could be an LVDS-to-DPI bridge for instance, or an LVDS-to-HDMI encoder. > This is what I understood so-far with DRM bridges. May be you can > correct if I'm wrong. > > > > struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; > > > struct gpio_desc *gpio_reset; > > > - struct drm_panel *panel; > > > > Are there #includes that you can drop ? > > I think, yes. I will update it in v2.
Hi Laurent, On Mon, Dec 13, 2021 at 6:16 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Dec 13, 2021 at 06:09:23PM +0530, Jagan Teki wrote: > > Hi Laurent, > > > > On Mon, Dec 13, 2021 at 6:02 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Jagan, > > > > > > Thank you for the patch. > > > > > > On Mon, Dec 13, 2021 at 05:49:29PM +0530, Jagan Teki wrote: > > > > Replace the manual panel handling code by a drm panel_bridge via > > > > devm_drm_of_get_bridge(). > > > > > > > > Adding panel_bridge handling, > > > > > > > > - Drops drm_connector and related operations as drm_bridge_attach > > > > creates connector during attachment. > > > > > > > > - Drops panel pointer and panel healpers. > > > > > > > > This simplifies the driver and allows all components in the display > > > > pipeline to be treated as bridges. > > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > --- > > > > drivers/gpu/drm/bridge/tc358764.c | 99 ++----------------------------- > > > > 1 file changed, 6 insertions(+), 93 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c > > > > index c1e35bdf9232..28480bdc4287 100644 > > > > --- a/drivers/gpu/drm/bridge/tc358764.c > > > > +++ b/drivers/gpu/drm/bridge/tc358764.c > > > > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { > > > > struct tc358764 { > > > > struct device *dev; > > > > struct drm_bridge bridge; > > > > - struct drm_connector connector; > > > > + struct drm_bridge *panel_bridge; > > > > > > s/panel_bridge/next_bridge/ as it may not be a panel. > > > > Sometime, I'm a strong believer of my own notation (I may be wrong) > > based on my understanding. This is downstream bridge and the only > > option it to connect is panel and panel in bridge terminology are > > treated as panel_bridge. This is the reason I have used panel_bridge. > > next_bridge notation will be used if the bridge connected to any > > downstream bridge, like we can use next_bridge notation in host bridge > > drivers as host bridge can be an option of connecting downstream > > bridge or panel. > > The downstream bridge doesn't have to be a DSI panel, it could be an > LVDS-to-DPI bridge for instance, or an LVDS-to-HDMI encoder. Okay. What are use-cases where we can use panel_bridge? Jagan.
On Mon, Dec 13, 2021 at 06:27:37PM +0530, Jagan Teki wrote: > On Mon, Dec 13, 2021 at 6:16 PM Laurent Pinchart wrote: > > On Mon, Dec 13, 2021 at 06:09:23PM +0530, Jagan Teki wrote: > > > On Mon, Dec 13, 2021 at 6:02 PM Laurent Pinchart wrote: > > > > On Mon, Dec 13, 2021 at 05:49:29PM +0530, Jagan Teki wrote: > > > > > Replace the manual panel handling code by a drm panel_bridge via > > > > > devm_drm_of_get_bridge(). > > > > > > > > > > Adding panel_bridge handling, > > > > > > > > > > - Drops drm_connector and related operations as drm_bridge_attach > > > > > creates connector during attachment. > > > > > > > > > > - Drops panel pointer and panel healpers. > > > > > > > > > > This simplifies the driver and allows all components in the display > > > > > pipeline to be treated as bridges. > > > > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > --- > > > > > drivers/gpu/drm/bridge/tc358764.c | 99 ++----------------------------- > > > > > 1 file changed, 6 insertions(+), 93 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c > > > > > index c1e35bdf9232..28480bdc4287 100644 > > > > > --- a/drivers/gpu/drm/bridge/tc358764.c > > > > > +++ b/drivers/gpu/drm/bridge/tc358764.c > > > > > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { > > > > > struct tc358764 { > > > > > struct device *dev; > > > > > struct drm_bridge bridge; > > > > > - struct drm_connector connector; > > > > > + struct drm_bridge *panel_bridge; > > > > > > > > s/panel_bridge/next_bridge/ as it may not be a panel. > > > > > > Sometime, I'm a strong believer of my own notation (I may be wrong) > > > based on my understanding. This is downstream bridge and the only > > > option it to connect is panel and panel in bridge terminology are > > > treated as panel_bridge. This is the reason I have used panel_bridge. > > > next_bridge notation will be used if the bridge connected to any > > > downstream bridge, like we can use next_bridge notation in host bridge > > > drivers as host bridge can be an option of connecting downstream > > > bridge or panel. > > > > The downstream bridge doesn't have to be a DSI panel, it could be an > > LVDS-to-DPI bridge for instance, or an LVDS-to-HDMI encoder. > > Okay. What are use-cases where we can use panel_bridge? The panel bridge wraps a drm_panel in a drm_bridge, which makes it completely transparent for the other bridges in the chain whether their output is connected to a panel or to something else. I would thus never call a variable panel_bridge unless in the panel bridge driver itself.
diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c index c1e35bdf9232..28480bdc4287 100644 --- a/drivers/gpu/drm/bridge/tc358764.c +++ b/drivers/gpu/drm/bridge/tc358764.c @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { struct tc358764 { struct device *dev; struct drm_bridge bridge; - struct drm_connector connector; + struct drm_bridge *panel_bridge; struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; struct gpio_desc *gpio_reset; - struct drm_panel *panel; int error; }; @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge) return container_of(bridge, struct tc358764, bridge); } -static inline -struct tc358764 *connector_to_tc358764(struct drm_connector *connector) -{ - return container_of(connector, struct tc358764, connector); -} - static int tc358764_init(struct tc358764 *ctx) { u32 v = 0; @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx) usleep_range(1000, 2000); } -static int tc358764_get_modes(struct drm_connector *connector) -{ - struct tc358764 *ctx = connector_to_tc358764(connector); - - return drm_panel_get_modes(ctx->panel, connector); -} - -static const -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = { - .get_modes = tc358764_get_modes, -}; - -static const struct drm_connector_funcs tc358764_connector_funcs = { - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, - .reset = drm_atomic_helper_connector_reset, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -static void tc358764_disable(struct drm_bridge *bridge) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel); - - if (ret < 0) - dev_err(ctx->dev, "error disabling panel (%d)\n", ret); -} - static void tc358764_post_disable(struct drm_bridge *bridge) { struct tc358764 *ctx = bridge_to_tc358764(bridge); int ret; - ret = drm_panel_unprepare(ctx->panel); - if (ret < 0) - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret); tc358764_reset(ctx); usleep_range(10000, 15000); ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); @@ -335,72 +296,25 @@ static void tc358764_pre_enable(struct drm_bridge *bridge) ret = tc358764_init(ctx); if (ret < 0) dev_err(ctx->dev, "error initializing bridge (%d)\n", ret); - ret = drm_panel_prepare(ctx->panel); - if (ret < 0) - dev_err(ctx->dev, "error preparing panel (%d)\n", ret); -} - -static void tc358764_enable(struct drm_bridge *bridge) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - int ret = drm_panel_enable(ctx->panel); - - if (ret < 0) - dev_err(ctx->dev, "error enabling panel (%d)\n", ret); } static int tc358764_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - struct drm_device *drm = bridge->dev; - int ret; - - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - - ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; - ret = drm_connector_init(drm, &ctx->connector, - &tc358764_connector_funcs, - DRM_MODE_CONNECTOR_LVDS); - if (ret) { - DRM_ERROR("Failed to initialize connector\n"); - return ret; - } - - drm_connector_helper_add(&ctx->connector, - &tc358764_connector_helper_funcs); - drm_connector_attach_encoder(&ctx->connector, bridge->encoder); - ctx->connector.funcs->reset(&ctx->connector); - drm_connector_register(&ctx->connector); - - return 0; -} - -static void tc358764_detach(struct drm_bridge *bridge) { struct tc358764 *ctx = bridge_to_tc358764(bridge); - drm_connector_unregister(&ctx->connector); - ctx->panel = NULL; - drm_connector_put(&ctx->connector); + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, bridge, flags); } static const struct drm_bridge_funcs tc358764_bridge_funcs = { - .disable = tc358764_disable, .post_disable = tc358764_post_disable, - .enable = tc358764_enable, .pre_enable = tc358764_pre_enable, .attach = tc358764_attach, - .detach = tc358764_detach, }; static int tc358764_parse_dt(struct tc358764 *ctx) { struct device *dev = ctx->dev; - int ret; ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(ctx->gpio_reset)) { @@ -408,12 +322,11 @@ static int tc358764_parse_dt(struct tc358764 *ctx) return PTR_ERR(ctx->gpio_reset); } - ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel, - NULL); - if (ret && ret != -EPROBE_DEFER) - dev_err(dev, "cannot find panel (%d)\n", ret); + ctx->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); + if (IS_ERR(ctx->panel_bridge)) + return PTR_ERR(ctx->panel_bridge); - return ret; + return 0; } static int tc358764_configure_regulators(struct tc358764 *ctx)
Replace the manual panel handling code by a drm panel_bridge via devm_drm_of_get_bridge(). Adding panel_bridge handling, - Drops drm_connector and related operations as drm_bridge_attach creates connector during attachment. - Drops panel pointer and panel healpers. This simplifies the driver and allows all components in the display pipeline to be treated as bridges. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/gpu/drm/bridge/tc358764.c | 99 ++----------------------------- 1 file changed, 6 insertions(+), 93 deletions(-)