[PATCH v5 0/7] Add tda998x (HDMI) support to atmel-hlcdc

[PATCH v5 0/7] Add tda998x (HDMI) support to atmel-hlcdc

[Peter Rosin]

Hi!

I naively thought that since there was support for both nxp,tda19988 (in
the tda998x driver) and the atmel-hlcdc, things would be a smooth ride.
But it wasn't, so I started looking around and realized I had to fix
things.

In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
component, but now I fix things by making the tda998x driver a bridge
instead. This was after a suggestion from Boris Brezillon the
atmel-hlcdc maintainer), so there was some risk of bias ... but after
comparing what was needed, I too find the bridge approach more direct.
In v4, an issue was noted by Russell King in that the tda998x device
might be unbound with shattering results for the main drm device.
This is of course a real problem and it needs to be fixed. I have
submitted a series [1] that tries to do that. However, since there are
currently *10* other bridges (analogix-anx78xx, adv7511, megachips_stdp*,
nxp-ptn3460, parade-ps8622, sii902x, sii9234, sil-sii8620, tc358767 and
ti-tfp410) with exactly the same problem (they are all I2C devices that
might be unexpectedly unbound without safety-net, at least that is how I
read it), I think it should be ok to add one more and then fix them all
instead of holding this series hostage. Apparently people don't go around
and unbind I2C devices all that often...

In addition to the above, our PCB interface between the SAMA5D3 and the
HDMI encoder is only using 16 bits, and this has to be described
somewhere, or the atmel-hlcdc driver have no chance of selecting the
correct output mode. Since I have similar problems with a ds90c185 lvds
encoder I added patches to override the atmel-hlcdc output format via
DT properties compatible with the media video-interface binding and
things start to play together.

Anyway, this series solves some real issues for my HW.

Also, is it perhaps possible that patches 1-3 can be taken independently
from 4-7? There is no hard dependency between the two parts of this series.
Patches 1-3 have the relevant tags and should be uncontroversial...

Cheers,
Peter

Changes since v4   https://lkml.org/lkml/2018/4/23/92
- added reviewed-by from Rob to patch 2/7
- dropped patch 8, since the issue noted by Russell King is not present
  when working with components as tilcdc currently do when handling tda998x

Changes since v3   https://lkml.org/lkml/2018/4/19/736
- moved the meat of the drm_of_media_bus_fmt function from patch 3/7
  to a driver-local atmel_hlcdc_of_bus_fmt function, and squashed with
  patch 4/7 (this is now patch 3/8).
- patch 3/8 now parses and stores the DT bus format together with the
  encoder in a reintroduced struct atmel_hlcdc_rgb_output instead of
  allocating a separate encoder-indexed array for the bus formats.
- patch 5/8 is new and splits tda988x_encoder_dpms into a pair of
  functions to enable/disable separately. This fits better with both
  the current code and for the followup drm_bridge patch (7/8).
- adjust patch 6/8 and 7/8 to the above simplification.
- added patch 8/8 that removes component master code from tilcdc.

Changes since v2   https://lkml.org/lkml/2018/4/17/385
- patch 2/7 fixed spelling and added an example
- patch 4/7 parse the DT up front and store the result indexed by encoder
- old patch 5/6 and 6/6 dropped
- patch 5-7/7 are new and makes the tda998x driver a drm_bridge

Changes since v1   https://lkml.org/lkml/2018/4/9/294
- added reviewed-by from Rob to patch 1/6
- patch 2/6 changed so that the bus format override is in the endpoint
  DT node, and follows the binding of media video-interfaces.
- patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above
  media video-interface binding (partially).
- patch 4/6 now makes use of the above helper (and also fixes problems
  with the 3/5 patch from v1 when no override was specified).
- do not mention unrelated connector display_info details in the cover
  letter and commit messages.

[1] https://lkml.org/lkml/2018/5/16/380

Peter Rosin (7):
  dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
  dt-bindings: display: atmel: optional video-interface of endpoints
  drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes
  drm/i2c: tda998x: find the drm_device via the drm_connector
  drm/i2c: tda998x: split tda998x_encoder_dpms into enable/disable
  drm/i2c: tda998x: split encoder and component functions from the work
  drm/i2c: tda998x: register as a drm bridge

 .../devicetree/bindings/display/atmel/hlcdc-dc.txt |  26 ++
 .../bindings/display/bridge/lvds-transmitter.txt   |   8 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     |  70 ++++--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h       |   1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  67 +++++-
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 268 ++++++++++++++++-----
 6 files changed, 351 insertions(+), 89 deletions(-)

-- 
2.11.0

[PATCH v5 1/7] dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185

[Peter Rosin]

Start list of actual chips compatible with "lvds-encoder".

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/bridge/lvds-transmitter.txt       | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index fd39ad34c383..50220190c203 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,7 +22,13 @@ among others.
 
 Required properties:
 
-- compatible: Must be "lvds-encoder"
+- compatible: Must be one or more of the following
+  - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
+  - "lvds-encoder" for a generic LVDS encoder device
+
+  When compatible with the generic version, nodes must list the
+  device-specific version corresponding to the device first
+  followed by the generic version.
 
 Required nodes:
 
-- 
2.11.0

[PATCH v5 2/7] dt-bindings: display: atmel: optional video-interface of endpoints

[Peter Rosin]

With bus-type/bus-width properties in the endpoint nodes, the video-
interface of the connection can be specified for cases where the
heuristic fails to select the correct output mode. This can happen
e.g. if not all RGB pins are routed on the PCB; the driver has no
way of knowing this, and needs to be told explicitly.

This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
index 82f2acb3d374..9de434a8f523 100644
--- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
+++ b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
@@ -15,6 +15,14 @@ Required children nodes:
  to external devices using the OF graph reprensentation (see ../graph.txt).
  At least one port node is required.
 
+Optional properties in grandchild nodes:
+ Any endpoint grandchild node may specify a desired video interface
+ according to ../../media/video-interfaces.txt, specifically
+ - bus-type: must be <0>.
+ - bus-width: recognized values are <12>, <16>, <18> and <24>, and
+   override any output mode selection heuristic, forcing "rgb444",
+   "rgb565", "rgb666" and "rgb888" respectively.
+
 Example:
 
 	hlcdc: hlcdc@f0030000 {
@@ -50,3 +58,21 @@ Example:
 			#pwm-cells = <3>;
 		};
 	};
+
+
+Example 2: With a video interface override to force rgb565; as above
+but with these changes/additions:
+
+	&hlcdc {
+		hlcdc-display-controller {
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
+
+			port@0 {
+				hlcdc_panel_output: endpoint@0 {
+					bus-type = <0>;
+					bus-width = <16>;
+				};
+			};
+		};
+	};
-- 
2.11.0

[PATCH v5 3/7] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes

[Peter Rosin]

This beats the heuristic that the connector is involved in what format
should be output for cases where this fails.

E.g. if there is a bridge that changes format between the encoder and the
connector, or if some of the RGB pins between the lcd controller and the
encoder are not routed on the PCB.

This is critical for the devices that have the "conflicting output
formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
RGB bits move around depending on the selected output mode. For
devices that do not have the "conflicting output formats" issue
(SAMA5D2, SAMA5D4), this is completely irrelevant.

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 70 +++++++++++++++++-------
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h     |  1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 67 ++++++++++++++++++++---
 3 files changed, 111 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index d73281095fac..c38a479ada98 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -226,6 +226,55 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
 #define ATMEL_HLCDC_RGB888_OUTPUT	BIT(3)
 #define ATMEL_HLCDC_OUTPUT_MODE_MASK	GENMASK(3, 0)
 
