Hi Neil,
with include order fixed and the comment below considered:
Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
Sam
On Thu, Oct 14, 2021 at 05:26:06PM +0200, Neil Armstrong wrote:
> Drop the local connector and move all callback to bridge funcs in order
> to leverage the generic CVBS diplay connector.
>
> This will also permit adding custom cvbs connectors for ADC based HPD
> detection on some Amlogic SoC based boards.
>
> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/meson/meson_encoder_cvbs.c | 178 +++++++++------------
> 1 file changed, 79 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> index 01024c5f610c..0b974667cf55 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> @@ -13,6 +13,9 @@
> #include <linux/of_graph.h>
>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> #include <drm/drm_device.h>
> #include <drm/drm_edid.h>
> #include <drm/drm_probe_helper.h>
alphabetic order.
> @@ -30,14 +33,14 @@
>
> struct meson_encoder_cvbs {
> struct drm_encoder encoder;
> - struct drm_connector connector;
> + struct drm_bridge bridge;
> + struct drm_bridge *next_bridge;
> + struct drm_connector *connector;
Maybe drop this - see later.
> struct meson_drm *priv;
> };
> -#define encoder_to_meson_encoder_cvbs(x) \
> - container_of(x, struct meson_encoder_cvbs, encoder)
>
> -#define connector_to_meson_encoder_cvbs(x) \
> - container_of(x, struct meson_encoder_cvbs, connector)
> +#define bridge_to_meson_encoder_cvbs(x) \
> + container_of(x, struct meson_encoder_cvbs, bridge)
>
> /* Supported Modes */
>
> @@ -81,21 +84,18 @@ meson_cvbs_get_mode(const struct drm_display_mode *req_mode)
> return NULL;
> }
>
> -/* Connector */
> -
> -static void meson_cvbs_connector_destroy(struct drm_connector *connector)
> +static int meson_encoder_cvbs_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> {
> - drm_connector_cleanup(connector);
> -}
> + struct meson_encoder_cvbs *meson_encoder_cvbs =
> + bridge_to_meson_encoder_cvbs(bridge);
>
> -static enum drm_connector_status
> -meson_cvbs_connector_detect(struct drm_connector *connector, bool force)
> -{
> - /* FIXME: Add load-detect or jack-detect if possible */
> - return connector_status_connected;
> + return drm_bridge_attach(bridge->encoder, meson_encoder_cvbs->next_bridge,
> + &meson_encoder_cvbs->bridge, flags);
> }
>
> -static int meson_cvbs_connector_get_modes(struct drm_connector *connector)
> +static int meson_encoder_cvbs_get_modes(struct drm_bridge *bridge,
> + struct drm_connector *connector)
> {
> struct drm_device *dev = connector->dev;
> struct drm_display_mode *mode;
> @@ -116,40 +116,18 @@ static int meson_cvbs_connector_get_modes(struct drm_connector *connector)
> return i;
> }
>
> -static int meson_cvbs_connector_mode_valid(struct drm_connector *connector,
> - struct drm_display_mode *mode)
> +static int meson_encoder_cvbs_mode_valid(struct drm_bridge *bridge,
> + const struct drm_display_info *display_info,
> + const struct drm_display_mode *mode)
> {
> - /* Validate the modes added in get_modes */
> - return MODE_OK;
> -}
> -
> -static const struct drm_connector_funcs meson_cvbs_connector_funcs = {
> - .detect = meson_cvbs_connector_detect,
> - .fill_modes = drm_helper_probe_single_connector_modes,
> - .destroy = meson_cvbs_connector_destroy,
> - .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 const
> -struct drm_connector_helper_funcs meson_cvbs_connector_helper_funcs = {
> - .get_modes = meson_cvbs_connector_get_modes,
> - .mode_valid = meson_cvbs_connector_mode_valid,
> -};
> + if (meson_cvbs_get_mode(mode))
> + return MODE_OK;
>
> -/* Encoder */
> -
> -static void meson_encoder_cvbs_encoder_destroy(struct drm_encoder *encoder)
> -{
> - drm_encoder_cleanup(encoder);
> + return MODE_BAD;
> }
>
> -static const struct drm_encoder_funcs meson_encoder_cvbs_encoder_funcs = {
> - .destroy = meson_encoder_cvbs_encoder_destroy,
> -};
> -
> -static int meson_encoder_cvbs_encoder_atomic_check(struct drm_encoder *encoder,
> +static int meson_encoder_cvbs_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> struct drm_crtc_state *crtc_state,
> struct drm_connector_state *conn_state)
> {
> @@ -159,10 +137,10 @@ static int meson_encoder_cvbs_encoder_atomic_check(struct drm_encoder *encoder,
> return -EINVAL;
> }
>
> -static void meson_encoder_cvbs_encoder_disable(struct drm_encoder *encoder)
> +static void meson_encoder_cvbs_disable(struct drm_bridge *bridge)
> {
> struct meson_encoder_cvbs *meson_encoder_cvbs =
> - encoder_to_meson_encoder_cvbs(encoder);
> + bridge_to_meson_encoder_cvbs(bridge);
> struct meson_drm *priv = meson_encoder_cvbs->priv;
>
> /* Disable CVBS VDAC */
> @@ -175,10 +153,10 @@ static void meson_encoder_cvbs_encoder_disable(struct drm_encoder *encoder)
> }
> }
>
> -static void meson_encoder_cvbs_encoder_enable(struct drm_encoder *encoder)
> +static void meson_encoder_cvbs_enable(struct drm_bridge *bridge)
> {
> struct meson_encoder_cvbs *meson_encoder_cvbs =
> - encoder_to_meson_encoder_cvbs(encoder);
> + bridge_to_meson_encoder_cvbs(bridge);
> struct meson_drm *priv = meson_encoder_cvbs->priv;
>
> /* VDAC0 source is not from ATV */
> @@ -198,13 +176,13 @@ static void meson_encoder_cvbs_encoder_enable(struct drm_encoder *encoder)
> }
> }
>
> -static void meson_encoder_cvbs_encoder_mode_set(struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> +static void meson_encoder_cvbs_mode_set(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + const struct drm_display_mode *adjusted_mode)
> {
> const struct meson_cvbs_mode *meson_mode = meson_cvbs_get_mode(mode);
> struct meson_encoder_cvbs *meson_encoder_cvbs =
> - encoder_to_meson_encoder_cvbs(encoder);
> + bridge_to_meson_encoder_cvbs(bridge);
> struct meson_drm *priv = meson_encoder_cvbs->priv;
>
> if (meson_mode) {
> @@ -218,76 +196,78 @@ static void meson_encoder_cvbs_encoder_mode_set(struct drm_encoder *encoder,
> }
> }
>
> -static const struct drm_encoder_helper_funcs
> - meson_encoder_cvbs_encoder_helper_funcs = {
> - .atomic_check = meson_encoder_cvbs_encoder_atomic_check,
> - .disable = meson_encoder_cvbs_encoder_disable,
> - .enable = meson_encoder_cvbs_encoder_enable,
> - .mode_set = meson_encoder_cvbs_encoder_mode_set,
> +static const struct drm_bridge_funcs meson_encoder_cvbs_bridge_funcs = {
> + .attach = meson_encoder_cvbs_attach,
> + .enable = meson_encoder_cvbs_enable,
> + .disable = meson_encoder_cvbs_disable,
> + .mode_valid = meson_encoder_cvbs_mode_valid,
> + .mode_set = meson_encoder_cvbs_mode_set,
> + .get_modes = meson_encoder_cvbs_get_modes,
> + .atomic_check = meson_encoder_cvbs_atomic_check,
> };
>
> -static bool meson_encoder_cvbs_connector_is_available(struct meson_drm *priv)
> -{
> - struct device_node *remote;
> -
> - remote = of_graph_get_remote_node(priv->dev->of_node, 0, 0);
> - if (!remote)
> - return false;
> -
> - of_node_put(remote);
> - return true;
> -}
> -
> int meson_encoder_cvbs_init(struct meson_drm *priv)
> {
> struct drm_device *drm = priv->drm;
> struct meson_encoder_cvbs *meson_encoder_cvbs;
> - struct drm_connector *connector;
> - struct drm_encoder *encoder;
> + struct device_node *remote;
> int ret;
>
> - if (!meson_encoder_cvbs_connector_is_available(priv)) {
> + meson_encoder_cvbs = devm_kzalloc(priv->dev, sizeof(*meson_encoder_cvbs), GFP_KERNEL);
> + if (!meson_encoder_cvbs)
> + return -ENOMEM;
> +
> + /* CVBS Connector Bridge */
> + remote = of_graph_get_remote_node(priv->dev->of_node, 0, 0);
> + if (!remote) {
> dev_info(drm->dev, "CVBS Output connector not available\n");
> return 0;
> }
>
> - meson_encoder_cvbs = devm_kzalloc(priv->dev, sizeof(*meson_encoder_cvbs),
> - GFP_KERNEL);
> - if (!meson_encoder_cvbs)
> - return -ENOMEM;
> + meson_encoder_cvbs->next_bridge = of_drm_find_bridge(remote);
> + if (!meson_encoder_cvbs->next_bridge) {
> + dev_err(priv->dev, "Failed to find CVBS Connector bridge\n");
> + return -EPROBE_DEFER;
> + }
>
> - meson_encoder_cvbs->priv = priv;
> - encoder = &meson_encoder_cvbs->encoder;
> - connector = &meson_encoder_cvbs->connector;
> + /* CVBS Encoder Bridge */
> + meson_encoder_cvbs->bridge.funcs = &meson_encoder_cvbs_bridge_funcs;
> + meson_encoder_cvbs->bridge.of_node = priv->dev->of_node;
> + meson_encoder_cvbs->bridge.type = DRM_MODE_CONNECTOR_Composite;
> + meson_encoder_cvbs->bridge.ops = DRM_BRIDGE_OP_MODES;
> + meson_encoder_cvbs->bridge.interlace_allowed = true;
>
> - /* Connector */
> + drm_bridge_add(&meson_encoder_cvbs->bridge);
>
> - drm_connector_helper_add(connector,
> - &meson_cvbs_connector_helper_funcs);
> + meson_encoder_cvbs->priv = priv;
>
> - ret = drm_connector_init(drm, connector, &meson_cvbs_connector_funcs,
> - DRM_MODE_CONNECTOR_Composite);
> + /* Encoder */
> + ret = drm_simple_encoder_init(priv->drm, &meson_encoder_cvbs->encoder,
> + DRM_MODE_ENCODER_TVDAC);
> if (ret) {
> - dev_err(priv->dev, "Failed to init CVBS connector\n");
> + dev_err(priv->dev, "Failed to init CVBS encoder: %d\n", ret);
> return ret;
> }
>
> - connector->interlace_allowed = 1;
> + meson_encoder_cvbs->encoder.possible_crtcs = BIT(0);
>
> - /* Encoder */
> -
> - drm_encoder_helper_add(encoder, &meson_encoder_cvbs_encoder_helper_funcs);
> -
> - ret = drm_encoder_init(drm, encoder, &meson_encoder_cvbs_encoder_funcs,
> - DRM_MODE_ENCODER_TVDAC, "meson_encoder_cvbs");
> + /* Attach CVBS Encoder Bridge to Encoder */
> + ret = drm_bridge_attach(&meson_encoder_cvbs->encoder, &meson_encoder_cvbs->bridge, NULL,
> + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> if (ret) {
> - dev_err(priv->dev, "Failed to init CVBS encoder\n");
> + dev_err(priv->dev, "Failed to attach bridge: %d\n", ret);
> return ret;
> }
>
> - encoder->possible_crtcs = BIT(0);
> -
> - drm_connector_attach_encoder(connector, encoder);
> + /* Initialize & attach Bridge Connector */
> + meson_encoder_cvbs->connector = drm_bridge_connector_init(priv->drm,
> + &meson_encoder_cvbs->encoder);
I did not see other uses of meson_encoder_cvbs->connector, so if I am
right a local variable can be used and the member dropped.
> + if (IS_ERR(meson_encoder_cvbs->connector)) {
> + dev_err(priv->dev, "Unable to create CVBS bridge connector\n");
> + return PTR_ERR(meson_encoder_cvbs->connector);
> + }
> + drm_connector_attach_encoder(meson_encoder_cvbs->connector,
> + &meson_encoder_cvbs->encoder);
>
> return 0;
> }
> --
> 2.25.1