+static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
+{
+	struct drm_connector *connector = state->connector;
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_encoder *encoder;
+	unsigned int supported_fmts = 0;
+	int j;
+
+	encoder = state->best_encoder;
+	if (!encoder)
+		encoder = connector->encoder;
+
+	switch (atmel_hlcdc_encoder_get_bus_fmt(encoder)) {
+	case 0:
+		break;
+	case MEDIA_BUS_FMT_RGB444_1X12:
+		return ATMEL_HLCDC_RGB444_OUTPUT;
+	case MEDIA_BUS_FMT_RGB565_1X16:
+		return ATMEL_HLCDC_RGB565_OUTPUT;
+	case MEDIA_BUS_FMT_RGB666_1X18:
+		return ATMEL_HLCDC_RGB666_OUTPUT;
+	case MEDIA_BUS_FMT_RGB888_1X24:
+		return ATMEL_HLCDC_RGB888_OUTPUT;
+	default:
+		return -EINVAL;
+	}
+
+	for (j = 0; j < info->num_bus_formats; j++) {
+		switch (info->bus_formats[j]) {
+		case MEDIA_BUS_FMT_RGB444_1X12:
+			supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB565_1X16:
+			supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB666_1X18:
+			supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
+			break;
+		case MEDIA_BUS_FMT_RGB888_1X24:
+			supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return supported_fmts;
+}
+
 static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
 {
 	unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
@@ -238,31 +287,12 @@ static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
 	crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
 
 	for_each_new_connector_in_state(state->state, connector, cstate, i) {
-		struct drm_display_info *info = &connector->display_info;
 		unsigned int supported_fmts = 0;
-		int j;
 
 		if (!cstate->crtc)
 			continue;
 
-		for (j = 0; j < info->num_bus_formats; j++) {
-			switch (info->bus_formats[j]) {
-			case MEDIA_BUS_FMT_RGB444_1X12:
-				supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB565_1X16:
-				supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB666_1X18:
-				supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
-				break;
-			case MEDIA_BUS_FMT_RGB888_1X24:
-				supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
-				break;
-			default:
-				break;
-			}
-		}
+		supported_fmts = atmel_hlcdc_connector_output_mode(cstate);
 
 		if (crtc->dc->desc->conflicting_output_formats)
 			output_fmts &= supported_fmts;
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
index 60c937f42114..4cc1e03f0aee 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
@@ -441,5 +441,6 @@ void atmel_hlcdc_crtc_irq(struct drm_crtc *c);
 int atmel_hlcdc_crtc_create(struct drm_device *dev);
 
 int atmel_hlcdc_create_outputs(struct drm_device *dev);
+int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder);
 
 #endif /* DRM_ATMEL_HLCDC_H */
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
index 8db51fb131db..db4d6aaaef93 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -27,33 +27,86 @@
 
 #include "atmel_hlcdc_dc.h"
 
+struct atmel_hlcdc_rgb_output {
+	struct drm_encoder encoder;
+	int bus_fmt;
+};
+
 static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
+static struct atmel_hlcdc_rgb_output *
+atmel_hlcdc_encoder_to_rgb_output(struct drm_encoder *encoder)
+{
+	return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
+}
+
+int atmel_hlcdc_encoder_get_bus_fmt(struct drm_encoder *encoder)
+{
+	struct atmel_hlcdc_rgb_output *output;
+
+	output = atmel_hlcdc_encoder_to_rgb_output(encoder);
+
+	return output->bus_fmt;
+}
+
+static int atmel_hlcdc_of_bus_fmt(struct device_node *ep)
+{
+	u32 bus_width = 0;
+
+	of_property_read_u32(ep, "bus-width", &bus_width);
+
+	switch (bus_width) {
+	case 0:
+		return 0;
+	case 12:
+		return MEDIA_BUS_FMT_RGB444_1X12;
+	case 16:
+		return MEDIA_BUS_FMT_RGB565_1X16;
+	case 18:
+		return MEDIA_BUS_FMT_RGB666_1X18;
+	case 24:
+		return MEDIA_BUS_FMT_RGB888_1X24;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
 {
-	struct drm_encoder *encoder;
+	struct atmel_hlcdc_rgb_output *output;
+	struct device_node *ep;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	int ret;
 
+	ep = of_graph_get_endpoint_by_regs(dev->dev->of_node, 0, endpoint);
+	if (!ep)
+		return -ENODEV;
+
 	ret = drm_of_find_panel_or_bridge(dev->dev->of_node, 0, endpoint,
 					  &panel, &bridge);
 	if (ret)
 		return ret;
 
-	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
-	if (!encoder)
+	output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
+	if (!output)
 		return -EINVAL;
 
-	ret = drm_encoder_init(dev, encoder,
+	output->bus_fmt = atmel_hlcdc_of_bus_fmt(ep);
+	if (output->bus_fmt < 0) {
+		dev_err(dev->dev, "invalid bus width\n");
+		return -EINVAL;
+	}
+
+	ret = drm_encoder_init(dev, &output->encoder,
 			       &atmel_hlcdc_panel_encoder_funcs,
 			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret)
 		return ret;
 
-	encoder->possible_crtcs = 0x1;
+	output->encoder.possible_crtcs = 0x1;
 
 	if (panel) {
 		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
@@ -62,7 +115,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
 	}
 
 	if (bridge) {
-		ret = drm_bridge_attach(encoder, bridge, NULL);
+		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
 		if (!ret)
 			return 0;
 
@@ -70,7 +123,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
 			drm_panel_bridge_remove(bridge);
 	}
 
-	drm_encoder_cleanup(encoder);
+	drm_encoder_cleanup(&output->encoder);
 
 	return ret;
 }
-- 
2.11.0

[PATCH v5 4/7] drm/i2c: tda998x: find the drm_device via the drm_connector

[Peter Rosin]

This prepares for being a drm_bridge which will not register the
encoder. That makes the connector the better choice.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 421c8a72369e..933d309d2e0f 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -630,7 +630,7 @@ static void tda998x_detect_work(struct work_struct *work)
 {
 	struct tda998x_priv *priv =
 		container_of(work, struct tda998x_priv, detect_work);
-	struct drm_device *dev = priv->encoder.dev;
+	struct drm_device *dev = priv->connector.dev;
 
 	if (dev)
 		drm_kms_helper_hotplug_event(dev);
-- 
2.11.0

[PATCH v5 5/7] drm/i2c: tda998x: split tda998x_encoder_dpms into enable/disable

[Peter Rosin]

This fits better with the drm_bridge callbacks for when this
driver becomes a drm_bridge.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 64 ++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 933d309d2e0f..3dda07a2fd2f 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1163,36 +1163,42 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 
 /* DRM encoder functions */
 
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+static void tda998x_enable(struct tda998x_priv *priv)
 {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
-	bool on;
+	if (priv->is_on)
+		return;
 
-	/* we only care about on or off: */
-	on = mode == DRM_MODE_DPMS_ON;
+	/* enable video ports, audio will be enabled later */
+	reg_write(priv, REG_ENA_VP_0, 0xff);
+	reg_write(priv, REG_ENA_VP_1, 0xff);
+	reg_write(priv, REG_ENA_VP_2, 0xff);
+	/* set muxing after enabling ports: */
+	reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
+	reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
+	reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
 
-	if (on == priv->is_on)
-		return;
+	priv->is_on = true;
+}
 
-	if (on) {
-		/* enable video ports, audio will be enabled later */
-		reg_write(priv, REG_ENA_VP_0, 0xff);
-		reg_write(priv, REG_ENA_VP_1, 0xff);
-		reg_write(priv, REG_ENA_VP_2, 0xff);
-		/* set muxing after enabling ports: */
-		reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
-		reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
-		reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
-
-		priv->is_on = true;
-	} else {
-		/* disable video ports */
-		reg_write(priv, REG_ENA_VP_0, 0x00);
-		reg_write(priv, REG_ENA_VP_1, 0x00);
-		reg_write(priv, REG_ENA_VP_2, 0x00);
+static void tda998x_disable(struct tda998x_priv *priv)
+{
+	/* disable video ports */
+	reg_write(priv, REG_ENA_VP_0, 0x00);
+	reg_write(priv, REG_ENA_VP_1, 0x00);
+	reg_write(priv, REG_ENA_VP_2, 0x00);
 
-		priv->is_on = false;
-	}
+	priv->is_on = false;
+}
+
+static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+{
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+
+	/* we only care about on or off: */
+	if (mode == DRM_MODE_DPMS_ON)
+		tda998x_enable(priv);
+	else
+		tda998x_disable(priv);
 }
 
 static void
@@ -1606,12 +1612,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 
 static void tda998x_encoder_prepare(struct drm_encoder *encoder)
 {
-	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+
+	tda998x_disable(priv);
 }
 
 static void tda998x_encoder_commit(struct drm_encoder *encoder)
 {
-	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+
+	tda998x_enable(priv);
 }
 
 static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
-- 
2.11.0

Re: [PATCH v5 5/7] drm/i2c: tda998x: split tda998x_encoder_dpms into enable/disable

[Russell King - ARM Linux]

On Wed, May 23, 2018 at 11:31:20AM +0200, Peter Rosin wrote:
> This fits better with the drm_bridge callbacks for when this
> driver becomes a drm_bridge.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 64 ++++++++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 933d309d2e0f..3dda07a2fd2f 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1163,36 +1163,42 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
>  
>  /* DRM encoder functions */
>  
> -static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
> +static void tda998x_enable(struct tda998x_priv *priv)
>  {
> -	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> -	bool on;
> +	if (priv->is_on)
> +		return;
>  
> -	/* we only care about on or off: */
> -	on = mode == DRM_MODE_DPMS_ON;
> +	/* enable video ports, audio will be enabled later */
> +	reg_write(priv, REG_ENA_VP_0, 0xff);
> +	reg_write(priv, REG_ENA_VP_1, 0xff);
> +	reg_write(priv, REG_ENA_VP_2, 0xff);
> +	/* set muxing after enabling ports: */
> +	reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
> +	reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
> +	reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
>  
> -	if (on == priv->is_on)
> -		return;
> +	priv->is_on = true;
> +}
>  
> -	if (on) {
> -		/* enable video ports, audio will be enabled later */
> -		reg_write(priv, REG_ENA_VP_0, 0xff);
> -		reg_write(priv, REG_ENA_VP_1, 0xff);
> -		reg_write(priv, REG_ENA_VP_2, 0xff);
> -		/* set muxing after enabling ports: */
> -		reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
> -		reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
> -		reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
> -
> -		priv->is_on = true;
> -	} else {
> -		/* disable video ports */
> -		reg_write(priv, REG_ENA_VP_0, 0x00);
> -		reg_write(priv, REG_ENA_VP_1, 0x00);
> -		reg_write(priv, REG_ENA_VP_2, 0x00);
> +static void tda998x_disable(struct tda998x_priv *priv)
> +{
> +	/* disable video ports */
> +	reg_write(priv, REG_ENA_VP_0, 0x00);
> +	reg_write(priv, REG_ENA_VP_1, 0x00);
> +	reg_write(priv, REG_ENA_VP_2, 0x00);
>  
> -		priv->is_on = false;
> -	}
> +	priv->is_on = false;
> +}
> +
> +static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
> +{
> +	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> +
> +	/* we only care about on or off: */
> +	if (mode == DRM_MODE_DPMS_ON)
> +		tda998x_enable(priv);
> +	else
> +		tda998x_disable(priv);
>  }
>  
>  static void
> @@ -1606,12 +1612,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
>  
>  static void tda998x_encoder_prepare(struct drm_encoder *encoder)
>  {
> -	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
> +	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> +
> +	tda998x_disable(priv);
>  }
>  
>  static void tda998x_encoder_commit(struct drm_encoder *encoder)
>  {
> -	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
> +	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> +
> +	tda998x_enable(priv);
>  }
>  
>  static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {

Please group the tda998x_encoder_* functions together, and in order of
their use in the function vtables, and just above the function vtables.
That'll mean that all the encoder veneers are together, all the bridge
veneers are together, etc, and probably means later on it's easier to
remove the encoder stuff (as all the encoder code will be in one hunk
rather than scattered throughout the file.)

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

[PATCH v5 6/7] drm/i2c: tda998x: split encoder and component functions from the work

[Peter Rosin]

This enables reuse of the machinery for the case where a drm_bridge
needs to do the same work via different interfaces.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 3dda07a2fd2f..d401b3d0095c 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1202,11 +1202,10 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 }
 
 static void
-tda998x_encoder_mode_set(struct drm_encoder *encoder,
-			 struct drm_display_mode *mode,
-			 struct drm_display_mode *adjusted_mode)
+tda998x_mode_set(struct tda998x_priv *priv,
+		 struct drm_display_mode *mode,
+		 struct drm_display_mode *adjusted_mode)
 {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
 	u16 ref_pix, ref_line, n_pix, n_line;
 	u16 hs_pix_s, hs_pix_e;
 	u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1413,6 +1412,16 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	mutex_unlock(&priv->audio_mutex);
 }
 
+static void
+tda998x_encoder_mode_set(struct drm_encoder *encoder,
+			 struct drm_display_mode *mode,
+			 struct drm_display_mode *adjusted_mode)
+{
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+
+	tda998x_mode_set(priv, mode, adjusted_mode);
+}
+
 static void tda998x_destroy(struct tda998x_priv *priv)
 {
 	/* disable all IRQs and free the IRQ handler */
@@ -1662,11 +1671,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
 	priv->audio_params = p->audio_params;
 }
 
-static int tda998x_bind(struct device *dev, struct device *master, void *data)
+static int tda998x_init(struct device *dev, struct drm_device *drm)
 {
 	struct tda998x_encoder_params *params = dev->platform_data;
 	struct i2c_client *client = to_i2c_client(dev);
-	struct drm_device *drm = data;
 	struct tda998x_priv *priv;
 	u32 crtcs = 0;
 	int ret;
@@ -1714,8 +1722,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	return ret;
 }
 
-static void tda998x_unbind(struct device *dev, struct device *master,
-			   void *data)
+static void tda998x_fini(struct device *dev)
 {
 	struct tda998x_priv *priv = dev_get_drvdata(dev);
 
@@ -1724,6 +1731,19 @@ static void tda998x_unbind(struct device *dev, struct device *master,
 	tda998x_destroy(priv);
 }
 
+static int tda998x_bind(struct device *dev, struct device *master, void *data)
+{
+	struct drm_device *drm = data;
+
+	return tda998x_init(dev, drm);
+}
+
+static void tda998x_unbind(struct device *dev, struct device *master,
+			   void *data)
+{
+	tda998x_fini(dev);
+}
+
 static const struct component_ops tda998x_ops = {
 	.bind = tda998x_bind,
 	.unbind = tda998x_unbind,
-- 
2.11.0

[PATCH v5 7/7] drm/i2c: tda998x: register as a drm bridge

[Peter Rosin]

This makes this driver work with all(?) drivers that are not
componentized and instead expect to connect to a panel/bridge. That
said, the only one tested is atmel-hlcdc.

This hooks the relevant work function previously called by the encoder
and the component also to the bridge, since the encoder goes away when
connecting to the bridge interface of the driver and the equivalent of
bind/unbind of the component is handled by bridge attach/detach.

The lifetime requirements of a bridge and a component are slightly
different, which is the reason for struct tda998x_bridge.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 170 ++++++++++++++++++++++++++++++++------
 1 file changed, 143 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index d401b3d0095c..d47e49c2991a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -36,6 +36,14 @@ struct tda998x_audio_port {
 	u8 config;		/* AP value */
 };
 
+struct tda998x_priv;
+
+struct tda998x_bridge {
+	struct tda998x_priv *priv;
+	struct device *dev;
+	struct drm_bridge bridge;
+};
+
 struct tda998x_priv {
 	struct i2c_client *cec;
 	struct i2c_client *hdmi;
@@ -63,6 +71,8 @@ struct tda998x_priv {
 	wait_queue_head_t edid_delay_waitq;
 	bool edid_delay_active;
 
+	struct tda998x_bridge *bridge;
+	bool local_encoder;
 	struct drm_encoder encoder;
 	struct drm_connector connector;
 
@@ -75,6 +85,9 @@ struct tda998x_priv {
 #define enc_to_tda998x_priv(x) \
 	container_of(x, struct tda998x_priv, encoder)
 
+#define bridge_to_tda998x_bridge(x) \
+	container_of(x, struct tda998x_bridge, bridge)
+
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
  * write a given register, we need to make sure CURPAGE register is set
@@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 				   struct hdmi_codec_daifmt *daifmt,
 				   struct hdmi_codec_params *params)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 	int i, ret;
 	struct tda998x_audio_params audio = {
 		.sample_width = params->sample_width,
@@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 
 static void tda998x_audio_shutdown(struct device *dev, void *data)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 
 	mutex_lock(&priv->audio_mutex);
 
@@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev, void *data)
 
 int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 
 	mutex_lock(&priv->audio_mutex);
 
@@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 static int tda998x_audio_get_eld(struct device *dev, void *data,
 				 uint8_t *buf, size_t len)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 
 	mutex_lock(&priv->audio_mutex);
 	memcpy(buf, priv->connector.eld,
@@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector *connector)
 {
 	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
 
-	return &priv->encoder;
+	if (priv->local_encoder)
+		return &priv->encoder;
+	else
+		return priv->bridge->bridge.encoder;
 }
 
 static
@@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 				  struct drm_device *drm)
 {
 	struct drm_connector *connector = &priv->connector;
+	struct drm_encoder *encoder;
 	int ret;
 
 	connector->interlace_allowed = 1;
@@ -1156,7 +1177,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 	if (ret)
 		return ret;
 
-	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
+	encoder = tda998x_connector_best_encoder(&priv->connector);
+	drm_mode_connector_attach_encoder(&priv->connector, encoder);
 
 	return 0;
 }
@@ -1671,8 +1693,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
 	priv->audio_params = p->audio_params;
 }
 
-static int tda998x_init(struct device *dev, struct drm_device *drm)
+static int tda998x_init(struct device *dev, struct drm_device *drm,
+			bool local_encoder)
 {
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
 	struct tda998x_encoder_params *params = dev->platform_data;
 	struct i2c_client *client = to_i2c_client(dev);
 	struct tda998x_priv *priv;
@@ -1683,18 +1707,22 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
 	if (!priv)
 		return -ENOMEM;
 
-	dev_set_drvdata(dev, priv);
+	bridge->priv = priv;
+	priv->bridge = bridge;
+	priv->local_encoder = local_encoder;
 
-	if (dev->of_node)
-		crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
+	if (local_encoder) {
+		if (dev->of_node)
+			crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
 
-	/* If no CRTCs were found, fall back to our old behaviour */
-	if (crtcs == 0) {
-		dev_warn(dev, "Falling back to first CRTC\n");
-		crtcs = 1 << 0;
-	}
+		/* If no CRTCs were found, fall back to our old behaviour */
+		if (crtcs == 0) {
+			dev_warn(dev, "Falling back to first CRTC\n");
+			crtcs = 1 << 0;
+		}
 
-	priv->encoder.possible_crtcs = crtcs;
+		priv->encoder.possible_crtcs = crtcs;
+	}
 
 	ret = tda998x_create(client, priv);
 	if (ret)
@@ -1703,11 +1731,15 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
 	if (!dev->of_node && params)
 		tda998x_set_config(priv, params);
 
-	drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
-	ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
-			       DRM_MODE_ENCODER_TMDS, NULL);
-	if (ret)
-		goto err_encoder;
+	if (local_encoder) {
+		drm_encoder_helper_add(&priv->encoder,
+				       &tda998x_encoder_helper_funcs);
+		ret = drm_encoder_init(drm, &priv->encoder,
+				       &tda998x_encoder_funcs,
+				       DRM_MODE_ENCODER_TMDS, NULL);
+		if (ret)
+			goto err_encoder;
+	}
 
 	ret = tda998x_connector_init(priv, drm);
 	if (ret)
@@ -1716,7 +1748,8 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
 	return 0;
 
 err_connector:
-	drm_encoder_cleanup(&priv->encoder);
+	if (local_encoder)
+		drm_encoder_cleanup(&priv->encoder);
 err_encoder:
 	tda998x_destroy(priv);
 	return ret;
@@ -1724,10 +1757,12 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
 
 static void tda998x_fini(struct device *dev)
 {
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+	struct tda998x_priv *priv = bridge->priv;
 
 	drm_connector_cleanup(&priv->connector);
-	drm_encoder_cleanup(&priv->encoder);
+	if (priv->local_encoder)
+		drm_encoder_cleanup(&priv->encoder);
 	tda998x_destroy(priv);
 }
 
@@ -1735,7 +1770,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
 
-	return tda998x_init(dev, drm);
+	return tda998x_init(dev, drm, true);
 }
 
 static void tda998x_unbind(struct device *dev, struct device *master,
@@ -1749,19 +1784,100 @@ static const struct component_ops tda998x_ops = {
 	.unbind = tda998x_unbind,
 };
 
+/* DRM bridge functions */
+
+static int tda998x_bridge_attach(struct drm_bridge *dbridge)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct device *dev = bridge->dev;
+	struct drm_device *drm = bridge->bridge.dev;
+
+	return tda998x_init(dev, drm, false);
+}
+
+static void tda998x_bridge_detach(struct drm_bridge *dbridge)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct device *dev = bridge->dev;
+
+	tda998x_fini(dev);
+}
+
+static void tda998x_bridge_enable(struct drm_bridge *dbridge)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct tda998x_priv *priv = bridge->priv;
+
+	tda998x_enable(priv);
+}
+
+static void tda998x_bridge_disable(struct drm_bridge *dbridge)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct tda998x_priv *priv = bridge->priv;
+
+	tda998x_disable(priv);
+}
+
+static void tda998x_bridge_mode_set(struct drm_bridge *dbridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adjusted_mode)
+{
+	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
+	struct tda998x_priv *priv = bridge->priv;
+
+	tda998x_mode_set(priv, mode, adjusted_mode);
+}
+
+static const struct drm_bridge_funcs tda998x_bridge_funcs = {
+	.attach = tda998x_bridge_attach,
+	.detach = tda998x_bridge_detach,
+	.enable = tda998x_bridge_enable,
+	.disable = tda998x_bridge_disable,
+	.mode_set = tda998x_bridge_mode_set,
+};
+
 static int
 tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
+	struct device *dev = &client->dev;
+	struct tda998x_bridge *bridge;
+	int ret;
+
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_warn(&client->dev, "adapter does not support I2C\n");
 		return -EIO;
 	}
-	return component_add(&client->dev, &tda998x_ops);
+
+	bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
+	if (!bridge)
+		return -ENOMEM;
+
+	bridge->dev = dev;
+	dev_set_drvdata(dev, bridge);
+
+	bridge->bridge.funcs = &tda998x_bridge_funcs;
+#ifdef CONFIG_OF
+	bridge->bridge.of_node = dev->of_node;
+#endif
+	drm_bridge_add(&bridge->bridge);
+
+	ret = component_add(dev, &tda998x_ops);
+
+	if (ret)
+		drm_bridge_remove(&bridge->bridge);
+
+	return ret;
 }
 
 static int tda998x_remove(struct i2c_client *client)
 {
-	component_del(&client->dev, &tda998x_ops);
+	struct device *dev = &client->dev;
+	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
+
+	drm_bridge_remove(&bridge->bridge);
+	component_del(dev, &tda998x_ops);
+
 	return 0;
 }
 
-- 
2.11.0

Re: [PATCH v5 7/7] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Wed, May 23, 2018 at 11:31:22AM +0200, Peter Rosin wrote:
> This makes this driver work with all(?) drivers that are not
> componentized and instead expect to connect to a panel/bridge. That
> said, the only one tested is atmel-hlcdc.
> 
> This hooks the relevant work function previously called by the encoder
> and the component also to the bridge, since the encoder goes away when
> connecting to the bridge interface of the driver and the equivalent of
> bind/unbind of the component is handled by bridge attach/detach.
> 
> The lifetime requirements of a bridge and a component are slightly
> different, which is the reason for struct tda998x_bridge.

Why not do this conversion similarly to other "bridge" drivers that have
this same problem (eg, dw-hdmi, dw-mipi-dsi) and always create the
bridge device, but optionally create the encoder and bind the bridge
to the encoder?

That way we don't end up with the veneer functions for bridge-only vs
encoder-only, and we have just one control path to care about - that
being the bridge interface.

> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 170 ++++++++++++++++++++++++++++++++------
>  1 file changed, 143 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index d401b3d0095c..d47e49c2991a 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -36,6 +36,14 @@ struct tda998x_audio_port {
>  	u8 config;		/* AP value */
>  };
>  
> +struct tda998x_priv;
> +
> +struct tda998x_bridge {
> +	struct tda998x_priv *priv;
> +	struct device *dev;
> +	struct drm_bridge bridge;
> +};
> +
>  struct tda998x_priv {
>  	struct i2c_client *cec;
>  	struct i2c_client *hdmi;
> @@ -63,6 +71,8 @@ struct tda998x_priv {
>  	wait_queue_head_t edid_delay_waitq;
>  	bool edid_delay_active;
>  
> +	struct tda998x_bridge *bridge;
> +	bool local_encoder;
>  	struct drm_encoder encoder;
>  	struct drm_connector connector;
>  
> @@ -75,6 +85,9 @@ struct tda998x_priv {
>  #define enc_to_tda998x_priv(x) \
>  	container_of(x, struct tda998x_priv, encoder)
>  
> +#define bridge_to_tda998x_bridge(x) \
> +	container_of(x, struct tda998x_bridge, bridge)
> +
>  /* The TDA9988 series of devices use a paged register scheme.. to simplify
>   * things we encode the page # in upper bits of the register #.  To read/
>   * write a given register, we need to make sure CURPAGE register is set
> @@ -842,7 +855,8 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
>  				   struct hdmi_codec_daifmt *daifmt,
>  				   struct hdmi_codec_params *params)
>  {
> -	struct tda998x_priv *priv = dev_get_drvdata(dev);
> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +	struct tda998x_priv *priv = bridge->priv;
>  	int i, ret;
>  	struct tda998x_audio_params audio = {
>  		.sample_width = params->sample_width,
> @@ -899,7 +913,8 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
>  
>  static void tda998x_audio_shutdown(struct device *dev, void *data)
>  {
> -	struct tda998x_priv *priv = dev_get_drvdata(dev);
> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +	struct tda998x_priv *priv = bridge->priv;
>  
>  	mutex_lock(&priv->audio_mutex);
>  
> @@ -912,7 +927,8 @@ static void tda998x_audio_shutdown(struct device *dev, void *data)
>  
>  int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
>  {
> -	struct tda998x_priv *priv = dev_get_drvdata(dev);
> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +	struct tda998x_priv *priv = bridge->priv;
>  
>  	mutex_lock(&priv->audio_mutex);
>  
> @@ -925,7 +941,8 @@ int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
>  static int tda998x_audio_get_eld(struct device *dev, void *data,
>  				 uint8_t *buf, size_t len)
>  {
> -	struct tda998x_priv *priv = dev_get_drvdata(dev);
> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +	struct tda998x_priv *priv = bridge->priv;
>  
>  	mutex_lock(&priv->audio_mutex);
>  	memcpy(buf, priv->connector.eld,
> @@ -1126,7 +1143,10 @@ tda998x_connector_best_encoder(struct drm_connector *connector)
>  {
>  	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
>  
> -	return &priv->encoder;
> +	if (priv->local_encoder)
> +		return &priv->encoder;
> +	else
> +		return priv->bridge->bridge.encoder;
>  }
>  
>  static
> @@ -1140,6 +1160,7 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
>  				  struct drm_device *drm)
>  {
>  	struct drm_connector *connector = &priv->connector;
> +	struct drm_encoder *encoder;
>  	int ret;
>  
>  	connector->interlace_allowed = 1;
> @@ -1156,7 +1177,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
>  	if (ret)
>  		return ret;
>  
> -	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
> +	encoder = tda998x_connector_best_encoder(&priv->connector);
> +	drm_mode_connector_attach_encoder(&priv->connector, encoder);
>  
>  	return 0;
>  }
> @@ -1671,8 +1693,10 @@ static void tda998x_set_config(struct tda998x_priv *priv,
>  	priv->audio_params = p->audio_params;
>  }
>  
> -static int tda998x_init(struct device *dev, struct drm_device *drm)
> +static int tda998x_init(struct device *dev, struct drm_device *drm,
> +			bool local_encoder)
>  {
> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
>  	struct tda998x_encoder_params *params = dev->platform_data;
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct tda998x_priv *priv;
> @@ -1683,18 +1707,22 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	dev_set_drvdata(dev, priv);
> +	bridge->priv = priv;
> +	priv->bridge = bridge;
> +	priv->local_encoder = local_encoder;
>  
> -	if (dev->of_node)
> -		crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
> +	if (local_encoder) {
> +		if (dev->of_node)
> +			crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
>  
> -	/* If no CRTCs were found, fall back to our old behaviour */
> -	if (crtcs == 0) {
> -		dev_warn(dev, "Falling back to first CRTC\n");
> -		crtcs = 1 << 0;
> -	}
> +		/* If no CRTCs were found, fall back to our old behaviour */
> +		if (crtcs == 0) {
> +			dev_warn(dev, "Falling back to first CRTC\n");
> +			crtcs = 1 << 0;
> +		}
>  
> -	priv->encoder.possible_crtcs = crtcs;
> +		priv->encoder.possible_crtcs = crtcs;
> +	}
>  
>  	ret = tda998x_create(client, priv);
>  	if (ret)
> @@ -1703,11 +1731,15 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
>  	if (!dev->of_node && params)
>  		tda998x_set_config(priv, params);
>  
> -	drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
> -	ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
> -			       DRM_MODE_ENCODER_TMDS, NULL);
> -	if (ret)
> -		goto err_encoder;
> +	if (local_encoder) {
> +		drm_encoder_helper_add(&priv->encoder,
> +				       &tda998x_encoder_helper_funcs);
> +		ret = drm_encoder_init(drm, &priv->encoder,
> +				       &tda998x_encoder_funcs,
> +				       DRM_MODE_ENCODER_TMDS, NULL);
> +		if (ret)
> +			goto err_encoder;
> +	}
>  
>  	ret = tda998x_connector_init(priv, drm);
>  	if (ret)
> @@ -1716,7 +1748,8 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
>  	return 0;
>  
>  err_connector:
> -	drm_encoder_cleanup(&priv->encoder);
> +	if (local_encoder)
> +		drm_encoder_cleanup(&priv->encoder);
>  err_encoder:
>  	tda998x_destroy(priv);
>  	return ret;
> @@ -1724,10 +1757,12 @@ static int tda998x_init(struct device *dev, struct drm_device *drm)
>  
>  static void tda998x_fini(struct device *dev)
>  {
> -	struct tda998x_priv *priv = dev_get_drvdata(dev);
> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +	struct tda998x_priv *priv = bridge->priv;
>  
>  	drm_connector_cleanup(&priv->connector);
> -	drm_encoder_cleanup(&priv->encoder);
> +	if (priv->local_encoder)
> +		drm_encoder_cleanup(&priv->encoder);
>  	tda998x_destroy(priv);
>  }
>  
> @@ -1735,7 +1770,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct drm_device *drm = data;
>  
> -	return tda998x_init(dev, drm);
> +	return tda998x_init(dev, drm, true);
>  }
>  
>  static void tda998x_unbind(struct device *dev, struct device *master,
> @@ -1749,19 +1784,100 @@ static const struct component_ops tda998x_ops = {
>  	.unbind = tda998x_unbind,
>  };
>  
> +/* DRM bridge functions */
> +
> +static int tda998x_bridge_attach(struct drm_bridge *dbridge)
> +{
> +	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
> +	struct device *dev = bridge->dev;
> +	struct drm_device *drm = bridge->bridge.dev;
> +
> +	return tda998x_init(dev, drm, false);
> +}
> +
> +static void tda998x_bridge_detach(struct drm_bridge *dbridge)
> +{
> +	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
> +	struct device *dev = bridge->dev;
> +
> +	tda998x_fini(dev);
> +}
> +
> +static void tda998x_bridge_enable(struct drm_bridge *dbridge)
> +{
> +	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
> +	struct tda998x_priv *priv = bridge->priv;
> +
> +	tda998x_enable(priv);
> +}
> +
> +static void tda998x_bridge_disable(struct drm_bridge *dbridge)
> +{
> +	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
> +	struct tda998x_priv *priv = bridge->priv;
> +
> +	tda998x_disable(priv);
> +}
> +
> +static void tda998x_bridge_mode_set(struct drm_bridge *dbridge,
> +				    struct drm_display_mode *mode,
> +				    struct drm_display_mode *adjusted_mode)
> +{
> +	struct tda998x_bridge *bridge = bridge_to_tda998x_bridge(dbridge);
> +	struct tda998x_priv *priv = bridge->priv;
> +
> +	tda998x_mode_set(priv, mode, adjusted_mode);
> +}
> +
> +static const struct drm_bridge_funcs tda998x_bridge_funcs = {
> +	.attach = tda998x_bridge_attach,
> +	.detach = tda998x_bridge_detach,
> +	.enable = tda998x_bridge_enable,
> +	.disable = tda998x_bridge_disable,
> +	.mode_set = tda998x_bridge_mode_set,
> +};
> +
>  static int
>  tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
> +	struct device *dev = &client->dev;
> +	struct tda998x_bridge *bridge;
> +	int ret;
> +
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		dev_warn(&client->dev, "adapter does not support I2C\n");
>  		return -EIO;
>  	}
> -	return component_add(&client->dev, &tda998x_ops);
> +
> +	bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
> +	if (!bridge)
> +		return -ENOMEM;
> +
> +	bridge->dev = dev;
> +	dev_set_drvdata(dev, bridge);
> +
> +	bridge->bridge.funcs = &tda998x_bridge_funcs;
> +#ifdef CONFIG_OF
> +	bridge->bridge.of_node = dev->of_node;
> +#endif
> +	drm_bridge_add(&bridge->bridge);
> +
> +	ret = component_add(dev, &tda998x_ops);
> +
> +	if (ret)
> +		drm_bridge_remove(&bridge->bridge);
> +
> +	return ret;
>  }
>  
>  static int tda998x_remove(struct i2c_client *client)
>  {
> -	component_del(&client->dev, &tda998x_ops);
> +	struct device *dev = &client->dev;
> +	struct tda998x_bridge *bridge = dev_get_drvdata(dev);
> +
> +	drm_bridge_remove(&bridge->bridge);
> +	component_del(dev, &tda998x_ops);
> +
>  	return 0;
>  }
>  
> -- 
> 2.11.0
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH v5 7/7] drm/i2c: tda998x: register as a drm bridge

[Russell King - ARM Linux]

On Fri, Jul 06, 2018 at 02:36:37PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 23, 2018 at 11:31:22AM +0200, Peter Rosin wrote:
> > This makes this driver work with all(?) drivers that are not
> > componentized and instead expect to connect to a panel/bridge. That
> > said, the only one tested is atmel-hlcdc.
> > 
> > This hooks the relevant work function previously called by the encoder
> > and the component also to the bridge, since the encoder goes away when
> > connecting to the bridge interface of the driver and the equivalent of
> > bind/unbind of the component is handled by bridge attach/detach.
> > 
> > The lifetime requirements of a bridge and a component are slightly
> > different, which is the reason for struct tda998x_bridge.
> 
> Why not do this conversion similarly to other "bridge" drivers that have
> this same problem (eg, dw-hdmi, dw-mipi-dsi) and always create the
> bridge device, but optionally create the encoder and bind the bridge
> to the encoder?
> 
> That way we don't end up with the veneer functions for bridge-only vs
> encoder-only, and we have just one control path to care about - that
> being the bridge interface.

So what I'm proposing is something along the lines of the following
(untested) patch series - I haven't gone to the extent of creating
just the bridge device, but as you will see, it's not that far away.

With the addition of the component helper into drm_bridge code, we
could probably push both armada and tilcdc to use bridges and create
their own encoders for the bridges rather trivially and make tda998x
encoderless, without sacrificing the existing ability to be able to
safely unload these components.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

[PATCH 1/6] drm/i2c: tda998x: find the drm_device via the drm_connector

[Russell King]

From: Peter Rosin <peda@axentia.se>

This prepares for being a drm_bridge which will not register the
encoder. That makes the connector the better choice.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index eb9916bd84a4..4061e293e671 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -753,7 +753,7 @@ static void tda998x_detect_work(struct work_struct *work)
 {
 	struct tda998x_priv *priv =
 		container_of(work, struct tda998x_priv, detect_work);
-	struct drm_device *dev = priv->encoder.dev;
+	struct drm_device *dev = priv->connector.dev;
 
 	if (dev)
 		drm_kms_helper_hotplug_event(dev);
-- 
2.7.4

[PATCH 2/6] drm/i2c: tda998x: split tda998x_encoder_dpms into enable/disable

[Russell King]

From: Peter Rosin <peda@axentia.se>

This fits better with the drm_bridge callbacks for when this
driver becomes a drm_bridge.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
[edited by rmk to just split the tda998x_encoder_dpms() function
 and restore the double-disable protection we originally had,
 preserving original behaviour.]
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 4061e293e671..e9a038a7bb30 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1299,18 +1299,9 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 
 /* DRM encoder functions */
 
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+static void tda998x_enable(struct tda998x_priv *priv)
 {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
-	bool on;
-
-	/* we only care about on or off: */
-	on = mode == DRM_MODE_DPMS_ON;
-
-	if (on == priv->is_on)
-		return;
-
-	if (on) {
+	if (!priv->is_on) {
 		/* enable video ports, audio will be enabled later */
 		reg_write(priv, REG_ENA_VP_0, 0xff);
 		reg_write(priv, REG_ENA_VP_1, 0xff);
@@ -1321,7 +1312,12 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 		reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
 
 		priv->is_on = true;
-	} else {
+	}
+}
+
+static void tda998x_disable(struct tda998x_priv *priv)
+{
+	if (!priv->is_on) {
 		/* disable video ports */
 		reg_write(priv, REG_ENA_VP_0, 0x00);
 		reg_write(priv, REG_ENA_VP_1, 0x00);
@@ -1331,6 +1327,23 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 	}
 }
 
+static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+{
+	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+	bool on;
+
+	/* we only care about on or off: */
+	on = mode == DRM_MODE_DPMS_ON;
+
+	if (on == priv->is_on)
+		return;
+
+	if (on)
+		tda998x_enable(priv);
+	else
+		tda998x_disable(priv);
+}
+
 static void
 tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			 struct drm_display_mode *mode,
-- 
2.7.4

[PATCH 3/6] drm/i2c: tda998x: move tda998x_set_config() into tda998x_create()

[Russell King]

Move the non-DT configuration of the TDA998x into tda998x_create()
so that we do all setup in one place.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 75 ++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index e9a038a7bb30..931135db979c 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1621,6 +1621,25 @@ static int tda998x_get_audio_ports(struct tda998x_priv *priv,
 	return 0;
 }
 
+static void tda998x_set_config(struct tda998x_priv *priv,
+			       const struct tda998x_encoder_params *p)
+{
+	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
+			    (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
+			    VIP_CNTRL_0_SWAP_B(p->swap_b) |
+			    (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0);
+	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) |
+			    (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) |
+			    VIP_CNTRL_1_SWAP_D(p->swap_d) |
+			    (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0);
+	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) |
+			    (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) |
+			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
+			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
+
+	priv->audio_params = p->audio_params;
+}
+
 static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
 	struct device_node *np = client->dev.of_node;
@@ -1772,23 +1791,28 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	/* enable EDID read irq: */
 	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
-	if (!np)
-		return 0;		/* non-DT */
+	if (np) {
+		/* get the device tree parameters */
+		ret = of_property_read_u32(np, "video-ports", &video);
+		if (ret == 0) {
+			priv->vip_cntrl_0 = video >> 16;
+			priv->vip_cntrl_1 = video >> 8;
+			priv->vip_cntrl_2 = video;
+		}
 
-	/* get the device tree parameters */
-	ret = of_property_read_u32(np, "video-ports", &video);
-	if (ret == 0) {
-		priv->vip_cntrl_0 = video >> 16;
-		priv->vip_cntrl_1 = video >> 8;
-		priv->vip_cntrl_2 = video;
-	}
+		ret = tda998x_get_audio_ports(priv, np);
+		if (ret)
+			goto fail;
 
-	ret = tda998x_get_audio_ports(priv, np);
-	if (ret)
-		goto fail;
+		if (priv->audio_port[0].format != AFMT_UNUSED)
+			tda998x_audio_codec_init(priv, &client->dev);
+	} else {
+		struct tda998x_encoder_params *params;
 
-	if (priv->audio_port[0].format != AFMT_UNUSED)
-		tda998x_audio_codec_init(priv, &client->dev);
+		params = client->dev.platform_data;
+		if (params)
+			tda998x_set_config(priv, params);
+	}
 
 	return 0;
 
@@ -1835,28 +1859,8 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = {
 	.destroy = tda998x_encoder_destroy,
 };
 
-static void tda998x_set_config(struct tda998x_priv *priv,
-			       const struct tda998x_encoder_params *p)
-{
-	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
-			    (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
-			    VIP_CNTRL_0_SWAP_B(p->swap_b) |
-			    (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0);
-	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) |
-			    (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) |
-			    VIP_CNTRL_1_SWAP_D(p->swap_d) |
-			    (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0);
-	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) |
-			    (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) |
-			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
-			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
-
-	priv->audio_params = p->audio_params;
-}
-
 static int tda998x_bind(struct device *dev, struct device *master, void *data)
 {
-	struct tda998x_encoder_params *params = dev->platform_data;
 	struct i2c_client *client = to_i2c_client(dev);
 	struct drm_device *drm = data;
 	struct tda998x_priv *priv;
@@ -1884,9 +1888,6 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	if (!dev->of_node && params)
-		tda998x_set_config(priv, params);
-
 	drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
 	ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
 			       DRM_MODE_ENCODER_TMDS, NULL);
-- 
2.7.4

[PATCH 4/6] drm/i2c: tda998x: convert to bridge driver

[Russell King]

Convert tda998x to a bridge driver with built-in encoder support for
compatibility with existing component drivers.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 147 +++++++++++++++++++-------------------
 1 file changed, 75 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 931135db979c..53f110e7d433 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -69,6 +69,7 @@ struct tda998x_priv {
 	bool edid_delay_active;
 
 	struct drm_encoder encoder;
+	struct drm_bridge bridge;
 	struct drm_connector connector;
 
 	struct tda998x_audio_port audio_port[2];
@@ -79,9 +80,10 @@ struct tda998x_priv {
 
 #define conn_to_tda998x_priv(x) \
 	container_of(x, struct tda998x_priv, connector)
-
 #define enc_to_tda998x_priv(x) \
 	container_of(x, struct tda998x_priv, encoder)
+#define bridge_to_tda998x_priv(x) \
+	container_of(x, struct tda998x_priv, bridge)
 
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
@@ -1297,10 +1299,24 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 	return 0;
 }
 
-/* DRM encoder functions */
+/* DRM bridge functions */
+
+static int tda998x_bridge_attach(struct drm_bridge *bridge)
+{
+	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+
+	return tda998x_connector_init(priv, bridge->dev);
+}
 
-static void tda998x_enable(struct tda998x_priv *priv)
+static void tda998x_bridge_detach(struct drm_bridge *bridge)
 {
+	/* We should remove the connector for symmetry with _attach() */
+}
+
+static void tda998x_bridge_enable(struct drm_bridge *bridge)
+{
+	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+
 	if (!priv->is_on) {
 		/* enable video ports, audio will be enabled later */
 		reg_write(priv, REG_ENA_VP_0, 0xff);
@@ -1315,7 +1331,7 @@ static void tda998x_enable(struct tda998x_priv *priv)
 	}
 }
 
-static void tda998x_disable(struct tda998x_priv *priv)
+static void tda998x_bridge_disable(struct drm_bridge *bridge)
 {
 	if (!priv->is_on) {
 		/* disable video ports */
@@ -1327,29 +1343,11 @@ static void tda998x_disable(struct tda998x_priv *priv)
 	}
 }
 
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adjusted_mode)
 {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
-	bool on;
-
-	/* we only care about on or off: */
-	on = mode == DRM_MODE_DPMS_ON;
-
-	if (on == priv->is_on)
-		return;
-
-	if (on)
-		tda998x_enable(priv);
-	else
-		tda998x_disable(priv);
-}
-
-static void
-tda998x_encoder_mode_set(struct drm_encoder *encoder,
-			 struct drm_display_mode *mode,
-			 struct drm_display_mode *adjusted_mode)
-{
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
 	u16 ref_pix, ref_line, n_pix, n_line;
 	u16 hs_pix_s, hs_pix_e;
 	u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1556,8 +1554,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	mutex_unlock(&priv->audio_mutex);
 }
 
+static const struct drm_bridge_funcs tda998x_bridge_funcs = {
+	.attach = tda998x_bridge_attach,
+	.detach = tda998x_bridge_detach,
+	.disable = tda998x_bridge_disable,
+	.mode_set = tda998x_bridge_mode_set,
+	.enable = tda998x_bridge_enable,
+};
+
 static void tda998x_destroy(struct tda998x_priv *priv)
 {
+	drm_bridge_remove(&priv->bridge);
+
 	/* disable all IRQs and free the IRQ handler */
 	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
 	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1650,6 +1658,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	mutex_init(&priv->mutex);	/* protect the page access */
 	mutex_init(&priv->audio_mutex); /* protect access from audio thread */
 	mutex_init(&priv->edid_mutex);
+	INIT_LIST_HEAD(&priv->bridge.list);
 	init_waitqueue_head(&priv->edid_delay_waitq);
 	timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
 	INIT_WORK(&priv->detect_work, tda998x_detect_work);
@@ -1814,44 +1823,23 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 			tda998x_set_config(priv, params);
 	}
 
+	priv->bridge.funcs = &tda998x_bridge_funcs;
+	priv->bridge.of_node = dev->of_node;
+
+	drm_bridge_add(&priv->bridge);
+
 	return 0;
 
 fail:
-	/* if encoder_init fails, the encoder slave is never registered,
-	 * so cleanup here:
-	 */
-	if (priv->cec)
-		i2c_unregister_device(priv->cec);
-	if (priv->cec_notify)
-		cec_notifier_put(priv->cec_notify);
-	if (client->irq)
-		free_irq(client->irq, priv);
+	tda998x_destroy(priv);
 err_irq:
 	return ret;
 }
 
-static void tda998x_encoder_prepare(struct drm_encoder *encoder)
-{
-	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
-}
-
-static void tda998x_encoder_commit(struct drm_encoder *encoder)
-{
-	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
-}
-
-static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
-	.dpms = tda998x_encoder_dpms,
-	.prepare = tda998x_encoder_prepare,
-	.commit = tda998x_encoder_commit,
-	.mode_set = tda998x_encoder_mode_set,
-};
+/* DRM encoder functions */
 
 static void tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
-
-	tda998x_destroy(priv);
 	drm_encoder_cleanup(encoder);
 }
 
@@ -1859,20 +1847,12 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = {
 	.destroy = tda998x_encoder_destroy,
 };
 
-static int tda998x_bind(struct device *dev, struct device *master, void *data)
+static int tda998x_encoder_init(struct device *dev, struct drm_device *drm)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct drm_device *drm = data;
-	struct tda998x_priv *priv;
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
 	u32 crtcs = 0;
 	int ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	dev_set_drvdata(dev, priv);
-
 	if (dev->of_node)
 		crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
 
@@ -1884,25 +1864,48 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 
 	priv->encoder.possible_crtcs = crtcs;
 
-	ret = tda998x_create(client, priv);
-	if (ret)
-		return ret;
-
-	drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
 	ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
 			       DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret)
 		goto err_encoder;
 
-	ret = tda998x_connector_init(priv, drm);
+	ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL);
 	if (ret)
-		goto err_connector;
+		goto err_bridge;
 
 	return 0;
 
-err_connector:
+err_bridge:
 	drm_encoder_cleanup(&priv->encoder);
 err_encoder:
+	return ret;
+}
+
+static int tda998x_bind(struct device *dev, struct device *master, void *data)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct drm_device *drm = data;
+	struct tda998x_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, priv);
+
+	ret = tda998x_create(client, priv);
+	if (ret)
+		return ret;
+
+	ret = tda998x_encoder_init(dev, drm);
+	if (ret) {
+		tda998x_destroy(priv);
+		return ret;
+	}
+	return 0;
+
+err_encoder:
 	tda998x_destroy(priv);
 	return ret;
 }
-- 
2.7.4

Re: [PATCH 4/6] drm/i2c: tda998x: convert to bridge driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2910 bytes --]

Hi Russell,

I love your patch! Yet something to improve:

[auto build test ERROR on arm/drm-tda998x-devel]
[cannot apply to v4.18-rc3 next-20180706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Russell-King/drm-i2c-tda998x-find-the-drm_device-via-the-drm_connector/20180707-030507
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-devel
config: i386-randconfig-x007-07071008 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_bridge_disable':
>> drivers/gpu/drm/i2c/tda998x_drv.c:1336:7: error: 'priv' undeclared (first use in this function); did you mean 'pid'?
     if (!priv->is_on) {
          ^~~~
          pid
   drivers/gpu/drm/i2c/tda998x_drv.c:1336:7: note: each undeclared identifier is reported only once for each function it appears in
   drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_create':
   drivers/gpu/drm/i2c/tda998x_drv.c:1827:25: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
     priv->bridge.of_node = dev->of_node;
                            ^~~
                            cdev
   drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_bind':
   drivers/gpu/drm/i2c/tda998x_drv.c:1908:1: warning: label 'err_encoder' defined but not used [-Wunused-label]
    err_encoder:
    ^~~~~~~~~~~

vim +1336 drivers/gpu/drm/i2c/tda998x_drv.c

1772fa6e Peter Rosin         2018-07-06  1333  
3b94aa13 Russell King        2018-07-06  1334  static void tda998x_bridge_disable(struct drm_bridge *bridge)
1772fa6e Peter Rosin         2018-07-06  1335  {
1772fa6e Peter Rosin         2018-07-06 @1336  	if (!priv->is_on) {
db6aaf4d Russell King        2013-09-24  1337  		/* disable video ports */
2f7f730a Jean-Francois Moine 2014-01-25  1338  		reg_write(priv, REG_ENA_VP_0, 0x00);
2f7f730a Jean-Francois Moine 2014-01-25  1339  		reg_write(priv, REG_ENA_VP_1, 0x00);
2f7f730a Jean-Francois Moine 2014-01-25  1340  		reg_write(priv, REG_ENA_VP_2, 0x00);
e7792ce2 Rob Clark           2013-01-08  1341  
3cb43378 Russell King        2016-10-23  1342  		priv->is_on = false;
3cb43378 Russell King        2016-10-23  1343  	}
e7792ce2 Rob Clark           2013-01-08  1344  }
e7792ce2 Rob Clark           2013-01-08  1345  

:::::: The code at line 1336 was first introduced by commit
:::::: 1772fa6e1cb0338cee15728fd16679b6b2fc9fcb drm/i2c: tda998x: split tda998x_encoder_dpms into enable/disable

:::::: TO: Peter Rosin <peda@axentia.se>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28779 bytes --]

Re: [PATCH 4/6] drm/i2c: tda998x: convert to bridge driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8324 bytes --]

Hi Russell,

I love your patch! Yet something to improve:

[auto build test ERROR on arm/drm-tda998x-devel]
[cannot apply to v4.18-rc3 next-20180706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Russell-King/drm-i2c-tda998x-find-the-drm_device-via-the-drm_connector/20180707-030507
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-devel
config: i386-randconfig-x018-201826 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_bridge_disable':
   drivers/gpu/drm/i2c/tda998x_drv.c:1336:7: error: 'priv' undeclared (first use in this function); did you mean 'pid'?
     if (!priv->is_on) {
          ^~~~
          pid
   drivers/gpu/drm/i2c/tda998x_drv.c:1336:7: note: each undeclared identifier is reported only once for each function it appears in
   drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_create':
>> drivers/gpu/drm/i2c/tda998x_drv.c:1827:14: error: 'struct drm_bridge' has no member named 'of_node'
     priv->bridge.of_node = dev->of_node;
                 ^
   drivers/gpu/drm/i2c/tda998x_drv.c:1827:25: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
     priv->bridge.of_node = dev->of_node;
                            ^~~
                            cdev
   drivers/gpu/drm/i2c/tda998x_drv.c: In function 'tda998x_bind':
   drivers/gpu/drm/i2c/tda998x_drv.c:1908:1: warning: label 'err_encoder' defined but not used [-Wunused-label]
    err_encoder:
    ^~~~~~~~~~~

vim +1827 drivers/gpu/drm/i2c/tda998x_drv.c

  1650	
  1651	static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
  1652	{
  1653		struct device_node *np = client->dev.of_node;
  1654		struct i2c_board_info cec_info;
  1655		u32 video;
  1656		int rev_lo, rev_hi, ret;
  1657	
  1658		mutex_init(&priv->mutex);	/* protect the page access */
  1659		mutex_init(&priv->audio_mutex); /* protect access from audio thread */
  1660		mutex_init(&priv->edid_mutex);
  1661		INIT_LIST_HEAD(&priv->bridge.list);
  1662		init_waitqueue_head(&priv->edid_delay_waitq);
  1663		timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
  1664		INIT_WORK(&priv->detect_work, tda998x_detect_work);
  1665	
  1666		priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
  1667		priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
  1668		priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
  1669	
  1670		/* CEC I2C address bound to TDA998x I2C addr by configuration pins */
  1671		priv->cec_addr = 0x34 + (client->addr & 0x03);
  1672		priv->current_page = 0xff;
  1673		priv->hdmi = client;
  1674	
  1675		/* wake up the device: */
  1676		cec_write(priv, REG_CEC_ENAMODS,
  1677				CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
  1678	
  1679		tda998x_reset(priv);
  1680	
  1681		/* read version: */
  1682		rev_lo = reg_read(priv, REG_VERSION_LSB);
  1683		if (rev_lo < 0) {
  1684			dev_err(&client->dev, "failed to read version: %d\n", rev_lo);
  1685			return rev_lo;
  1686		}
  1687	
  1688		rev_hi = reg_read(priv, REG_VERSION_MSB);
  1689		if (rev_hi < 0) {
  1690			dev_err(&client->dev, "failed to read version: %d\n", rev_hi);
  1691			return rev_hi;
  1692		}
  1693	
  1694		priv->rev = rev_lo | rev_hi << 8;
  1695	
  1696		/* mask off feature bits: */
  1697		priv->rev &= ~0x30; /* not-hdcp and not-scalar bit */
  1698	
  1699		switch (priv->rev) {
  1700		case TDA9989N2:
  1701			dev_info(&client->dev, "found TDA9989 n2");
  1702			break;
  1703		case TDA19989:
  1704			dev_info(&client->dev, "found TDA19989");
  1705			break;
  1706		case TDA19989N2:
  1707			dev_info(&client->dev, "found TDA19989 n2");
  1708			break;
  1709		case TDA19988:
  1710			dev_info(&client->dev, "found TDA19988");
  1711			break;
  1712		default:
  1713			dev_err(&client->dev, "found unsupported device: %04x\n",
  1714				priv->rev);
  1715			return -ENXIO;
  1716		}
  1717	
  1718		/* after reset, enable DDC: */
  1719		reg_write(priv, REG_DDC_DISABLE, 0x00);
  1720	
  1721		/* set clock on DDC channel: */
  1722		reg_write(priv, REG_TX3, 39);
  1723	
  1724		/* if necessary, disable multi-master: */
  1725		if (priv->rev == TDA19989)
  1726			reg_set(priv, REG_I2C_MASTER, I2C_MASTER_DIS_MM);
  1727	
  1728		cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL,
  1729				CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
  1730	
  1731		/* ensure interrupts are disabled */
  1732		cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
  1733	
  1734		/* clear pending interrupts */
  1735		cec_read(priv, REG_CEC_RXSHPDINT);
  1736		reg_read(priv, REG_INT_FLAGS_0);
  1737		reg_read(priv, REG_INT_FLAGS_1);
  1738		reg_read(priv, REG_INT_FLAGS_2);
  1739	
  1740		/* initialize the optional IRQ */
  1741		if (client->irq) {
  1742			unsigned long irq_flags;
  1743	
  1744			/* init read EDID waitqueue and HDP work */
  1745			init_waitqueue_head(&priv->wq_edid);
  1746	
  1747			irq_flags =
  1748				irqd_get_trigger_type(irq_get_irq_data(client->irq));
  1749	
  1750			priv->cec_glue.irq_flags = irq_flags;
  1751	
  1752			irq_flags |= IRQF_SHARED | IRQF_ONESHOT;
  1753			ret = request_threaded_irq(client->irq, NULL,
  1754						   tda998x_irq_thread, irq_flags,
  1755						   "tda998x", priv);
  1756			if (ret) {
  1757				dev_err(&client->dev,
  1758					"failed to request IRQ#%u: %d\n",
  1759					client->irq, ret);
  1760				goto err_irq;
  1761			}
  1762	
  1763			/* enable HPD irq */
  1764			cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
  1765		}
  1766	
  1767		priv->cec_notify = cec_notifier_get(&client->dev);
  1768		if (!priv->cec_notify) {
  1769			ret = -ENOMEM;
  1770			goto fail;
  1771		}
  1772	
  1773		priv->cec_glue.parent = &client->dev;
  1774		priv->cec_glue.data = priv;
  1775		priv->cec_glue.init = tda998x_cec_hook_init;
  1776		priv->cec_glue.exit = tda998x_cec_hook_exit;
  1777		priv->cec_glue.open = tda998x_cec_hook_open;
  1778		priv->cec_glue.release = tda998x_cec_hook_release;
  1779	
  1780		/*
  1781		 * Some TDA998x are actually two I2C devices merged onto one piece
  1782		 * of silicon: TDA9989 and TDA19989 combine the HDMI transmitter
  1783		 * with a slightly modified TDA9950 CEC device.  The CEC device
  1784		 * is at the TDA9950 address, with the address pins strapped across
  1785		 * to the TDA998x address pins.  Hence, it always has the same
  1786		 * offset.
  1787		 */
  1788		memset(&cec_info, 0, sizeof(cec_info));
  1789		strlcpy(cec_info.type, "tda9950", sizeof(cec_info.type));
  1790		cec_info.addr = priv->cec_addr;
  1791		cec_info.platform_data = &priv->cec_glue;
  1792		cec_info.irq = client->irq;
  1793	
  1794		priv->cec = i2c_new_device(client->adapter, &cec_info);
  1795		if (!priv->cec) {
  1796			ret = -ENODEV;
  1797			goto fail;
  1798		}
  1799	
  1800		/* enable EDID read irq: */
  1801		reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
  1802	
  1803		if (np) {
  1804			/* get the device tree parameters */
  1805			ret = of_property_read_u32(np, "video-ports", &video);
  1806			if (ret == 0) {
  1807				priv->vip_cntrl_0 = video >> 16;
  1808				priv->vip_cntrl_1 = video >> 8;
  1809				priv->vip_cntrl_2 = video;
  1810			}
  1811	
  1812			ret = tda998x_get_audio_ports(priv, np);
  1813			if (ret)
  1814				goto fail;
  1815	
  1816			if (priv->audio_port[0].format != AFMT_UNUSED)
  1817				tda998x_audio_codec_init(priv, &client->dev);
  1818		} else {
  1819			struct tda998x_encoder_params *params;
  1820	
  1821			params = client->dev.platform_data;
  1822			if (params)
  1823				tda998x_set_config(priv, params);
  1824		}
  1825	
  1826		priv->bridge.funcs = &tda998x_bridge_funcs;
> 1827		priv->bridge.of_node = dev->of_node;
  1828	
  1829		drm_bridge_add(&priv->bridge);
  1830	
  1831		return 0;
  1832	
  1833	fail:
  1834		tda998x_destroy(priv);
  1835	err_irq:
  1836		return ret;
  1837	}
  1838	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30360 bytes --]

[PATCH 5/6] drm/i2c: tda998x: allocate tda998x_priv inside tda998x_create()

[Russell King]

Move the tda998x_priv allocation inside tda998x_create() and simplify
the tda998x_create()'s arguments.  Pass the same to tda998x_destroy().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 53f110e7d433..08ccaf8cf852 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1562,8 +1562,10 @@ static const struct drm_bridge_funcs tda998x_bridge_funcs = {
 	.enable = tda998x_bridge_enable,
 };
 
-static void tda998x_destroy(struct tda998x_priv *priv)
+static void tda998x_destroy(struct device *dev)
 {
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
 	drm_bridge_remove(&priv->bridge);
 
 	/* disable all IRQs and free the IRQ handler */
@@ -1648,13 +1650,21 @@ static void tda998x_set_config(struct tda998x_priv *priv,
 	priv->audio_params = p->audio_params;
 }
 
-static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
+static int tda998x_create(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
 	struct device_node *np = client->dev.of_node;
 	struct i2c_board_info cec_info;
+	struct tda998x_priv *priv;
 	u32 video;
 	int rev_lo, rev_hi, ret;
 
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, priv);
+
 	mutex_init(&priv->mutex);	/* protect the page access */
 	mutex_init(&priv->audio_mutex); /* protect access from audio thread */
 	mutex_init(&priv->edid_mutex);
@@ -1831,7 +1841,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	return 0;
 
 fail:
-	tda998x_destroy(priv);
+	tda998x_destroy(dev);
 err_irq:
 	return ret;
 }
@@ -1883,30 +1893,22 @@ static int tda998x_encoder_init(struct device *dev, struct drm_device *drm)
 
 static int tda998x_bind(struct device *dev, struct device *master, void *data)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct drm_device *drm = data;
-	struct tda998x_priv *priv;
 	int ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	dev_set_drvdata(dev, priv);
-
-	ret = tda998x_create(client, priv);
+	ret = tda998x_create(dev);
 	if (ret)
 		return ret;
 
 	ret = tda998x_encoder_init(dev, drm);
 	if (ret) {
-		tda998x_destroy(priv);
+		tda998x_destroy(dev);
 		return ret;
 	}
 	return 0;
 
 err_encoder:
-	tda998x_destroy(priv);
+	tda998x_destroy(dev);
 	return ret;
 }
 
@@ -1917,7 +1919,7 @@ static void tda998x_unbind(struct device *dev, struct device *master,
 
 	drm_connector_cleanup(&priv->connector);
 	drm_encoder_cleanup(&priv->encoder);
-	tda998x_destroy(priv);
+	tda998x_destroy(dev);
 }
 
 static const struct component_ops tda998x_ops = {
-- 
2.7.4

[PATCH 6/6] drm/i2c: tda998x: cleanup from previous changes

[Russell King]

Cleanup the code a little from the effects of the previous changes:
- Move tda998x_destroy() to be above tda998x_create()
- Use 'dev' directly in tda998x_create() where appropriate.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 76 +++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 08ccaf8cf852..6f74b3451567 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1562,31 +1562,6 @@ static const struct drm_bridge_funcs tda998x_bridge_funcs = {
 	.enable = tda998x_bridge_enable,
 };
 
-static void tda998x_destroy(struct device *dev)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-
-	drm_bridge_remove(&priv->bridge);
-
-	/* disable all IRQs and free the IRQ handler */
-	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
-	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
-
-	if (priv->audio_pdev)
-		platform_device_unregister(priv->audio_pdev);
-
-	if (priv->hdmi->irq)
-		free_irq(priv->hdmi->irq, priv);
-
-	del_timer_sync(&priv->edid_delay_timer);
-	cancel_work_sync(&priv->detect_work);
-
-	i2c_unregister_device(priv->cec);
-
-	if (priv->cec_notify)
-		cec_notifier_put(priv->cec_notify);
-}
-
 /* I2C driver functions */
 
 static int tda998x_get_audio_ports(struct tda998x_priv *priv,
@@ -1650,6 +1625,31 @@ static void tda998x_set_config(struct tda998x_priv *priv,
 	priv->audio_params = p->audio_params;
 }
 
+static void tda998x_destroy(struct device *dev)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	drm_bridge_remove(&priv->bridge);
+
+	/* disable all IRQs and free the IRQ handler */
+	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
+	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+
+	if (priv->audio_pdev)
+		platform_device_unregister(priv->audio_pdev);
+
+	if (priv->hdmi->irq)
+		free_irq(priv->hdmi->irq, priv);
+
+	del_timer_sync(&priv->edid_delay_timer);
+	cancel_work_sync(&priv->detect_work);
+
+	i2c_unregister_device(priv->cec);
+
+	if (priv->cec_notify)
+		cec_notifier_put(priv->cec_notify);
+}
+
 static int tda998x_create(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -1691,13 +1691,13 @@ static int tda998x_create(struct device *dev)
 	/* read version: */
 	rev_lo = reg_read(priv, REG_VERSION_LSB);
 	if (rev_lo < 0) {
-		dev_err(&client->dev, "failed to read version: %d\n", rev_lo);
+		dev_err(dev, "failed to read version: %d\n", rev_lo);
 		return rev_lo;
 	}
 
 	rev_hi = reg_read(priv, REG_VERSION_MSB);
 	if (rev_hi < 0) {
-		dev_err(&client->dev, "failed to read version: %d\n", rev_hi);
+		dev_err(dev, "failed to read version: %d\n", rev_hi);
 		return rev_hi;
 	}
 
@@ -1708,20 +1708,19 @@ static int tda998x_create(struct device *dev)
 
 	switch (priv->rev) {
 	case TDA9989N2:
-		dev_info(&client->dev, "found TDA9989 n2");
+		dev_info(dev, "found TDA9989 n2");
 		break;
 	case TDA19989:
-		dev_info(&client->dev, "found TDA19989");
+		dev_info(dev, "found TDA19989");
 		break;
 	case TDA19989N2:
-		dev_info(&client->dev, "found TDA19989 n2");
+		dev_info(dev, "found TDA19989 n2");
 		break;
 	case TDA19988:
-		dev_info(&client->dev, "found TDA19988");
+		dev_info(dev, "found TDA19988");
 		break;
 	default:
-		dev_err(&client->dev, "found unsupported device: %04x\n",
-			priv->rev);
+		dev_err(dev, "found unsupported device: %04x\n", priv->rev);
 		return -ENXIO;
 	}
 
@@ -1764,8 +1763,7 @@ static int tda998x_create(struct device *dev)
 					   tda998x_irq_thread, irq_flags,
 					   "tda998x", priv);
 		if (ret) {
-			dev_err(&client->dev,
-				"failed to request IRQ#%u: %d\n",
+			dev_err(dev, "failed to request IRQ#%u: %d\n",
 				client->irq, ret);
 			goto err_irq;
 		}
@@ -1774,13 +1772,13 @@ static int tda998x_create(struct device *dev)
 		cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
 	}
 
-	priv->cec_notify = cec_notifier_get(&client->dev);
+	priv->cec_notify = cec_notifier_get(dev);
 	if (!priv->cec_notify) {
 		ret = -ENOMEM;
 		goto fail;
 	}
 
-	priv->cec_glue.parent = &client->dev;
+	priv->cec_glue.parent = dev;
 	priv->cec_glue.data = priv;
 	priv->cec_glue.init = tda998x_cec_hook_init;
 	priv->cec_glue.exit = tda998x_cec_hook_exit;
@@ -1826,9 +1824,7 @@ static int tda998x_create(struct device *dev)
 		if (priv->audio_port[0].format != AFMT_UNUSED)
 			tda998x_audio_codec_init(priv, &client->dev);
 	} else {
-		struct tda998x_encoder_params *params;
-
-		params = client->dev.platform_data;
+		struct tda998x_encoder_params *params = dev->platform_data;
 		if (params)
 			tda998x_set_config(priv, params);
 	}
-- 
2.7.4